Re: [RFC PATCH v6 07/32] KVM: x86: apic_test_vector() to common code

[Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>]

On 6/7/2025 12:17 AM, Sean Christopherson wrote:
...

> 
>  1. Rename VEC_POS/REG_POS => APIC_VECTOR_TO_BIT_NUMBER/APIC_VECTOR_TO_REG_OFFSET
>  2. Rename all of the KVM helpers you intend to move out of KVM.
>  3. Move all of the helpers out of KVM.
> 
> That way #1 and #2 are pure KVM changes, and the code review movement is easy to
> review because it'll be _just_ code movement.
> 

Thanks for providing guidance on patch structuring! I am working on this.


> Actually (redux), we should probably kill off __apic_test_and_set_vector() and
> __apic_test_and_clear_vector(), because the _only_ register that's safe to modify
> with a non-atomic operation is ISR, because KVM isn't running the vCPU, i.e.
> hardware can't service an IRQ or process an EOI for the relevant (virtual) APIC.
> 
> So this on top somewhere? (completely untested)
> 

Ok, makes sense. I will include this.


- Neeraj

> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 8ecc3e960121..95921e5c3eb2 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -104,16 +104,6 @@ bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector)
>                 apic_test_vector(vector, apic->regs + APIC_IRR);
>  }
>  
> -static inline int __apic_test_and_set_vector(int vec, void *bitmap)
> -{
> -       return __test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> -}
> -
> -static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
> -{
> -       return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
> -}
> -
>  __read_mostly DEFINE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
>  EXPORT_SYMBOL_GPL(kvm_has_noapic_vcpu);
>  
> @@ -706,9 +696,15 @@ void kvm_apic_clear_irr(struct kvm_vcpu *vcpu, int vec)
>  }
>  EXPORT_SYMBOL_GPL(kvm_apic_clear_irr);
>  
> +static void *apic_vector_to_isr(int vec, struct kvm_lapic *apic)
> +{
> +       return apic->regs + APIC_ISR + APIC_VECTOR_TO_REG_OFFSET(vec);
> +}
> +
>  static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
>  {
> -       if (__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
> +       if (__test_and_set_bit(APIC_VECTOR_TO_BIT_NUMBER(vec),
> +                              apic_vector_to_isr(vec, apic)))
>                 return;
>  
>         /*
> @@ -751,7 +747,8 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
>  
>  static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
>  {
> -       if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
> +       if (!__test_and_clear_bit(APIC_VECTOR_TO_BIT_NUMBER(vec),
> +                                 apic_vector_to_isr(vec, apic)))
>                 return;
>  
>         /*
>