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

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

a bit more superficial comments, for now:

On 03/16/20 16:01, Liran Alon wrote:
> The following commits will complete the implementation of
> device initialization.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Reviewed-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 77 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index ff6b50b7020f..fb2407d2adb2 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -29,6 +29,76 @@
>  // Ext SCSI Pass Thru utilities
>  //
>
> +
> +//
> +// Writes a 32-bit value into BAR0 using MMIO
> +//

(1) Please use the /** **/ comment style for top-level function comment
blocks.

> +STATIC
> +EFI_STATUS
> +PvScsiMmioWrite32 (
> +  IN CONST PVSCSI_DEV   *Dev,
> +  IN UINT64             Offset,
> +  IN UINT32             Value
> +  )
> +{
> +  return Dev->PciIo->Mem.Write(
> +                           Dev->PciIo,
> +                           EfiPciIoWidthUint32,
> +                           0,   // BarIndex

(2) Style improvement: please use the PCI_BAR_IDX0 macro.

> +                           Offset,
> +                           1,   // Count
> +                           &Value
> +                           );
> +}
> +
> +//
> +// Send PVSCSI command to device
> +//

(3) Same as (1).

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

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). But, for this function, it really is unclear.

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

>  //
>  // Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
>  // EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> @@ -348,6 +418,13 @@ PvScsiInit (
>      return Status;
>    }
>
> +  //
> +  // Reset adapter
> +  //
> +  Status = PvScsiWriteCmdDesc (Dev, PVSCSI_CMD_ADAPTER_RESET, NULL, 0);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
>    //
>    // Populate the exported interface's attributes
>    //
>

OK, so this ties back to my comments on resource management, under
patch#9.

Let me quote the PvScsiInit() function in full, after the present patch
is applied:

> STATIC
> EFI_STATUS
> PvScsiInit (
>   IN OUT PVSCSI_DEV *Dev
>   )
> {
>   EFI_STATUS Status;
>
>   //
>   // Init configuration
>   //
>   Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
>   Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
>
>   //
>   // Set PCI Attributes
>   //
>   Status = PvScsiSetPCIAttributes (Dev);
>   if (EFI_ERROR (Status)) {
>     return Status;
>   }
>
>   //
>   // Reset adapter
>   //
>   Status = PvScsiWriteCmdDesc (Dev, PVSCSI_CMD_ADAPTER_RESET, NULL, 0);
>   if (EFI_ERROR (Status)) {
>     return Status;
>   }

(7) So this is precisely the spot where we have to jump to a new error
handling label -- called "RestorePciAttributes" --, to be introduced at
the end of the function. And we need to call
PvScsiRestorePciAttributes() there.

Otherwise, the original PCI attributes will never be restored.

Thanks!
Laszlo

On 03/16/20 16:01, Liran Alon wrote:
>   //
>   // Populate the exported interface's attributes
>   //
>   Dev->PassThru.Mode             = &Dev->PassThruMode;
>   Dev->PassThru.PassThru         = &PvScsiPassThru;
>   Dev->PassThru.GetNextTargetLun = &PvScsiGetNextTargetLun;
>   Dev->PassThru.BuildDevicePath  = &PvScsiBuildDevicePath;
>   Dev->PassThru.GetTargetLun     = &PvScsiGetTargetLun;
>   Dev->PassThru.ResetChannel     = &PvScsiResetChannel;
>   Dev->PassThru.ResetTargetLun   = &PvScsiResetTargetLun;
>   Dev->PassThru.GetNextTarget    = &PvScsiGetNextTarget;
>
>   //
>   // AdapterId is a target for which no handle will be created during bus scan.
>   // Prevent any conflict with real devices.
>   //
>   Dev->PassThruMode.AdapterId = MAX_UINT32;
>
>   //
>   // Set both physical and logical attributes for non-RAID SCSI channel
>   //
>   Dev->PassThruMode.Attributes = EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_PHYSICAL |
>                                  EFI_EXT_SCSI_PASS_THRU_ATTRIBUTES_LOGICAL;
>
>   //
>   // No restriction on transfer buffer alignment
>   //
>   Dev->PassThruMode.IoAlign = 0;
>
>   return EFI_SUCCESS;
> }