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

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]

Date: Mon, 5 Feb 2024 16:40:35 -0800
From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: LKML <linux-kernel@...r.kernel.org>, X86 Kernel <x86@...nel.org>, Peter
 Zijlstra <peterz@...radead.org>, iommu@...ts.linux.dev, Thomas Gleixner
 <tglx@...utronix.de>, Lu Baolu <baolu.lu@...ux.intel.com>,
 kvm@...r.kernel.org, Dave Hansen <dave.hansen@...el.com>, Joerg Roedel
 <joro@...tes.org>, "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov
 <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>, Paul Luse
 <paul.e.luse@...el.com>, Dan Williams <dan.j.williams@...el.com>, Jens
 Axboe <axboe@...nel.dk>, Raj Ashok <ashok.raj@...el.com>, Kevin Tian
 <kevin.tian@...el.com>, maz@...nel.org, Robin Murphy
 <robin.murphy@....com>, jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH 03/15] x86/irq: Use bitfields exclusively in posted
 interrupt descriptor

Hi Sean,

On Tue, 30 Jan 2024 17:48:04 -0800, Sean Christopherson <seanjc@...gle.com>
wrote:

> On Fri, Jan 26, 2024, Jacob Pan wrote:
> > From: Thomas Gleixner <tglx@...utronix.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