Re: [PATCH v1 04/11] KVM: x86: Add emulation support for Extented LVT registers

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 8/1/2025 5:33 PM, Manali Shukla wrote:
> 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@amd.com>
>>>>>
>>>>> 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@amd.com>
>>>>> Co-developed-by: Manali Shukla <manali.shukla@amd.com>
>>>>> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
>>>>> ---
>>>>>  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.

Yes, I know. IMO, it's worth to do it. 


>
> - Manali
>