Re: [PATCH 1/3] OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap()

[Laszlo Ersek <lersek@redhat.com>]

On 08/28/17 13:26, Brijesh Singh wrote:
> When device is behind the IOMMU then driver need to pass the device
> address when programing the bus master. The patch uses VirtioRingMap() to
> map the VRING system physical address to device address.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.h |  1 +
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 47 +++++++++++++++-----
>  2 files changed, 38 insertions(+), 10 deletions(-)

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

Thanks,
Laszlo

> 
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> index 6d00567e8cb8..05a6bf567263 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h
> @@ -60,6 +60,7 @@ typedef struct {
>    VRING                           Ring;           // VirtioRingInit      2
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;       // VirtioScsiInit      1
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;   // VirtioScsiInit      1
> +  VOID                            *RingMap;       // VirtioRingMap       2
>  } VSCSI_DEV;
>  
>  #define VIRTIO_SCSI_FROM_PASS_THRU(PassThruPointer) \
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index a983b3df7b9c..5e72b1a24b59 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -707,7 +707,7 @@ VirtioScsiInit (
>  {
>    UINT8      NextDevStat;
>    EFI_STATUS Status;
> -
> +  UINT64     RingBaseShift;
>    UINT64     Features;
>    UINT16     MaxChannel; // for validation only
>    UINT32     NumQueues;  // for validation only
> @@ -839,25 +839,42 @@ VirtioScsiInit (
>    }
>  
>    //
> +  // If anything fails from here on, we must release the ring resources
> +  //
> +  Status = VirtioRingMap (
> +             Dev->VirtIo,
> +             &Dev->Ring,
> +             &RingBaseShift,
> +             &Dev->RingMap
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto ReleaseQueue;
> +  }
> +
> +  //
>    // Additional steps for MMIO: align the queue appropriately, and set the
> -  // size. If anything fails from here on, we must release the ring resources.
> +  // size. If anything fails from here on, we must unmap the ring resources.
>    //
>    Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
>    // step 4c -- Report GPFN (guest-physical frame number) of queue.
>    //
> -  Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0);
> +  Status = Dev->VirtIo->SetQueueAddress (
> +                          Dev->VirtIo,
> +                          &Dev->Ring,
> +                          RingBaseShift
> +                          );
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -867,7 +884,7 @@ VirtioScsiInit (
>      Features &= ~(UINT64)VIRTIO_F_VERSION_1;
>      Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
>      if (EFI_ERROR (Status)) {
> -      goto ReleaseQueue;
> +      goto UnmapQueue;
>      }
>    }
>  
> @@ -877,11 +894,11 @@ VirtioScsiInit (
>    //
>    Status = VIRTIO_CFG_WRITE (Dev, CdbSize, VIRTIO_SCSI_CDB_SIZE);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>    Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -890,7 +907,7 @@ VirtioScsiInit (
>    NextDevStat |= VSTAT_DRIVER_OK;
>    Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -926,6 +943,9 @@ VirtioScsiInit (
>  
>    return EFI_SUCCESS;
>  
> +UnmapQueue:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
> +
>  ReleaseQueue:
>    VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>  
> @@ -965,6 +985,7 @@ VirtioScsiUninit (
>    Dev->MaxLun         = 0;
>    Dev->MaxSectors     = 0;
>  
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
>    VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>  
>    SetMem (&Dev->PassThru,     sizeof Dev->PassThru,     0x00);
> @@ -995,6 +1016,12 @@ VirtioScsiExitBoot (
>    //
>    Dev = Context;
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> +
> +  //
> +  // Unmap the ring buffer so that hypervisor will not be able to get
> +  // readable data after device reset.
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
>  }
>  
>  
>