On 25/03/2020 18:40, Liran Alon wrote:

On 25/03/2020 18:31, Laszlo Ersek wrote:
On 03/25/20 02:11, Liran Alon wrote:
To avoid further style comments, what is the coding convention in EDK2
to align the "PVSCSI_CMD_DESC_SETUP_RINGS Cmd;" var properly?
The best I can recommend off-hand is:

union {
   PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
   UINT32                      Uint32;
} AlignAtUint32;

Perhaps someone else can recommend something less awkward.

Note: PVSCSI_CMD_DESC_SETUP_RINGS is a packed structure (and I do agree
that's good, if at least for documentation purposes). If it weren't
packed, then the following passage from the UEFI spec would apply:

     2.3.1 Data Types

     Table 5 lists the common data types that are used in the interface
     definitions, and Table 6 lists their modifiers. Unless otherwise
     specified all data types are naturally aligned. Structures are
     aligned on boundaries equal to the largest internal datum of the
     structure and internal data are implicitly padded to achieve natural
     alignment.

Because PVSCSI_CMD_DESC_SETUP_RINGS only contains members with types
listed in Table 5 (namely, UINT32 and UINT64), the above language would
normally guarantee the proper alignment. *But*, because the structure is
packed, I don't think we can rely on the spec's description (cf. "unless
otherwise specified").

So, in theory, there are two options:

- drop the packing (and rely on the natural alignment providing what you
   need anyway),

- keep the packing, and use other methods to guarantee struct-level
   alignment (such as the above union).

I prefer keeping the packing, if for nothing else then for documentation
purposes (it says "wire format" loud and clear). If you use the union
above, I'll be OK with it.
Ok. I will use this union approach.
Unfortunately, I have seen this comment only after submitting v2.
So I will wait for the rest of your v2 review comments and make sure to do this change for v3 as-well.

Thanks.

Actually, I'm not sure I understand how this union approach helps with anything. Isn't the PVSCSI_CMD_DESC_SETUP_RINGS structure already aligned because it have only UINT32 and UINT64 fields? And if alignment is not guaranteed, how does putting it in a union together with another UINT32 provides the required alignment it didn't had before? Because the union itself is not marked with packed(), in contrast to PVSCSI_CMD_DESC_SETUP_RINGS?

-Liran



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56331): https://edk2.groups.io/g/devel/message/56331
Mute This Topic: https://groups.io/mt/72001280/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to