[PATCH 0/3] OvmfPkg/VirtioScsiDxe: map host address to device address

[PATCH 0/3] OvmfPkg/VirtioScsiDxe: map host address to device address

[Brijesh Singh]

The patch updates VirtioScsiDxe to use IOMMU-like member functions to map
the system physical address to device address for buffers (including vring,
device specific request and response pointed by vring descriptor, and any
furter memory reference by those request and response).

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>

Repo: https://github.com/codomania/edk2
Branch: virtio-scsi-1

Brijesh Singh (3):
  OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap()
  Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers
  OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM

 OvmfPkg/VirtioScsiDxe/VirtioScsi.h |   1 +
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 220 +++++++++++++++++---
 2 files changed, 193 insertions(+), 28 deletions(-)

-- 
2.7.4

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

[Brijesh Singh]

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(-)

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);
 }
 
 
-- 
2.7.4

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

[Laszlo Ersek]

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

[PATCH 2/3] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers

[Brijesh Singh]

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/VirtioScsiDxe/VirtioScsi.c | 168 ++++++++++++++++++--
 1 file changed, 152 insertions(+), 16 deletions(-)

diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 5e72b1a24b59..a1ee810919e4 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -409,11 +409,19 @@ VirtioScsiPassThru (
   UINT16                    TargetValue;
   EFI_STATUS                Status;
   volatile VIRTIO_SCSI_REQ  Request;
-  volatile VIRTIO_SCSI_RESP Response;
+  volatile VIRTIO_SCSI_RESP *Response;
+  VOID                      *ResponseBuffer;
   DESC_INDICES              Indices;
+  VOID                      *RequestMapping;
+  VOID                      *ResponseMapping;
+  VOID                      *InDataMapping;
+  VOID                      *OutDataMapping;
+  EFI_PHYSICAL_ADDRESS      RequestDeviceAddress;
+  EFI_PHYSICAL_ADDRESS      ResponseDeviceAddress;
+  EFI_PHYSICAL_ADDRESS      InDataDeviceAddress;
+  EFI_PHYSICAL_ADDRESS      OutDataDeviceAddress;
 
   ZeroMem ((VOID*) &Request, sizeof (Request));
-  ZeroMem ((VOID*) &Response, sizeof (Response));
 
   Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
   CopyMem (&TargetValue, Target, sizeof TargetValue);
@@ -423,12 +431,96 @@ VirtioScsiPassThru (
     return Status;
   }
 
-  VirtioPrepare (&Dev->Ring, &Indices);
+  //
+  // Map the scsi-blk Request header buffer
+  //
+  Status = VirtioMapAllBytesInSharedBuffer (
+             Dev->VirtIo,
+             VirtioOperationBusMasterRead,
+             (VOID *) &Request,
+             sizeof Request,
+             &RequestDeviceAddress,
+             &RequestMapping);
+  if (EFI_ERROR (Status)) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  //
+  // Map the input buffer
+  //
+  if (Packet->InTransferLength > 0) {
+    Status = VirtioMapAllBytesInSharedBuffer (
+               Dev->VirtIo,
+               VirtioOperationBusMasterWrite,
+               Packet->InDataBuffer,
+               Packet->InTransferLength,
+               &InDataDeviceAddress,
+               &InDataMapping
+               );
+    if (EFI_ERROR (Status)) {
+      Status = EFI_DEVICE_ERROR;
+      goto UnmapRequestBuffer;
+    }
+  }
+
+  //
+  // Map the output buffer
+  //
+  if (Packet->OutTransferLength > 0) {
+    Status = VirtioMapAllBytesInSharedBuffer (
+               Dev->VirtIo,
+               VirtioOperationBusMasterRead,
+               Packet->OutDataBuffer,
+               Packet->OutTransferLength,
+               &OutDataDeviceAddress,
+               &OutDataMapping
+               );
+    if (EFI_ERROR (Status)) {
+      Status = EFI_DEVICE_ERROR;
+      goto UnmapInDataBuffer;
+    }
+  }
+
+  //
+  // Response header is bi-direction (we preset with host status and expect the
+  // device to update it). Allocate a response buffer which can be mapped to
+  // access equally by both processor and device.
+  //
+  Status = Dev->VirtIo->AllocateSharedPages (
+                          Dev->VirtIo,
+                          EFI_SIZE_TO_PAGES (sizeof *Response),
+                          &ResponseBuffer
+                          );
+  if (EFI_ERROR (Status)) {
+    Status = EFI_DEVICE_ERROR;
+    goto UnmapOutDataBuffer;
+  }
+
+  Response = ResponseBuffer;
 
   //
   // preset a host status for ourselves that we do not accept as success
   //
-  Response.Response = VIRTIO_SCSI_S_FAILURE;
+  Response->Response = VIRTIO_SCSI_S_FAILURE;
+
+  //
+  // Map the response buffer with BusMasterCommonBuffer so that response
+  // buffer can be accessed by both host and device.
+  //
+  Status = VirtioMapAllBytesInSharedBuffer (
+             Dev->VirtIo,
+             VirtioOperationBusMasterCommonBuffer,
+             ResponseBuffer,
+             sizeof (*Response),
+             &ResponseDeviceAddress,
+             &ResponseMapping
+             );
+  if (EFI_ERROR (Status)) {
+    Status = EFI_DEVICE_ERROR;
+    goto FreeResponseBuffer;
+  }
+
+  VirtioPrepare (&Dev->Ring, &Indices);
 
   //
   // ensured by VirtioScsiInit() -- this predicate, in combination with the
@@ -439,31 +531,49 @@ VirtioScsiPassThru (
   //
   // enqueue Request
   //
-  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
-    VRING_DESC_F_NEXT, &Indices);
+  VirtioAppendDesc (
+    &Dev->Ring,
+    RequestDeviceAddress,
+    sizeof Request,
+    VRING_DESC_F_NEXT,
+    &Indices
+    );
 
   //
   // enqueue "dataout" if any
   //
   if (Packet->OutTransferLength > 0) {
-    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer,
-      Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices);
+    VirtioAppendDesc (
+      &Dev->Ring,
+      OutDataDeviceAddress,
+      Packet->OutTransferLength,
+      VRING_DESC_F_NEXT,
+      &Indices
+      );
   }
 
   //
   // enqueue Response, to be written by the host
   //
