Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response

["Laszlo Ersek" <lersek@redhat.com>]

On 03/25/20 17:10, Liran Alon wrote:
> Implement EXT_SCSI_PASS_THRU.PassThru().
>
> Machines should be able to boot after this commit.
> Tested with Ubuntu 16.04 guest.
>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567
> Signed-off-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/OvmfPkg.dec             |   6 +
>  OvmfPkg/PvScsiDxe/PvScsi.c      | 442 +++++++++++++++++++++++++++++++-
>  OvmfPkg/PvScsiDxe/PvScsi.h      |   1 +
>  OvmfPkg/PvScsiDxe/PvScsiDxe.inf |   5 +-
>  4 files changed, 451 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index a04aee5c2cd4..ff49ec0e9e6a 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -130,6 +130,12 @@
>    gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x36
>    gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x37
>
> +  ## After PvScsiDxe sends a SCSI request to the device, it waits for
> +  #  the request completion in a polling loop.
> +  #  This constant defines how many micro-seconds to wait between each
> +  #  polling loop iteration.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x38
> +
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 928984099520..de4122e39a81 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -31,6 +31,27 @@
>  // Ext SCSI Pass Thru utilities
>  //
>
> +/**
> +  Reads a 32-bit value into BAR0 using MMIO
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiMmioRead32 (
> +  IN CONST PVSCSI_DEV   *Dev,
> +  IN UINT64             Offset,
> +  OUT UINT32            *Value
> +  )
> +{
> +  return Dev->PciIo->Mem.Read (
> +                           Dev->PciIo,
> +                           EfiPciIoWidthUint32,
> +                           PCI_BAR_IDX0,
> +                           Offset,
> +                           1,   // Count
> +                           Value
> +                           );
> +}
> +
>  /**
>    Writes a 32-bit value into BAR0 using MMIO
>  **/
> @@ -109,6 +130,352 @@ PvScsiWriteCmdDesc (
>    return EFI_SUCCESS;
>  }
>
> +/**
> +  Returns if PVSCSI request ring is full
> +**/
> +STATIC
> +BOOLEAN
> +PvScsiIsReqRingFull (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingsState;
> +  UINT32             ReqNumEntries;
> +
> +  RingsState = Dev->RingDesc.RingState;
> +  ReqNumEntries = 1U << RingsState->ReqNumEntriesLog2;
> +  return (RingsState->ReqProdIdx - RingsState->CmpConsIdx) >= ReqNumEntries;
> +}

(Just some thoughts, not a request for changing the code.)

Normally I prefer accessing buffers shared with the device though
volatile-qualified  pointers.

Meaning, in this case, that every "PCI host" pointer (i.e., each pointer
that is associated with a PVSCSI_DMA_DESC) would have to be
volatile-qualified. In particular:

- in patch#13, PVSCSI_RING_DESC would have to be updated like this:

> typedef struct {
>   volatile PVSCSI_RINGS_STATE   *RingState;
>   PVSCSI_DMA_DESC               RingStateDmaDesc;
>
>   volatile PVSCSI_RING_REQ_DESC *RingReqs;
>   PVSCSI_DMA_DESC               RingReqsDmaDesc;
>
>   volatile PVSCSI_RING_CMP_DESC *RingCmps;
>   PVSCSI_DMA_DESC               RingCmpsDmaDesc;
> } PVSCSI_RING_DESC;

- in patch#14, PVSCSI_DEV would change as follows:

> typedef struct {
>   UINT32                          Signature;
>   EFI_PCI_IO_PROTOCOL             *PciIo;
>   EFI_EVENT                       ExitBoot;
>   UINT64                          OriginalPciAttributes;
>   PVSCSI_RING_DESC                RingDesc;
>   volatile PVSCSI_DMA_BUFFER      *DmaBuf;
>   PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>   UINT8                           MaxTarget;
>   UINT8                           MaxLun;
>   UINTN                           WaitForCmpStallInUsecs;
>   EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>   EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
> } PVSCSI_DEV;

After these changes, the compiler would (justifiedly) flag a bunch of
code locations casting away the volatile qualification -- for example,
in the above function, in the assignment to the "RingsState" local
variable.

Clearly, most of these compilation errors would have to be fixed (not
suppressed), because they would be valid. Meaning:

