Re: [PATCH v1 1/9] KVM: x86: Refactor APIC register mask handling to support extended APIC registers

[Manali Shukla <manali.shukla@amd.com>]

Hi Naveen,

Thank you for reviewing my patches.

On 5/14/2026 6:18 PM, Naveen N Rao wrote:
> On Wed, Feb 04, 2026 at 07:44:44AM +0000, Manali Shukla wrote:
>> Expand the APIC register mask infrastructure from a single u64 to
>> a u64[2] array to accommodate both standard APIC registers (0x0-0x3f0)
>> and extended APIC registers (0x400-0x530).  Current AMD processors
>> support four extended LVT registers; future processors may support up to
>> 255.
>>
>> The existing single 64-bit mask can track at most 64 registers (with 16-
>> byte granularity), which is insufficient for the extended APIC register
>> space.  A 128-bit mask (u64[2]) provides coverage for both standard and
>> extended register ranges.
>>
>> Convert the bitmask infrastructure to use kernel bitmap APIs.
>> APIC_REG_MASK now sets bits in the u64[2] array via bitmap_set(),
>> APIC_REG_TEST checks register validity via test_bit(), and
>> kvm_lapic_readable_reg_mask() populates the expanded mask as an output
>> parameter instead of a return value.
>>
>> Update all callers, including vmx_update_msr_bitmap_x2apic() in VMX, to
>> pass and utilize the new two-element mask array. Currently, extended
>> APIC registers are only supported for AMD processors.
>>
>> No functional change intended; extended APIC register emulation will be
>> added in a subsequent patch.
>>
>> Suggested-by: Naveen N Rao (AMD) <naveen@kernel.org>
>> Suggested-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
>> ---
>>  arch/x86/kvm/lapic.c   | 96 ++++++++++++++++++++++++++----------------
>>  arch/x86/kvm/lapic.h   |  2 +-
>>  arch/x86/kvm/vmx/vmx.c | 10 +++--
>>  3 files changed, 67 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 2e513f1c8988..66819397e073 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1664,53 +1664,74 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
>>  	return container_of(dev, struct kvm_lapic, dev);
>>  }
>>  
>> -#define APIC_REG_MASK(reg)	(1ull << ((reg) >> 4))
>> -#define APIC_REGS_MASK(first, count) \
>> -	(APIC_REG_MASK(first) * ((1ull << (count)) - 1))
>> -
>> -u64 kvm_lapic_readable_reg_mask(struct kvm_lapic *apic)
>> -{
>> -	/* Leave bits '0' for reserved and write-only registers. */
>> -	u64 valid_reg_mask =
>> -		APIC_REG_MASK(APIC_ID) |
>> -		APIC_REG_MASK(APIC_LVR) |
>> -		APIC_REG_MASK(APIC_TASKPRI) |
>> -		APIC_REG_MASK(APIC_PROCPRI) |
>> -		APIC_REG_MASK(APIC_LDR) |
>> -		APIC_REG_MASK(APIC_SPIV) |
>> -		APIC_REGS_MASK(APIC_ISR, APIC_ISR_NR) |
>> -		APIC_REGS_MASK(APIC_TMR, APIC_ISR_NR) |
>> -		APIC_REGS_MASK(APIC_IRR, APIC_ISR_NR) |
>> -		APIC_REG_MASK(APIC_ESR) |
>> -		APIC_REG_MASK(APIC_ICR) |
>> -		APIC_REG_MASK(APIC_LVTT) |
>> -		APIC_REG_MASK(APIC_LVTTHMR) |
>> -		APIC_REG_MASK(APIC_LVTPC) |
>> -		APIC_REG_MASK(APIC_LVT0) |
>> -		APIC_REG_MASK(APIC_LVT1) |
>> -		APIC_REG_MASK(APIC_LVTERR) |
>> -		APIC_REG_MASK(APIC_TMICT) |
>> -		APIC_REG_MASK(APIC_TMCCT) |
>> -		APIC_REG_MASK(APIC_TDCR);
>> +/*
>> + * Helper macros for APIC register bitmask handling
>> + * 2 element array is being used to represent 128-bit mask, where:
>> + * - mask[0] tracks standard APIC registers
>> + * - mask[1] tracks extended APIC registers
>> + */
>> +
>> +#define APIC_REG_BITMAP_WORDS   2  /* 2 x 64-bit words = 128 bits */
>> +#define APIC_REG_BITMAP_BITS    (APIC_REG_BITMAP_WORDS * BITS_PER_LONG)
> 
> Since we are going for the full 4k page with [G|S]ET_LAPIC2, it's 
> probably simpler to have this cover the full range.
> 

Sure. Makes sense to add the full 4k page.

>> +
>> +#define APIC_REG_TO_BIT(reg)		(((reg) < 0x400) ? ((reg) >> 4) : \
>> +					 (64 + (((reg) - 0x400) >> 4)))
>> +
>> +#define APIC_REG_MASK(reg, mask) \
>> +	bitmap_set((unsigned long *)(mask), APIC_REG_TO_BIT(reg), 1)
>> +
>> +#define APIC_REGS_MASK(first, count, mask) \
>> +	bitmap_set((unsigned long *)(mask), APIC_REG_TO_BIT(first), (count))
>> +
>> +#define APIC_REG_TEST(reg, mask) \
>> +	test_bit(APIC_REG_TO_BIT(reg), (unsigned long *)(mask))
>> +
>> +#define APIC_LAST_REG_OFFSET		0x3f0
>> +
>> +void kvm_lapic_readable_reg_mask(struct kvm_lapic *apic, u64 mask[2])
>> +{
>> +	bitmap_zero((unsigned long *)(mask), APIC_REG_BITMAP_BITS);
> 
> You can have this function take an unsigned long pointer, which will 
> allow much of this to be simplified.
> 
> Untested, but something like this?
> 
> 
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 6b6083aa50b6..552d416e5fc6 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -44,6 +44,9 @@ enum lapic_lvt_entry {
>  	KVM_APIC_MAX_NR_LVT_ENTRIES,
>  };
>  
> +#define APIC_REG_BITMAP_WORDS   	4
> +#define APIC_REG_BITMAP_BITS    	(APIC_REG_BITMAP_WORDS * BITS_PER_LONG)
> +
>  #define APIC_LVTx(x) ((x) == LVT_CMCI ? APIC_LVTCMCI : APIC_LVTT + 0x10 * (x))
>  
>  struct kvm_timer {
> @@ -157,7 +160,7 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
>  int kvm_lapic_set_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len);
>  void kvm_lapic_exit(void);
>  
> -void kvm_lapic_readable_reg_mask(struct kvm_lapic *apic, u64 mask[2]);
> +void kvm_lapic_readable_reg_mask(struct kvm_lapic *apic, unsigned long *mask);
>  
>  static inline void kvm_lapic_set_irr(int vec, struct kvm_lapic *apic)
>  {
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 5a0e8195206d..8f4b63c9807b 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1731,27 +1731,17 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
>   * - mask[1] tracks extended APIC registers
>   */
>  
> -#define APIC_REG_BITMAP_WORDS   2  /* 2 x 64-bit words = 128 bits */
> -#define APIC_REG_BITMAP_BITS    (APIC_REG_BITMAP_WORDS * BITS_PER_LONG)
> -
> -#define APIC_REG_TO_BIT(reg)		(((reg) < 0x400) ? ((reg) >> 4) : \
> -					 (64 + (((reg) - 0x400) >> 4)))
> -
> -#define APIC_REG_MASK(reg, mask) \
> -	bitmap_set((unsigned long *)(mask), APIC_REG_TO_BIT(reg), 1)
> -
> -#define APIC_REGS_MASK(first, count, mask) \
> -	bitmap_set((unsigned long *)(mask), APIC_REG_TO_BIT(first), (count))
> -
> -#define APIC_REG_TEST(reg, mask) \
> -	test_bit(APIC_REG_TO_BIT(reg), (unsigned long *)(mask))
> +#define APIC_REG_TO_BIT(reg)		((reg) >> 4)
> +#define APIC_REG_MASK(reg, mask)	bitmap_set(mask, APIC_REG_TO_BIT(reg), 1)
> +#define APIC_REGS_MASK(first, count, mask)	bitmap_set(mask, APIC_REG_TO_BIT(first), (count))
> +#define APIC_REG_TEST(reg, mask)	test_bit(APIC_REG_TO_BIT(reg), (unsigned long *)(mask))
>  
>  #define APIC_LAST_REG_OFFSET		0x3f0
>  #define APIC_EXT_LAST_REG_OFFSET(n)	APIC_EILVTn((n))
>  
> -void kvm_lapic_readable_reg_mask(struct kvm_lapic *apic, u64 mask[2])
> +void kvm_lapic_readable_reg_mask(struct kvm_lapic *apic, unsigned long *mask)
>  {
> -	bitmap_zero((unsigned long *)(mask), APIC_REG_BITMAP_BITS);
> +	bitmap_zero(mask, APIC_REG_BITMAP_BITS);
>  
>  	APIC_REG_MASK(APIC_ID, mask);
>  	APIC_REG_MASK(APIC_LVR, mask);
> @@ -1796,9 +1786,9 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
>  			      void *data)
>  {
>  	unsigned int last_reg = APIC_LAST_REG_OFFSET;
> +	DECLARE_BITMAP(mask, APIC_REG_BITMAP_BITS);
>  	unsigned char alignment = offset & 0xf;
>  	u32 result;
> -	u64 mask[2];
>  
>  	/*
>  	 * WARN if KVM reads ICR in x2APIC mode, as it's an 8-byte register in
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 063e56abac35..6915f1392dff 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4138,10 +4138,10 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
>  	 * mode, only the current timer count needs on-demand emulation by KVM.
>  	 */
>  	if (mode & MSR_BITMAP_MODE_X2APIC_APICV) {
> -		u64 mask[2];
> +		DECLARE_BITMAP(mask, APIC_REG_BITMAP_BITS);
>  
>  		kvm_lapic_readable_reg_mask(vcpu->arch.apic, mask);
> -		msr_bitmap[read_idx] = ~mask[0];
> +		msr_bitmap[read_idx] = ~bitmap_read(mask, 0, 64);
>  	} else {
>  		msr_bitmap[read_idx] = ~0ull;
>  	}
> 
> 
> - Naveen
> 

Ack.

-Manali