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

["Gary Lin" <glin@suse.com>]

On Tue, Jul 07, 2020 at 11:59:57AM +0200, Laszlo Ersek wrote:
> 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>
> 
Will update the commit in v2.

Thanks,

Gary Lin

> 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;
> > 
>