Re: [edk2-devel] [PATCH v5 11/12] OvmfPkg/MptScsiDxe: Implement the PassThru method

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

On 04/24/20 19:59, Nikita Leshenko wrote:
> Machines should be able to boot after this commit. Tested with different
> Linux distributions (Ubuntu, CentOS) and different Windows
> versions (Windows 7, Windows 10, Server 2016).
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
>  .../Include/IndustryStandard/FusionMptScsi.h  |   9 +
>  OvmfPkg/MptScsiDxe/MptScsi.c                  | 409 +++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf             |   3 +
>  OvmfPkg/OvmfPkg.dec                           |   3 +
>  4 files changed, 422 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> index 655d629d902e..99778d1537da 100644
> --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> @@ -44,6 +44,15 @@
>  
>  #define MPT_IOC_WHOINIT_ROM_BIOS 0x02
>  
> +#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE  (0x00 << 24)
> +#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE (0x01 << 24)
> +#define MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ  (0x02 << 24)
> +
> +#define MPT_SCSI_IOCSTATUS_SUCCESS          0x0000
> +#define MPT_SCSI_IOCSTATUS_DEVICE_NOT_THERE 0x0043
> +#define MPT_SCSI_IOCSTATUS_DATA_OVERRUN     0x0044
> +#define MPT_SCSI_IOCSTATUS_DATA_UNDERRUN    0x0045
> +
>  //
>  // Device structures
>  //
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 15d671b544c2..9cb5088bfbf9 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -17,6 +17,7 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
>  #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
>  #include <Protocol/ScsiPassThruExt.h>
>  #include <Uefi/UefiSpec.h>
>  
> @@ -30,19 +31,50 @@
>  // Runtime Structures
>  //
>  
> +typedef struct {
> +  MPT_SCSI_REQUEST_ALIGNED        IoRequest;
> +  MPT_SCSI_IO_REPLY_ALIGNED       IoReply;
> +  //
> +  // As EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.SenseDataLength is defined
> +  // as UINT8, defining here SenseData size to MAX_UINT8 will guarantee it
> +  // cannot overflow when passed to device.
> +  //
> +  UINT8                           Sense[MAX_UINT8];
> +  //
> +  // This size of the data is arbitrarily chosen.
> +  // It seems to be sufficient for all I/O requests sent through
> +  // EFI_SCSI_PASS_THRU_PROTOCOL.PassThru() for common boot scenarios.
> +  //
> +  UINT8                           Data[0x2000];
> +} MPT_SCSI_DMA_BUFFER;
> +
>  #define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
>  typedef struct {
>    UINT32                          Signature;
>    EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru;
>    EFI_EXT_SCSI_PASS_THRU_MODE     PassThruMode;
>    UINT8                           MaxTarget;
> +  UINT32                          StallPerPollUsec;
>    EFI_PCI_IO_PROTOCOL             *PciIo;
>    UINT64                          OriginalPciAttributes;
> +  MPT_SCSI_DMA_BUFFER             *Dma;
> +  EFI_PHYSICAL_ADDRESS            DmaPhysical;
> +  VOID                            *DmaMapping;
> +  BOOLEAN                         IoReplyEnqueued;
>  } MPT_SCSI_DEV;
>  
>  #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \
>    CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE)
>  
> +#define MPT_SCSI_DMA_ADDR(Dev, MemberName) \
> +  (Dev->DmaPhysical + OFFSET_OF (MPT_SCSI_DMA_BUFFER, MemberName))
> +
> +#define MPT_SCSI_DMA_ADDR_HIGH(Dev, MemberName) \
> +  ((UINT32)(MPT_SCSI_DMA_ADDR (Dev, MemberName) >> 32))

(1) Please use the RShiftU64() from BaseLib.

