From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukas Wunner Subject: Re: [PATCH] efi/cper: Fix endianness of PCI class code Date: Thu, 25 May 2017 14:30:47 +0200 Message-ID: <20170525123047.GA4172@wunner.de> References: <771bc335fb5856792d086ae7db288dcf244cb4cd.1493964354.git.lukas@wunner.de> <20170506090755.GB19740@wunner.de> <20170510084143.GA16261@wunner.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ard Biesheuvel Cc: Arnd Bergmann , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Ashok Raj , linux-pci , Huang Ying List-Id: linux-efi@vger.kernel.org On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: > On 10 May 2017 at 09:41, Lukas Wunner wrote: > > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: > >> On 6 May 2017 at 10:07, Lukas Wunner wrote: > >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote: > >> >> > The CPER parser assumes that the class code is big endian, but at least > >> >> > on this edk2-derived Intel Purley platform it's little endian: > >> > [snip] > >> >> > --- a/include/linux/cper.h > >> >> > +++ b/include/linux/cper.h > >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie { > >> >> > struct { > >> >> > __u16 vendor_id; > >> >> > __u16 device_id; > >> >> > - __u8 class_code[3]; > >> >> > + __u32 class_code:24; > >> >> > >> >> I'd like to avoid this change if we can. Couldn't we simply invert the > >> >> order of p[] above? > >> > > >> > Hm, why would you like to avoid it? > >> > >> Because we shouldn't use bitfields in structs in code that should be > >> portable across archs with different endiannesses. > > > > The CPER header is defined in the UEFI spec and UEFI mandates that the > > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6). > > > > No it does not mandate that at all. It mandates how the core should be > configured when running in UEFI, but the OS can do anything it likes. > > We are still interested in adding limited UEFI support to big endian > arm64 in the future (i.e., access to a limited set of firmware tables > but no runtime services), and I am not going to merge anything that > moves us away from that goal. > > > So your argument seems moot to me. Am I missing something? Do you > > have another argument? > > > > Moreover, the vendor_id and device_id fields are little endian as well > > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER parser in > > drivers/firmware/efi/cper.c to convert them to the endianness of the host. > > > > Indeed. I am aware we will need to add various endian-neutral > accessors in the future. > > >> > The class_code element isn't > >> > referenced anywhere else in the kernel and this isn't a uapi header, > >> > so the change would only impact out-of-tree drivers. Not sure if > >> > any exist which might be interested in CPER parsing. > >> > > >> > >> The point is that the change in the struct definition is simply not > >> necessary, given that inverting the order of p[] already achieves > >> exactly what we want. > > > > It seems clumsy and unnecessary to me so I'd prefer the bitfield. > > Please excuse my stubbornness. > > > > Stubbornness alone is not going to convince me. What *could* convince > me (although unlikely) is a quote from the C spec which explains why > it is 100% legal to make assumptions about how bitfields are projected > onto byte locations in memory. All structs in cper.h are declared "packed", so what you're asking for isn't defined in the C spec but in the GCC documentation: "The packed attribute specifies that a variable or structure field should have the smallest possible alignment -- one byte for a variable, and one bit for a field, unless you specify a larger value with the aligned attribute." So I maintain that the patch is fine, but you'll need to use le32_to_cpu(), le16_to_cpu() etc both for the class_code changed by the patch as well as all the other members of the struct not touched by the patch when adding "endianness mixed mode" for aarch64. Thanks, Lukas From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout3.hostsharing.net ([176.9.242.54]:41807 "EHLO mailout3.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756740AbdEYMaa (ORCPT ); Thu, 25 May 2017 08:30:30 -0400 Date: Thu, 25 May 2017 14:30:47 +0200 From: Lukas Wunner To: Ard Biesheuvel Cc: Arnd Bergmann , "linux-efi@vger.kernel.org" , Ashok Raj , linux-pci , Huang Ying Subject: Re: [PATCH] efi/cper: Fix endianness of PCI class code Message-ID: <20170525123047.GA4172@wunner.de> References: <771bc335fb5856792d086ae7db288dcf244cb4cd.1493964354.git.lukas@wunner.de> <20170506090755.GB19740@wunner.de> <20170510084143.GA16261@wunner.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-pci-owner@vger.kernel.org List-ID: On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: > On 10 May 2017 at 09:41, Lukas Wunner wrote: > > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: > >> On 6 May 2017 at 10:07, Lukas Wunner wrote: > >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > >> >> On 5 May 2017 at 19:38, Lukas Wunner wrote: > >> >> > The CPER parser assumes that the class code is big endian, but at least > >> >> > on this edk2-derived Intel Purley platform it's little endian: > >> > [snip] > >> >> > --- a/include/linux/cper.h > >> >> > +++ b/include/linux/cper.h > >> >> > @@ -416,7 +416,7 @@ struct cper_sec_pcie { > >> >> > struct { > >> >> > __u16 vendor_id; > >> >> > __u16 device_id; > >> >> > - __u8 class_code[3]; > >> >> > + __u32 class_code:24; > >> >> > >> >> I'd like to avoid this change if we can. Couldn't we simply invert the > >> >> order of p[] above? > >> > > >> > Hm, why would you like to avoid it? > >> > >> Because we shouldn't use bitfields in structs in code that should be > >> portable across archs with different endiannesses. > > > > The CPER header is defined in the UEFI spec and UEFI mandates that the > > arch is little endian (UEFI r2.6, sec. 2.3.5, 2.3.6). > > > > No it does not mandate that at all. It mandates how the core should be > configured when running in UEFI, but the OS can do anything it likes. > > We are still interested in adding limited UEFI support to big endian > arm64 in the future (i.e., access to a limited set of firmware tables > but no runtime services), and I am not going to merge anything that > moves us away from that goal. > > > So your argument seems moot to me. Am I missing something? Do you > > have another argument? > > > > Moreover, the vendor_id and device_id fields are little endian as well > > (PCI r3.0, sec. 6.1), yet there are no provisions in our CPER parser in > > drivers/firmware/efi/cper.c to convert them to the endianness of the host. > > > > Indeed. I am aware we will need to add various endian-neutral > accessors in the future. > > >> > The class_code element isn't > >> > referenced anywhere else in the kernel and this isn't a uapi header, > >> > so the change would only impact out-of-tree drivers. Not sure if > >> > any exist which might be interested in CPER parsing. > >> > > >> > >> The point is that the change in the struct definition is simply not > >> necessary, given that inverting the order of p[] already achieves > >> exactly what we want. > > > > It seems clumsy and unnecessary to me so I'd prefer the bitfield. > > Please excuse my stubbornness. > > > > Stubbornness alone is not going to convince me. What *could* convince > me (although unlikely) is a quote from the C spec which explains why > it is 100% legal to make assumptions about how bitfields are projected > onto byte locations in memory. All structs in cper.h are declared "packed", so what you're asking for isn't defined in the C spec but in the GCC documentation: "The packed attribute specifies that a variable or structure field should have the smallest possible alignment -- one byte for a variable, and one bit for a field, unless you specify a larger value with the aligned attribute." So I maintain that the patch is fine, but you'll need to use le32_to_cpu(), le16_to_cpu() etc both for the class_code changed by the patch as well as all the other members of the struct not touched by the patch when adding "endianness mixed mode" for aarch64. Thanks, Lukas