Re: [PATCH 08/11] OvmfPkg/LsiScsiDxe: Map DMA buffer

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

On 07/01/20 06:04, Gary Lin wrote:
> Map DMA buffer and perpare for the implementation of LsiScsiPassThru().
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Gary Lin <glin@suse.com>
> ---
>  OvmfPkg/LsiScsiDxe/LsiScsi.c | 85 +++++++++++++++++++++++++++++++++++-
>  OvmfPkg/LsiScsiDxe/LsiScsi.h | 10 +++++
>  2 files changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.c b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> index f03774cc4ced..b728d18d51df 100644
> --- a/OvmfPkg/LsiScsiDxe/LsiScsi.c
> +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.c
> @@ -359,6 +359,8 @@ LsiScsiControllerStart (
>  {
>    EFI_STATUS           Status;
>    LSI_SCSI_DEV         *Dev;
> +  UINTN                Pages;
> +  UINTN                BytesMapped;
>  
>    Dev = AllocateZeroPool (sizeof (*Dev));
>    if (Dev == NULL) {
> @@ -406,11 +408,68 @@ LsiScsiControllerStart (
>      goto CloseProtocol;
>    }
>  
> -  Status = LsiScsiReset (Dev);
> +  //
> +  // Signal device supports 64-bit DMA addresses
> +  //
> +  Status = Dev->PciIo->Attributes (
> +                         Dev->PciIo,
> +                         EfiPciIoAttributeOperationEnable,
> +                         EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
> +                         NULL
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // Warn user that device will only be using 32-bit DMA addresses.
> +    //
> +    // Note that this does not prevent the device/driver from working
> +    // and therefore we only warn and continue as usual.
> +    //
> +    DEBUG ((
> +      DEBUG_WARN,
> +      "%a: failed to enable 64-bit DMA addresses\n",
> +      __FUNCTION__
> +      ));
> +  }
> +
> +  //
> +  // Create buffers for data transfer
> +  //
> +  Pages = EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma));
> +  Status = Dev->PciIo->AllocateBuffer (
> +                         Dev->PciIo,
> +                         AllocateAnyPages,
> +                         EfiBootServicesData,
> +                         Pages,
> +                         (VOID **)&Dev->Dma,
> +                         EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> +                         );
>    if (EFI_ERROR (Status)) {
>      goto RestoreAttributes;
>    }
>  
> +  BytesMapped = EFI_PAGES_TO_SIZE (Pages);
> +  Status = Dev->PciIo->Map (
> +                         Dev->PciIo,
> +                         EfiPciIoOperationBusMasterCommonBuffer,
> +                         Dev->Dma,
> +                         &BytesMapped,
> +                         &Dev->DmaPhysical,
> +                         &Dev->DmaMapping
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeBuffer;
> +  }
> +
> +  if (BytesMapped != EFI_PAGES_TO_SIZE (Pages)) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto Unmap;
> +  }
> +
> +  Status = LsiScsiReset (Dev);
> +  if (EFI_ERROR (Status)) {
> +    goto Unmap;
> +  }
> +
>    Status = gBS->CreateEvent (
>                    EVT_SIGNAL_EXIT_BOOT_SERVICES,
>                    TPL_CALLBACK,
> @@ -457,6 +516,19 @@ CloseExitBoot:
>  UninitDev:
>    LsiScsiReset (Dev);
>  
> +Unmap:
> +  Dev->PciIo->Unmap (
> +                Dev->PciIo,
> +                Dev->DmaMapping
> +                );
> +
> +FreeBuffer:
> +  Dev->PciIo->FreeBuffer (
> +                Dev->PciIo,
> +                Pages,
> +                Dev->Dma
> +                );
> +
>  RestoreAttributes:
>    Dev->PciIo->Attributes (
>                  Dev->PciIo,
> @@ -519,6 +591,17 @@ LsiScsiControllerStop (
>  
>    LsiScsiReset (Dev);
>  
> +  Dev->PciIo->Unmap (
> +                Dev->PciIo,
> +                Dev->DmaMapping
> +                );
> +
> +  Dev->PciIo->FreeBuffer (
> +                Dev->PciIo,
> +                EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
> +                Dev->Dma
> +                );
> +
>    Dev->PciIo->Attributes (
>                  Dev->PciIo,
>                  EfiPciIoAttributeOperationSet,
> diff --git a/OvmfPkg/LsiScsiDxe/LsiScsi.h b/OvmfPkg/LsiScsiDxe/LsiScsi.h
> index ffaee6188536..1e4bbc56f933 100644
> --- a/OvmfPkg/LsiScsiDxe/LsiScsi.h
> +++ b/OvmfPkg/LsiScsiDxe/LsiScsi.h
> @@ -12,6 +12,13 @@
>  #ifndef _LSI_SCSI_DXE_H_
>  #define _LSI_SCSI_DXE_H_
>  
> +typedef struct {
> +  //
> +  // Allocate 64KB for read/write buffer.
> +  //
> +  UINT8                           Data[0x10000];

(1) Using SIZE_64KB for the size would be more idiomatic.

(2) Please add a comment that limiting the transfer size to 64KB seems
OK in practice.

With those updates:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> +} LSI_SCSI_DMA_BUFFER;
> +
>  typedef struct {
>    UINT32                          Signature;
>    UINT64                          OrigPciAttrs;
> @@ -19,6 +26,9 @@ typedef struct {
>    EFI_PCI_IO_PROTOCOL             *PciIo;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
> +  LSI_SCSI_DMA_BUFFER             *Dma;
> +  EFI_PHYSICAL_ADDRESS            DmaPhysical;
> +  VOID                            *DmaMapping;
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>  } LSI_SCSI_DEV;
>