Re: [edk2-devel] [PATCH 12/17] OvmfPkg/PvScsiDxe: Reset adapter on init

["Laszlo Ersek" <lersek@redhat.com>]

On 03/25/20 18:13, Liran Alon wrote:
> 
> 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?

Yes, that would be sufficient, due to the UEFI spec, and due to edk2
conforming to the UEFI spec the best it can. Except, the structure is
packed.

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

Exactly.

Laszlo