Re: [PATCH v2 02/12] KVM: x86: Refactor APIC register mask handling to support extended APIC registers

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

Hi Naveen,

On 10/6/2025 9:42 PM, Naveen N Rao wrote:
> On Mon, Sep 01, 2025 at 10:51:18AM +0530, Manali Shukla wrote:
>> Modify the APIC register mask infrastructure to support both standard
>> APIC registers (0x0-0x3f0) and extended APIC registers (0x400-0x530).
>>
>> This refactoring:
>> - Replaces the single u64 bitmask with a u64[2] array to accommodate
>>   the extended register range(128 bitmask)
>> - Updates the APIC_REG_MASK macro to handle both standard and extended
>>   register spaces
>> - Adapts kvm_lapic_readable_reg_mask() to use the new approach
>> - Adds APIC_REG_TEST macro to check register validity for standard
>>   APIC registers and Exended APIC registers
>> - Updates all callers to use the new interface
>>
>> This is purely an infrastructure change to support the upcoming
>> extended APIC register emulation.
>>
>> Suggested-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> Signed-off-by: Manali Shukla <manali.shukla@amd.com>
>> ---
>>  arch/x86/kvm/lapic.c   | 99 ++++++++++++++++++++++++++----------------
>>  arch/x86/kvm/lapic.h   |  2 +-
>>  arch/x86/kvm/vmx/vmx.c | 10 +++--
>>  3 files changed, 70 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index e19545b8cc98..f92e3f53ee75 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1587,53 +1587,77 @@ 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 (0x0-0x3f0)
>> + * - mask[1] tracks extended APIC registers (0x400-0x530)
>> + */
>> +
>> +#define APIC_REG_INDEX(reg)	(((reg) < 0x400) ? 0 : 1)
>> +#define APIC_REG_BIT(reg)	(((reg) < 0x400) ? ((reg) >> 4) : (((reg) - 0x400) >> 4))
>> +
>> +/* Set a bit in the mask for a single APIC register. */
>> +#define APIC_REG_MASK(reg, mask) do { \
>> +	(mask)[APIC_REG_INDEX(reg)] |= (1ULL << APIC_REG_BIT(reg)); \
>> +} while (0)
>> +
>> +/* Set bits in the mask for a range of consecutive APIC registers. */
>> +#define APIC_REGS_MASK(first, count, mask) do { \
>> +	(mask)[APIC_REG_INDEX(first)] |= ((1ULL << (count)) - 1) << APIC_REG_BIT(first); \
>> +} while (0)
>> +
>> +/* Macro to check whether the an APIC register bit is set in the mask. */
>> +#define APIC_REG_TEST(reg, mask) \
>> +	((mask)[APIC_REG_INDEX(reg)] & (1ULL << APIC_REG_BIT(reg)))
>> +
>> +#define APIC_LAST_REG_OFFSET		0x3f0
>> +#define APIC_EXT_LAST_REG_OFFSET	0x530
>> +
>> +void kvm_lapic_readable_reg_mask(struct kvm_lapic *apic, u64 mask[2])
>> +{
>> +	mask[0] = 0;
>> +	mask[1] = 0;
> 
> Would it be simpler to use a bitmap for the mask?
> 
> 
> - Naveen
> 

Thanks for the suggestion. I'll revisit this code and explore using
the bitmap APIs from lib/bitmap.c and include/linux/bitmap.h. I'll
include any improvements in v3.

-Manali