| Message ID | 20240126234237.547278-4-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Coalesced Interrupt Delivery with posted MSI | expand |
On Fri, Jan 26, 2024, Jacob Pan wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > Mixture of bitfields and types is weird and really not intuitive, remove > types and use bitfields exclusively. I agree it's weird, and maybe not immediately intuitive, but that doesn't mean there's no a good reason for the code being the way it is, i.e. "it's weird" isn't sufficient justification for touching this type of code. Bitfields almost always generate inferior code when accessing a subset of the overall thing. And even worse, there are subtle side effects that I really don't want to find out whether or not they are benign. E.g. before this change, setting the notification vector is: movb $0xf2,0x62(%rsp) whereas after this change it becomes: mov %eax,%edx and $0xff00fffd,%edx or $0xf20000,%edx mov %edx,0x60(%rsp) Writing extra bytes _shouln't_ be a problem, as KVM needs to atomically write the entire control chunk no matter what, but changing this without very good cause scares me. If we really want to clean things up, my very strong vote is to remove the bitfields entirely. SN is the only bit that's accessed without going through an accessor, and those should be easy enough to fixup one by one (and we can add more non-atomic accessors/mutators if it makes sense to do so). E.g. end up with /* Posted-Interrupt Descriptor */ struct pi_desc { u32 pir[8]; /* Posted interrupt requested */ union { struct { u16 notification_bits; u8 nv; u8 rsvd_2; u32 ndst; }; u64 control; }; u32 rsvd[6]; } __aligned(64);
Hi Sean, On Tue, 30 Jan 2024 17:48:04 -0800, Sean Christopherson <seanjc@google.com> wrote: > On Fri, Jan 26, 2024, Jacob Pan wrote: > > From: Thomas Gleixner <tglx@linutronix.de> > > > > Mixture of bitfields and types is weird and really not intuitive, remove > > types and use bitfields exclusively. > > I agree it's weird, and maybe not immediately intuitive, but that doesn't > mean there's no a good reason for the code being the way it is, i.e. > "it's weird" isn't sufficient justification for touching this type of > code. > > Bitfields almost always generate inferior code when accessing a subset of > the overall thing. And even worse, there are subtle side effects that I > really don't want to find out whether or not they are benign. > > E.g. before this change, setting the notification vector is: > > movb $0xf2,0x62(%rsp) > > whereas after this change it becomes: > > mov %eax,%edx > and $0xff00fffd,%edx > or $0xf20000,%edx > mov %edx,0x60(%rsp) > hmm, that is weird. However, my kernel build with the patch does not exhibit such code. I am getting the same as before for setting up NV: 112: 75 06 jne 11a <vmx_vcpu_pi_load+0xaa> ... 135: c6 44 24 22 f2 movb $0xf2,0x22(%rsp) However, I do agree having types is more robust, we can also use this_cpu_write() and friends if needed. > Writing extra bytes _shouln't_ be a problem, as KVM needs to atomically > write the entire control chunk no matter what, but changing this without > very good cause scares me. > > If we really want to clean things up, my very strong vote is to remove the > bitfields entirely. SN is the only bit that's accessed without going > through an accessor, and those should be easy enough to fixup one by one > (and we can add more non-atomic accessors/mutators if it makes sense to > do so). > > E.g. end up with > > /* Posted-Interrupt Descriptor */ > struct pi_desc { > u32 pir[8]; /* Posted interrupt requested */ > union { > struct { > u16 notification_bits; > u8 nv; > u8 rsvd_2; > u32 ndst; > }; > u64 control; > }; > u32 rsvd[6]; > } __aligned(64); Sounds good to me. Thanks, Jacob
diff --git a/arch/x86/include/asm/posted_intr.h b/arch/x86/include/asm/posted_intr.h index acf237b2882e..896b3462f3dd 100644 --- a/arch/x86/include/asm/posted_intr.h +++ b/arch/x86/include/asm/posted_intr.h @@ -16,17 +16,17 @@ struct pi_desc { union { struct { /* bit 256 - Outstanding Notification */ - u16 on : 1, + u64 on : 1, /* bit 257 - Suppress Notification */ - sn : 1, + sn : 1, /* bit 271:258 - Reserved */ - rsvd_1 : 14; + : 14, /* bit 279:272 - Notification Vector */ - u8 nv; + nv : 8, /* bit 287:280 - Reserved */ - u8 rsvd_2; + : 8, /* bit 319:288 - Notification Destination */ - u32 ndst; + ndst : 32; }; u64 control; };