- you'd have to volatile-qualify the "RingsState" local variable in all
  of PvScsiIsReqRingFull(), PvScsiGetCurrentRequest(),
  PvScsiWaitForRequestCompletion();

- you'd also have to volatile-qualify the return types of
  PvScsiGetCurrentRequest() and PvScsiWaitForRequestCompletion();

- you'd have to update PopulateRequest() and HandleResponse() too; and
  the most annoying part of that would be that you could no longer use
  CopyMem() and ZeroMem() -- because those functions take
  pointer-to-void parameters, rather than pointer-to-volatile-void ones.

(FWIW, we wouldn't have to change the PvScsiFreeSharedPages() prototype
-- it would be OK to cast away volatile in those calls, as we wouldn't
dereference the pointers in that case.)

So... the reason I'm not actually requesting these
volatile-qualifications is that (a) your use of MemoryFence() seems
mostly OK, and (b) the UEFI Driver Writer's guide recommends *either*
volatile *or* MemoryFence(). Of course using both techniques at the same
time is not a problem -- and in code I write I actually like to use both
at the same time --, but just one suffices too. (See section 4.2.6
"Memory ordering" in the DWG.)

The reason I'm writing this up here is because I want the "record" (the
mailing list archive) to show that we have considered this topic
explicitly.

Back to your patch:

On 03/25/20 17:10, Liran Alon wrote:
> +
> +/**
> +  Returns pointer to current request descriptor to produce
> +**/
> +STATIC
> +PVSCSI_RING_REQ_DESC *
> +PvScsiGetCurrentRequest (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingState;
> +  UINT32             ReqNumEntries;
> +
> +  RingState = Dev->RingDesc.RingState;
> +  ReqNumEntries = 1U << RingState->ReqNumEntriesLog2;
> +  return Dev->RingDesc.RingReqs +
> +         (RingState->ReqProdIdx & (ReqNumEntries - 1));
> +}
> +
> +/**
> +  Returns pointer to current completion descriptor to consume
> +**/
> +STATIC
> +PVSCSI_RING_CMP_DESC *
> +PvScsiGetCurrentResponse (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  PVSCSI_RINGS_STATE *RingState;
> +  UINT32             CmpNumEntries;
> +
> +  RingState = Dev->RingDesc.RingState;
> +  CmpNumEntries = 1U << RingState->CmpNumEntriesLog2;
> +  return Dev->RingDesc.RingCmps +
> +         (RingState->CmpConsIdx & (CmpNumEntries - 1));
> +}
> +
> +/**
> +  Wait for device to signal completion of submitted requests
> +**/
> +STATIC
> +EFI_STATUS
> +PvScsiWaitForRequestCompletion (
> +  IN CONST PVSCSI_DEV   *Dev
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINT32     IntrStatus;
> +
> +  //
> +  // Note: We don't yet support Timeout according to
> +  // EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.Timeout.
> +  //
> +  // This is consistent with some other Scsi PassThru drivers
> +  // such as VirtioScsi.
> +  //
> +  for (;;) {
> +    Status = PvScsiMmioRead32 (Dev, PvScsiRegOffsetIntrStatus, &IntrStatus);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    //
> +    // PVSCSI_INTR_CMPL_MASK is set if device completed submitted requests
> +    //
> +    if ((IntrStatus & PVSCSI_INTR_CMPL_MASK) != 0) {
> +        break;

(1) Wrong indentation.

> +    }
> +
> +    gBS->Stall (Dev->WaitForCmpStallInUsecs);
> +  }
> +
> +  //
> +  // Acknowledge PVSCSI_INTR_CMPL_MASK in device interrupt-status register
> +  //
> +  return PvScsiMmioWrite32 (
> +           Dev,
> +           PvScsiRegOffsetIntrStatus,
> +           PVSCSI_INTR_CMPL_MASK
> +           );
> +}
> +
> +/**
> +  Populate a PVSCSI request descriptor from the Extended SCSI Pass Thru
> +  Protocol packet.
> +**/
> +STATIC
> +EFI_STATUS
> +PopulateRequest (
> +  IN CONST PVSCSI_DEV                               *Dev,
> +  IN UINT8                                          *Target,
> +  IN UINT64                                         Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  OUT PVSCSI_RING_REQ_DESC                          *Request
> +  )
> +{
> +  UINT8 TargetValue;
> +
> +  //
> +  // We only use first byte of target identifer
> +  //
> +  TargetValue = *Target;
> +
> +  //
> +  // Check for unsupported requests
> +  //
> +  if (
> +      //
> +      // Bidirectional transfer was requested
> +      //
> +      (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
> +      (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
> +      //
> +      // Command Descriptor Block bigger than this constant should be considered
> +      // out-of-band. We currently don't support these CDBs.
> +      //
> +      (Packet->CdbLength > PVSCSI_CDB_MAX_SIZE)
> +      ) {
> +
> +    //
> +    // This error code doesn't require updates to the Packet output fields
> +    //
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // Check for invalid parameters
> +  //
> +  if (
> +      //
> +      // Addressed invalid device
> +      //
> +      (TargetValue > Dev->MaxTarget) || (Lun > Dev->MaxLun) ||
> +      //
> +      // Invalid direction (there doesn't seem to be a macro for the "no data
> +      // transferred" "direction", eg. for TEST UNIT READY)
> +      //
> +      (Packet->DataDirection > EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) ||
> +      //
> +      // Trying to receive, but destination pointer is NULL, or contradicting
> +      // transfer direction
> +      //
> +      ((Packet->InTransferLength > 0) &&
> +       ((Packet->InDataBuffer == NULL) ||
> +        (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_WRITE)
> +        )
> +       ) ||
> +      //
> +      // Trying to send, but source pointer is NULL, or contradicting
> +      // transfer direction
> +      //
> +      ((Packet->OutTransferLength > 0) &&
> +       ((Packet->OutDataBuffer == NULL) ||
> +        (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ)
> +        )
> +       )
> +      ) {
> +
> +    //
> +    // This error code doesn't require updates to the Packet output fields
> +    //
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Check for input/output buffer too large for DMA communication buffer
> +  //
> +  if (Packet->InTransferLength > sizeof (Dev->DmaBuf->Data)) {
> +    Packet->InTransferLength = sizeof (Dev->DmaBuf->Data);
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +  if (Packet->OutTransferLength > sizeof (Dev->DmaBuf->Data)) {
> +    Packet->OutTransferLength = sizeof (Dev->DmaBuf->Data);
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  //
> +  // Encode PVSCSI request
> +  //
> +  ZeroMem (Request, sizeof (*Request));
> +
> +  Request->Bus = 0;
> +  Request->Target = TargetValue;
> +  //
> +  // This cast is safe as MaxLun is defined as UINT8
> +  //
> +  Request->Lun[1] = (UINT8)Lun;
> +  Request->SenseLen = Packet->SenseDataLength;

Ah, *now* I understand why you chose MAX_UINT8 as the size of
"PVSCSI_DMA_BUFFER.SenseData". Because, "Packet->SenseDataLength" has
type UINT8, and this way you guarantee that the SCSI client's
"Packet->SenseDataLength" will always fit in the DMA buffer.

Good solution, but it *absolutely* needs to be documented in patch#14
("OvmfPkg/PvScsiDxe: Introduce DMA communication buffer") -- in fact,
see my question (4) under patch#14.

(2) Also, please add a comment here that a "Dev->DmaBuf->SenseData"
overflow is not possible due to "Packet->SenseDataLength" having type
UINT8.

This would be a comment in the same vein as the "MaxLun" reference just
above -- I find *that* comment very helpful, too.

> +  Request->SenseAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, SenseData);

(3) Please drop the (UINT64) cast -- see my earlier comment about
EFI_PHYSICAL_ADDRESS being a typedef to UINT64.

> +  Request->CdbLen = Packet->CdbLength;
> +  CopyMem (Request->Cdb, Packet->Cdb, Packet->CdbLength);

Right, this is safe, due to the PVSCSI_CDB_MAX_SIZE near the top of the
function (PVSCSI_CDB_MAX_SIZE is 16, and "sizeof
PVSCSI_RING_REQ_DESC.Cdb" is also 16).

> +  Request->VcpuHint = 0;
> +  Request->Tag = PVSCSI_SIMPLE_QUEUE_TAG;
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    Request->Flags = PVSCSI_FLAG_CMD_DIR_TOHOST;
> +    Request->DataLen = Packet->InTransferLength;
> +  } else {
> +    Request->Flags = PVSCSI_FLAG_CMD_DIR_TODEVICE;
> +    Request->DataLen = Packet->OutTransferLength;
> +    CopyMem (
> +      Dev->DmaBuf->Data,
> +      Packet->OutDataBuffer,
> +      Packet->OutTransferLength
> +      );
> +  }
> +  Request->DataAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, Data);

(4) Same as (3).

> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Handle the PVSCSI device response:
> +  - Copy returned data from DMA communication buffer.
> +  - Update fields in Extended SCSI Pass Thru Protocol packet as required.
> +  - Translate response code to EFI status code and host adapter status.
> +**/
> +STATIC
> +EFI_STATUS
> +HandleResponse (
> +  IN PVSCSI_DEV                                     *Dev,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  IN CONST PVSCSI_RING_CMP_DESC                     *Response
> +  )
> +{
> +  //
> +  // Check if device returned sense data
> +  //
> +  if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) {
> +    //
> +    // Fix SenseDataLength to amount of data returned
> +    //
> +    if (Packet->SenseDataLength > Response->SenseLen) {
> +      Packet->SenseDataLength = (UINT8)Response->SenseLen;
> +    }
> +    //
> +    // Copy sense data from DMA communication buffer
> +    //
> +    CopyMem (
> +      Packet->SenseData,
> +      Dev->DmaBuf->SenseData,
> +      Packet->SenseDataLength
> +      );
> +  } else {
> +    //
> +    // Signal no sense data returned
> +    //
> +    Packet->SenseDataLength = 0;
> +  }
> +
> +  //
> +  // Copy device output from DMA communication buffer
> +  //
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength);
> +  }

I'm unfamilar with the PVSCSI device model, but I think this is not
general enough. The "PVSCSI_RING_CMP_DESC.DataLen" field suggests that
short reads are possible at least in theory.

(5) If a short read occurs (Response->DataLen <
Packet->InTransferLength), then we should adjust
"Packet->InTransferLength", and also copy that many bytes only.

(6) I think it would be prudent to update "Packet->OutTransferLength"
too, for short writes.

> +
> +  //
> +  // Report target status
> +  //
> +  Packet->TargetStatus = Response->ScsiStatus;
> +
> +  //
> +  // Host adapter status and function return value depend on
> +  // device response's host status
> +  //
> +  switch (Response->HostStatus) {
> +    case PvScsiBtStatSuccess:
> +    case PvScsiBtStatLinkedCommandCompleted:
> +    case PvScsiBtStatLinkedCommandCompletedWithFlag:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +      return EFI_SUCCESS;
> +
> +    case PvScsiBtStatSelTimeout:
> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
> +      return EFI_TIMEOUT;
> +
> +    case PvScsiBtStatDatarun:
> +    case PvScsiBtStatDataUnderrun:
> +      //
> +      // Report residual data in overrun/underrun
> +      //
> +      if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +        Packet->InTransferLength = Response->DataLen;
> +      } else {
> +        Packet->OutTransferLength = Response->DataLen;
> +      }

OK, if we are sure that (a) the device will always report short
reads/writes like this, and that (b) the above assignments will never
cause InTransferLength / OutTransferLength to *grow*, then the
InTransferLength / OutTransferLength adjustments are sufficiently
covered.

Still:

(7) Please cast DataLen to UINT32, otherwise Visual Studio might whine.

(8) The CopyMem() call above should not copy garbage (at the tail).

Honestly, *if* the PVSCSI device model always sets "Response->DataLen",
then I would prefer if:

- we always updated InTransferLength / OutTransferLength (regardless of
"Response->HostStatus"),

- and we only used these case labels (PvScsiBtStatDatarun /
PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus".

> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> +      return EFI_BAD_BUFFER_SIZE;

I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec,
EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed".
But that's not the case here -- we do have a partially completed
transfer.

(9) Thus I feel we should use a "break" here.

> +
> +    case PvScsiBtStatBusFree:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_FREE;
> +      break;
> +
> +    case PvScsiBtStatInvPhase:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PHASE_ERROR;
> +      break;
> +
> +    case PvScsiBtStatSensFailed:
> +      Packet->HostAdapterStatus =
> +                EFI_EXT_SCSI_STATUS_HOST_ADAPTER_REQUEST_SENSE_FAILED;
> +      break;
> +
> +    case PvScsiBtStatTagReject:
> +    case PvScsiBtStatBadMsg:
> +      Packet->HostAdapterStatus =
> +          EFI_EXT_SCSI_STATUS_HOST_ADAPTER_MESSAGE_REJECT;
> +      break;
> +
> +    case PvScsiBtStatBusReset:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET;
> +      break;
> +
> +    case PvScsiBtStatHaTimeout:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT;
> +      return EFI_TIMEOUT;
> +
> +    case PvScsiBtStatScsiParity:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR;
> +      break;
> +
> +    default:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +      break;
> +  }
> +
> +  return EFI_DEVICE_ERROR;
> +}
> +
>  /**
>    Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and
>    EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized
> @@ -129,6 +496,23 @@ IsTargetInitialized (
>    return FALSE;
>  }
>
> +/**
> +  Create a fake host adapter error
> +**/
> +STATIC
> +EFI_STATUS
> +ReportHostAdapterError (
> +  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> +  )
> +{
> +  Packet->InTransferLength = 0;
> +  Packet->OutTransferLength = 0;
> +  Packet->SenseDataLength = 0;
> +  Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +  Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> +  return EFI_DEVICE_ERROR;
> +}
> +
>  //
>  // Ext SCSI Pass Thru
>  //
> @@ -144,7 +528,62 @@ PvScsiPassThru (
>    IN EFI_EVENT                                      Event    OPTIONAL
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  PVSCSI_DEV            *Dev;
> +  EFI_STATUS            Status;
> +  PVSCSI_RING_REQ_DESC *Request;
> +  PVSCSI_RING_CMP_DESC *Response;
> +
> +  Dev = PVSCSI_FROM_PASS_THRU (This);
> +
> +  if (PvScsiIsReqRingFull (Dev)) {
> +    return EFI_NOT_READY;
> +  }
> +
> +  Request = PvScsiGetCurrentRequest (Dev);
> +
> +  Status = PopulateRequest (Dev, Target, Lun, Packet, Request);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // Writes to Request must be globally visible before making request
> +  // available to device
> +  //
> +  MemoryFence ();
> +  Dev->RingDesc.RingState->ReqProdIdx++;
> +

(10) Please insert another MemoryFence () here.

> +  Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetKickRwIo, 0);
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // 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.
> +    //
> +    return ReportHostAdapterError (Packet);
> +  }
> +
> +  Status = PvScsiWaitForRequestCompletion (Dev);
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // If waiting for request completion 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.
> +    //
> +    return ReportHostAdapterError (Packet);
> +  }
> +