-  VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response,
-    VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ?
-                          VRING_DESC_F_NEXT : 0),
-    &Indices);
+  VirtioAppendDesc (
+    &Dev->Ring,
+    ResponseDeviceAddress,
+    sizeof *Response,
+    VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0),
+    &Indices
+    );
 
   //
   // enqueue "datain" if any, to be written by the host
   //
   if (Packet->InTransferLength > 0) {
-    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer,
-      Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices);
+    VirtioAppendDesc (
+      &Dev->Ring,
+      InDataDeviceAddress,
+      Packet->InTransferLength,
+      VRING_DESC_F_WRITE,
+      &Indices
+      );
   }
 
   // If kicking the host fails, we must fake a host adapter error.
@@ -477,10 +587,36 @@ VirtioScsiPassThru (
     Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
     Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
     Packet->SenseDataLength   = 0;
-    return EFI_DEVICE_ERROR;
+    Status = EFI_DEVICE_ERROR;
+    goto UnmapResponseBuffer;
   }
 
-  return ParseResponse (Packet, &Response);
+  Status = ParseResponse (Packet, Response);
+
+UnmapResponseBuffer:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
+
+FreeResponseBuffer:
+  Dev->VirtIo->FreeSharedPages (
+                 Dev->VirtIo,
+                 EFI_SIZE_TO_PAGES (sizeof *Response),
+                 ResponseBuffer
+                 );
+
+UnmapOutDataBuffer:
+  if (Packet->OutTransferLength > 0) {
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
+  }
+
+UnmapInDataBuffer:
+  if (Packet->InTransferLength > 0) {
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
+  }
+
+UnmapRequestBuffer:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
+
+  return Status;
 }
 
 
-- 
2.7.4

Re: [PATCH 2/3] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers

[Laszlo Ersek]

Hi Brijesh,

On 08/28/17 13:26, 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/VirtioScsiDxe/VirtioScsi.c | 168 ++++++++++++++++++--
>  1 file changed, 152 insertions(+), 16 deletions(-)

I have several comments here. I believe that this time my comments are
best kept together, so I'm not going to scatter them all over the patch.

