Re: [edk2-devel] [PATCH 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

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

On 03/16/20 16:01, Liran Alon wrote:
> In case device is constrained by IOMMU or guest is running under AMD SEV,
> input/output buffers provided to device (DataBuffer and SenseData) needs
> to be explicitly mapped to device by PciIo->Map().
> 
> To avoid the overhead of mapping/unmapping the DataBuffer and SenseData
> to the device for every SCSI requst (And to simplify code), introduce a
> single DMA communication buffer that will be mapped to device on
> initialization. When a SCSI request needs to be sent to device, the
> DataBuffer and SenseData will be copied from/to the DMA communication
> buffer as required. This will be done by the following commits.
> 
> 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 | 24 ++++++++++++++++++++++++
>  OvmfPkg/PvScsiDxe/PvScsi.h | 10 ++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index c3f5d38f3d30..e48929bf044c 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -638,6 +638,20 @@ PvScsiInit (
>      return Status;
>    }
>  
> +  //
> +  // Allocate DMA communication buffer
> +  //
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->DmaBuf,
> +             &Dev->DmaBufDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +

(1) Superficial review:

in my set of "resource management comments", just pointing out that here
we both fail to restore the original PCI attributes, and leak the rings.

More careful review of this patch slated for v2.

Thanks
Laszlo

>    //
>    // Populate the exported interface's attributes
>    //
> @@ -676,6 +690,16 @@ PvScsiUninit (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> +  //
> +  // Free DMA communication buffer
> +  //
> +  PvScsiFreeSharedPages (
> +    Dev,
> +    EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> +    (VOID **)&Dev->DmaBuf,
> +    &Dev->DmaBufDmaDesc
> +    );
> +
>    //
>    // Free PVSCSI rings
>    //
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 6d23b6e1eccf..7f91d70fec79 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -31,6 +31,11 @@ typedef struct {
>    PVSCSI_DMA_DESC      RingCmpsDmaDesc;
>  } PVSCSI_RING_DESC;
>  
> +typedef struct {
> +  UINT8     SenseData[MAX_UINT8];
> +  UINT8     Data[0x2000];
> +} PVSCSI_DMA_BUFFER;
> +
>  #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S')
>  
>  typedef struct {
> @@ -38,6 +43,8 @@ typedef struct {
>    EFI_PCI_IO_PROTOCOL             *PciIo;
>    UINT64                          OriginalPciAttributes;
>    PVSCSI_RING_DESC                RingDesc;
> +  PVSCSI_DMA_BUFFER               *DmaBuf;
> +  PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
> @@ -47,4 +54,7 @@ typedef struct {
>  #define PVSCSI_FROM_PASS_THRU(PassThruPointer) \
>    CR (PassThruPointer, PVSCSI_DEV, PassThru, PVSCSI_SIG)
>  
> +#define PVSCSI_DMA_BUF_DEV_ADDR(Dev, MemberName) \
> +  (Dev->DmaBufDmaDesc.DeviceAddress + OFFSET_OF(PVSCSI_DMA_BUFFER, MemberName))
> +
>  #endif // __PVSCSI_DXE_H_
>