Received-SPF: pass (google.com: domain of linux-kernel+bounces-54127-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 5 Feb 2024 16:40:35 -0800
From: Jacob Pan <jacob.jun.pan@linux.intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: LKML <linux-kernel@vger.kernel.org>, X86 Kernel <x86@kernel.org>, Peter
 Zijlstra <peterz@infradead.org>, iommu@lists.linux.dev, Thomas Gleixner
 <tglx@linutronix.de>, Lu Baolu <baolu.lu@linux.intel.com>,
 kvm@vger.kernel.org, Dave Hansen <dave.hansen@intel.com>, Joerg Roedel
 <joro@8bytes.org>, "H. Peter Anvin" <hpa@zytor.com>, Borislav Petkov
 <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>, Paul Luse
 <paul.e.luse@intel.com>, Dan Williams <dan.j.williams@intel.com>, Jens
 Axboe <axboe@kernel.dk>, Raj Ashok <ashok.raj@intel.com>, Kevin Tian
 <kevin.tian@intel.com>, maz@kernel.org, Robin Murphy
 <robin.murphy@arm.com>, jacob.jun.pan@linux.intel.com
Subject: Re: [PATCH 03/15] x86/irq: Use bitfields exclusively in posted
 interrupt descriptor
Message-ID: <20240205164035.79d10122@jacob-builder>
In-Reply-To: <Zbmm1DZPmbFm8gan@google.com>
References: <20240126234237.547278-1-jacob.jun.pan@linux.intel.com>
	<20240126234237.547278-4-jacob.jun.pan@linux.intel.com>
	<Zbmm1DZPmbFm8gan@google.com>
Organization: OTC
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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