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


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

View/Reply Online (#56227): https://edk2.groups.io/g/devel/message/56227
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