(1) You have a typo below, "scsi-blk". I think you meant "virtio-scsi"
instead.


(2) "ResponseBuffer" is not adequately zeroed before it is mapped. The
Response->Response field is set correctly, but the entire structure
should be zeroed, after it is allocated. (Practically in place of the
ZeroMem() that you remove at the top of the patch.)

For VirtioBlkDxe, this wasn't an issue, because there the entire
response buffer was a single byte, "HostStatus". By setting that byte,
no stale data was left in the area that would be exposed to the host.


(3) By reviewing this patch, I realized that I missed an error in commit
3540f2cef57a ("Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response
buffers", 2017-08-27).

In other words, this point is about the VirtioBlkDxe driver.

Namely, when "RequestIsWrite" is FALSE -- i.e., the CPU wants data from
the device --, we map "Buffer" for VirtioOperationBusMasterWrite. In
this case, checking the return status of

  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);

is critical -- we must not do that unmapping on the error path only. If
the unmapping fails, then the device's data don't reach the caller, and
we must fail the request with EFI_DEVICE_ERROR.

I believe the mitigation could be:

> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 6abd937f86c6..5a63986b3f39 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -260,6 +260,7 @@ SynchronousRequest (
>    EFI_PHYSICAL_ADDRESS    HostStatusDeviceAddress;
>    EFI_PHYSICAL_ADDRESS    RequestDeviceAddress;
>    EFI_STATUS              Status;
> +  EFI_STATUS              UnmapStatus;
>
>    BlockSize = Dev->BlockIoMedia.BlockSize;
>
> @@ -430,7 +431,13 @@ SynchronousRequest (
>
>  UnmapDataBuffer:
>    if (BufferSize > 0) {
> -    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> +    UnmapStatus = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> +    if (EFI_ERROR (UnmapStatus) && !RequestIsWrite && !EFI_ERROR (Status)) {
> +      //
> +      // Data from the bus master may not reach the caller; fail the request.
> +      //
> +      Status = EFI_DEVICE_ERROR;
> +    }
>    }
>
>  UnmapRequestBuffer:

I'm very sorry about this. If you agree with the above suggestion, can
you please submit it as a standalone patch?


(4) Back to this patch. The EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru()
member function is required to implement elaborate error reporting, and
some output fields must be set even if we report EFI_DEVICE_ERROR. The
pre-patch code conforms to the UEFI spec's requirements, and we should
keep that up.

Please locate the following code:

  // If kicking the host fails, we must fake a host adapter error.
  // EFI_NOT_READY would save us the effort, but it would also suggest that the
  // caller retry.
  //
  if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
        &Indices, NULL) != EFI_SUCCESS) {
    Packet->InTransferLength  = 0;
    Packet->OutTransferLength = 0;
    Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
    Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
    Packet->SenseDataLength   = 0;
    return EFI_DEVICE_ERROR;
  }

This entire block of code should be factored out to a helper function,
in a separate patch:

STATIC
EFI_STATUS
ReportHostAdapterError (
  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
  )
{
  Packet->InTransferLength  = 0;
  Packet->OutTransferLength = 0;
  Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
  Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
  Packet->SenseDataLength   = 0;
  return EFI_DEVICE_ERROR;
}

Then, in this patch, *whenever* we intend to return EFI_DEVICE_ERROR
from *before* VirtioFlush(), do:

  /* attempt shared pages allocation or mapping, as appropriate */
  /* ... */
  if (EFI_ERROR (Status)) {
    Status = ReportHostAdapterError (Packet);
    goto ErrorHandlingLabel;
  }

(The same should also be performed when VirtioFlush() itself fails, of
course.)

The idea is, if PopulateRequest() succeeds, but we don't reach
VirtioFlush() for another -- new -- error scenario, or VirtioFlush()
itself fails (like before), then we must fake this kind of host adapter
error in *all* cases. The above helper function will simplify that.


(5) The same issue as (3) is present in this patch (i.e., for
VirtioScsiDxe); however, it is more complicated here.

Assume that

- the caller submits a bi-directional request,

- we actually perform the request fine,

- but then we fail to unmap "InDataMapping".

