linux-kernel - Re: [RFC PATCH v6 07/32] KVM: x86: apic_test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <515a52f5-c7a9-44e3-b33b-dfb9939de099@amd.com>
Date: Tue, 10 Jun 2025 09:55:08 +0530
From: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Borislav Petkov <bp@...en8.de>, linux-kernel@...r.kernel.org,
 tglx@...utronix.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
 Thomas.Lendacky@....com, nikunj@....com, Santosh.Shukla@....com,
 Vasant.Hegde@....com, Suravee.Suthikulpanit@....com, David.Kaplan@....com,
 x86@...nel.org, hpa@...or.com, peterz@...radead.org, pbonzini@...hat.com,
 kvm@...r.kernel.org, kirill.shutemov@...ux.intel.com, huibo.wang@....com,
 naveen.rao@....com, francescolavra.fl@...il.com, tiala@...rosoft.com
Subject: Re: [RFC PATCH v6 07/32] KVM: x86: apic_test_vector() to common code



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;
>  
>         /*
>