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

[Leif Lindholm <leif.lindholm@linaro.org>]

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

> 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)