| 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
| ||
|
Message-ID: <20240305083524.GE10568@ls.amr.corp.intel.com>
Date: Tue, 5 Mar 2024 00:35:24 -0800
From: Isaku Yamahata <isaku.yamahata@...ux.intel.com>
To: Binbin Wu <binbin.wu@...ux.intel.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
Sean Christopherson <seanjc@...gle.com>,
Sagi Shahar <sagis@...gle.com>, Kai Huang <kai.huang@...el.com>,
chen.bo@...el.com, hang.yuan@...el.com, tina.zhang@...el.com,
isaku.yamahata@...ux.intel.com
Subject: Re: [PATCH v19 091/130] KVM: TDX: remove use of struct vcpu_vmx from
posted_interrupt.c
On Tue, Feb 27, 2024 at 04:52:01PM +0800,
Binbin Wu <binbin.wu@...ux.intel.com> wrote:
>
>
> On 2/26/2024 4:26 PM, isaku.yamahata@...el.com wrote:
> > From: Isaku Yamahata <isaku.yamahata@...el.com>
> >
> > As TDX will use posted_interrupt.c, the use of struct vcpu_vmx is a
> > blocker. Because the members of
>
> Extra "of"
>
> > struct pi_desc pi_desc and struct
> > list_head pi_wakeup_list are only used in posted_interrupt.c, introduce
> > common structure, struct vcpu_pi, make vcpu_vmx and vcpu_tdx has same
> > layout in the top of structure.
> >
> > To minimize the diff size, avoid code conversion like,
> > vmx->pi_desc => vmx->common->pi_desc. Instead add compile time check
> > if the layout is expected.
> >
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > ---
> > arch/x86/kvm/vmx/posted_intr.c | 41 ++++++++++++++++++++++++++--------
> > arch/x86/kvm/vmx/posted_intr.h | 11 +++++++++
> > arch/x86/kvm/vmx/tdx.c | 1 +
> > arch/x86/kvm/vmx/tdx.h | 8 +++++++
> > arch/x86/kvm/vmx/vmx.h | 14 +++++++-----
> > 5 files changed, 60 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c
> > index af662312fd07..b66add9da0f3 100644
> > --- a/arch/x86/kvm/vmx/posted_intr.c
> > +++ b/arch/x86/kvm/vmx/posted_intr.c
> > @@ -11,6 +11,7 @@
> > #include "posted_intr.h"
> > #include "trace.h"
> > #include "vmx.h"
> > +#include "tdx.h"
> > /*
> > * Maintain a per-CPU list of vCPUs that need to be awakened by wakeup_handler()
> > @@ -31,9 +32,29 @@ static DEFINE_PER_CPU(struct list_head, wakeup_vcpus_on_cpu);
> > */
> > static DEFINE_PER_CPU(raw_spinlock_t, wakeup_vcpus_on_cpu_lock);
> > +/*
> > + * The layout of the head of struct vcpu_vmx and struct vcpu_tdx must match with
> > + * struct vcpu_pi.
> > + */
> > +static_assert(offsetof(struct vcpu_pi, pi_desc) ==
> > + offsetof(struct vcpu_vmx, pi_desc));
> > +static_assert(offsetof(struct vcpu_pi, pi_wakeup_list) ==
> > + offsetof(struct vcpu_vmx, pi_wakeup_list));
> > +#ifdef CONFIG_INTEL_TDX_HOST
> > +static_assert(offsetof(struct vcpu_pi, pi_desc) ==
> > + offsetof(struct vcpu_tdx, pi_desc));
> > +static_assert(offsetof(struct vcpu_pi, pi_wakeup_list) ==
> > + offsetof(struct vcpu_tdx, pi_wakeup_list));
> > +#endif
> > +
> > +static inline struct vcpu_pi *vcpu_to_pi(struct kvm_vcpu *vcpu)
> > +{
> > + return (struct vcpu_pi *)vcpu;
> > +}
> > +
> > static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu)
> > {
> > - return &(to_vmx(vcpu)->pi_desc);
> > + return &vcpu_to_pi(vcpu)->pi_desc;
> > }
> > static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)
> > @@ -52,8 +73,8 @@ static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)
> > void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
> > {
> > - struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > - struct vcpu_vmx *vmx = to_vmx(vcpu);
> > + struct vcpu_pi *vcpu_pi = vcpu_to_pi(vcpu);
> > + struct pi_desc *pi_desc = &vcpu_pi->pi_desc;
> > struct pi_desc old, new;
> > unsigned long flags;
> > unsigned int dest;
> > @@ -90,7 +111,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
> > */
> > if (pi_desc->nv == POSTED_INTR_WAKEUP_VECTOR) {
> > raw_spin_lock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
> > - list_del(&vmx->pi_wakeup_list);
> > + list_del(&vcpu_pi->pi_wakeup_list);
> > raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
> > }
> > @@ -145,15 +166,15 @@ static bool vmx_can_use_vtd_pi(struct kvm *kvm)
> > */
> > static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu)
> > {
> > - struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > - struct vcpu_vmx *vmx = to_vmx(vcpu);
> > + struct vcpu_pi *vcpu_pi = vcpu_to_pi(vcpu);
> > + struct pi_desc *pi_desc = &vcpu_pi->pi_desc;
> > struct pi_desc old, new;
> > unsigned long flags;
> > local_irq_save(flags);
> > raw_spin_lock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
> > - list_add_tail(&vmx->pi_wakeup_list,
> > + list_add_tail(&vcpu_pi->pi_wakeup_list,
> > &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu));
> > raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
> > @@ -190,7 +211,8 @@ static bool vmx_needs_pi_wakeup(struct kvm_vcpu *vcpu)
> > * notification vector is switched to the one that calls
> > * back to the pi_wakeup_handler() function.
> > */
> > - return vmx_can_use_ipiv(vcpu) || vmx_can_use_vtd_pi(vcpu->kvm);
> > + return (vmx_can_use_ipiv(vcpu) && !is_td_vcpu(vcpu)) ||
> > + vmx_can_use_vtd_pi(vcpu->kvm);
> > }
> > void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
> > @@ -200,7 +222,8 @@ void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
> > if (!vmx_needs_pi_wakeup(vcpu))
> > return;
> > - if (kvm_vcpu_is_blocking(vcpu) && !vmx_interrupt_blocked(vcpu))
> > + if (kvm_vcpu_is_blocking(vcpu) &&
> > + (is_td_vcpu(vcpu) || !vmx_interrupt_blocked(vcpu)))
> > pi_enable_wakeup_handler(vcpu);
> > /*
> > diff --git a/arch/x86/kvm/vmx/posted_intr.h b/arch/x86/kvm/vmx/posted_intr.h
> > index 26992076552e..2fe8222308b2 100644
> > --- a/arch/x86/kvm/vmx/posted_intr.h
> > +++ b/arch/x86/kvm/vmx/posted_intr.h
> > @@ -94,6 +94,17 @@ static inline bool pi_test_sn(struct pi_desc *pi_desc)
> > (unsigned long *)&pi_desc->control);
> > }
> > +struct vcpu_pi {
> > + struct kvm_vcpu vcpu;
> > +
> > + /* Posted interrupt descriptor */
> > + struct pi_desc pi_desc;
> > +
> > + /* Used if this vCPU is waiting for PI notification wakeup. */
> > + struct list_head pi_wakeup_list;
> > + /* Until here common layout betwwn vcpu_vmx and vcpu_tdx. */
>
> s/betwwn/between
>
> Also, in pi_wakeup_handler(), it is still using struct vcpu_vmx, but it
> could
> be vcpu_tdx.
> Functionally it is OK, however, since you have added vcpu_pi, should it use
> vcpu_pi instead of vcpu_vmx in pi_wakeup_handler()?
Makes sense.
diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c
index b66add9da0f3..5b71aef931dc 100644
--- a/arch/x86/kvm/vmx/posted_intr.c
+++ b/arch/x86/kvm/vmx/posted_intr.c
@@ -243,13 +243,13 @@ void pi_wakeup_handler(void)
int cpu = smp_processor_id();
struct list_head *wakeup_list = &per_cpu(wakeup_vcpus_on_cpu, cpu);
raw_spinlock_t *spinlock = &per_cpu(wakeup_vcpus_on_cpu_lock, cpu);
- struct vcpu_vmx *vmx;
+ struct vcpu_pi *pi;
raw_spin_lock(spinlock);
- list_for_each_entry(vmx, wakeup_list, pi_wakeup_list) {
+ list_for_each_entry(pi, wakeup_list, pi_wakeup_list) {
- if (pi_test_on(&vmx->pi_desc))
- kvm_vcpu_wake_up(&vmx->vcpu);
+ if (pi_test_on(&pi->pi_desc))
+ kvm_vcpu_wake_up(&pi->vcpu);
}
--
Isaku Yamahata <isaku.yamahata@...ux.intel.com>
Powered by blists - more mailing lists