Re: [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for SoC-specific i.MX packages to use

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

> On 4 Dec 2018, at 02:44, Chris Co <Christopher.Co@microsoft.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Leif Lindholm <leif.lindholm@linaro.org>
>> Sent: Monday, December 3, 2018 1:43 AM
>> To: Chris Co <Christopher.Co@microsoft.com>
>> Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
>> Michael D Kinney <michael.d.kinney@intel.com>
>> Subject: Re: [PATCH edk2-platforms 09/27] Silicon/NXP: Add headers for SoC-
>> specific i.MX packages to use
>> 
>> On Sat, Dec 01, 2018 at 12:22:17AM +0000, Chris Co wrote:
>>>> If using EFI_ACPI prefix, these #defines really should be in edk2
>>>> MdePkg. And CSRT itself is, so that might not be a bad idea.
>>>> 
>>>>> +
>>>>> +#pragma pack(push, 1)
>>>> 
>>>> I don't see this #pragma making any difference to the structs below,
>>>> can it be dropped?
>>> 
>>> The pragma pack is defensive. Without it, we rely on the compiler
>>> packing structures by default and this may not happen on 64 bit
>>> compiles.
>> 
>> I understand that is what the pragma does. My comment was because all of the
>> variables in the below structs are naturally aligned.
>> The reason I dislike its use when effectively a no-op, is that it makes it look like it
>> it isn't a no-op.
>> 
>> If it covers a larger set of structs, some of which require the packed attribute I'm
>> OK with that. But I'm not a fan of adding it "just in case" without contemplating
>> the statement's (lack of) effect.
>> 
>> Regards,
>> 
>> Leif
>> 
> 
> Makes sense. I am checking to make sure this pragma wasn't included due to some observed compiler behavior on our end, since this header is also used on our ARM64 work.
> Will remove it once confirmed it is safe.


I’d prefer to keep the pragma. It doesn’t only remove padding, it also informs the compiler that the whole struct may appear at any unaligned offset. On 32 bit ARM, this means the compiler will refrain from using load/store double or load/store multiple to access the contents when dereferencing a pointer to such a struct, which would result in a crash otherwise.

> 
>>> I have addressed the remaining feedback and will resubmit with v2.
>>> 
>>> Thanks,
>>> Chris
>>> 
>>>>> +//---------------------------------------------------------------
>>>>> +----
>>>>> +----- // CSRT Resource Group header 24 bytes long
>>>>> +//---------------------------------------------------------------
>>>>> +----
>>>>> +-----
>>>>> +typedef struct {
>>>>> +  UINT32 Length;
>>>>> +  UINT32 VendorID;
>>>>> +  UINT32 SubVendorId;
>>>>> +  UINT16 DeviceId;
>>>>> +  UINT16 SubdeviceId;
>>>>> +  UINT16 Revision;
>>>>> +  UINT16 Reserved;
>>>>> +  UINT32 SharedInfoLength;
>>>>> +} EFI_ACPI_5_0_CSRT_RESOURCE_GROUP_HEADER;
>>>>> +
>>>>> +//---------------------------------------------------------------
>>>>> +----
>>>>> +----- // CSRT Resource Descriptor 12 bytes total
>>>>> +//---------------------------------------------------------------
>>>>> +----
>>>>> +-----
>>>>> +typedef struct {
>>>>> +  UINT32 Length;
>>>>> +  UINT16 ResourceType;
>>>>> +  UINT16 ResourceSubType;
>>>>> +  UINT32 UID;
>>>>> +} EFI_ACPI_5_0_CSRT_RESOURCE_DESCRIPTOR_HEADER;
>>>>> +#pragma pack (pop)