[edk2] [PATCH v2 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers

Brijesh Singh posted 3 patches 7 years, 4 months ago
[edk2] [PATCH v2 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers
Posted by Brijesh Singh 7 years, 4 months ago
When device is behind the IOMMU, driver is require to pass the device
address of virtio request, response and any memory referenced by those
request/response to the bus master.

The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to
map request and response buffers system physical address to the device
address.

- If the buffer need to be accessed by both the processor and a bus
  master then map with BusMasterCommonBuffer.

- If the buffer need to be accessed for a write operation by a bus master
  then map with BusMasterWrite.

- If the buffer need to be accessed for a read  operation by a bus master
  then map with BusMasterRead.

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.c | 138 ++++++++++++++++++--
 1 file changed, 125 insertions(+), 13 deletions(-)

diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
index 663ba281ab73..c9c42aa41243 100644
--- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
+++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
@@ -232,7 +232,8 @@ VerifyReadWriteRequest (
 
   @retval EFI_DEVICE_ERROR     Failed to notify host side via VirtIo write, or
                                unable to parse host response, or host response
-                               is not VIRTIO_BLK_S_OK.
+                               is not VIRTIO_BLK_S_OK or failed to map Buffer
+                               for a bus master operation.
 
 **/
 
@@ -249,8 +250,16 @@ SynchronousRequest (
 {
   UINT32                  BlockSize;
   volatile VIRTIO_BLK_REQ Request;
-  volatile UINT8          HostStatus;
+  volatile UINT8          *HostStatus;
+  VOID                    *HostStatusBuffer;
   DESC_INDICES            Indices;
+  VOID                    *RequestMapping;
+  VOID                    *StatusMapping;
+  VOID                    *BufferMapping;
+  EFI_PHYSICAL_ADDRESS    BufferDeviceAddress;
+  EFI_PHYSICAL_ADDRESS    HostStatusDeviceAddress;
+  EFI_PHYSICAL_ADDRESS    RequestDeviceAddress;
+  EFI_STATUS              Status;
 
   BlockSize = Dev->BlockIoMedia.BlockSize;
 
@@ -275,12 +284,82 @@ SynchronousRequest (
   Request.IoPrio = 0;
   Request.Sector = MultU64x32(Lba, BlockSize / 512);
 
-  VirtioPrepare (&Dev->Ring, &Indices);
+  //
+  // Host status is bi-directional (we preset with a value and expect the
+  // device to update it). Allocate a host status buffer which can be mapped
+  // to access equally by both processor and the device.
+  //
+  Status = Dev->VirtIo->AllocateSharedPages (
+                          Dev->VirtIo,
+                          EFI_SIZE_TO_PAGES (sizeof *HostStatus),
+                          &HostStatusBuffer
+                          );
+  if (EFI_ERROR (Status)) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  HostStatus = HostStatusBuffer;
+
+  //
+  // Map virtio-blk request header (must be done after request header is
+  // populated)
+  //
+  Status = VirtioMapAllBytesInSharedBuffer (
+             Dev->VirtIo,
+             VirtioOperationBusMasterRead,
+             (VOID *) &Request,
+             sizeof Request,
+             &RequestDeviceAddress,
+             &RequestMapping
+             );
+  if (EFI_ERROR (Status)) {
+    Status = EFI_DEVICE_ERROR;
+    goto FreeHostStatusBuffer;
+  }
+
+  //
+  // Map data buffer
+  //
+  if (BufferSize > 0) {
+    Status = VirtioMapAllBytesInSharedBuffer (
+               Dev->VirtIo,
+               (RequestIsWrite ?
+                VirtioOperationBusMasterRead :
+                VirtioOperationBusMasterWrite),
+               (VOID *) Buffer,
+               BufferSize,
+               &BufferDeviceAddress,
+               &BufferMapping
+               );
+    if (EFI_ERROR (Status)) {
+      Status = EFI_DEVICE_ERROR;
+      goto UnmapRequestBuffer;
+    }
+  }
 
   //
   // preset a host status for ourselves that we do not accept as success
   //
-  HostStatus = VIRTIO_BLK_S_IOERR;
+  *HostStatus = VIRTIO_BLK_S_IOERR;
+
+  //
+  // Map the Status Buffer with VirtioOperationBusMasterCommonBuffer so that
+  // both processor and device can access it.
+  //
+  Status = VirtioMapAllBytesInSharedBuffer (
+             Dev->VirtIo,
+             VirtioOperationBusMasterCommonBuffer,
+             HostStatusBuffer,
+             sizeof *HostStatus,
+             &HostStatusDeviceAddress,
+             &StatusMapping
+             );
+  if (EFI_ERROR (Status)) {
+    Status = EFI_DEVICE_ERROR;
+    goto UnmapDataBuffer;
+  }
+
+  VirtioPrepare (&Dev->Ring, &Indices);
 
   //
   // ensured by VirtioBlkInit() -- this predicate, in combination with the
@@ -291,8 +370,13 @@ SynchronousRequest (
   //
   // virtio-blk header in first desc
   //
-  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
-    VRING_DESC_F_NEXT, &Indices);
+  VirtioAppendDesc (
+    &Dev->Ring,
+    RequestDeviceAddress,
+    sizeof Request,
+    VRING_DESC_F_NEXT,
+    &Indices
+    );
 
   //
   // data buffer for read/write in second desc
@@ -311,27 +395,55 @@ SynchronousRequest (
     //
     // VRING_DESC_F_WRITE is interpreted from the host's point of view.
     //
-    VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize,
+    VirtioAppendDesc (
+      &Dev->Ring,
+      BufferDeviceAddress,
+      (UINT32) BufferSize,
       VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE),
-      &Indices);
+      &Indices
+      );
   }
 
   //
   // host status in last (second or third) desc
   //
-  VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus,
-    VRING_DESC_F_WRITE, &Indices);
+  VirtioAppendDesc (
+    &Dev->Ring,
+    HostStatusDeviceAddress,
+    sizeof *HostStatus,
+    VRING_DESC_F_WRITE,
+    &Indices
+    );
 
   //
   // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D).
   //
   if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices,
         NULL) == EFI_SUCCESS &&
-      HostStatus == VIRTIO_BLK_S_OK) {
-    return EFI_SUCCESS;
+      *HostStatus == VIRTIO_BLK_S_OK) {
+    Status = EFI_SUCCESS;
+  } else {
+    Status = EFI_DEVICE_ERROR;
   }
 
-  return EFI_DEVICE_ERROR;
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping);
+
+UnmapDataBuffer:
+  if (BufferSize > 0) {
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
+  }
+
+UnmapRequestBuffer:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
+
+FreeHostStatusBuffer:
+  Dev->VirtIo->FreeSharedPages (
+                 Dev->VirtIo,
+                 EFI_SIZE_TO_PAGES (sizeof *HostStatus),
+                 HostStatusBuffer
+                 );
+
+  return Status;
 }
 
 
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers
Posted by Laszlo Ersek 7 years, 4 months ago
On 08/28/17 02:34, Brijesh Singh wrote:
> When device is behind the IOMMU, driver is require to pass the device
> address of virtio request, response and any memory referenced by those
> request/response to the bus master.
> 
> The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to
> map request and response buffers system physical address to the device
> address.
> 
> - If the buffer need to be accessed by both the processor and a bus
>   master then map with BusMasterCommonBuffer.
> 
> - If the buffer need to be accessed for a write operation by a bus master
>   then map with BusMasterWrite.
> 
> - If the buffer need to be accessed for a read  operation by a bus master
>   then map with BusMasterRead.
> 
> 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.c | 138 ++++++++++++++++++--
>  1 file changed, 125 insertions(+), 13 deletions(-)

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

