| Message ID | 771bc335fb5856792d086ae7db288dcf244cb4cd.1493964354.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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: > > efi: EFI v2.50 by EDK II BIOS ID:PLYDCRB1.86B.0119.R05.1701181843 > DMI: Intel Corporation PURLEY/PURLEY, BIOS PLYDCRB1.86B.0119.R05.1701181843 01/18/2017 > > {1}[Hardware Error]: device_id: 0000:5d:00.0 > {1}[Hardware Error]: slot: 0 > {1}[Hardware Error]: secondary_bus: 0x5e > {1}[Hardware Error]: vendor_id: 0x8086, device_id: 0x2030 > {1}[Hardware Error]: class_code: 000406 > ^^^^^^ (should be 060400) > > Cc: Huang Ying <ying.huang@intel.com> > Cc: Ashok Raj <ashok.raj@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > > @Ashok Raj: Could you test if this patch results in the correct > PCI class being logged? Thanks! > > drivers/firmware/efi/cper.c | 5 ++--- > include/linux/cper.h | 2 +- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c > index d42537425438..e360c8b77bd9 100644 > --- a/drivers/firmware/efi/cper.c > +++ b/drivers/firmware/efi/cper.c > @@ -364,7 +364,6 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie, > printk("%s""command: 0x%04x, status: 0x%04x\n", pfx, > pcie->command, pcie->status); > if (pcie->validation_bits & CPER_PCIE_VALID_DEVICE_ID) { > - const __u8 *p; > printk("%s""device_id: %04x:%02x:%02x.%x\n", pfx, > pcie->device_id.segment, pcie->device_id.bus, > pcie->device_id.device, pcie->device_id.function); > @@ -374,8 +373,8 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie, > pcie->device_id.secondary_bus); > printk("%s""vendor_id: 0x%04x, device_id: 0x%04x\n", pfx, > pcie->device_id.vendor_id, pcie->device_id.device_id); > - p = pcie->device_id.class_code; > - printk("%s""class_code: %02x%02x%02x\n", pfx, p[0], p[1], p[2]); > + printk("%s""class_code: 0x%06x\n", pfx, > + pcie->device_id.class_code); > } > if (pcie->validation_bits & CPER_PCIE_VALID_SERIAL_NUMBER) > printk("%s""serial number: 0x%04x, 0x%04x\n", pfx, > diff --git a/include/linux/cper.h b/include/linux/cper.h > index dcacb1a72e26..fbfb50f52362 100644 > --- 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? > __u8 function; > __u8 device; > __u16 segment; > -- > 2.11.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-efi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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? 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. Thanks, Lukas
(+ Arnd) Full patch here: http://marc.info/?l=linux-pci&m=149400953408528 On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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 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.
On Wed, May 10, 2017 at 10:03 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: >> On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: >>> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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 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. Agreed on both, the code needs to remain endian-neutral. If it's currently wrong on all architectures, then it needs to be fixed on all architectures too. Arnd
On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: > On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: > > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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). 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. > > 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. Thanks, Lukas
On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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.
On 05/11/2017 07:06 AM, Ard Biesheuvel wrote: > On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: >> On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: [...] >> >> 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. I don't think you will find that in the C specifications. Structure layout is specified per-architecture/per-ABI so for the Linux kernel you would only have to check that it is strongly specified and compatible across `ls arch | wc -l` different architectures. If you want to get started with MIPS, for example, you could look in the "SYSTEM V APPLICATION BINARY INTERFACE MIPS(r) RISC Processor Supplement 3rd Edition" starting on page 3-7. For x86 and arm{,64} I am sure you could find similar specifications. David Daney
On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: > On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: > > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: > >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: > >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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
On 25 May 2017 at 05:30, Lukas Wunner <lukas@wunner.de> wrote: > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: >> On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: >> >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: >> >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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.
On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote: > On 25 May 2017 at 05:30, Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: > >> On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: > >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: > >> >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: > >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > >> >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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. What relevance does that have as long as the header file uses a pragma specific to gcc (or other compilers that are compatible to gcc with respect to that pragma (such as clang)), and gcc guarantees the correct layout regardless of endianness?
On 25 May 2017 at 05:44, Lukas Wunner <lukas@wunner.de> wrote: > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote: >> On 25 May 2017 at 05:30, Lukas Wunner <lukas@wunner.de> wrote: >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: >> >> On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: >> >> >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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. > > What relevance does that have as long as the header file uses a pragma > specific to gcc (or other compilers that are compatible to gcc with > respect to that pragma (such as clang)), and gcc guarantees the > correct layout regardless of endianness? The relevance is that we should not add GCC specific code because you think it looks prettier. And where does GCC guarantee the correct layout? Did you find an unambiguous GCC documentation reference that explains how bitfields are mapped onto byte locations? Or does 'guarantee' mean 'I tested it and it works'?
On Thu, May 25, 2017 at 05:47:59AM -0700, Ard Biesheuvel wrote: > On 25 May 2017 at 05:44, Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote: > >> On 25 May 2017 at 05:30, Lukas Wunner <lukas@wunner.de> wrote: > >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: > >> >> On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: > >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: > >> >> >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: > >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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. > > > > What relevance does that have as long as the header file uses a pragma > > specific to gcc (or other compilers that are compatible to gcc with > > respect to that pragma (such as clang)), and gcc guarantees the > > correct layout regardless of endianness? > > The relevance is that we should not add GCC specific code because you > think it looks prettier. The code already *is* gcc-specific. > And where does GCC guarantee the correct layout? Did you find an > unambiguous GCC documentation reference that explains how bitfields > are mapped onto byte locations? See the excerpt I quoted above. > Or does 'guarantee' mean 'I tested it and it works'? I tested it with x86_64 (le) and ppc32 (be) and it works. I don't have an aarch64 machine available here. Thanks, Lukas
On 25 May 2017 at 05:56, Lukas Wunner <lukas@wunner.de> wrote: > On Thu, May 25, 2017 at 05:47:59AM -0700, Ard Biesheuvel wrote: >> On 25 May 2017 at 05:44, Lukas Wunner <lukas@wunner.de> wrote: >> > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote: >> >> On 25 May 2017 at 05:30, Lukas Wunner <lukas@wunner.de> wrote: >> >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: >> >> >> On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: >> >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: >> >> >> >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: >> >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: >> >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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. >> > >> > What relevance does that have as long as the header file uses a pragma >> > specific to gcc (or other compilers that are compatible to gcc with >> > respect to that pragma (such as clang)), and gcc guarantees the >> > correct layout regardless of endianness? >> >> The relevance is that we should not add GCC specific code because you >> think it looks prettier. > > The code already *is* gcc-specific. > The entire kernel is GCC specific. But that does not justify adding more GCC-isms throughout the code. > >> And where does GCC guarantee the correct layout? Did you find an >> unambiguous GCC documentation reference that explains how bitfields >> are mapped onto byte locations? > > See the excerpt I quoted above. > 'packed' has nothing to do with it. This is about bitfields in structs. > >> Or does 'guarantee' mean 'I tested it and it works'? > > I tested it with x86_64 (le) and ppc32 (be) and it works. > I don't have an aarch64 machine available here. > Good.
On Thu, May 25, 2017 at 06:07:35AM -0700, Ard Biesheuvel wrote: > On 25 May 2017 at 05:56, Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, May 25, 2017 at 05:47:59AM -0700, Ard Biesheuvel wrote: > >> On 25 May 2017 at 05:44, Lukas Wunner <lukas@wunner.de> wrote: > >> > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote: > >> >> On 25 May 2017 at 05:30, Lukas Wunner <lukas@wunner.de> wrote: > >> >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: > >> >> >> On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: > >> >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: > >> >> >> >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: > >> >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > >> >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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. > >> > > >> > What relevance does that have as long as the header file uses a pragma > >> > specific to gcc (or other compilers that are compatible to gcc with > >> > respect to that pragma (such as clang)), and gcc guarantees the > >> > correct layout regardless of endianness? > >> > >> The relevance is that we should not add GCC specific code because you > >> think it looks prettier. > > > > The code already *is* gcc-specific. > > > > The entire kernel is GCC specific. But that does not justify adding > more GCC-isms throughout the code. How is the patch adding a GCC-ism? > >> And where does GCC guarantee the correct layout? Did you find an > >> unambiguous GCC documentation reference that explains how bitfields > >> are mapped onto byte locations? > > > > See the excerpt I quoted above. > > > > 'packed' has nothing to do with it. This is about bitfields in structs. 'packed' has *everything* to do with it. :-) The struct contains a *single* bitfield surrounded by non-bitfields. If there were multiple consecutive bitfields, then yes, things wouldn't be as clear. The bitfield as well as all surrounding non-bitfields have a size which is a multiple of full bytes. And this is where the 'packed' attribute comes into play, it guarantees that there's no padding as long as all members of the struct are byte-aligned. > > > > >> Or does 'guarantee' mean 'I tested it and it works'? > > > > I tested it with x86_64 (le) and ppc32 (be) and it works. > > I don't have an aarch64 machine available here. > > > > Good. Good to merge then? Thanks, Lukas
On Fri, May 26, 2017 at 8:08 AM, Lukas Wunner <lukas@wunner.de> wrote: > On Thu, May 25, 2017 at 06:07:35AM -0700, Ard Biesheuvel wrote: >> On 25 May 2017 at 05:56, Lukas Wunner <lukas@wunner.de> wrote: >> >> Or does 'guarantee' mean 'I tested it and it works'? >> > >> > I tested it with x86_64 (le) and ppc32 (be) and it works. >> > I don't have an aarch64 machine available here. >> > >> >> Good. > > Good to merge then? Please remove the bit fields and find a way to do the patch without them. We really don't want any bitfields in structures that are interpreted by hardware: The rules for struct packing are hard enough to understand without bitfields, so please don't give any bad examples even if you have proved from the C99 spec that your specific case is ok (which so far you have refused to do). Arnd
On 25 May 2017 at 23:08, Lukas Wunner <lukas@wunner.de> wrote: > On Thu, May 25, 2017 at 06:07:35AM -0700, Ard Biesheuvel wrote: >> On 25 May 2017 at 05:56, Lukas Wunner <lukas@wunner.de> wrote: >> > On Thu, May 25, 2017 at 05:47:59AM -0700, Ard Biesheuvel wrote: >> >> On 25 May 2017 at 05:44, Lukas Wunner <lukas@wunner.de> wrote: >> >> > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote: >> >> >> On 25 May 2017 at 05:30, Lukas Wunner <lukas@wunner.de> wrote: >> >> >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: >> >> >> >> On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: >> >> >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: >> >> >> >> >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: >> >> >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: >> >> >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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. >> >> > >> >> > What relevance does that have as long as the header file uses a pragma >> >> > specific to gcc (or other compilers that are compatible to gcc with >> >> > respect to that pragma (such as clang)), and gcc guarantees the >> >> > correct layout regardless of endianness? >> >> >> >> The relevance is that we should not add GCC specific code because you >> >> think it looks prettier. >> > >> > The code already *is* gcc-specific. >> > >> >> The entire kernel is GCC specific. But that does not justify adding >> more GCC-isms throughout the code. > > How is the patch adding a GCC-ism? > Because you rely on behavior which is not defined by the C spec. > >> >> And where does GCC guarantee the correct layout? Did you find an >> >> unambiguous GCC documentation reference that explains how bitfields >> >> are mapped onto byte locations? >> > >> > See the excerpt I quoted above. >> > >> >> 'packed' has nothing to do with it. This is about bitfields in structs. > > 'packed' has *everything* to do with it. :-) > > The struct contains a *single* bitfield surrounded by non-bitfields. > If there were multiple consecutive bitfields, then yes, things wouldn't > be as clear. > > The bitfield as well as all surrounding non-bitfields have a size which > is a multiple of full bytes. And this is where the 'packed' attribute > comes into play, it guarantees that there's no padding as long as all > members of the struct are byte-aligned. > No, it does not guarantee that at all. Observed behavior != guarantee. 'Guarantee' implies that it is documented in a pertinent spec, and that we can file a bug with the GCC/Clang projects if the behavior changes at any point. Nobody is asking you to theorize and make inferences about how attribute X and behavior Y offer guarantee Z. All it takes is an unambiguous quote from the C spec that describes how the struct definition is mapped onto bits in memory. You have offered no such quote, for which I don't blame you because I am convinced that the C spec does not define this in sufficient detail. > >> >> > >> >> Or does 'guarantee' mean 'I tested it and it works'? >> > >> > I tested it with x86_64 (le) and ppc32 (be) and it works. >> > I don't have an aarch64 machine available here. >> > >> >> Good. > > Good to merge then? > No. For the last time, this patch will not be merged. The only approach that will be merged is keeping the char[3] array and inverting the order of the printk() arguments.
On Fri, May 26, 2017 at 11:16:55AM +0200, Ard Biesheuvel wrote: > No. For the last time Alright, don't get mad at me. I was hoping that we could have a technical discussion on whether the proposed patch is technically correct, on all arches. You seem to dispute that, I maintain it. I'd like to learn something new and was hoping I'm not alone there. > On 25 May 2017 at 23:08, Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, May 25, 2017 at 06:07:35AM -0700, Ard Biesheuvel wrote: > >> On 25 May 2017 at 05:56, Lukas Wunner <lukas@wunner.de> wrote: > >> > On Thu, May 25, 2017 at 05:47:59AM -0700, Ard Biesheuvel wrote: > >> >> On 25 May 2017 at 05:44, Lukas Wunner <lukas@wunner.de> wrote: > >> >> > On Thu, May 25, 2017 at 05:36:01AM -0700, Ard Biesheuvel wrote: > >> >> >> On 25 May 2017 at 05:30, Lukas Wunner <lukas@wunner.de> wrote: > >> >> >> > On Thu, May 11, 2017 at 03:06:42PM +0100, Ard Biesheuvel wrote: > >> >> >> >> On 10 May 2017 at 09:41, Lukas Wunner <lukas@wunner.de> wrote: > >> >> >> >> > On Wed, May 10, 2017 at 09:03:11AM +0100, Ard Biesheuvel wrote: > >> >> >> >> >> On 6 May 2017 at 10:07, Lukas Wunner <lukas@wunner.de> wrote: > >> >> >> >> >> > On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote: > >> >> >> >> >> >> On 5 May 2017 at 19:38, Lukas Wunner <lukas@wunner.de> 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. > >> >> > > >> >> > What relevance does that have as long as the header file uses a pragma > >> >> > specific to gcc (or other compilers that are compatible to gcc with > >> >> > respect to that pragma (such as clang)), and gcc guarantees the > >> >> > correct layout regardless of endianness? > >> >> > >> >> The relevance is that we should not add GCC specific code because you > >> >> think it looks prettier. > >> > > >> > The code already *is* gcc-specific. > >> > >> The entire kernel is GCC specific. But that does not justify adding > >> more GCC-isms throughout the code. > > > > How is the patch adding a GCC-ism? > > > > Because you rely on behavior which is not defined by the C spec. I'm relying on the same behavior that the existing code relies on: That 'packed' guarantees no padding if all struct members have a size which is a multiple of full bytes. So I'm not *adding* a GCC-ism. > >> >> And where does GCC guarantee the correct layout? Did you find an > >> >> unambiguous GCC documentation reference that explains how bitfields > >> >> are mapped onto byte locations? > >> > > >> > See the excerpt I quoted above. > >> > > >> > >> 'packed' has nothing to do with it. This is about bitfields in structs. > > > > 'packed' has *everything* to do with it. :-) > > > > The struct contains a *single* bitfield surrounded by non-bitfields. > > If there were multiple consecutive bitfields, then yes, things wouldn't > > be as clear. > > > > The bitfield as well as all surrounding non-bitfields have a size which > > is a multiple of full bytes. And this is where the 'packed' attribute > > comes into play, it guarantees that there's no padding as long as all > > members of the struct are byte-aligned. > > > > No, it does not guarantee that at all. Observed behavior != guarantee. > 'Guarantee' implies that it is documented in a pertinent spec, and > that we can file a bug with the GCC/Clang projects if the behavior > changes at any point. I did quote the pertinent spec above, the source is: https://gcc.gnu.org/onlinedocs/gcc-7.1.0/gcc/Common-Variable-Attributes.html > Nobody is asking you to theorize and make inferences about how > attribute X and behavior Y offer guarantee Z. All it takes is an > unambiguous quote from the C spec that describes how the struct > definition is mapped onto bits in memory. You have offered no such > quote, for which I don't blame you because I am convinced that the C > spec does not define this in sufficient detail. I thought I did prove that, let me try again: C99 sec. 6.7.2.1: An implementation may allocate any addressable storage unit large enough to hold a bit-field. => So the bit-field could occupy more than 3 bytes. Why doesn't it do that? Because GCC's 'packed' attribute guarantees "the smallest possible alignment -- one byte for a variable, and one bit for a field". The existing declaration "__u8 class_code[3]" leverages that same guarantee. Further in 6.7.2.1: If enough space remains, a bit-field that immediately follows another bit-field in a structure shall be packed into adjacent bits of the same unit. If insufficient space remains, whether a bit-field that does not fit is put into the next unit or overlaps adjacent units is implementation-defined. The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined. => That's what I meant when I said it would be a different story if there were multiple consecutive bit-fields. But in this case there's just a single bit-field surrounded by non-bit-fields. Further in 6.7.2.1: Each non-bit-field member of a structure or union object is aligned in an implementation-defined manner appropriate to its type. => So the alignment of all the non-bit-field members is likewise solely enforced by gcc's 'packed' attribute. Further in 6.7.2.1: Within a structure object, the non-bit-field members and the units in which bit-fields reside have addresses that increase in the order in which they are declared. => This should answer your question how struct members are projected onto byte locations in memory. => Bottom line is that the behavior both of the existing code, as well as of the proposed patch, relies on the semantics of gcc's 'packed' attribute. Agree or disagree? > >> >> Or does 'guarantee' mean 'I tested it and it works'? > >> > > >> > I tested it with x86_64 (le) and ppc32 (be) and it works. > >> > I don't have an aarch64 machine available here. Forgot to mention, I tested with both clang and gcc. > > Good to merge then? > > this patch will not be merged. The only > approach that will be merged is keeping the char[3] array and > inverting the order of the printk() arguments. Fine by me. However I note that your original objection was based on the allegation that the patch would move you away from your goal of implementing endianness-mixed-mode on aarch64. After I've shown that it does not, you're unconditionally saying no. I respectfully submit that moving the goal posts like that may not be the best approach to maintainership. Thanks, Lukas
On Fri, May 26, 2017 at 10:01:28AM +0200, Arnd Bergmann wrote: > On Fri, May 26, 2017 at 8:08 AM, Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, May 25, 2017 at 06:07:35AM -0700, Ard Biesheuvel wrote: > >> On 25 May 2017 at 05:56, Lukas Wunner <lukas@wunner.de> wrote: > >> >> Or does 'guarantee' mean 'I tested it and it works'? > >> > > >> > I tested it with x86_64 (le) and ppc32 (be) and it works. > >> > I don't have an aarch64 machine available here. > >> > >> Good. > > > > Good to merge then? > > Please remove the bit fields and find a way to do the patch without them. > We really don't want any bitfields in structures that are interpreted by > hardware: This structure is not interpreted by hardware. Thanks, Lukas
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c index d42537425438..e360c8b77bd9 100644 --- a/drivers/firmware/efi/cper.c +++ b/drivers/firmware/efi/cper.c @@ -364,7 +364,6 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie, printk("%s""command: 0x%04x, status: 0x%04x\n", pfx, pcie->command, pcie->status); if (pcie->validation_bits & CPER_PCIE_VALID_DEVICE_ID) { - const __u8 *p; printk("%s""device_id: %04x:%02x:%02x.%x\n", pfx, pcie->device_id.segment, pcie->device_id.bus, pcie->device_id.device, pcie->device_id.function); @@ -374,8 +373,8 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie, pcie->device_id.secondary_bus); printk("%s""vendor_id: 0x%04x, device_id: 0x%04x\n", pfx, pcie->device_id.vendor_id, pcie->device_id.device_id); - p = pcie->device_id.class_code; - printk("%s""class_code: %02x%02x%02x\n", pfx, p[0], p[1], p[2]); + printk("%s""class_code: 0x%06x\n", pfx, + pcie->device_id.class_code); } if (pcie->validation_bits & CPER_PCIE_VALID_SERIAL_NUMBER) printk("%s""serial number: 0x%04x, 0x%04x\n", pfx, diff --git a/include/linux/cper.h b/include/linux/cper.h index dcacb1a72e26..fbfb50f52362 100644 --- 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; __u8 function; __u8 device; __u16 segment;
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: efi: EFI v2.50 by EDK II BIOS ID:PLYDCRB1.86B.0119.R05.1701181843 DMI: Intel Corporation PURLEY/PURLEY, BIOS PLYDCRB1.86B.0119.R05.1701181843 01/18/2017 {1}[Hardware Error]: device_id: 0000:5d:00.0 {1}[Hardware Error]: slot: 0 {1}[Hardware Error]: secondary_bus: 0x5e {1}[Hardware Error]: vendor_id: 0x8086, device_id: 0x2030 {1}[Hardware Error]: class_code: 000406 ^^^^^^ (should be 060400) Cc: Huang Ying <ying.huang@intel.com> Cc: Ashok Raj <ashok.raj@intel.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- @Ashok Raj: Could you test if this patch results in the correct PCI class being logged? Thanks! drivers/firmware/efi/cper.c | 5 ++--- include/linux/cper.h | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)