Re: [edk2-devel] [PATCH 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings

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

really trivial style comments, for now (I intend to look at this in more
detail in v2):

On 03/16/20 16:01, Liran Alon wrote:
> These rings are shared memory buffers between host and device in which
> a cyclic buffer is managed to send request descriptors from host to
> device and receive completion descriptors from device to host.
> 
> Note that because device may be constrained by IOMMU or guest may be run
> under AMD SEV, we make sure to map these rings to device by using
> PciIo->Map().
> 
> 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 | 235 +++++++++++++++++++++++++++++++++++++
>  OvmfPkg/PvScsiDxe/PvScsi.h |  17 +++
>  2 files changed, 252 insertions(+)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index fb2407d2adb2..c3f5d38f3d30 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -16,6 +16,7 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>  #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
>  
>  #include "PvScsi.h"
>  
> @@ -396,6 +397,209 @@ PvScsiSetPCIAttributes (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +EFI_STATUS
> +PvScsiAllocatePages (
> +  IN PVSCSI_DEV     *Dev,
> +  IN UINTN          Pages,
> +  IN OUT VOID       **HostAddress
> +  )
> +{
> +  return Dev->PciIo->AllocateBuffer (
> +                       Dev->PciIo,
> +                       AllocateAnyPages,
> +                       EfiBootServicesData,
> +                       Pages,
> +                       HostAddress,
> +                       EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> +                       );
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreePages (
> +  IN PVSCSI_DEV     *Dev,
> +  IN UINTN          Pages,
> +  IN VOID           *HostAddress
> +  )
> +{
> +  Dev->PciIo->FreeBuffer (
> +                Dev->PciIo,
> +                Pages,
> +                HostAddress
> +                );
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiMapBuffer (
> +  IN PVSCSI_DEV                     *Dev,
> +  IN EFI_PCI_IO_PROTOCOL_OPERATION  PciIoOperation,
> +  IN VOID                           *HostAddress,
> +  IN UINTN                          NumberOfBytes,
> +  OUT PVSCSI_DMA_DESC               *DmaDesc
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINTN      BytesMapped;
> +
> +  BytesMapped = NumberOfBytes;
> +  Status = Dev->PciIo->Map (
> +                         Dev->PciIo,
> +                         PciIoOperation,
> +                         HostAddress,
> +                         &BytesMapped,
> +                         &DmaDesc->DeviceAddress,
> +                         &DmaDesc->Mapping
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (BytesMapped != NumberOfBytes) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto Unmap;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +Unmap:
> +  Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> +  DmaDesc->Mapping = NULL;
> +
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiUnmapBuffer (
> +  IN PVSCSI_DEV                 *Dev,
> +  IN OUT PVSCSI_DMA_DESC        *DmaDesc)
> +{
> +  Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping);
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiAllocateSharedPages (
> +  IN PVSCSI_DEV                     *Dev,
> +  IN UINTN                          Pages,
> +  IN EFI_PCI_IO_PROTOCOL_OPERATION  PciIoOperation,
> +  OUT VOID                          **HostAddress,
> +  OUT PVSCSI_DMA_DESC               *DmaDesc
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  *HostAddress = NULL;
> +  DmaDesc->Mapping = NULL;
> +
> +  Status = PvScsiAllocatePages (Dev, Pages, HostAddress);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = PvScsiMapBuffer (
> +             Dev,
> +             PciIoOperation,
> +             *HostAddress,
> +             Pages * EFI_PAGE_SIZE,

(1) Please use EFI_PAGES_TO_SIZE(); it's more idiomatic. (The argument
should have type UINTN -- "Pages" is already UINTN, so that's good.)

> +             DmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto FreePages;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +FreePages:
> +  PvScsiFreePages (Dev, Pages, *HostAddress);
> +  *HostAddress = NULL;
> +
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +PvScsiFreeSharedPages (
> +  IN PVSCSI_DEV                     *Dev,
> +  IN UINTN                          Pages,
> +  IN OUT VOID                       **HostAddress,
> +  IN OUT PVSCSI_DMA_DESC            *DmaDesc
> +  )
> +{
> +  if (*HostAddress) {
> +      if (DmaDesc->Mapping) {
> +        PvScsiUnmapBuffer (Dev, DmaDesc);
> +        DmaDesc->Mapping = NULL;
> +      }
> +
> +      PvScsiFreePages (Dev, Pages, *HostAddress);
> +      *HostAddress = NULL;
> +  }
> +}
> +
> +STATIC
> +EFI_STATUS
> +PvScsiInitRings (
> +  IN OUT PVSCSI_DEV *Dev
> +  )
> +{
> +  EFI_STATUS                  Status;
> +  PVSCSI_CMD_DESC_SETUP_RINGS Cmd;
> +
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             1,
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->RingDesc.RingState,
> +             &Dev->RingDesc.RingStateDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE);
> +
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             1,
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->RingDesc.RingReqs,
> +             &Dev->RingDesc.RingReqsDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE);
> +
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             1,
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->RingDesc.RingCmps,
> +             &Dev->RingDesc.RingCmpsDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +      return Status;
> +  }
> +  ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE);
> +
> +  ZeroMem (&Cmd, sizeof Cmd);
> +  Cmd.ReqRingNumPages = 1;
> +  Cmd.CmpRingNumPages = 1;
> +  Cmd.RingsStatePPN =
> +        ((UINT64) Dev->RingDesc.RingStateDmaDesc.DeviceAddress) >>
> +        EFI_PAGE_SHIFT;
> +  Cmd.ReqRingPPNs[0] =
> +        ((UINT64) Dev->RingDesc.RingReqsDmaDesc.DeviceAddress) >>
> +        EFI_PAGE_SHIFT;
> +  Cmd.CmpRingPPNs[0] =
> +        ((UINT64) Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress) >>
> +        EFI_PAGE_SHIFT;

(2) Edk2 allows the << and >> operators when the LHS operand is not
wider than UINTN. For shifting UINT64, please call the LShiftU64() and
RShiftU64() functions from BaseLib.

> +
> +  return PvScsiWriteCmdDesc(Dev, PVSCSI_CMD_SETUP_RINGS, &Cmd, sizeof Cmd);
> +}
> +
>  STATIC
>  EFI_STATUS
>  PvScsiInit (
> @@ -425,6 +629,15 @@ PvScsiInit (
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> +
> +  //
> +  // Init PVSCSI rings
> +  //
> +  Status = PvScsiInitRings (Dev);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +

(3) Again, we should jump to "RestorePciAttributes" here.

(More careful review postponed to v2.)

Thanks
Laszlo

>    //
>    // Populate the exported interface's attributes
>    //
> @@ -463,6 +676,28 @@ PvScsiUninit (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> +  //
> +  // Free PVSCSI rings
> +  //
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    (VOID **)&Dev->RingDesc.RingCmps,
> +    &Dev->RingDesc.RingCmpsDmaDesc
> +    );
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    (VOID **)&Dev->RingDesc.RingReqs,
> +    &Dev->RingDesc.RingReqsDmaDesc
> +    );
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    1,
> +    (VOID **)&Dev->RingDesc.RingState,
> +    &Dev->RingDesc.RingStateDmaDesc
> +    );
> +
>    //
>    // Restore PCI Attributes
>    //
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 5f611dbbc98c..6d23b6e1eccf 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -15,12 +15,29 @@
>  #include <Library/DebugLib.h>
>  #include <Protocol/ScsiPassThruExt.h>
>  
> +typedef struct {
> +  EFI_PHYSICAL_ADDRESS DeviceAddress;
> +  VOID                 *Mapping;
> +} PVSCSI_DMA_DESC;
> +
> +typedef struct {
> +  PVSCSI_RINGS_STATE   *RingState;
> +  PVSCSI_DMA_DESC      RingStateDmaDesc;
> +
> +  PVSCSI_RING_REQ_DESC *RingReqs;
> +  PVSCSI_DMA_DESC      RingReqsDmaDesc;
> +
> +  PVSCSI_RING_CMP_DESC *RingCmps;
> +  PVSCSI_DMA_DESC      RingCmpsDmaDesc;
> +} PVSCSI_RING_DESC;
> +
>  #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
>  
>  typedef struct {
>    UINT32                          Signature;
>    EFI_PCI_IO_PROTOCOL             *PciIo;
>    UINT64                          OriginalPciAttributes;
> +  PVSCSI_RING_DESC                RingDesc;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>