Thanks!
Laszlo

> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 663ba281ab73..c9c42aa41243 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -232,7 +232,8 @@ VerifyReadWriteRequest (
>  
>    @retval EFI_DEVICE_ERROR     Failed to notify host side via VirtIo write, or
>                                 unable to parse host response, or host response
> -                               is not VIRTIO_BLK_S_OK.
> +                               is not VIRTIO_BLK_S_OK or failed to map Buffer
> +                               for a bus master operation.
>  
>  **/
>  
> @@ -249,8 +250,16 @@ SynchronousRequest (
>  {
>    UINT32                  BlockSize;
>    volatile VIRTIO_BLK_REQ Request;
> -  volatile UINT8          HostStatus;
> +  volatile UINT8          *HostStatus;
> +  VOID                    *HostStatusBuffer;
>    DESC_INDICES            Indices;
> +  VOID                    *RequestMapping;
> +  VOID                    *StatusMapping;
> +  VOID                    *BufferMapping;
> +  EFI_PHYSICAL_ADDRESS    BufferDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS    HostStatusDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS    RequestDeviceAddress;
> +  EFI_STATUS              Status;
>  
>    BlockSize = Dev->BlockIoMedia.BlockSize;
>  
> @@ -275,12 +284,82 @@ SynchronousRequest (
>    Request.IoPrio = 0;
>    Request.Sector = MultU64x32(Lba, BlockSize / 512);
>  
> -  VirtioPrepare (&Dev->Ring, &Indices);
> +  //
> +  // Host status is bi-directional (we preset with a value and expect the
> +  // device to update it). Allocate a host status buffer which can be mapped
> +  // to access equally by both processor and the device.
> +  //
> +  Status = Dev->VirtIo->AllocateSharedPages (
> +                          Dev->VirtIo,
> +                          EFI_SIZE_TO_PAGES (sizeof *HostStatus),
> +                          &HostStatusBuffer
> +                          );
> +  if (EFI_ERROR (Status)) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  HostStatus = HostStatusBuffer;
> +
> +  //
> +  // Map virtio-blk request header (must be done after request header is
> +  // populated)
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             (VOID *) &Request,
> +             sizeof Request,
> +             &RequestDeviceAddress,
> +             &RequestMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto FreeHostStatusBuffer;
> +  }
> +
> +  //
> +  // Map data buffer
> +  //
> +  if (BufferSize > 0) {
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               (RequestIsWrite ?
> +                VirtioOperationBusMasterRead :
> +                VirtioOperationBusMasterWrite),
> +               (VOID *) Buffer,
> +               BufferSize,
> +               &BufferDeviceAddress,
> +               &BufferMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = EFI_DEVICE_ERROR;
> +      goto UnmapRequestBuffer;
> +    }
> +  }
>  
>    //
>    // preset a host status for ourselves that we do not accept as success
>    //
> -  HostStatus = VIRTIO_BLK_S_IOERR;
> +  *HostStatus = VIRTIO_BLK_S_IOERR;
> +
> +  //
> +  // Map the Status Buffer with VirtioOperationBusMasterCommonBuffer so that
> +  // both processor and device can access it.
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterCommonBuffer,
> +             HostStatusBuffer,
> +             sizeof *HostStatus,
> +             &HostStatusDeviceAddress,
> +             &StatusMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto UnmapDataBuffer;
> +  }
> +
> +  VirtioPrepare (&Dev->Ring, &Indices);
>  
>    //
>    // ensured by VirtioBlkInit() -- this predicate, in combination with the
> @@ -291,8 +370,13 @@ SynchronousRequest (
>    //
>    // virtio-blk header in first desc
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> -    VRING_DESC_F_NEXT, &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    RequestDeviceAddress,
> +    sizeof Request,
> +    VRING_DESC_F_NEXT,
> +    &Indices
> +    );
>  
>    //
>    // data buffer for read/write in second desc
> @@ -311,27 +395,55 @@ SynchronousRequest (
>      //
>      // VRING_DESC_F_WRITE is interpreted from the host's point of view.
>      //
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize,
> +    VirtioAppendDesc (
> +      &Dev->Ring,
> +      BufferDeviceAddress,
> +      (UINT32) BufferSize,
>        VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE),
> -      &Indices);
> +      &Indices
> +      );
>    }
>  
>    //
>    // host status in last (second or third) desc
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus,
> -    VRING_DESC_F_WRITE, &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    HostStatusDeviceAddress,
> +    sizeof *HostStatus,
> +    VRING_DESC_F_WRITE,
> +    &Indices
> +    );
>  
>    //
>    // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D).
>    //
>    if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices,
>          NULL) == EFI_SUCCESS &&
> -      HostStatus == VIRTIO_BLK_S_OK) {
> -    return EFI_SUCCESS;
> +      *HostStatus == VIRTIO_BLK_S_OK) {
> +    Status = EFI_SUCCESS;
> +  } else {
> +    Status = EFI_DEVICE_ERROR;
>    }
>  
> -  return EFI_DEVICE_ERROR;
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping);
> +
> +UnmapDataBuffer:
> +  if (BufferSize > 0) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> +  }
> +
> +UnmapRequestBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +
> +FreeHostStatusBuffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *HostStatus),
> +                 HostStatusBuffer
> +                 );
> +
> +  return Status;
>  }
>  
>  
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel