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

[PATCH v2 0/4] 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-2

Changes since v1:
 * changes to address v1 feedback


Brijesh Singh (4):
  OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap()
  OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error
  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 | 301 +++++++++++++++++---
 2 files changed, 269 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] 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
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
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

[PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error

[Brijesh Singh]

When virtio request fails we return EFI_DEVICE_ERROR, as per the spec
EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() member function is required
to implement elaborated error reporting.

The patch refactors out entire block of the code that creates the host
adapter error into a separate helper function (ReportHostAdapterError).

Suggested-by: Laszlo Ersek <lersek@redhat.com>
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 | 27 +++++++++++++++-----
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 5e72b1a24b59..cac213129409 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -387,6 +387,26 @@ ParseResponse (
   return EFI_DEVICE_ERROR;
 }
 
+/**
+ * The function can be used to create a fake host adapter error.
+ * When VirtioScsiPassThru is failed due to some reasons then this function
+ * can be called to contstruct a host adapter error.
+ *
+ */
+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;
+}
+
 
 //
 // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL
@@ -472,12 +492,7 @@ VirtioScsiPassThru (
   //
   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;
+    return ReportHostAdapterError (Packet);
   }
 
   return ParseResponse (Packet, &Response);
-- 
2.7.4

[PATCH v2 3/4] 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 | 225 ++++++++++++++++++--
 1 file changed, 209 insertions(+), 16 deletions(-)

diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index cac213129409..3e04097ddd11 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -429,26 +429,161 @@ 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;
+  VOID                      *InDataBuffer;
+  UINTN                     InDataNumPages;
+  BOOLEAN                   InDataBufferIsMapped;
+  BOOLEAN                   OutDataBufferIsMapped;
 
   ZeroMem ((VOID*) &Request, sizeof (Request));
-  ZeroMem ((VOID*) &Response, sizeof (Response));
 
   Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
   CopyMem (&TargetValue, Target, sizeof TargetValue);
 
+  InDataBuffer = NULL;
+  InDataBufferIsMapped = FALSE;
+  OutDataBufferIsMapped = FALSE;
+  InDataNumPages = 0;
+
   Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request);
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
-  VirtioPrepare (&Dev->Ring, &Indices);
+  //
+  // Map the virtio-scsi Request header buffer
+  //
+  Status = VirtioMapAllBytesInSharedBuffer (
+             Dev->VirtIo,
+             VirtioOperationBusMasterRead,
+             (VOID *) &Request,
+             sizeof Request,
+             &RequestDeviceAddress,
+             &RequestMapping);
+  if (EFI_ERROR (Status)) {
+    return ReportHostAdapterError (Packet);
+  }
+
+  //
+  // Map the input buffer
+  //
+  if (Packet->InTransferLength > 0) {
+    //
+    // Allocate a intermediate input buffer. This is mainly to handle the
+    // following case:
+    //  * caller submits a bi-directional request
+    //  * we perform the request fine
+    //  * but we fail to unmap the "InDataMapping"
+    //
+    //  In that case simply returing the EFI_DEVICE_ERROR is not sufficient.
+    //  In addition to the error code we also need to update Packet-xxx fields
+    //  accordingly so that we report the full loss of the incoming transfer.
+    //
+    //  We allocate a temporary buffer and map it with BusMasterCommon. If the
+    //  Virtio request is successful then we copy the data from temporary
+    //  buffer into Packet->InDataBuffer.
+    //
+    InDataNumPages = EFI_SIZE_TO_PAGES (Packet->InTransferLength);
+    Status = Dev->VirtIo->AllocateSharedPages (
+                            Dev->VirtIo,
+                            InDataNumPages,
+                            &InDataBuffer
+                            );
+    if (EFI_ERROR (Status)) {
+      Status = ReportHostAdapterError (Packet);
+      goto UnmapRequestBuffer;
+    }
+
+    ZeroMem (InDataBuffer, Packet->InTransferLength);
+
+    Status = VirtioMapAllBytesInSharedBuffer (
+               Dev->VirtIo,
+               VirtioOperationBusMasterCommonBuffer,
+               InDataBuffer,
+               Packet->InTransferLength,
+               &InDataDeviceAddress,
+               &InDataMapping
+               );
+    if (EFI_ERROR (Status)) {
+      Status = ReportHostAdapterError (Packet);
+      goto FreeInDataBuffer;
+    }
+
+    InDataBufferIsMapped = TRUE;
+  }
+
+  //
+  // Map the output buffer
+  //
+  if (Packet->OutTransferLength > 0) {
+    Status = VirtioMapAllBytesInSharedBuffer (
+               Dev->VirtIo,
+               VirtioOperationBusMasterRead,
+               Packet->OutDataBuffer,
+               Packet->OutTransferLength,
+               &OutDataDeviceAddress,
+               &OutDataMapping
+               );
+    if (EFI_ERROR (Status)) {
+      Status = ReportHostAdapterError (Packet);
+      goto UnmapInDataBuffer;
+    }
+
+    OutDataBufferIsMapped = TRUE;
+  }
+
+  //
+  // 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 = ReportHostAdapterError (Packet);
+    goto UnmapOutDataBuffer;
+  }
+
+  Response = ResponseBuffer;
+
+  ZeroMem ((VOID *)Response, sizeof (*Response));
 
   //
   // 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 = ReportHostAdapterError (Packet);
