| Message ID | 20240405223110.1609888-4-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Coalesced Interrupt Delivery with posted MSI | expand |
"KVM" here would be nice too. On Fri, Apr 05, 2024, Jacob Pan wrote: > Mixture of bitfields and types is weird and really not intuitive, remove > bitfields and use typed data exclusively. > > Link: https://lore.kernel.org/all/20240404101735.402feec8@jacob-builder/T/#mf66e34a82a48f4d8e2926b5581eff59a122de53a > Suggested-by: Sean Christopherson <seanjc@google.com> > Suggested-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > > --- > v2: > - Replace bitfields, no more mix. > --- > arch/x86/include/asm/posted_intr.h | 10 +--------- > arch/x86/kvm/vmx/posted_intr.c | 4 ++-- > arch/x86/kvm/vmx/vmx.c | 2 +- > 3 files changed, 4 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/include/asm/posted_intr.h b/arch/x86/include/asm/posted_intr.h > index acf237b2882e..c682c41d4e44 100644 > --- a/arch/x86/include/asm/posted_intr.h > +++ b/arch/x86/include/asm/posted_intr.h > @@ -15,17 +15,9 @@ struct pi_desc { > }; > union { > struct { > - /* bit 256 - Outstanding Notification */ > - u16 on : 1, > - /* bit 257 - Suppress Notification */ > - sn : 1, > - /* bit 271:258 - Reserved */ > - rsvd_1 : 14; > - /* bit 279:272 - Notification Vector */ > + u16 notif_ctrl; /* Suppress and outstanding bits */ > u8 nv; > - /* bit 287:280 - Reserved */ > u8 rsvd_2; > - /* bit 319:288 - Notification Destination */ > u32 ndst; > }; > u64 control; > diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c > index af662312fd07..592dbb765675 100644 > --- a/arch/x86/kvm/vmx/posted_intr.c > +++ b/arch/x86/kvm/vmx/posted_intr.c > @@ -107,7 +107,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) > * handle task migration (@cpu != vcpu->cpu). > */ > new.ndst = dest; > - new.sn = 0; > + new.notif_ctrl &= ~POSTED_INTR_SN; At the risk of creating confusing, would it make sense to add double-underscore, non-atomic versions of the set/clear helpers for ON and SN? I can't tell if that's a net positive versus open coding clear() and set() here and below. > /* > * Restore the notification vector; in the blocking case, the > @@ -157,7 +157,7 @@ static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu) > &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu)); > raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu)); > > - WARN(pi_desc->sn, "PI descriptor SN field set before blocking"); > + WARN(pi_desc->notif_ctrl & POSTED_INTR_SN, "PI descriptor SN field set before blocking"); This can use pi_test_sn(), as test_bit() isn't atomic, i.e. doesn't incur a LOCK. > > old.control = READ_ONCE(pi_desc->control); > do { > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index d94bb069bac9..50580bbfba5d 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -4843,7 +4843,7 @@ static void __vmx_vcpu_reset(struct kvm_vcpu *vcpu) > * or POSTED_INTR_WAKEUP_VECTOR. > */ > vmx->pi_desc.nv = POSTED_INTR_VECTOR; > - vmx->pi_desc.sn = 1; > + vmx->pi_desc.notif_ctrl |= POSTED_INTR_SN;
Hi Sean, On Tue, 16 Apr 2024 17:39:42 -0700, Sean Christopherson <seanjc@google.com> wrote: > "KVM" here would be nice too. > > On Fri, Apr 05, 2024, Jacob Pan wrote: > > Mixture of bitfields and types is weird and really not intuitive, remove > > bitfields and use typed data exclusively. > > > > Link: > > https://lore.kernel.org/all/20240404101735.402feec8@jacob-builder/T/#mf66e34a82a48f4d8e2926b5581eff59a122de53a > > Suggested-by: Sean Christopherson <seanjc@google.com> Suggested-by: > > Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jacob Pan > > <jacob.jun.pan@linux.intel.com> > > > > --- > > v2: > > - Replace bitfields, no more mix. > > --- > > arch/x86/include/asm/posted_intr.h | 10 +--------- > > arch/x86/kvm/vmx/posted_intr.c | 4 ++-- > > arch/x86/kvm/vmx/vmx.c | 2 +- > > 3 files changed, 4 insertions(+), 12 deletions(-) > > > > diff --git a/arch/x86/include/asm/posted_intr.h > > b/arch/x86/include/asm/posted_intr.h index acf237b2882e..c682c41d4e44 > > 100644 --- a/arch/x86/include/asm/posted_intr.h > > +++ b/arch/x86/include/asm/posted_intr.h > > @@ -15,17 +15,9 @@ struct pi_desc { > > }; > > union { > > struct { > > - /* bit 256 - Outstanding Notification > > */ > > - u16 on : 1, > > - /* bit 257 - Suppress Notification */ > > - sn : 1, > > - /* bit 271:258 - Reserved */ > > - rsvd_1 : 14; > > - /* bit 279:272 - Notification Vector */ > > + u16 notif_ctrl; /* Suppress and > > outstanding bits */ u8 nv; > > - /* bit 287:280 - Reserved */ > > u8 rsvd_2; > > - /* bit 319:288 - Notification > > Destination */ u32 ndst; > > }; > > u64 control; > > diff --git a/arch/x86/kvm/vmx/posted_intr.c > > b/arch/x86/kvm/vmx/posted_intr.c index af662312fd07..592dbb765675 100644 > > --- a/arch/x86/kvm/vmx/posted_intr.c > > +++ b/arch/x86/kvm/vmx/posted_intr.c > > @@ -107,7 +107,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int > > cpu) > > * handle task migration (@cpu != vcpu->cpu). > > */ > > new.ndst = dest; > > - new.sn = 0; > > + new.notif_ctrl &= ~POSTED_INTR_SN; > > At the risk of creating confusing, would it make sense to add > double-underscore, non-atomic versions of the set/clear helpers for ON > and SN? > > I can't tell if that's a net positive versus open coding clear() and > set() here and below. IMHO, we can add non-atomic helpers when we have more than one user for each operation. I do have a stupid bug here, it should be: - new.notif_ctrl &= ~POSTED_INTR_SN; + new.notif_ctrl &= ~BIT(POSTED_INTR_SN); Same as below. Thanks to Oliver(LKP kvm self test). I didn't catch that in my VFIO device assignment test. > > > /* > > * Restore the notification vector; in the blocking > > case, the @@ -157,7 +157,7 @@ static void > > pi_enable_wakeup_handler(struct kvm_vcpu *vcpu) > > &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu)); > > raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu)); > > - WARN(pi_desc->sn, "PI descriptor SN field set before > > blocking"); > > + WARN(pi_desc->notif_ctrl & POSTED_INTR_SN, "PI descriptor SN > > field set before blocking"); > > This can use pi_test_sn(), as test_bit() isn't atomic, i.e. doesn't incur > a LOCK. make sense. will do. > > > > old.control = READ_ONCE(pi_desc->control); > > do { > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index d94bb069bac9..50580bbfba5d 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -4843,7 +4843,7 @@ static void __vmx_vcpu_reset(struct kvm_vcpu > > *vcpu) > > * or POSTED_INTR_WAKEUP_VECTOR. > > */ > > vmx->pi_desc.nv = POSTED_INTR_VECTOR; > > - vmx->pi_desc.sn = 1; > > + vmx->pi_desc.notif_ctrl |= POSTED_INTR_SN; Thanks, Jacob
On Wed, Apr 17 2024 at 11:01, Jacob Pan wrote: > On Tue, 16 Apr 2024 17:39:42 -0700, Sean Christopherson <seanjc@google.com> > wrote: >> > diff --git a/arch/x86/kvm/vmx/posted_intr.c >> > b/arch/x86/kvm/vmx/posted_intr.c index af662312fd07..592dbb765675 100644 >> > --- a/arch/x86/kvm/vmx/posted_intr.c >> > +++ b/arch/x86/kvm/vmx/posted_intr.c >> > @@ -107,7 +107,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int >> > cpu) >> > * handle task migration (@cpu != vcpu->cpu). >> > */ >> > new.ndst = dest; >> > - new.sn = 0; >> > + new.notif_ctrl &= ~POSTED_INTR_SN; >> >> At the risk of creating confusing, would it make sense to add >> double-underscore, non-atomic versions of the set/clear helpers for ON >> and SN? >> >> I can't tell if that's a net positive versus open coding clear() and >> set() here and below. > IMHO, we can add non-atomic helpers when we have more than one user for > each operation. > > I do have a stupid bug here, it should be: > - new.notif_ctrl &= ~POSTED_INTR_SN; > + new.notif_ctrl &= ~BIT(POSTED_INTR_SN); That's a perfect reason to use a proper helper.
Hi Thomas, On Thu, 18 Apr 2024 19:30:52 +0200, Thomas Gleixner <tglx@linutronix.de> wrote: > On Wed, Apr 17 2024 at 11:01, Jacob Pan wrote: > > On Tue, 16 Apr 2024 17:39:42 -0700, Sean Christopherson > > <seanjc@google.com> wrote: > >> > diff --git a/arch/x86/kvm/vmx/posted_intr.c > >> > b/arch/x86/kvm/vmx/posted_intr.c index af662312fd07..592dbb765675 > >> > 100644 --- a/arch/x86/kvm/vmx/posted_intr.c > >> > +++ b/arch/x86/kvm/vmx/posted_intr.c > >> > @@ -107,7 +107,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int > >> > cpu) > >> > * handle task migration (@cpu != vcpu->cpu). > >> > */ > >> > new.ndst = dest; > >> > - new.sn = 0; > >> > + new.notif_ctrl &= ~POSTED_INTR_SN; > >> > >> At the risk of creating confusing, would it make sense to add > >> double-underscore, non-atomic versions of the set/clear helpers for ON > >> and SN? > >> > >> I can't tell if that's a net positive versus open coding clear() and > >> set() here and below. > > IMHO, we can add non-atomic helpers when we have more than one user for > > each operation. > > > > I do have a stupid bug here, it should be: > > - new.notif_ctrl &= ~POSTED_INTR_SN; > > + new.notif_ctrl &= ~BIT(POSTED_INTR_SN); > > That's a perfect reason to use a proper helper. I just proved I was wrong:) will do. Thanks, Jacob
diff --git a/arch/x86/include/asm/posted_intr.h b/arch/x86/include/asm/posted_intr.h index acf237b2882e..c682c41d4e44 100644 --- a/arch/x86/include/asm/posted_intr.h +++ b/arch/x86/include/asm/posted_intr.h @@ -15,17 +15,9 @@ struct pi_desc { }; union { struct { - /* bit 256 - Outstanding Notification */ - u16 on : 1, - /* bit 257 - Suppress Notification */ - sn : 1, - /* bit 271:258 - Reserved */ - rsvd_1 : 14; - /* bit 279:272 - Notification Vector */ + u16 notif_ctrl; /* Suppress and outstanding bits */ u8 nv; - /* bit 287:280 - Reserved */ u8 rsvd_2; - /* bit 319:288 - Notification Destination */ u32 ndst; }; u64 control; diff --git a/arch/x86/kvm/vmx/posted_intr.c b/arch/x86/kvm/vmx/posted_intr.c index af662312fd07..592dbb765675 100644 --- a/arch/x86/kvm/vmx/posted_intr.c +++ b/arch/x86/kvm/vmx/posted_intr.c @@ -107,7 +107,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu) * handle task migration (@cpu != vcpu->cpu). */ new.ndst = dest; - new.sn = 0; + new.notif_ctrl &= ~POSTED_INTR_SN; /* * Restore the notification vector; in the blocking case, the @@ -157,7 +157,7 @@ static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu) &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu)); raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu)); - WARN(pi_desc->sn, "PI descriptor SN field set before blocking"); + WARN(pi_desc->notif_ctrl & POSTED_INTR_SN, "PI descriptor SN field set before blocking"); old.control = READ_ONCE(pi_desc->control); do { diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index d94bb069bac9..50580bbfba5d 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -4843,7 +4843,7 @@ static void __vmx_vcpu_reset(struct kvm_vcpu *vcpu) * or POSTED_INTR_WAKEUP_VECTOR. */ vmx->pi_desc.nv = POSTED_INTR_VECTOR; - vmx->pi_desc.sn = 1; + vmx->pi_desc.notif_ctrl |= POSTED_INTR_SN; } static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
Mixture of bitfields and types is weird and really not intuitive, remove bitfields and use typed data exclusively. Link: https://lore.kernel.org/all/20240404101735.402feec8@jacob-builder/T/#mf66e34a82a48f4d8e2926b5581eff59a122de53a Suggested-by: Sean Christopherson <seanjc@google.com> Suggested-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> --- v2: - Replace bitfields, no more mix. --- arch/x86/include/asm/posted_intr.h | 10 +--------- arch/x86/kvm/vmx/posted_intr.c | 4 ++-- arch/x86/kvm/vmx/vmx.c | 2 +- 3 files changed, 4 insertions(+), 12 deletions(-)