Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer

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

On 03/25/20 17:10, 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

(1) s/And/and/

> 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
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/PvScsiDxe/PvScsi.c | 27 +++++++++++++++++++++++++++
>  OvmfPkg/PvScsiDxe/PvScsi.h | 10 ++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 59863f83c60c..928984099520 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -710,6 +710,20 @@ PvScsiInit (
>      goto RestorePciAttributes;
>    }
>  
> +  //
> +  // Allocate DMA communication buffer
> +  //
> +  Status = PvScsiAllocateSharedPages (
> +             Dev,
> +             EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> +             EfiPciIoOperationBusMasterCommonBuffer,
> +             (VOID **)&Dev->DmaBuf,
> +             &Dev->DmaBufDmaDesc
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeRings;
> +  }
> +
>    //
>    // Populate the exported interface's attributes
>    //
> @@ -741,6 +755,9 @@ PvScsiInit (
>  
>    return EFI_SUCCESS;
>  
> +FreeRings:
> +  PvScsiFreeRings (Dev);
> +
>  RestorePciAttributes:
>    PvScsiRestorePciAttributes (Dev);
>  
> @@ -753,6 +770,16 @@ PvScsiUninit (
>    IN OUT PVSCSI_DEV *Dev
>    )
>  {
> +  //
> +  // Free DMA communication buffer
> +  //
> +  PvScsiFreeSharedPages (

(2) From peeking ahead at the next patch, the following seems possible:

- we send a data transfer request to the device model, passing some
pointers into the DMA communication buffer to the hypervisor

- PvScsiWaitForRequestCompletion() fails (for whatever reason), and so
we can't be sure whether the device is completely done with the buffer
that we exposed to it.

Therefore, please *prepend* a Reset operation to this
PvScsiFreeSharedPages() call as well, at the top of PvScsiUninit().

(Note: we could be tempted to somehow "centralize" all of these Reset
operations into a single spot. Bad idea. We are revoking the device's
access rights to different resources, so the revocation operations will
show up in different spots. It's a mere circumstance that the
revocations all happen to be Reset operations.)

I might be paranoid of course -- I just feel that maybe-superfluous
reset operations on error paths are much better than silently corrupted
guest memory and/or disk contents.

> +    Dev,
> +    EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)),
> +    (VOID **)&Dev->DmaBuf,

(3) Copy-paste typo: you should only pass "Dev->DmaBuf".

The compiler doesn't catch this for you because PvScsiFreeSharedPages()
takes a "VOID *HostAddress" here, and (VOID **) -- like all other
pointer-to-object types -- converts to (VOID *) silently.

> +    &Dev->DmaBufDmaDesc
> +    );
> +
>    PvScsiFreeRings (Dev);
>  
>    PvScsiRestorePciAttributes (Dev);
> 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];

(4) Is the maximum possible size of the sense data specified somewhere?
If so, it would be nice to document it with a comment at least.

> +  UINT8     Data[0x2000];

(5) Same here. From peeking at the next patch, we seem to be choosing
this size arbitrarily.

If it works for you in all relevant boot scenarios, I'm OK with it, but
we should be clear that this value is arbitrarily chosen. No need for a
#define, but a comment would be nice.

(6) Should we declare this structure as packed?

> +} 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_
> 

Thanks,
Laszlo