+    goto FreeResponseBuffer;
+  }
+
+  VirtioPrepare (&Dev->Ring, &Indices);
 
   //
   // ensured by VirtioScsiInit() -- this predicate, in combination with the
@@ -459,31 +594,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.
@@ -492,10 +645,50 @@ VirtioScsiPassThru (
   //
   if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
         &Indices, NULL) != EFI_SUCCESS) {
-    return ReportHostAdapterError (Packet);
+    Status = ReportHostAdapterError (Packet);
+    goto UnmapResponseBuffer;
   }
 
-  return ParseResponse (Packet, &Response);
+  Status = ParseResponse (Packet, Response);
+
+  //
+  // If virtio request was successful and it was a CPU read request then we
+  // have used an intermediate buffer. Copy the data from intermediate buffer
+  // to the final buffer.
+  //
+  if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) {
+    CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength);
+  }
+
+UnmapResponseBuffer:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
+
+FreeResponseBuffer:
+  Dev->VirtIo->FreeSharedPages (
+                 Dev->VirtIo,
+                 EFI_SIZE_TO_PAGES (sizeof *Response),
+                 ResponseBuffer
+                 );
+
+UnmapOutDataBuffer:
+  if (OutDataBufferIsMapped == TRUE) {
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
+  }
+
+UnmapInDataBuffer:
+  if (InDataBufferIsMapped == TRUE) {
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
+  }
+
+FreeInDataBuffer:
+  if (InDataBuffer != NULL) {
+    Dev->VirtIo->FreeSharedPages (Dev->VirtIo, InDataNumPages, InDataBuffer);
+  }
+
+UnmapRequestBuffer:
+  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
+
+  return Status;
 }
 
 
-- 
2.7.4

[PATCH v2 4/4] 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
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
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 3e04097ddd11..4ec22ccd1abe 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -1009,7 +1009,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
@@ -1089,7 +1090,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 v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error

[Laszlo Ersek]

On 08/30/17 22:45, Brijesh Singh wrote:
> When virtio request fails we return EFI_DEVICE_ERROR, as per the spec
> EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() member function is required
> to implement elaborated error reporting.
>
> The patch refactors out entire block of the code that creates the host
> adapter error into a separate helper function (ReportHostAdapterError).
>
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> 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 | 27 +++++++++++++++-----
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e72b1a24b59..cac213129409 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -387,6 +387,26 @@ ParseResponse (
>    return EFI_DEVICE_ERROR;
>  }

OK, I'm glad that you found the best place to introduce the new function
(just below ParseResponse()).

> +/**
> + * The function can be used to create a fake host adapter error.
> + * When VirtioScsiPassThru is failed due to some reasons then this function
> + * can be called to contstruct a host adapter error.
> + *
> + */

