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

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

On 03/25/20 02:11, Liran Alon wrote:
>
> On 24/03/2020 18:00, Laszlo Ersek wrote:
>> On 03/16/20 16:01, Liran Alon wrote:
>>> +STATIC
>>> +EFI_STATUS
>>> +PvScsiWriteCmdDesc (
>>> +  IN CONST PVSCSI_DEV   *Dev,
>>> +  IN UINT32             Cmd,
>>> +  IN VOID               *Desc,
>>> +  IN UINTN              Length
>>> +  )
>>> +{
>>> +  EFI_STATUS Status;
>>> +  UINTN      LengthInWords;
>>> +  UINT8      *WordPtr;
>>> +  UINT8      *DescEndPtr;
>>> +  UINT32     Word;
>>> +
>>> +  LengthInWords = Length / sizeof (UINT32);
>> (4) What guarantees that "Length" is a whole multiple of sizeof
>> (UINT32)?
>
> Nothing.
> Besides the fact that all commands passed to this function are indeed
> multiple of sizeof (UINT32).
>
>> In this review I have not insisted on including full-blown interface
>> contracts in the top-level function comment blocks (with @param[in]
>> and @retval etc).

> Thanks for that. I think too it would be an overkill with little
> value.

>> But, for this function, it really is unclear.

> Will it be sufficient for you if I just replace the prototype with
> something like the following?
>
> /**
>   Send PVSCSI command to device
> **/
> STATIC
> EFI_STATUS
> PvScsiWriteCmdDesc (
>    IN CONST PVSCSI_DEV   *Dev,
>    IN UINT32                     Cmd,
>    IN VOID                         *Desc,
>    IN UINTN                       LengthInWords     // Note: Word is UINT32
>    )

The comment "// Note: ..." on a particular parameter is really
non-idiomatic. So please either (a) rename "LengthInWords" so that
"UINT32" is obvious from it, or (for this one particular function) (b)
please do add a full-blown function-level comment, with @param[in] and
@return / @retval markers.

>>> +
>>> +  if (LengthInWords > PVSCSI_MAX_CMD_DATA_WORDS) {
>>> +    return EFI_INVALID_PARAMETER;
>>> +  }
>>> +
>>> +  Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_COMMAND, Cmd);
>>> +  if (EFI_ERROR (Status)) {
>>> +    return Status;
>>> +  }
>>> +
>>> +  WordPtr = Desc;
>>> +  DescEndPtr = WordPtr + Length;
>>> +
>>> +  while (WordPtr != DescEndPtr) {
>>> +    //
>>> +    // CopyMem() is used to avoid strict-aliasing issues
>>> +    //

>> (5) In edk2, we -- completely intentionally -- disable the
>> enforcement of the effective type rules / strict aliasing rules. See
>> "-fno-strict-aliasing" in "BaseTools/Conf/tools_def.template".

>>
>>> +    CopyMem (&Word, WordPtr, sizeof (UINT32));
>>> +
>>> +    Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_COMMAND_DATA,
>>> Word);
>>> +    if (EFI_ERROR (Status)) {
>>> +      return Status;
>>> +    }
>>> +
>>> +    WordPtr += sizeof (UINT32);
>>> +  }
>>> +
>>> +  return EFI_SUCCESS;
>>> +}

>> (6) I think the open-coded loop is suboptimal -- the PciIo protocol
>> seems to offer EfiPciIoWidthFifoUint32 for exactly this purpose (=
>> advance in the memory buffer, while accessing the same offset in the
>> BAR).
>>
>> Have you perhaps tried that?

> I actually haven't noticed EfiPciIoWidthFifoUint32 until you mentioned
> it.
> As it seems there isn't even a single line of code in EDK2 that use
> it. :)

"MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c" seems to use it,
doesn't it? From commit 9bfaa3da1ee5 ("MdeModulePkg/SdMmcPciHcDxe: Fix
PIO transfer mode", 2020-03-05).

> In fact, the only code that use one of the EfiPciIoWidthFifo* is
> MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c.

>> (I can imagine that you ruled it out, due to "Desc" being unaligned.
>> The UEFI spec does say, "The caller is responsible for any alignment
>> and I/O width issues which the bus, device, platform, or type of I/O
>> might require.)

> Why is this an issue?

If it's not an issue for you, it's definitely not an issue for me. :) I
didn't know how difficult it would be for you to satisfy such an
alignment requirement, at all the call sites.

> It's easy to document with one-line comment at end of Desc parameter
> deceleration that it must be aligned.

Documenting the requirement is good, but *if* you do that, you can
*only* do it with a @param[in] marker.

> It's also not difficult to modify callers as only a single caller
> actually pass a descriptor (The caller PvScsiInitRings()).

OK.

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

> In addition, I assume I don't need to add any validation of alignment
> to PvScsiWriteCmdDesc().

No, I don't think that's necessary. I think the generic edk2 machinery
rejects a misaligned memory buffer explicitly, as follows:

(a) The PciIo->Mem.Read/Write members are implemented in PciIoMemRead()
    / PciIoMemWrite() [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c].

    Two comments about them:

    - These functions break up unaligned requests into byte-size
      requests if "PcdUnalignedPciIoEnable" is TRUE. (Note: it is FALSE
      for OvmfPkg, and we should not change that.)

    - These functions do not actually check the alignment of either
      "Buffer" or the MMIO "Offset" within the BAR. They just delegate
      to PciRootBridgeIo->Mem.Read() and PciRootBridgeIo->Mem.Write().

(b) For those, we need to look at RootBridgeIoMemRead() and
    RootBridgeIoMemWrite(), in
    "MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c".

    Both of these functions call RootBridgeIoCheckParameter(),
    RootBridgeIoCheckParameter() -- which checks the alignment on the
    MMIO "Address" within the BAR (--> EFI_UNSUPPORTED), but still
    doesn't check the alignment of "Buffer".

    So, let's continue analyzing RootBridgeIoMemRead() and
    RootBridgeIoMemWrite()... Once RootBridgeIoCheckParameter() is
    happy, the work is delegated to CpuIo->Mem.Read/Write.

(c) For those, we need to look at CpuMemoryServiceRead() and
    CpuMemoryServiceWrite(), in "UefiCpuPkg/CpuIo2Dxe/CpuIo2Dxe.c".

    These functions call CpuIoCheckParameter(). And that function
    finally does seem to check the alignment of "Buffer":

  //
  // Check to see if Buffer is aligned
  // (IA-32 allows UINT64 and INT64 data types to be 32-bit aligned.)
  //
  if (((UINTN)Buffer & ((MIN (sizeof (UINTN), mInStride[Width])  - 1))) != 0) {
    return EFI_UNSUPPORTED;
  }

    (See related commit 36de860619c2, "Update the check condition to
    allows UINT64 and INT64 data types to be 32-bit aligned on IA32
    system.", 2011-12-01.)

Thus I believe any mis-alignment in "Desc" would be caught for you.

Thanks,
Laszlo