Re: [PATCH 12/14] OvmfPkg/MptScsiDxe: Implement the PassThru method

[Laszlo Ersek <lersek@redhat.com>]

On 01/31/19 11:07, 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).
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> ---
>  OvmfPkg/MptScsiDxe/MptScsi.c      | 255 +++++++++++++++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf |   3 +
>  OvmfPkg/OvmfPkg.dec               |   2 +
>  3 files changed, 256 insertions(+), 4 deletions(-)

So, this is the patch where you pass pointers (DRAM addresses, as
expressed from the CPU's point of view) to the PCI device:

> +STATIC
> +EFI_STATUS
> +MptScsiPopulateRequest (
> +  IN UINT8                                          Target,
> +  IN UINT64                                         Lun,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet,
> +  OUT MPT_SCSI_REQUEST_WITH_SG                      *Request
> +  )
> +{
> +  if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
> +      Packet->CdbLength > sizeof (Request->Header.CDB)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  if (Target > 0 || Lun > 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Packet->InTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) {
> +    Packet->InTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH;
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +  if (Packet->OutTransferLength > MPT_SG_ENTRY_SIMPLE_MAX_LENGTH) {
> +    Packet->OutTransferLength = MPT_SG_ENTRY_SIMPLE_MAX_LENGTH;
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  ZeroMem (Request, sizeof (*Request));
> +  Request->Header.TargetID = Target;
> +  // It's 1 and not 0, for some reason...
> +  Request->Header.LUN[1] = Lun;
> +  Request->Header.Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST;
> +  Request->Header.MessageContext = 1; // Hardcoded, we handle one request at a time
> +
> +  Request->Header.CDBLength = Packet->CdbLength;
> +  CopyMem (Request->Header.CDB, Packet->Cdb, Packet->CdbLength);
> +
> +  Request->Header.SenseBufferLength = Packet->SenseDataLength;
> +  ASSERT ((UINTN) Packet->SenseData <= MAX_UINT32);
> +  Request->Header.SenseBufferLowAddress = (UINT32)(UINTN) Packet->SenseData;
> +
> +  Request->Sg.EndOfList = 1;
> +  Request->Sg.EndOfBuffer = 1;
> +  Request->Sg.LastElement = 1;
> +  Request->Sg.ElementType = MPT_SG_ENTRY_TYPE_SIMPLE;
> +
> +  switch (Packet->DataDirection)
> +  {
> +  case EFI_EXT_SCSI_DATA_DIRECTION_READ:
> +    Request->Header.DataLength = Packet->InTransferLength;
> +    Request->Sg.Length = Packet->InTransferLength;
> +    ASSERT ((UINTN) Packet->InDataBuffer <= MAX_UINT32);
> +    Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->InDataBuffer;
> +    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ;
> +    break;
> +  case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
> +    Request->Header.DataLength = Packet->OutTransferLength;
> +    Request->Sg.Length = Packet->OutTransferLength;
> +    ASSERT ((UINTN) Packet->OutDataBuffer <= MAX_UINT32);
> +    Request->Sg.DataBufferAddress = (UINT32)(UINTN) Packet->OutDataBuffer;
> +    Request->Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE;
> +    Request->Sg.BufferContainsData = 1;
> +    break;
> +  }
> +
> +  return EFI_SUCCESS;
> +}

and

> +STATIC
> +EFI_STATUS
> +MptScsiSendRequest (
> +  IN MPT_SCSI_DEV                                   *Dev,
> +  IN MPT_SCSI_REQUEST_WITH_SG                       *Request,
> +  IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  // Make sure Request is fully written
> +  MemoryFence ();
> +
> +  ASSERT ((UINTN) Request <= MAX_UINT32);
> +  Status = Out32 (Dev, MPT_REG_REQ_Q, (UINT32)(UINTN) Request);
> +  if (EFI_ERROR (Status)) {
> +    // We couldn't enqueue the request, report it as an adapter error
> +    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 EFI_SUCCESS;
> +}

and

>  
> @@ -291,10 +496,52 @@ MptScsiPassThru (
>    IN EFI_EVENT                                      Event     OPTIONAL
>    )
>  {
> -  DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n",
> -          Packet->DataDirection,
> -          Packet->InTransferLength, Packet->OutTransferLength));
> -  return EFI_UNSUPPORTED;
> +  EFI_STATUS Status;
> +  MPT_SCSI_DEV *Dev = MPT_SCSI_FROM_PASS_THRU (This);
> +
> +  // Noisy print, but useful when debugging this area
> +  // DEBUG ((EFI_D_INFO, "MptScsiPassThru Direction %d In %d Out %d\n",
> +  //         Packet->DataDirection,
> +  //         Packet->InTransferLength, Packet->OutTransferLength));
> +
> +  Status = MptScsiPopulateRequest (*Target, Lun, Packet, &Dev->IoRequest);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = MptScsiSendRequest (Dev, &Dev->IoRequest, Packet);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }

You will have to map each such memory transfer explicitly, with the
PciIo->Map(), PciIo->Unmap(), PciIo->AllocateBuffer() and
PciIo->FreeBuffer() functions. Otherwise, if there is an IOMMU in the
system, or the CPU view of memory addresses is not 1:1 identical with
the PCI device view of memory addresses for some other reason, then this
driver will fail.

And, this is a practical concern at this point, not a theoretical one.
If OVMF runs in an AMD SEV (Secure Encrypted Virtualization) guest, then
the PciIo Map / Unmap operations handle the memory decryption /
re-encryption that's necessary for QEMU to see the data in guest RAM.

In brief:

(1) Use PciIo->Map() with BusMasterRead for a single-shot transfer from
the CPU to the PCI device through DRAM.

(2) Use PciIo->Map() with BusMasterWrite for a single-shot transfer from
the PCI device to the CPU through DRAM.

(3) For bi-directional communication (such as request/response within
the same buffer), use PciIo->AllocateBuffer(), plus PciIo->Map() with
BusMasterCommonBuffer.

(4) If the Fusion MPT SCSI controller can handle 64-bit addresses
(*after* mapping; that is, in the device address space), then you might
want to set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in

  [PATCH 10/14] OvmfPkg/MptScsiDxe: Set and restore PCI attributes

but in a *separate* call from setting (EFI_PCI_IO_ATTRIBUTE_IO |
EFI_PCI_IO_ATTRIBUTE_BUS_MASTER). If setting DUAL_ADDRESS_CYCLE fails,
that in itself won't prevent the driver from working.

If the controller can only handle 32-bit device addresses (or you only
want to deal with 32-bit device addresses), then do not set
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE.


All of the above is described (much better) in the UEFI spec. See
EFI_PCI_IO_PROTOCOL | Description, in UEFI-2.7, pages 832-833 for
example (those are the page numbers printed on the pages, in reality
they are pages 902-903 in the PDF file).

Thanks!
Laszlo