(1) This comment block should be formatted according to the edk2 coding
style, and it should document the Packet parameter, and the return
value:

-------
/**

  The function can be used to create a fake host adapter error.

  When VirtioScsiPassThru() is failed due to some reasons then this function
  can be called to construct a host adapter error.

  @param[out] Packet  The Extended SCSI Pass Thru Protocol packet that the host
                      adapter error shall be placed in.


  @retval EFI_DEVICE_ERROR  The function returns this status code
                            unconditionally, to be propagated by
                            VirtioScsiPassThru().

**/
-------

There's no need to resubmit just because of this; if more serious
updates are not needed, I can take care of it.


(2) Another documentation update: pls see the following comment block
(higher up in the source code):

// UEFI Spec 2.3.1 + Errata C, 14.7 Extended SCSI Pass Thru Protocol specifies
// the PassThru() interface. Beside returning a status code, the function must
// set some fields in the EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET in/out
// parameter on return. The following is a full list of those fields, for
// easier validation of PopulateRequest(), ParseResponse(), and
// VirtioScsiPassThru() below.

After this patch, VirtioScsiPassThru() will no longer massage
Packet->xxx fields directly, only via helper functions.

So please replace the "VirtioScsiPassThru()" reference in the comment
block with "ReportHostAdapterError()".

I can take care of this as well, if v3 is not necessary otherwise.

With these documentation updates,

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

Thanks,
Laszlo

> +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;
> +}
> +
>
>  //
>  // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL
> @@ -472,12 +492,7 @@ VirtioScsiPassThru (
>    //
>    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;
> +    return ReportHostAdapterError (Packet);
>    }
>
>    return ParseResponse (Packet, &Response);
>

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

[Laszlo Ersek]

(1) should have noticed it in my previous review: the subject should be
changed like this ("pkg" -> "Pkg"):

-Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers
+OvmfPkg/VirtioScsiDxe: map virtio-scsi request and response buffers

No need to repost just because of this.

On 08/30/17 22:45, 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.

(2) We no longer use BusMasterWrite in this patch -- as I requested, so
that's good --, so I suggest that we continue this paragraph as follows:

"However, after a BusMasterWrite Unmap() failure, error reporting via
EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET would be very complex,
therefore we map such buffers too with BusMasterCommonBuffer."

I can update the commit message.

