On Wed, May 10, 2017 at 10:03 AM, Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> On 6 May 2017 at 10:07, Lukas Wunner <lukas@xxxxxxxxx> wrote:
>> On Sat, May 06, 2017 at 08:46:07AM +0100, Ard Biesheuvel wrote:
>>> On 5 May 2017 at 19:38, Lukas Wunner <lukas@xxxxxxxxx> 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
