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

[Laszlo Ersek <lersek@redhat.com>]

On 08/25/17 23:43, 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/VirtioBlkDxe/VirtioBlk.h |  1 +
>  OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 45 ++++++++++++++++----
>  2 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h
> index 6c402ca88ea4..9ec0b956b818 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h
> @@ -41,6 +41,7 @@ typedef struct {
>    VRING                  Ring;                 // VirtioRingInit      2
>    EFI_BLOCK_IO_PROTOCOL  BlockIo;              // VirtioBlkInit       1
>    EFI_BLOCK_IO_MEDIA     BlockIoMedia;         // VirtioBlkInit       1
> +  VOID                   *RingMap;             // VirtioRingMap       2
>  } VBLK_DEV;
>  
>  #define VIRTIO_BLK_FROM_BLOCK_IO(BlockIoPointer) \
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index bff15fe3add1..663ba281ab73 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -580,7 +580,8 @@ VirtioBlkDriverBindingSupported (
>                             virtio-blk attributes the host provides.
>  
>    @return                  Error codes from VirtioRingInit() or
> -                           VIRTIO_CFG_READ() / VIRTIO_CFG_WRITE().
> +                           VIRTIO_CFG_READ() / VIRTIO_CFG_WRITE or
> +                           VirtioRingMap().
>  
>  **/
>  
> @@ -601,6 +602,7 @@ VirtioBlkInit (
>    UINT8      AlignmentOffset;
>    UINT32     OptIoSize;
>    UINT16     QueueSize;
> +  UINT64     RingBaseShift;
>  
>    PhysicalBlockExp = 0;
>    AlignmentOffset = 0;
> @@ -729,25 +731,42 @@ VirtioBlkInit (
>    }
>  
>    //
> +  // 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;
>    }
>  
>  
> @@ -758,7 +777,7 @@ VirtioBlkInit (
>      Features &= ~(UINT64)VIRTIO_F_VERSION_1;
>      Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
>      if (EFI_ERROR (Status)) {
> -      goto ReleaseQueue;
> +      goto UnmapQueue;
>      }
>    }
>  
> @@ -768,7 +787,7 @@ VirtioBlkInit (
>    NextDevStat |= VSTAT_DRIVER_OK;
>    Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -811,6 +830,9 @@ VirtioBlkInit (
>    }
>    return EFI_SUCCESS;
>  
> +UnmapQueue:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
> +
>  ReleaseQueue:
>    VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>  
> @@ -849,6 +871,7 @@ VirtioBlkUninit (
>    //
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
>  
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
>    VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>  
>    SetMem (&Dev->BlockIo,      sizeof Dev->BlockIo,      0x00);
> @@ -885,6 +908,12 @@ VirtioBlkExitBoot (
>    //
>    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 is reset.
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
>  }
>  
>  /**
> 

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