On 7/17/2025 7:32 AM, Mi, Dapeng wrote: > > On 7/16/2025 6:10 PM, Manali Shukla wrote: >> Hi Dapeng Mi, >> >> Thanks for reviewing my patches. >> >> On 7/15/2025 8:28 AM, Mi, Dapeng wrote: >>> On 6/28/2025 12:25 AM, Manali Shukla wrote: >>>> From: Santosh Shukla <santosh.shukla@xxxxxxx> >>>> >>>> The local interrupts are extended to include more LVT registers in >>>> order to allow additional interrupt sources, like Instruction Based >>>> Sampling (IBS) and many more. >>>> >>>> Currently there are four additional LVT registers defined and they are >>>> located at APIC offsets 400h-530h. >>>> >>>> AMD IBS driver is designed to use EXTLVT (Extended interrupt local >>>> vector table) by default for driver initialization. >>>> >>>> Extended LVT registers are required to be emulated to initialize the >>>> guest IBS driver successfully. >>>> >>>> Please refer to Section 16.4.5 in AMD Programmer's Manual Volume 2 at >>>> https://bugzilla.kernel.org/attachment.cgi?id=306250 for more details >>>> on Extended LVT. >>>> >>>> Signed-off-by: Santosh Shukla <santosh.shukla@xxxxxxx> >>>> Co-developed-by: Manali Shukla <manali.shukla@xxxxxxx> >>>> Signed-off-by: Manali Shukla <manali.shukla@xxxxxxx> >>>> --- >>>> arch/x86/include/asm/apicdef.h | 17 +++++++++ >>>> arch/x86/kvm/cpuid.c | 6 +++ >>>> arch/x86/kvm/lapic.c | 69 +++++++++++++++++++++++++++++++++- >>>> arch/x86/kvm/lapic.h | 1 + >>>> arch/x86/kvm/svm/avic.c | 4 ++ >>>> arch/x86/kvm/svm/svm.c | 4 ++ >>>> 6 files changed, 99 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h >>>> index 094106b6a538..4c0f580578aa 100644 >>>> --- a/arch/x86/include/asm/apicdef.h >>>> +++ b/arch/x86/include/asm/apicdef.h >>>> @@ -146,6 +146,23 @@ >>>> #define APIC_EILVT_MSG_EXT 0x7 >>>> #define APIC_EILVT_MASKED (1 << 16) >>>> >>>> +/* >>>> + * Initialize extended APIC registers to the default value when guest >>>> + * is started and EXTAPIC feature is enabled on the guest. >>>> + * >>>> + * APIC_EFEAT is a read only Extended APIC feature register, whose >>>> + * default value is 0x00040007. However, bits 0, 1, and 2 represent >>>> + * features that are not currently emulated by KVM. Therefore, these >>>> + * bits must be cleared during initialization. As a result, the >>>> + * default value used for APIC_EFEAT in KVM is 0x00040000. >>>> + * >>>> + * APIC_ECTRL is a read-write Extended APIC control register, whose >>>> + * default value is 0x0. >>>> + */ >>>> + >>>> +#define APIC_EFEAT_DEFAULT 0x00040000 >>>> +#define APIC_ECTRL_DEFAULT 0x0 >>>> + >>>> #define APIC_BASE (fix_to_virt(FIX_APIC_BASE)) >>>> #define APIC_BASE_MSR 0x800 >>>> #define APIC_X2APIC_ID_MSR 0x802 >>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>>> index eb7be340138b..7270d22fbf31 100644 >>>> --- a/arch/x86/kvm/cpuid.c >>>> +++ b/arch/x86/kvm/cpuid.c >>>> @@ -458,6 +458,12 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) >>>> /* Invoke the vendor callback only after the above state is updated. */ >>>> kvm_x86_call(vcpu_after_set_cpuid)(vcpu); >>>> >>>> + /* >>>> + * Initialize extended LVT registers at guest startup to support delivery >>>> + * of interrupts via the extended APIC space (offsets 0x400–0x530). >>>> + */ >>>> + kvm_apic_init_eilvt_regs(vcpu); >>>> + >>>> /* >>>> * Except for the MMU, which needs to do its thing any vendor specific >>>> * adjustments to the reserved GPA bits. >>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >>>> index 00ca2b0faa45..cffe44eb3f2b 100644 >>>> --- a/arch/x86/kvm/lapic.c >>>> +++ b/arch/x86/kvm/lapic.c >>>> @@ -1624,9 +1624,13 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev) >>>> } >>>> >>>> #define APIC_REG_MASK(reg) (1ull << ((reg) >> 4)) >>>> +#define APIC_REG_EXT_MASK(reg) (1ull << (((reg) >> 4) - 0x40)) >>> It seems there is no difference on the MASK definition between >>> APIC_REG_MASK() and APIC_REG_EXT_MASK(). Why not directly use the original >>> APIC_REG_MASK()? >>> >> The Extended LVT registers range from 0x400 to 0x530. When using >> APIC_REG_MASK(reg) with reg = 0x400 (as an example), the operation >> results in a right shift of 64(0x40) bits, causing an overflow. This was >> the actual reason of creating a new macro for extended APIC register space. > > I see. Just ignored that the bit could extend 64 bits. > > >> >>> BTW, If we indeed need to define this new macro, could we define the macro >>> like blow? >>> >>> #define APIC_REG_EXT_MASK(reg) (1ull << (((reg) - 0x400) >> 4)) >>> >>> It's more easily to understand. >>> >> I can define the macro in this way. >> >>>> #define APIC_REGS_MASK(first, count) \ >>>> (APIC_REG_MASK(first) * ((1ull << (count)) - 1)) >>>> >>>> +#define APIC_LAST_REG_OFFSET 0x3f0 >>>> +#define APIC_EXT_LAST_REG_OFFSET 0x530 >>>> + >>>> u64 kvm_lapic_readable_reg_mask(struct kvm_lapic *apic) >>>> { >>>> /* Leave bits '0' for reserved and write-only registers. */ >>>> @@ -1668,6 +1672,8 @@ EXPORT_SYMBOL_GPL(kvm_lapic_readable_reg_mask); >>>> static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len, >>>> void *data) >>>> { >>>> + u64 valid_reg_ext_mask = 0; >>>> + unsigned int last_reg = APIC_LAST_REG_OFFSET; >>>> unsigned char alignment = offset & 0xf; >>>> u32 result; >>>> >>>> @@ -1677,13 +1683,44 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len, >>>> */ >>>> WARN_ON_ONCE(apic_x2apic_mode(apic) && offset == APIC_ICR); >>>> >>>> + /* >>>> + * The local interrupts are extended to include LVT registers to allow >>>> + * additional interrupt sources when the EXTAPIC feature bit is enabled. >>>> + * The Extended Interrupt LVT registers are located at APIC offsets 400-530h. >>>> + */ >>>> + if (guest_cpu_cap_has(apic->vcpu, X86_FEATURE_EXTAPIC)) { >>>> + valid_reg_ext_mask = >>>> + APIC_REG_EXT_MASK(APIC_EFEAT) | >>>> + APIC_REG_EXT_MASK(APIC_ECTRL) | >>>> + APIC_REG_EXT_MASK(APIC_EILVTn(0)) | >>>> + APIC_REG_EXT_MASK(APIC_EILVTn(1)) | >>>> + APIC_REG_EXT_MASK(APIC_EILVTn(2)) | >>>> + APIC_REG_EXT_MASK(APIC_EILVTn(3)); >>>> + last_reg = APIC_EXT_LAST_REG_OFFSET; >>>> + } >>> Why not move this code piece into kvm_lapic_readable_reg_mask() and >>> directly use APIC_REG_MASK() for these extended regs? Then we don't need to >>> modify the below code. > > I still think we should get a unified APIC reg mask even for the extended > APIC with kvm_lapic_readable_reg_mask() helper. We can extend current > kvm_lapic_readable_reg_mask() and let it return a 128 bits bitmap, maybe > like this, > > void kvm_lapic_readable_reg_mask(struct kvm_lapic *apic, u64 *mask) > > This makes code more easily maintain. > > Sorry for the delay. The reason why I am wary of this approach is because kvm_lapic_readable_reg_mask() is currently being used in vmx_update_msr_bitmap_x2apic(), where we directly use its return value: if (mode & MSR_BITMAP_MODE_X2APIC_APICV) msr_bitmap[read_idx] = ~kvm_lapic_readable_reg_mask(vcpu->arch.apic); else msr_bitmap[read_idx] = ~0ull; msr_bitmap[write_idx] = ~0ull; Where msr_bitmap is a u64 array. Changing kvm_lapic_readable_reg_mask() to return a 128-bit mask would require changes in vmx_update_msr_bitmap_x2apic() too. - Manali >>> >>