(11) Please insert a MemoryFence() here.

> +  Response = PvScsiGetCurrentResponse (Dev);
> +  Status = HandleResponse (Dev, Packet, Response);
> +
> +  //
> +  // Reads from response must complete before releasing completion entry
> +  // to device
> +  //
> +  MemoryFence ();
> +  Dev->RingDesc.RingState->CmpConsIdx++;
> +
> +  return Status;
>  }
>
>  STATIC
> @@ -685,6 +1124,7 @@ PvScsiInit (
>    //
>    Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit);
>    Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit);
> +  Dev->WaitForCmpStallInUsecs = PcdGet32 (PcdPvScsiWaitForCmpStallInUsecs);
>
>    //
>    // Set PCI Attributes
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h
> index 7f91d70fec79..08e876b75930 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.h
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.h
> @@ -47,6 +47,7 @@ typedef struct {
>    PVSCSI_DMA_DESC                 DmaBufDmaDesc;
>    UINT8                           MaxTarget;
>    UINT8                           MaxLun;
> +  UINTN                           WaitForCmpStallInUsecs;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>  } PVSCSI_DEV;
> diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> index fcffc90d46c8..ae6d8753623f 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> +++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf
> @@ -38,5 +38,6 @@
>    gEfiPciIoProtocolGuid             ## TO_START
>
>  [Pcd]
> -  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit       ## CONSUMES
> -  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit    ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit               ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit            ## CONSUMES
> +  gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs    ## CONSUMES
>

Thanks,
Laszlo