Re: [PATCH 03/15] x86/irq: Use bitfields exclusively in posted interrupt descriptor

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

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