> 
> - 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 | 225 ++++++++++++++++++--
>  1 file changed, 209 insertions(+), 16 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index cac213129409..3e04097ddd11 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -429,26 +429,161 @@ 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;
> +  VOID                      *InDataBuffer;
> +  UINTN                     InDataNumPages;
> +  BOOLEAN                   InDataBufferIsMapped;
> +  BOOLEAN                   OutDataBufferIsMapped;
>  
>    ZeroMem ((VOID*) &Request, sizeof (Request));
> -  ZeroMem ((VOID*) &Response, sizeof (Response));
>  
>    Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
>    CopyMem (&TargetValue, Target, sizeof TargetValue);
>  
> +  InDataBuffer = NULL;
> +  InDataBufferIsMapped = FALSE;
> +  OutDataBufferIsMapped = FALSE;
> +  InDataNumPages = 0;
> +
>    Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>  
> -  VirtioPrepare (&Dev->Ring, &Indices);
> +  //
> +  // Map the virtio-scsi Request header buffer
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             (VOID *) &Request,
> +             sizeof Request,
> +             &RequestDeviceAddress,
> +             &RequestMapping);
> +  if (EFI_ERROR (Status)) {
> +    return ReportHostAdapterError (Packet);
> +  }
> +
> +  //
> +  // Map the input buffer
> +  //
> +  if (Packet->InTransferLength > 0) {
> +    //
> +    // Allocate a intermediate input buffer. This is mainly to handle the
> +    // following case:
> +    //  * caller submits a bi-directional request
> +    //  * we perform the request fine
> +    //  * but we fail to unmap the "InDataMapping"
> +    //
> +    //  In that case simply returing the EFI_DEVICE_ERROR is not sufficient.
> +    //  In addition to the error code we also need to update Packet-xxx fields

(3) Please replace "Packet-xxx" with just "Packet"

> +    //  accordingly so that we report the full loss of the incoming transfer.
> +    //
> +    //  We allocate a temporary buffer and map it with BusMasterCommon. If the

(4) s/BusMasterCommon/BusMasterCommonBuffer/

Please don't forget to rewrap the text to 79 or 80 columns.

> +    //  Virtio request is successful then we copy the data from temporary
> +    //  buffer into Packet->InDataBuffer.
> +    //

(5) The last two paragraphs in the comment should be un-indented one column.

> +    InDataNumPages = EFI_SIZE_TO_PAGES (Packet->InTransferLength);

(6) "Packet->InTransferLength" has type (UINT32), but
EFI_SIZE_TO_PAGES() takes UINTN.

Please cast the argument to (UINTN).

> +    Status = Dev->VirtIo->AllocateSharedPages (
> +                            Dev->VirtIo,
> +                            InDataNumPages,
> +                            &InDataBuffer
> +                            );
> +    if (EFI_ERROR (Status)) {
> +      Status = ReportHostAdapterError (Packet);
> +      goto UnmapRequestBuffer;
> +    }
> +
> +    ZeroMem (InDataBuffer, Packet->InTransferLength);
> +
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterCommonBuffer,
> +               InDataBuffer,
> +               Packet->InTransferLength,
> +               &InDataDeviceAddress,
> +               &InDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = ReportHostAdapterError (Packet);
> +      goto FreeInDataBuffer;
> +    }
> +
> +    InDataBufferIsMapped = TRUE;
> +  }
> +
> +  //
> +  // Map the output buffer
> +  //
> +  if (Packet->OutTransferLength > 0) {
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterRead,
> +               Packet->OutDataBuffer,
> +               Packet->OutTransferLength,
> +               &OutDataDeviceAddress,
> +               &OutDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = ReportHostAdapterError (Packet);
> +      goto UnmapInDataBuffer;
> +    }
> +
> +    OutDataBufferIsMapped = TRUE;
> +  }
> +
> +  //
> +  // 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 = ReportHostAdapterError (Packet);
> +    goto UnmapOutDataBuffer;
> +  }
> +
> +  Response = ResponseBuffer;
> +
> +  ZeroMem ((VOID *)Response, sizeof (*Response));
>  
>    //
>    // 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 = ReportHostAdapterError (Packet);
> +    goto FreeResponseBuffer;
> +  }
> +
> +  VirtioPrepare (&Dev->Ring, &Indices);
>  
>    //
>    // ensured by VirtioScsiInit() -- this predicate, in combination with the
> @@ -459,31 +594,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.
> @@ -492,10 +645,50 @@ VirtioScsiPassThru (
>    //
>    if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
>          &Indices, NULL) != EFI_SUCCESS) {
> -    return ReportHostAdapterError (Packet);
> +    Status = ReportHostAdapterError (Packet);
> +    goto UnmapResponseBuffer;
>    }
>  
> -  return ParseResponse (Packet, &Response);
> +  Status = ParseResponse (Packet, Response);
> +
> +  //
> +  // If virtio request was successful and it was a CPU read request then we
> +  // have used an intermediate buffer. Copy the data from intermediate buffer
> +  // to the final buffer.
> +  //
> +  if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) {
> +    CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength);
> +  }

(7) The comment is exactly right, but the condition that you check after
is incorrect.

The right thing to do is to call CopyMem() *unconditionally*.

Namely, at this point we are past ParseResponse(). As I wrote before,
ParseResponse() updates the Packet->... fields in every case, even if it
reports an EFI_STATUS that is different from EFI_SUCCESS. And whatever
we expose to the caller through "Packet->InTransferLength" *must* be
reflected in "Packet->InDataBuffer" regardless of return status.

Therefore the Status check must be dropped. And then we need not check
(Packet->InTransferLength>0) either, because the CopyMem() will deal
with it internally.