> +
> +#define MPT_SCSI_DMA_ADDR_LOW(Dev, MemberName) \
> +  ((UINT32)MPT_SCSI_DMA_ADDR (Dev, MemberName))
> +
>  //
>  // Hardware functions
>  //
> @@ -157,6 +189,9 @@ MptScsiInit (
>      "Req supports 255 targets only (max target is 254)");
>    Req.MaxDevices = Dev->MaxTarget + 1;
>    Req.MaxBuses = 1;
> +  Req.ReplyFrameSize = sizeof Dev->Dma->IoReply.Data;
> +  Req.HostMfaHighAddr = MPT_SCSI_DMA_ADDR_HIGH (Dev, IoRequest);
> +  Req.SenseBufferHighAddr = MPT_SCSI_DMA_ADDR_HIGH (Dev, Sense);
>  
>    //
>    // Send controller init through doorbell
> @@ -218,6 +253,289 @@ MptScsiInit (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +EFI_STATUS
> +ReportHostAdapterError (
> +  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> +  )
> +{
> +  DEBUG ((DEBUG_ERROR, "%a: fatal error in scsi request\n", __FUNCTION__));
> +  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_TASK_ABORTED;
> +  return EFI_DEVICE_ERROR;
> +}
> +
> +STATIC
> +EFI_STATUS
> +ReportHostAdapterOverrunError (
> +  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> +  )
> +{
> +  Packet->SenseDataLength = 0;
> +  Packet->HostAdapterStatus =
> +            EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> +  Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> +  return EFI_BAD_BUFFER_SIZE;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiPopulateRequest (
> +  IN MPT_SCSI_DEV                                   *Dev,
> +  IN UINT8                                          Target,
> +  IN UINT64                                         Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> +  )
> +{
> +  MPT_SCSI_REQUEST_WITH_SG *Request;
> +
> +  Request = &Dev->Dma->IoRequest.Data;
> +
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
> +      (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
> +      Packet->CdbLength > sizeof (Request->Header.Cdb)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  if (Target > Dev->MaxTarget || Lun > 0 ||
> +      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
> +         )
> +        )
> +    ) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Packet->InTransferLength > sizeof (Dev->Dma->Data)) {
> +    Packet->InTransferLength = sizeof (Dev->Dma->Data);
> +    return ReportHostAdapterOverrunError (Packet);
> +  }
> +  if (Packet->OutTransferLength > sizeof (Dev->Dma->Data)) {
> +    Packet->OutTransferLength = sizeof (Dev->Dma->Data);
> +    return ReportHostAdapterOverrunError (Packet);
> +  }
> +
> +  ZeroMem (Request, sizeof (*Request));
> +  Request->Header.TargetId = Target;
> +  //
> +  // Only LUN 0 is currently supported, hence the cast is safe
> +  //
> +  Request->Header.Lun[1] = (UINT8)Lun;
> +  Request->Header.Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST;
> +  Request->Header.MessageContext = 1; // We handle one request at a time
> +
> +  Request->Header.CdbLength = Packet->CdbLength;
> +  CopyMem (Request->Header.Cdb, Packet->Cdb, Packet->CdbLength);
> +
> +  //
> +  // SenseDataLength is UINT8, Sense[] is MAX_UINT8, so we can't overflow
> +  //
> +  ZeroMem (Dev->Dma->Sense, Packet->SenseDataLength);
> +  Request->Header.SenseBufferLength = Packet->SenseDataLength;
> +  Request->Header.SenseBufferLowAddress = MPT_SCSI_DMA_ADDR_LOW (Dev, Sense);
> +
> +  Request->Sg.EndOfList = 1;
> +  Request->Sg.EndOfBuffer = 1;
> +  Request->Sg.LastElement = 1;
> +  Request->Sg.ElementType = MPT_SG_ENTRY_TYPE_SIMPLE;
> +  Request->Sg.Is64BitAddress = 1;
> +  Request->Sg.DataBufferAddress = MPT_SCSI_DMA_ADDR (Dev, Data);
> +
> +  //
> +  // "MPT_SG_ENTRY_SIMPLE.Length" is a 24-bit quantity.
> +  //
> +  STATIC_ASSERT (
> +    sizeof (Dev->Dma->Data) < SIZE_16MB,
> +    "MPT_SCSI_DMA_BUFFER.Data must be smaller than 16MB"
> +    );
> +
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    Request->Header.DataLength = Packet->InTransferLength;
> +    Request->Sg.Length = Packet->InTransferLength;
> +    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ;
> +  } else {
> +    Request->Header.DataLength = Packet->OutTransferLength;
> +    Request->Sg.Length = Packet->OutTransferLength;
> +    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE;
> +
> +    CopyMem (Dev->Dma->Data, Packet->OutDataBuffer, Packet->OutTransferLength);
> +    Request->Sg.BufferContainsData = 1;
> +  }
> +
> +  if (Request->Header.DataLength == 0) {
> +    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiSendRequest (
> +  IN MPT_SCSI_DEV                                   *Dev,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  if (!Dev->IoReplyEnqueued) {
> +    //
> +    // Put one free reply frame on the reply queue, the hardware may use it to
> +    // report an error to us.
> +    //
> +    Status = Out32 (Dev, MPT_REG_REP_Q, MPT_SCSI_DMA_ADDR_LOW (Dev, IoReply));
> +    if (EFI_ERROR (Status)) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +    Dev->IoReplyEnqueued = TRUE;
> +  }
> +
> +  Status = Out32 (Dev, MPT_REG_REQ_Q, MPT_SCSI_DMA_ADDR_LOW (Dev, IoRequest));
> +  if (EFI_ERROR (Status)) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiGetReply (
> +  IN MPT_SCSI_DEV                                   *Dev,
> +  OUT UINT32                                        *Reply
> +  )
> +{
> +  EFI_STATUS Status;
> +  UINT32     Istatus;
> +  UINT32     EmptyReply;
> +
> +  //
> +  // Timeouts are not supported for
> +  // EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() in this implementation.
> +  //
> +  for (;;) {
> +    Status = In32 (Dev, MPT_REG_ISTATUS, &Istatus);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    //
> +    // Interrupt raised
> +    //
> +    if (Istatus & MPT_IMASK_REPLY) {
> +      break;
> +    }
> +
> +    gBS->Stall (Dev->StallPerPollUsec);
> +  }
> +
> +  Status = In32 (Dev, MPT_REG_REP_Q, Reply);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // The driver is supposed to fetch replies until 0xffffffff is returned, which
> +  // will reset the interrupt status. We put only one request, so we expect the
> +  // next read reply to be the last.
> +  //
> +  Status = In32 (Dev, MPT_REG_REP_Q, &EmptyReply);
> +  if (EFI_ERROR (Status) || EmptyReply != MAX_UINT32) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
> +MptScsiHandleReply (
> +  IN MPT_SCSI_DEV                                   *Dev,
> +  IN UINT32                                         Reply,
> +  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET    *Packet
> +  )
> +{
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +    CopyMem (Packet->InDataBuffer, Dev->Dma->Data, Packet->InTransferLength);
> +  }
> +
> +  if (Reply == Dev->Dma->IoRequest.Data.Header.MessageContext) {
> +    //
> +    // This is a turbo reply, everything is good
> +    //
> +    Packet->SenseDataLength = 0;
> +    Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +    Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
> +
> +  } else if ((Reply & BIT31) != 0) {
> +    DEBUG ((DEBUG_INFO, "%a: Full reply returned\n", __FUNCTION__));

(2) Is this a frequent event? If we expect this to happen frequently,
then it should be DEBUG_VERBOSE. Otherwise (= infrequent event),
DEBUG_INFO is fine.

> +    //
> +    // When reply MSB is set, we got a full reply. Since we submitted only one
> +    // reply frame, we know it's IoReply.
> +    //
> +    Dev->IoReplyEnqueued = FALSE;
> +
> +    Packet->TargetStatus = Dev->Dma->IoReply.Data.ScsiStatus;
> +    //
> +    // Make sure device only lowers SenseDataLength before copying sense
> +    //
> +    ASSERT (Dev->Dma->IoReply.Data.SenseCount <= Packet->SenseDataLength);
> +    Packet->SenseDataLength =
> +      (UINT8)MIN (Dev->Dma->IoReply.Data.SenseCount, Packet->SenseDataLength);
> +    CopyMem (Packet->SenseData, Dev->Dma->Sense, Packet->SenseDataLength);
> +
> +    if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> +      Packet->InTransferLength = Dev->Dma->IoReply.Data.TransferCount;
> +    } else {
> +      Packet->OutTransferLength = Dev->Dma->IoReply.Data.TransferCount;
> +    }
> +
> +    switch (Dev->Dma->IoReply.Data.IocStatus) {
> +    case MPT_SCSI_IOCSTATUS_SUCCESS:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> +      break;
> +    case MPT_SCSI_IOCSTATUS_DEVICE_NOT_THERE:
> +      Packet->HostAdapterStatus =
> +        EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT;
> +      return EFI_TIMEOUT;
> +    case MPT_SCSI_IOCSTATUS_DATA_UNDERRUN:
> +    case MPT_SCSI_IOCSTATUS_DATA_OVERRUN:
> +      Packet->HostAdapterStatus =
> +        EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> +      break;
> +    default:
> +      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "%a: unexpected reply (%x)\n", __FUNCTION__, Reply));
> +    Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> +    return EFI_DEVICE_ERROR;
> +  }

I'm really pleased with the updates to this function. In both the "turbo
reply" and "full reply" cases, we make sure that all 6 of the following
are correctly set on output:

- InTransferLength
- OutTransferLength
- HostAdapterStatus
- TargetStatus
- SenseDataLength
- SenseData

(3) I do have a question about the last branch ("unexpected reply")
though -- would you agree that we should call ReportHostAdapterError()
there, rather than only setting HostAdapterStatus?

Because, per EFI_DEVICE_ERROR, the caller will first consult
HostAdapterStatus, yes, but then (upon seeing "OTHER") it will likely
proceed to TargetStatus and Sense. And we currently leave garbage in
those fields.

(I guess I could have made the same comment under v4, but there I was
hung up on the other output fields in the more common branches. All of
those have been nicely covered in v5, so I've arrived at this last branch.)


> +
> +  return EFI_SUCCESS;
> +}
> +
>  //
>  // Ext SCSI Pass Thru
>  //
> @@ -233,7 +551,33 @@ MptScsiPassThru (
>    IN EFI_EVENT                                      Event     OPTIONAL
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  EFI_STATUS   Status;
> +  MPT_SCSI_DEV *Dev;
> +  UINT32       Reply;
> +
> +  Dev = MPT_SCSI_FROM_PASS_THRU (This);
> +  //
> +  // We only use first byte of target identifer
> +  //
> +  Status = MptScsiPopulateRequest (Dev, *Target, Lun, Packet);
> +  if (EFI_ERROR (Status)) {
> +    //
> +    // MptScsiPopulateRequest modified packet according to the error
> +    //
> +    return Status;
> +  }
> +
> +  Status = MptScsiSendRequest (Dev, Packet);
> +  if (EFI_ERROR (Status)) {
> +    return ReportHostAdapterError (Packet);
> +  }
> +
> +  Status = MptScsiGetReply (Dev, &Reply);
> +  if (EFI_ERROR (Status)) {
> +    return ReportHostAdapterError (Packet);
> +  }
> +
> +  return MptScsiHandleReply (Dev, Reply, Packet);
>  }
>  
>  STATIC
> @@ -488,6 +832,8 @@ MptScsiControllerStart (
>  {
>    EFI_STATUS           Status;
>    MPT_SCSI_DEV         *Dev;
> +  UINTN                Pages;
> +  UINTN                BytesMapped;
>  
>    Dev = AllocateZeroPool (sizeof (*Dev));
>    if (Dev == NULL) {
> @@ -497,6 +843,7 @@ MptScsiControllerStart (
>    Dev->Signature = MPT_SCSI_DEV_SIGNATURE;
>  
>    Dev->MaxTarget = PcdGet8 (PcdMptScsiMaxTargetLimit);
> +  Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
>  
>    Status = gBS->OpenProtocol (
>                    ControllerHandle,
> @@ -557,11 +904,45 @@ MptScsiControllerStart (
>        ));
>    }
>  
> -  Status = MptScsiInit (Dev);
> +  //
> +  // Create buffers for data transfer
> +  //
> +  Pages = EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma));
> +  Status = Dev->PciIo->AllocateBuffer (
> +                         Dev->PciIo,
> +                         AllocateAnyPages,
> +                         EfiBootServicesData,
> +                         Pages,
> +                         (VOID **)&Dev->Dma,
> +                         EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> +                         );
>    if (EFI_ERROR (Status)) {
>      goto RestoreAttributes;
>    }
>  
> +  BytesMapped = EFI_PAGES_TO_SIZE (Pages);
> +  Status = Dev->PciIo->Map (
> +                         Dev->PciIo,
> +                         EfiPciIoOperationBusMasterCommonBuffer,
> +                         Dev->Dma,
> +                         &BytesMapped,
> +                         &Dev->DmaPhysical,
> +                         &Dev->DmaMapping
> +                         );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeBuffer;
> +  }
> +
> +  if (BytesMapped != EFI_PAGES_TO_SIZE (Pages)) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto Unmap;
> +  }
> +
> +  Status = MptScsiInit (Dev);
> +  if (EFI_ERROR (Status)) {
> +    goto Unmap;
> +  }
> +
>    //
>    // Host adapter channel, doesn't exist
>    //
> @@ -594,6 +975,19 @@ MptScsiControllerStart (
>  UninitDev:
>    MptScsiReset (Dev);
>  
> +Unmap:
> +  Dev->PciIo->Unmap (
> +                Dev->PciIo,
> +                Dev->DmaMapping
> +                );
> +
> +FreeBuffer:
> +  Dev->PciIo->FreeBuffer (
> +                Dev->PciIo,
> +                EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),

(4) Not wrong, but still, please simplify this function call by passing
the local variable "Pages" here.

> +                Dev->Dma
> +                );
> +
>  RestoreAttributes:
>    Dev->PciIo->Attributes (
>                  Dev->PciIo,
> @@ -655,6 +1049,17 @@ MptScsiControllerStop (
>  
>    MptScsiReset (Dev);
>  
> +  Dev->PciIo->Unmap (
> +                Dev->PciIo,
> +                Dev->DmaMapping
> +                );
> +
> +  Dev->PciIo->FreeBuffer (
> +                Dev->PciIo,
> +                EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
> +                Dev->Dma
> +                );
> +
>    Dev->PciIo->Attributes (
>                  Dev->PciIo,
>                  EfiPciIoAttributeOperationSet,
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 4862ff9dd497..bb89e50e08f0 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -37,3 +37,6 @@ [Protocols]
>  
>  [FixedPcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit   ## CONSUMES
> +
> +[Pcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 2d09444bbb16..3bf26e8df82d 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -167,6 +167,9 @@ [PcdsFixedAtBuild]
>    #  by ScsiBusDxe.
>    gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit|7|UINT8|0x39
>  
> +  ## Microseconds to stall between polling for MptScsi request result
> +  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x40
> +
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
> 

With (1) through (4) addressed:

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

(Well, (1) is a must, (2) through (4) are open for discussion, of course.)

Thanks!
Laszlo