In this case, flipping the return status to EFI_DEVICE_ERROR just
doesn't cut it: we'd have to update the Packet->xxxx fields accordingly,
so that they report the full loss of the incoming transfer. This would
be hellishly difficult; the update would have to be different for a
bidirectional transfer vs. for a simple read, and in general, who wants
to mess with SCSI sense data?

Therefore we have to guarantee *in advance* that this error won't
present itself. Which means, of course, a CommonBuffer mapping for the
input transfer (if any):

(5a) If "Packet->InTransferLength" is positive on input, allocate a
shared buffer, zero it (!), and finally map it for CommonBuffer
operation (--> InDataMapping). Proceed with the rest of the patch.

(5b) If VirtioFlush() fails, then do as before (see point (4) above) --
report a host adapter error.

(5c) if VirtioFlush() succeeds, then call ParseResponse(), storing its
return value in "Status".

(5d) Now, ParseResponse() will *always* update
"Packet->InTransferLength". Therefore, after ParseResponse() returns, if
"Packet->InTransferLength" is positive, then we have to CopyMem()
"Packet->InTransferLength" bytes from the common buffer to
"Packet->InDataBuffer".

This way, we'll only have to unmap BusMasterCommonBuffer and
BusMasterRead mappings on the final path (both on success and error),
and we won't have to care about their return values.

Also, on the final path (on both success and error), we don't have to
touch "Packet":

- we either reached ParseResponse(), and then it set the output fields
  appropriately,

- or we jumped over ParseResponse() due to an error, but in all such
  cases, we called ReportHostAdapterError(), so the output fields are
  set again.

Thank you,
Laszlo


On 08/28/17 13:26, Brijesh Singh wrote:

> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e72b1a24b59..a1ee810919e4 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -409,11 +409,19 @@ VirtioScsiPassThru (
>    UINT16                    TargetValue;
>    EFI_STATUS                Status;
>    volatile VIRTIO_SCSI_REQ  Request;
> -  volatile VIRTIO_SCSI_RESP Response;
> +  volatile VIRTIO_SCSI_RESP *Response;
> +  VOID                      *ResponseBuffer;
>    DESC_INDICES              Indices;
> +  VOID                      *RequestMapping;
> +  VOID                      *ResponseMapping;
> +  VOID                      *InDataMapping;
> +  VOID                      *OutDataMapping;
> +  EFI_PHYSICAL_ADDRESS      RequestDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      ResponseDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      InDataDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      OutDataDeviceAddress;
>
>    ZeroMem ((VOID*) &Request, sizeof (Request));
> -  ZeroMem ((VOID*) &Response, sizeof (Response));
>
>    Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
>    CopyMem (&TargetValue, Target, sizeof TargetValue);
> @@ -423,12 +431,96 @@ VirtioScsiPassThru (
>      return Status;
>    }
>
> -  VirtioPrepare (&Dev->Ring, &Indices);
> +  //
> +  // Map the scsi-blk Request header buffer
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             (VOID *) &Request,
> +             sizeof Request,
> +             &RequestDeviceAddress,
> +             &RequestMapping);
> +  if (EFI_ERROR (Status)) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  //
> +  // Map the input buffer
> +  //
> +  if (Packet->InTransferLength > 0) {
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterWrite,
> +               Packet->InDataBuffer,
> +               Packet->InTransferLength,
> +               &InDataDeviceAddress,
> +               &InDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = EFI_DEVICE_ERROR;
> +      goto UnmapRequestBuffer;
> +    }
> +  }
> +
> +  //
> +  // Map the output buffer
> +  //
> +  if (Packet->OutTransferLength > 0) {
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterRead,
> +               Packet->OutDataBuffer,
> +               Packet->OutTransferLength,
> +               &OutDataDeviceAddress,
> +               &OutDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = EFI_DEVICE_ERROR;
> +      goto UnmapInDataBuffer;
> +    }
> +  }
> +
> +  //
> +  // Response header is bi-direction (we preset with host status and expect the
> +  // device to update it). Allocate a response buffer which can be mapped to
> +  // access equally by both processor and device.
> +  //
> +  Status = Dev->VirtIo->AllocateSharedPages (
> +                          Dev->VirtIo,
> +                          EFI_SIZE_TO_PAGES (sizeof *Response),
> +                          &ResponseBuffer
> +                          );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto UnmapOutDataBuffer;
> +  }
> +
> +  Response = ResponseBuffer;
>
>    //
>    // preset a host status for ourselves that we do not accept as success
>    //
> -  Response.Response = VIRTIO_SCSI_S_FAILURE;
> +  Response->Response = VIRTIO_SCSI_S_FAILURE;
> +
> +  //
> +  // Map the response buffer with BusMasterCommonBuffer so that response
> +  // buffer can be accessed by both host and device.
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterCommonBuffer,
> +             ResponseBuffer,
> +             sizeof (*Response),
> +             &ResponseDeviceAddress,
> +             &ResponseMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto FreeResponseBuffer;
> +  }
> +
> +  VirtioPrepare (&Dev->Ring, &Indices);
>
>    //
>    // ensured by VirtioScsiInit() -- this predicate, in combination with the
> @@ -439,31 +531,49 @@ VirtioScsiPassThru (
>    //
>    // enqueue Request
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> -    VRING_DESC_F_NEXT, &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    RequestDeviceAddress,
> +    sizeof Request,
> +    VRING_DESC_F_NEXT,
> +    &Indices
> +    );
>
>    //
>    // enqueue "dataout" if any
>    //
>    if (Packet->OutTransferLength > 0) {
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer,
> -      Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices);
> +    VirtioAppendDesc (
> +      &Dev->Ring,
> +      OutDataDeviceAddress,
> +      Packet->OutTransferLength,
> +      VRING_DESC_F_NEXT,
> +      &Indices
> +      );
>    }
>
>    //
>    // enqueue Response, to be written by the host
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response,
> -    VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ?
> -                          VRING_DESC_F_NEXT : 0),
> -    &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    ResponseDeviceAddress,
> +    sizeof *Response,
> +    VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0),
> +    &Indices
> +    );
>
>    //
>    // enqueue "datain" if any, to be written by the host
>    //
>    if (Packet->InTransferLength > 0) {
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer,
> -      Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices);
> +    VirtioAppendDesc (
> +      &Dev->Ring,
> +      InDataDeviceAddress,
> +      Packet->InTransferLength,
> +      VRING_DESC_F_WRITE,
> +      &Indices
> +      );
>    }
>
>    // If kicking the host fails, we must fake a host adapter error.
> @@ -477,10 +587,36 @@ VirtioScsiPassThru (
>      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
>      Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
>      Packet->SenseDataLength   = 0;
> -    return EFI_DEVICE_ERROR;
> +    Status = EFI_DEVICE_ERROR;
> +    goto UnmapResponseBuffer;
>    }
>
> -  return ParseResponse (Packet, &Response);
> +  Status = ParseResponse (Packet, Response);
> +
> +UnmapResponseBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
> +
> +FreeResponseBuffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *Response),
> +                 ResponseBuffer
> +                 );
> +
> +UnmapOutDataBuffer:
> +  if (Packet->OutTransferLength > 0) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
> +  }
> +
> +UnmapInDataBuffer:
> +  if (Packet->InTransferLength > 0) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
> +  }
> +
> +UnmapRequestBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +
> +  return Status;
>  }
>
>
>

Re: [PATCH 2/3] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers

[Brijesh Singh]

On 8/29/17 11:02 AM, Laszlo Ersek wrote:
> Hi Brijesh,
>
> On 08/28/17 13:26, 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/VirtioScsiDxe/VirtioScsi.c | 168 ++++++++++++++++++--
>>  1 file changed, 152 insertions(+), 16 deletions(-)
> I have several comments here. I believe that this time my comments are
> best kept together, so I'm not going to scatter them all over the patch.
>
> (1) You have a typo below, "scsi-blk". I think you meant "virtio-scsi"
> instead.

Will fix it, thanks
>
> (2) "ResponseBuffer" is not adequately zeroed before it is mapped. The
> Response->Response field is set correctly, but the entire structure
> should be zeroed, after it is allocated. (Practically in place of the
> ZeroMem() that you remove at the top of the patch.)
>
> For VirtioBlkDxe, this wasn't an issue, because there the entire
> response buffer was a single byte, "HostStatus". By setting that byte,
> no stale data was left in the area that would be exposed to the host.

Yes good catch. will fix in v2

