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

["Liran Alon" <liran.alon@oracle.com>]

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
    )
>> +
>> +  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. :)
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?
It's easy to document with one-line comment at end of Desc parameter 
deceleration that it must be aligned.
It's also not difficult to modify callers as only a single caller 
actually pass a descriptor (The caller PvScsiInitRings()).
To avoid further style comments, what is the coding convention in EDK2 
to align the "PVSCSI_CMD_DESC_SETUP_RINGS Cmd;" var properly?
In addition, I assume I don't need to add any validation of alignment to 
PvScsiWriteCmdDesc().

-Liran