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

[PATCH v3 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-3

Changes since v2:
 * changes to address v2 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 | 306 +++++++++++++++++---
 2 files changed, 273 insertions(+), 34 deletions(-)

-- 
2.7.4

[PATCH v3 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 v3 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
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 36 ++++++++++++++++----
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 5e72b1a24b59..5e977c636a0a 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -97,7 +97,7 @@
 // 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.
+// ReportHostAdapterError() below.
 //
 // - InTransferLength
 // - OutTransferLength
@@ -387,6 +387,33 @@ 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.
+ *
+ * @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().
+ */
+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 +499,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 v3 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.

  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.

- 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 | 220 ++++++++++++++++++--
 1 file changed, 204 insertions(+), 16 deletions(-)

diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 5e977c636a0a..337fb4b2f1e0 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -436,26 +436,156 @@ 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                   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;
+  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 fields
+    //  accordingly so that we report the full loss of the incoming transfer.
+    //  We allocate a temporary buffer and map it with BusMasterCommonBuffer.
+    //  If the Virtio request is successful then we copy the data from
+    //  temporary buffer into Packet->InDataBuffer.
+    //
+    InDataNumPages = (UINTN)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;
+    }
+  }
+
+  //
+  // 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
@@ -466,31 +596,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.
@@ -499,10 +647,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 (InDataBuffer != NULL) {
+    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) {
+    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
+  }
+
+UnmapInDataBuffer:
+  if (InDataBuffer != NULL) {
+    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 v3 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 337fb4b2f1e0..313b4f66c7f8 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -1011,7 +1011,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
@@ -1091,7 +1092,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 v3 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error

[Laszlo Ersek]

On 08/31/17 17:01, 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
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 36 ++++++++++++++++----
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e72b1a24b59..5e977c636a0a 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -97,7 +97,7 @@
>  // 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.
> +// ReportHostAdapterError() below.
>  //
>  // - InTransferLength
>  // - OutTransferLength
> @@ -387,6 +387,33 @@ 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.
> + *
> + * @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().
> + */

(1) I hoped that you'd just copy & paste the comment block from my v2
review -- in v3, the bar of "*" characters to the left has not been
removed (it should have been, it is not edk2 coding style), plus the
"contstruct" typo has not been fixed, etc.

I'll update the comment block before pushing.

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 +499,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 v3 3/4] OvmfPkg/VirtioScsiDxe: map virtio-scsi request and response buffers

[Laszlo Ersek]

On 08/31/17 17:01, 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.
>
>   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.

(1) In my v2 review, I wrote (emphasis added now):

On 08/31/17 15:23, Laszlo Ersek wrote:
> On 08/30/17 22:45, Brijesh Singh wrote:
>> - 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."

You didn't *continue* the paragraph in v3, you removed it. And now the
"However,..." sentence appears to refer to the BusMasterCommonBuffer
paragraph. That's incorrect.

I'll fix up 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 | 220 ++++++++++++++++++--
>  1 file changed, 204 insertions(+), 16 deletions(-)
>
> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e977c636a0a..337fb4b2f1e0 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -436,26 +436,156 @@ 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                   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;
> +  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 fields
> +    //  accordingly so that we report the full loss of the incoming transfer.
> +    //  We allocate a temporary buffer and map it with BusMasterCommonBuffer.
> +    //  If the Virtio request is successful then we copy the data from
> +    //  temporary buffer into Packet->InDataBuffer.
> +    //

(2) In my v2 review, point (5), I asked, "The last two paragraphs in the
comment should be un-indented one column.".

You forgot to do that, I'll fix it up.

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

(3) In my v2 review, I wrote (emphasis added now):

On 08/31/17 15:23, Laszlo Ersek wrote:
> On 08/30/17 22:45, Brijesh Singh wrote:
>> +    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).

In v3, you didn't cast the *argument* to UINTN, you cast the result.

I'll fix it up.

The rest is good.

After I apply the above changes:

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

Thanks
Laszlo

> +    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;
> +    }
> +  }
> +
> +  //
> +  // 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
> @@ -466,31 +596,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.
> @@ -499,10 +647,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 (InDataBuffer != NULL) {
> +    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) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
> +  }
> +
> +UnmapInDataBuffer:
> +  if (InDataBuffer != NULL) {
> +    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;
>  }
>
>
>

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

[Laszlo Ersek]

On 08/31/17 17:01, Brijesh Singh wrote:
> 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-3
>
> Changes since v2:
>  * changes to address v2 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 | 306 +++++++++++++++++---
>  2 files changed, 273 insertions(+), 34 deletions(-)
>

Okay, so these are the updates (cumulatively) that I implemented here:

> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 313b4f66c7f8..7b8c3d22c8de 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -387,19 +387,23 @@ 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.
> - *
> - * @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().
> - */
> +
> +  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().
> +
> +**/
>  STATIC
>  EFI_STATUS
>  ReportHostAdapterError (
> @@ -490,14 +494,15 @@ VirtioScsiPassThru (
>      //  * 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 fields
> -    //  accordingly so that we report the full loss of the incoming transfer.
> -    //  We allocate a temporary buffer and map it with BusMasterCommonBuffer.
> -    //  If the Virtio request is successful then we copy the data from
> -    //  temporary buffer into Packet->InDataBuffer.
> +    // In that case simply returing the EFI_DEVICE_ERROR is not sufficient. In
> +    // addition to the error code we also need to update Packet fields
> +    // accordingly so that we report the full loss of the incoming transfer.
>      //
> -    InDataNumPages = (UINTN)EFI_SIZE_TO_PAGES (Packet->InTransferLength);
> +    // We allocate a temporary buffer and map it with BusMasterCommonBuffer. If
> +    // the Virtio request is successful then we copy the data from temporary
> +    // buffer into Packet->InDataBuffer.
> +    //
> +    InDataNumPages = EFI_SIZE_TO_PAGES ((UINTN)Packet->InTransferLength);
>      Status = Dev->VirtIo->AllocateSharedPages (
>                              Dev->VirtIo,
>                              InDataNumPages,

To patch #2, I added the following editing remark:

[lersek@redhat.com: fix style & typo in ReportHostAdapterError() comment]

To patch #3, I added the following editing remarks:

[lersek@redhat.com: restore lost sentence/paragraph in commit message]
[lersek@redhat.com: reindent/reflow "InDataBuffer" comment block]
[lersek@redhat.com: cast arg, not result, of EFI_SIZE_TO_PAGES() to UINTN]

With the above, the series is now fully Reviewed-by me.

I tested this series as follows:

                    virtio-mmio  legacy PCI        modern PCI
                    -----------  ----------------- --------------------------
  test scenario     AARCH64      IA32     X64      IA32     X64/SEV- X64/SEV+
  ----------------  -----------  -------- -------- -------- -------- --------
  R/W text editor   PASS         PASS     PASS     PASS     PASS     PASS

  shell RECONNECT   PASS         PASS     PASS     PASS     PASS     PASS

  OS boot from it   PASS         PASS     PASS     PASS     PASS     PASS

Therefore, for the series:

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

Pushed as commit range fefeb416e63b..17cbf7359f04.

Thanks,
Laszlo