>
> (3) By reviewing this patch, I realized that I missed an error in commit
> 3540f2cef57a ("Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response
> buffers", 2017-08-27).
>
> In other words, this point is about the VirtioBlkDxe driver.
>
> Namely, when "RequestIsWrite" is FALSE -- i.e., the CPU wants data from
> the device --, we map "Buffer" for VirtioOperationBusMasterWrite. In
> this case, checking the return status of
>
>   Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
>
> is critical -- we must not do that unmapping on the error path only. If
> the unmapping fails, then the device's data don't reach the caller, and
> we must fail the request with EFI_DEVICE_ERROR.
>
> I believe the mitigation could be:

I will send separate patch to handle this case.

>> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>> b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>> index 6abd937f86c6..5a63986b3f39 100644
>> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
>> @@ -260,6 +260,7 @@ SynchronousRequest (
>>    EFI_PHYSICAL_ADDRESS    HostStatusDeviceAddress;
>>    EFI_PHYSICAL_ADDRESS    RequestDeviceAddress;
>>    EFI_STATUS              Status;
>> +  EFI_STATUS              UnmapStatus;
>>
>>    BlockSize = Dev->BlockIoMedia.BlockSize;
>>
>> @@ -430,7 +431,13 @@ SynchronousRequest (
>>
>>  UnmapDataBuffer:
>>    if (BufferSize > 0) {
>> -    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
>> +    UnmapStatus = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
>> +    if (EFI_ERROR (UnmapStatus) && !RequestIsWrite && !EFI_ERROR (Status)) {
>> +      //
>> +      // Data from the bus master may not reach the caller; fail the request.
>> +      //
>> +      Status = EFI_DEVICE_ERROR;
>> +    }
>>    }
>>
>>  UnmapRequestBuffer:
> I'm very sorry about this. If you agree with the above suggestion, can
> you please submit it as a standalone patch?
>
>
> (4) Back to this patch. The EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru()
> member function is required to implement elaborate error reporting, and
> some output fields must be set even if we report EFI_DEVICE_ERROR. The
> pre-patch code conforms to the UEFI spec's requirements, and we should
> keep that up.
>
> Please locate the following code:
>
>   // If kicking the host fails, we must fake a host adapter error.
>   // EFI_NOT_READY would save us the effort, but it would also suggest that the
>   // caller retry.
>   //
>   if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
>         &Indices, NULL) != EFI_SUCCESS) {
>     Packet->InTransferLength  = 0;
>     Packet->OutTransferLength = 0;
>     Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
>     Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
>     Packet->SenseDataLength   = 0;
>     return EFI_DEVICE_ERROR;
>   }
>
> This entire block of code should be factored out to a helper function,
> in a separate patch:
>
> STATIC
> EFI_STATUS
> ReportHostAdapterError (
>   OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
>   )
> {
>   Packet->InTransferLength  = 0;
>   Packet->OutTransferLength = 0;
>   Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
>   Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
>   Packet->SenseDataLength   = 0;
>   return EFI_DEVICE_ERROR;
> }
>
> Then, in this patch, *whenever* we intend to return EFI_DEVICE_ERROR
> from *before* VirtioFlush(), do:
>
>   /* attempt shared pages allocation or mapping, as appropriate */
>   /* ... */
>   if (EFI_ERROR (Status)) {
>     Status = ReportHostAdapterError (Packet);
>     goto ErrorHandlingLabel;
>   }
>
> (The same should also be performed when VirtioFlush() itself fails, of
> course.)
>
> The idea is, if PopulateRequest() succeeds, but we don't reach
> VirtioFlush() for another -- new -- error scenario, or VirtioFlush()
> itself fails (like before), then we must fake this kind of host adapter
> error in *all* cases. The above helper function will simplify that.
>
>
> (5) The same issue as (3) is present in this patch (i.e., for
> VirtioScsiDxe); however, it is more complicated here.
>
> Assume that
>
> - the caller submits a bi-directional request,
>
> - we actually perform the request fine,
>
> - but then we fail to unmap "InDataMapping".
>
> In this case, flipping the return status to EFI_DEVICE_ERROR just
> doesn't cut it: we'd have to update the Packet->xxxx fields accordingly,
> so that they report the full loss of the incoming transfer. This would
> be hellishly difficult; the update would have to be different for a
> bidirectional transfer vs. for a simple read, and in general, who wants
> to mess with SCSI sense data?
>
> Therefore we have to guarantee *in advance* that this error won't
> present itself. Which means, of course, a CommonBuffer mapping for the
> input transfer (if any):
>
> (5a) If "Packet->InTransferLength" is positive on input, allocate a
> shared buffer, zero it (!), and finally map it for CommonBuffer
> operation (--> InDataMapping). Proceed with the rest of the patch.
>
> (5b) If VirtioFlush() fails, then do as before (see point (4) above) --
> report a host adapter error.
>
> (5c) if VirtioFlush() succeeds, then call ParseResponse(), storing its
> return value in "Status".
>
> (5d) Now, ParseResponse() will *always* update
> "Packet->InTransferLength". Therefore, after ParseResponse() returns, if
> "Packet->InTransferLength" is positive, then we have to CopyMem()
> "Packet->InTransferLength" bytes from the common buffer to
> "Packet->InDataBuffer".
>
> This way, we'll only have to unmap BusMasterCommonBuffer and
> BusMasterRead mappings on the final path (both on success and error),
> and we won't have to care about their return values.
>
> Also, on the final path (on both success and error), we don't have to
> touch "Packet":
>
> - we either reached ParseResponse(), and then it set the output fields
>   appropriately,
>
> - or we jumped over ParseResponse() due to an error, but in all such
>   cases, we called ReportHostAdapterError(), so the output fields are
>   set again.

I will go ahead and implement your feedback in v2 and send it possibly
today. thanks

[PATCH 3/3] OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM

[Brijesh Singh]

VirtioScsiDxe driver has been updated to use IOMMU-like member functions
from VIRTIO_DEVICE_PROTOCOL to translate the system physical address to
device address. We do not need to do anything special when
VIRTIO_F_IOMMU_PLATFORM bit is present hence treat it in parallel with
VIRTIO_F_VERSION_1.

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.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index a1ee810919e4..7028d04f5165 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -937,7 +937,8 @@ VirtioScsiInit (
     goto Failed;
   }
 
-  Features &= VIRTIO_SCSI_F_INOUT | VIRTIO_F_VERSION_1;
+  Features &= VIRTIO_SCSI_F_INOUT | VIRTIO_F_VERSION_1 |
+              VIRTIO_F_IOMMU_PLATFORM;
 
   //
   // In virtio-1.0, feature negotiation is expected to complete before queue
@@ -1017,7 +1018,7 @@ VirtioScsiInit (
   // step 5 -- Report understood features and guest-tuneables.
   //
   if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) {
-    Features &= ~(UINT64)VIRTIO_F_VERSION_1;
+    Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM);
     Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
     if (EFI_ERROR (Status)) {
       goto UnmapQueue;
-- 
2.7.4

Re: [PATCH 3/3] OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM

[Laszlo Ersek]

On 08/28/17 13:26, Brijesh Singh wrote:
> VirtioScsiDxe driver has been updated to use IOMMU-like member functions
> from VIRTIO_DEVICE_PROTOCOL to translate the system physical address to
> device address. We do not need to do anything special when
> VIRTIO_F_IOMMU_PLATFORM bit is present hence treat it in parallel with
> VIRTIO_F_VERSION_1.
> 
> 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.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index a1ee810919e4..7028d04f5165 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -937,7 +937,8 @@ VirtioScsiInit (
>      goto Failed;
>    }
>  
> -  Features &= VIRTIO_SCSI_F_INOUT | VIRTIO_F_VERSION_1;
> +  Features &= VIRTIO_SCSI_F_INOUT | VIRTIO_F_VERSION_1 |
> +              VIRTIO_F_IOMMU_PLATFORM;
>  
>    //
>    // In virtio-1.0, feature negotiation is expected to complete before queue
> @@ -1017,7 +1018,7 @@ VirtioScsiInit (
>    // step 5 -- Report understood features and guest-tuneables.
>    //
>    if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) {
> -    Features &= ~(UINT64)VIRTIO_F_VERSION_1;
> +    Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM);
>      Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
>      if (EFI_ERROR (Status)) {
>        goto UnmapQueue;
> 

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