Think of it like this: the "worst" that can happen, on error, is that
"Packet->InTransferLength" is unchanged from its "input" value, and we
overwrite the caller's "Packet->InDataBuffer" entirely. What is the data
we are going to put there? It's all zeroes, from your

  ZeroMem (InDataBuffer, Packet->InTransferLength);

higher up.

So, again, this CopyMem() needs to be unconditional -- as the comment
says, if the *virtio* request was successful (== we talked to the
virtio-scsi adapter), then we have to copy the data, even if the *SCSI*
request produced an error status in ParseResponse.

> +
> +UnmapResponseBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
> +
> +FreeResponseBuffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *Response),
> +                 ResponseBuffer
> +                 );
> +
> +UnmapOutDataBuffer:
> +  if (OutDataBufferIsMapped == TRUE) {

(8) Just say "if (OutDataBufferIsMapped)"

> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
> +  }
> +
> +UnmapInDataBuffer:
> +  if (InDataBufferIsMapped == TRUE) {

(9) The "InDataBufferIsMapped" variable can be eliminated safely.
Instead, just say

  if (InDataBuffer != NULL)

Because, if you reach this error label, then InDataBuffer!=NULL
guarantees that you have mapped it as well, and InDataBuffer==NULL
guarantees that you have not mapped it either.

Put differently, if InDataBuffer!=NULL, then we must have attempted to
map it (because Packet->InTransferLength was positive on input). If we
succeeded in mapping it, then we have to unmap it. If we failed to map
it, then we're not here! Because we jumped to FreeInDataBuffer, just below.

> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
> +  }
> +
> +FreeInDataBuffer:
> +  if (InDataBuffer != NULL) {
> +    Dev->VirtIo->FreeSharedPages (Dev->VirtIo, InDataNumPages, InDataBuffer);
> +  }
> +
> +UnmapRequestBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +
> +  return Status;
>  }
>  
>  
> 

Otherwise, the logic is very good in this patch; all the updates I'm
requesting are localized tweaks, and the only control flow change is
point (7) -- which is also very tight.

For the updates above, I must ask you for v3, but I trust that v3 is
going to be the final version.

Thank you!
Laszlo

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

[Laszlo Ersek]

On 08/31/17 15:23, Laszlo Ersek wrote:
> On 08/30/17 22:45, Brijesh Singh wrote:

>> @@ -492,10 +645,50 @@ VirtioScsiPassThru (
>>    //
>>    if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
>>          &Indices, NULL) != EFI_SUCCESS) {
>> -    return ReportHostAdapterError (Packet);
>> +    Status = ReportHostAdapterError (Packet);
>> +    goto UnmapResponseBuffer;
>>    }
>>
>> -  return ParseResponse (Packet, &Response);
>> +  Status = ParseResponse (Packet, Response);
>> +
>> +  //
>> +  // If virtio request was successful and it was a CPU read request then we
>> +  // have used an intermediate buffer. Copy the data from intermediate buffer
>> +  // to the final buffer.
>> +  //
>> +  if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) {
>> +    CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength);
>> +  }
>
> (7) The comment is exactly right, but the condition that you check
> after is incorrect.
>
> The right thing to do is to call CopyMem() *unconditionally*.
>
> Namely, at this point we are past ParseResponse(). As I wrote before,
> ParseResponse() updates the Packet->... fields in every case, even if
> it reports an EFI_STATUS that is different from EFI_SUCCESS. And
> whatever we expose to the caller through "Packet->InTransferLength"
> *must* be reflected in "Packet->InDataBuffer" regardless of return
> status.
>
> Therefore the Status check must be dropped. And then we need not check
> (Packet->InTransferLength>0) either, because the CopyMem() will deal
> with it internally.
>
> Think of it like this: the "worst" that can happen, on error, is that
> "Packet->InTransferLength" is unchanged from its "input" value, and we
> overwrite the caller's "Packet->InDataBuffer" entirely. What is the
> data we are going to put there? It's all zeroes, from your
>
>   ZeroMem (InDataBuffer, Packet->InTransferLength);
>
> higher up.
>
> So, again, this CopyMem() needs to be unconditional -- as the comment
> says, if the *virtio* request was successful (== we talked to the
> virtio-scsi adapter), then we have to copy the data, even if the
> *SCSI* request produced an error status in ParseResponse.

