From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: [PATCH] efi/cper: Fix endianness of PCI class code Date: Thu, 25 May 2017 05:36:01 -0700 Message-ID: References: <771bc335fb5856792d086ae7db288dcf244cb4cd.1493964354.git.lukas@wunner.de> <20170506090755.GB19740@wunner.de> <20170510084143.GA16261@wunner.de> <20170525123047.GA4172@wunner.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20170525123047.GA4172-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Lukas Wunner Cc: Arnd Bergmann , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Ashok Raj , linux-pci , Huang Ying List-Id: linux-efi@vger.kernel.org On 25 May 2017 at 05:30, Lukas Wunner wrote: > 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. > I'm not talking about the 'packed' attribute but about the fact that the C spec does not guarantee that bitfields are projected onto byte locations in memory in the way you expect. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f43.google.com ([209.85.214.43]:37107 "EHLO mail-it0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759765AbdEYMgD (ORCPT ); Thu, 25 May 2017 08:36:03 -0400 Received: by mail-it0-f43.google.com with SMTP id g126so56485982ith.0 for ; Thu, 25 May 2017 05:36:02 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20170525123047.GA4172@wunner.de> References: <771bc335fb5856792d086ae7db288dcf244cb4cd.1493964354.git.lukas@wunner.de> <20170506090755.GB19740@wunner.de> <20170510084143.GA16261@wunner.de> <20170525123047.GA4172@wunner.de> From: Ard Biesheuvel Date: Thu, 25 May 2017 05:36:01 -0700 Message-ID: Subject: Re: [PATCH] efi/cper: Fix endianness of PCI class code To: Lukas Wunner Cc: Arnd Bergmann , "linux-efi@vger.kernel.org" , Ashok Raj , linux-pci , Huang Ying Content-Type: text/plain; charset="UTF-8" Sender: linux-pci-owner@vger.kernel.org List-ID: On 25 May 2017 at 05:30, Lukas Wunner wrote: > 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. > I'm not talking about the 'packed' attribute but about the fact that the C spec does not guarantee that bitfields are projected onto byte locations in memory in the way you expect.