I have to correct myself a little bit -- although I think you would have
caught me anyway :) --, namely we should keep the "if", but the
condition should be:

  InDataBuffer != NULL

Admittedly, it is likely that none of the CopyMem() implementations
would have problems with a NULL "SourceBuffer", if "Length" was zero.

Nonetheless, the interface contract in

  MdePkg/Include/Library/BaseMemoryLib.h

does not mark SourceBuffer OPTIONAL -- neither does the UEFI spec, for
the similar gBS->CopyMem() boot service --, for the case when Length==0,
so we should do an explicit check:

  if (InDataBuffer != NULL) {
    CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength);
  }

Thank you,
Laszlo

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

[Brijesh Singh]

Thanks for quick feedback Laszlo.

I am addressing all your review comments and submitting the v3 soon

-Brijesh

On 08/31/2017 08:49 AM, Laszlo Ersek wrote:
> On 08/31/17 15:23, Laszlo Ersek wrote:
>> On 08/30/17 22:45, Brijesh Singh wrote:
> 
>>> @@ -492,10 +645,50 @@ VirtioScsiPassThru (
>>>     //
>>>     if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
>>>           &Indices, NULL) != EFI_SUCCESS) {
>>> -    return ReportHostAdapterError (Packet);
>>> +    Status = ReportHostAdapterError (Packet);
>>> +    goto UnmapResponseBuffer;
>>>     }
>>>
>>> -  return ParseResponse (Packet, &Response);
>>> +  Status = ParseResponse (Packet, Response);
>>> +
>>> +  //
>>> +  // If virtio request was successful and it was a CPU read request then we
>>> +  // have used an intermediate buffer. Copy the data from intermediate buffer
>>> +  // to the final buffer.
>>> +  //
>>> +  if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) {
>>> +    CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength);
>>> +  }
>>
>> (7) The comment is exactly right, but the condition that you check
>> after is incorrect.
>>
>> The right thing to do is to call CopyMem() *unconditionally*.
>>
>> Namely, at this point we are past ParseResponse(). As I wrote before,
>> ParseResponse() updates the Packet->... fields in every case, even if
>> it reports an EFI_STATUS that is different from EFI_SUCCESS. And
>> whatever we expose to the caller through "Packet->InTransferLength"
>> *must* be reflected in "Packet->InDataBuffer" regardless of return
>> status.
>>
>> Therefore the Status check must be dropped. And then we need not check
>> (Packet->InTransferLength>0) either, because the CopyMem() will deal
>> with it internally.
>>
>> Think of it like this: the "worst" that can happen, on error, is that
>> "Packet->InTransferLength" is unchanged from its "input" value, and we
>> overwrite the caller's "Packet->InDataBuffer" entirely. What is the
>> data we are going to put there? It's all zeroes, from your
>>
>>    ZeroMem (InDataBuffer, Packet->InTransferLength);
>>
>> higher up.
>>
>> So, again, this CopyMem() needs to be unconditional -- as the comment
>> says, if the *virtio* request was successful (== we talked to the
>> virtio-scsi adapter), then we have to copy the data, even if the
>> *SCSI* request produced an error status in ParseResponse.
> 
> I have to correct myself a little bit -- although I think you would have
> caught me anyway :) --, namely we should keep the "if", but the
> condition should be:
> 
>    InDataBuffer != NULL
> 
> Admittedly, it is likely that none of the CopyMem() implementations
> would have problems with a NULL "SourceBuffer", if "Length" was zero.
> 
> Nonetheless, the interface contract in
> 
>    MdePkg/Include/Library/BaseMemoryLib.h
> 
> does not mark SourceBuffer OPTIONAL -- neither does the UEFI spec, for
> the similar gBS->CopyMem() boot service --, for the case when Length==0,
> so we should do an explicit check:
> 
>    if (InDataBuffer != NULL) {
>      CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength);
>    }
> 
> Thank you,
> Laszlo
>