[edk2-devel] [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method
Laszlo Ersek
lersek at redhat.com
Mon Apr 20 17:30:24 UTC 2020
On 04/14/20 19:38, 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 at oracle.com>
> ---
> .../Include/IndustryStandard/FusionMptScsi.h | 19 +-
> OvmfPkg/MptScsiDxe/MptScsi.c | 369 +++++++++++++++++-
> OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 +
> OvmfPkg/OvmfPkg.dec | 3 +
> 4 files changed, 390 insertions(+), 4 deletions(-)
>
> diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h
> index d00a9e6db0bf..4be36adedd8f 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
> //
> @@ -141,6 +150,14 @@ typedef union {
> } Data;
> #pragma pack ()
> UINT64 Uint64; // 8 byte alignment required by HW
> -} MPT_SCSI_IO_ERROR_REPLY;
> +} MPT_SCSI_IO_REPLY;
> +
(1) Is it really useful to call this union "MPT_SCSI_IO_ERROR_REPLY"
temporarily, just for patch#10?
Can patch#10 introduce the union as "MPT_SCSI_IO_REPLY" at once?
> +typedef union {
> + struct {
> + MPT_SCSI_IO_REQUEST Header;
> + MPT_SG_ENTRY_SIMPLE Sg;
> + } Data;
(2) Do we intend to prevent padding between "Header" and "Sg"? Because
then we should pack "Data" too.
> + UINT64 Uint64; // 8 byte alignment required by HW
> +} MPT_SCSI_REQUEST_WITH_SG;
>
> #endif // __FUSION_MPT_SCSI_H__
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 9c3bdc430e1a..fcdaa4c338a4 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,6 +31,13 @@
> // Runtime Structures
> //
>
> +typedef struct {
> + MPT_SCSI_REQUEST_WITH_SG IoRequest;
> + MPT_SCSI_IO_REPLY IoReply;
> + UINT8 Sense[MAX_UINT8];
> + UINT8 Data[0x2000];
(3) Please refer to "PVSCSI_DMA_BUFFER" in "OvmfPkg/PvScsiDxe/PvScsi.h"
-- the element counts "MAX_UINT8" and "0x2000" are (I think) arbitrary
here too, so similar comments as in "PvScsi.h" would be welcome.
> +} MPT_SCSI_DMA_BUFFER;
> +
> #define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S')
> typedef struct {
> UINT32 Signature;
> @@ -37,11 +45,19 @@ typedef struct {
> EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode;
> EFI_PCI_IO_PROTOCOL *PciIo;
> UINT64 OriginalPciAttributes;
> + UINT32 StallPerPollUsec;
> + 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))
> +
(4) (I missed this in PvScsi, alas.)
Please insert a space character after "OFFSET_OF".
> //
> // Hardware functions
> //
> @@ -142,6 +158,8 @@ MptScsiInit (
> UINT8 *ReplyBytes;
> UINT32 ReplyWord;
>
> + Dev->StallPerPollUsec = PcdGet32 (PcdMptScsiStallPerPollUsec);
> +
> Status = MptScsiReset (Dev);
> if (EFI_ERROR (Status)) {
> return Status;
> @@ -153,7 +171,7 @@ MptScsiInit (
> Req.Data.Function = MPT_MESSAGE_HDR_FUNCTION_IOC_INIT;
> Req.Data.MaxDevices = 1;
> Req.Data.MaxBuses = 1;
> - Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_ERROR_REPLY);
> + Req.Data.ReplyFrameSize = sizeof (MPT_SCSI_IO_REPLY);
(5) Same as (1).
>
> //
> // Send controller init through doorbell
> @@ -203,6 +221,257 @@ MptScsiInit (
> return EFI_SUCCESS;
> }
>
> +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;
> +
> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL ||
> + (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) ||
> + Packet->CdbLength > sizeof (Request->Data.Header.CDB)) {
> + return EFI_UNSUPPORTED;
> + }
> +
> + if (Target > 0 || 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 EFI_BAD_BUFFER_SIZE;
> + }
> + if (Packet->OutTransferLength > sizeof (Dev->Dma->Data)) {
> + Packet->OutTransferLength = sizeof (Dev->Dma->Data);
> + return EFI_BAD_BUFFER_SIZE;
> + }
(6) Please adopt the ReportHostAdapterOverrunError() helper function,
and call, from "PvScsi.c".
> +
> + ZeroMem (Request, sizeof (*Request));
> + Request->Data.Header.TargetID = Target;
> + //
> + // It's 1 and not 0, for some reason...
> + //
(7) I think this comment doesn't add any information. Unless we can
explain why offset 1 is used, I suggest we just remove the comment.
> + Request->Data.Header.LUN[1] = Lun;
(8) This deserves an explicit cast to UINT8 (Lun is UINT64, and Visual
Studio might complain about "loss of precision").
(9) Furthermore, to parallel the comment in "PvScsi.c" ("This cast is
safe as PVSCSI_DEV.MaxLun is defined as UINT8"), we could add a comment
here saying that "only LUN 0 is supported, hence the cast is safe".
> + Request->Data.Header.Function = MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_REQUEST;
> + Request->Data.Header.MessageContext = 1; // We handle one request at a time
> +
> + Request->Data.Header.CDBLength = Packet->CdbLength;
> + CopyMem (Request->Data.Header.CDB, Packet->Cdb, Packet->CdbLength);
> +
> + //
> + // SenseDataLength is UINT8, Sense[] is MAX_UINT8, so we can't overflow
> + //
> + ZeroMem (&Dev->Dma->Sense, Packet->SenseDataLength);
(10) Style nit: while this is not a bug, we usually just let arrays
decay to pointers to their first elements. Right now you are passing a
pointer to the whole array, not just to the first element. (Put
differently, the type of the first argument is not (UINT8*), but
(UINT8(*)[255]).)
IOW, please drop the ampersand "&" from the first arg of ZeroMem().
> + Request->Data.Header.SenseBufferLength = Packet->SenseDataLength;
> + Request->Data.Header.SenseBufferLowAddress = MPT_SCSI_DMA_ADDR (Dev, Sense);
(11) Please add an explicit (UINT32) cast here; MPT_SCSI_DMA_ADDR()
produces a UINT64 (aka EFI_PHYSICAL_ADDRESS) result.
(12) Does this device support 64-bit DMA?
(12a) If it does, then I think:
- we should explicitly *clear* the DUAL_ADDRESS_CYCLE attribute when
setting up the PCI attributes (--> EfiPciIoAttributeOperationDisable),
- and make a comment here that we cleared DUAL_ADDRESS_CYCLE, therefore
the cast is safe.
(12b) If the device does not support 64-bit DMA, then we should make a
comment about *that*, here (i.e., the cast is safe because the device
does not support 64-bit DMA).
> +
> + Request->Data.Sg.EndOfList = 1;
> + Request->Data.Sg.EndOfBuffer = 1;
> + Request->Data.Sg.LastElement = 1;
> + Request->Data.Sg.ElementType = MPT_SG_ENTRY_TYPE_SIMPLE;
> + Request->Data.Sg.DataBufferAddress = MPT_SCSI_DMA_ADDR (Dev, Data);
Right, this seems to support my concern about 64-bit DMA.
Namely, if the "DataBufferAddress" field were 32-bit, then I'd assume
the device is only 32-bit capable. And then (12b) would apply.
However, because "DataBufferAddress" is 64-bit, it looks like the device
is generally capable of 64-bit DMA, and the 32-bit restriction applies
only to the sense buffer. (This is probably the justification for the
word "Low", in the field name "SenseBuffer*Low*Address").
That means we need to keep the DMA mapping for at least the sense array
in 32-bit space. And because we want a single mapping for the whole
thing, I think (12a) applies.
See EFI_PCI_IO_PROTOCOL.AllocateBuffer() in the UEFI spec:
"""
If the current attributes of the PCI controller has the
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE bit set, then when the buffer
allocated by this function is mapped with a call to Map(), the device
address that is returned by Map() must be within the 64-bit device
address space of the PCI Bus Master. The attributes for a PCI controller
can be managed by calling Attributes().
If the current attributes for the PCI controller has the
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE bit clear, then when the buffer
allocated by this function is mapped with a call to Map(), the device
address that is returned by Map() must be within the 32-bit device
address space of the PCI Bus Master. The attributes for a PCI controller
can be managed by calling Attributes().
"""
(13) I further observe that the "Is64BitAddress" field is cleared (with
the ZeroMem() call on Request), and we can hard-code that -- I believe
anyway -- only if we explicitly clear DUAL_ADDRESS_CYCLE.
> +
> + Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE;
> + switch (Packet->DataDirection)
> + {
> + case EFI_EXT_SCSI_DATA_DIRECTION_READ:
> + if (Packet->InTransferLength == 0) {
> + break;
> + }
> + Request->Data.Header.DataLength = Packet->InTransferLength;
> + Request->Data.Sg.Length = Packet->InTransferLength;
> + Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_READ;
> + break;
> + case EFI_EXT_SCSI_DATA_DIRECTION_WRITE:
> + if (Packet->OutTransferLength == 0) {
> + break;
> + }
> + Request->Data.Header.DataLength = Packet->OutTransferLength;
> + Request->Data.Sg.Length = Packet->OutTransferLength;
> + Request->Data.Header.Control = MPT_SCSIIO_REQUEST_CONTROL_TXDIR_WRITE;
> +
> + CopyMem (Dev->Dma->Data, Packet->OutDataBuffer, Packet->OutTransferLength);
> + Request->Data.Sg.BufferContainsData = 1;
> + break;
> + }
(14) I request that we please simplify this switch statement.
My primary reason is that edk2 really dislikes "switch" statements
without "default" case labels. And, from that remark, we can further
simplify this "switch".
Near the top of the function, we exclude both
EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL (EFI_UNSUPPORTED) and
undefined DataDirection values (EFI_INVALID_PARAMETER).
Therefore, here we only need an
if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
...
} else {
...
}
(maybe with a comment on the data directions).
(15) I don't see why hoisting MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NONE
before the "switch" is helpful. That value can only take effect if the
XxxTransferLength field (matching the direction) is zero. But I don't
think those explicit zero checks buy us much.
I would simply remove the zero comparisons, and always go with either
TXDIR_READ or TXDIR_WRITE (dependent on Packet->DataDirection). And, in
case the data size is zero, then just set / copy zero bytes.
(16) "Sg.Length" is a 24-bit value. It will never overflow when assigned
from XxxTransferLength, due to the initial "EFI_BAD_BUFFER_SIZE" checks
(higher up in the function), which enforce 0x2000 bytes as an upper
limit on XxxTransferLength.
But this guarantee would be nice to spell out somewhere. For example:
//
// "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"
);
> +
> + 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 (Dev, IoReply));
(17) Same as (11) -- please explicitly cast the result of
MPT_SCSI_DMA_ADDR() to UINT32.
Furthermore, a comment (according to (12a) vs. (12b)) would be nice,
regarding the safety of the cast.
> + if (EFI_ERROR (Status)) {
> + return EFI_DEVICE_ERROR;
> + }
> + Dev->IoReplyEnqueued = TRUE;
> + }
> +
> + Status = Out32 (Dev, MPT_REG_REQ_Q, MPT_SCSI_DMA_ADDR (Dev, IoRequest));
(18) Same as (17).
> + if (EFI_ERROR (Status)) {
> + return EFI_DEVICE_ERROR;
> + }
(19) Assuming we can expose the reply frame, but not the request, will
the driver continue to behave fine (without explicit rollback actions
for the reply frame)?
Does "IoReplyEnqueued" help with handling this?
(Just a question, not necessarily a code change request.)
> +
> + 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
> + )
> +{
> + CopyMem (Packet->SenseData, Dev->Dma->Sense, Packet->SenseDataLength);
> + 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->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
(20) Does "turbo reply" mean that you have to update neither
XxxTransferLength, nor SenseDataLength, on output?
> +
> + } else if (Reply & (1 << 31)) {
(21) Please write:
(Reply & BIT31) != 0
You could also consider adding a new macro to
"OvmfPkg/Include/IndustryStandard/FusionMptScsi.h", for this bitmask.
(If you choose to do that, then please use BIT31 there as well.)
> + DEBUG ((DEBUG_ERROR, "%a: request failed\n", __FUNCTION__));
> + //
> + // 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;
OK, both are UINT8 objects.
> + Packet->SenseDataLength = Dev->Dma->IoReply.Data.SenseCount;
(22) Please add an explicit cast at least; the LHS is UINT8, the RHS is
UINT32. Visual Studio could emit a warning otherwise.
(Obviously if you know *why* the cast is safe, capturing that in a
comment would also be nice.)
(23) Would it make sense to check that the device only *lowers*
SenseDataLength with the above assignment?
> +
> + switch (Dev->Dma->IoReply.Data.IOCStatus) {
> + case MPT_SCSI_IOCSTATUS_SUCCESS:
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK;
> + break;
(24) This branch seems to lead to an EFI_SUCCESS return code from the
function. (And then it's going to be propagated from MptScsiPassThru(),
IIRC.)
The success return is OK, I believe -- but then the DEBUG_ERROR above
("request failed") seems unjustified. I just feel that the "request
failed" debug message is not consistent with EFI_SUCCESS.
(25) Don't we need to update XxxTransferLength here?
> + 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:
> + if (Packet->TargetStatus == EFI_EXT_SCSI_STATUS_TARGET_GOOD) {
> + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) {
> + Packet->InTransferLength = Dev->Dma->IoReply.Data.TransferCount;
> + } else {
> + Packet->OutTransferLength = Dev->Dma->IoReply.Data.TransferCount;
> + }
> + }
> + Packet->HostAdapterStatus =
> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> + return EFI_SUCCESS;
(26) Basically, same as (24) -- do we want to debug-log a short transfer
as "request failed"? In particular a short read doesn't look unusual at all.
(27) XxxTransferLength is modified only if
"Dev->Dma->IoReply.Data.SCSIStatus" brought us
EFI_EXT_SCSI_STATUS_TARGET_GOOD.
I don't think that's a good idea: if "SCSIStatus" is different from
TARGET_GOOD, then we still return EFI_SUCCESS to the caller. And, per spec:
EFI_SUCCESS -- The SCSI Request Packet was sent by the host. For
bi-directional commands, InTransferLength bytes were
transferred from InDataBuffer. For write and
bi-directional commands, OutTransferLength bytes were
transferred by OutDataBuffer. See HostAdapterStatus,
TargetStatus, SenseDataLength, and SenseData in that
order for additional status information.
(I think the spec has a small typo above: InTransferLength applies to
read *and* bi-dir commands.)
So the caller will go and check InTransferLength and OutTransferLength,
and look at HostAdapterStatus etc. for "additional" info.
I think TransferCount should be set in all cases. Does the device really
only set TransferCount for TARGET_GOOD?
> + case MPT_SCSI_IOCSTATUS_DATA_OVERRUN:
> + Packet->HostAdapterStatus =
> + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN;
> + return EFI_SUCCESS;
(28) Same as (25).
I guess my general point is that, we should update XxxTransferLength
*whenever* we return EFI_SUCCESS.
(29) Style nit:
under MPT_SCSI_IOCSTATUS_SUCCESS, we use "break" for finishing the
function with EFI_SUCCESS. However, under UNDERRUN / OVERRUN, we use
explicit "return EFI_SUCCESS" statements. Please make this easier to
read by sticking with one -- just "break", or "just return EFI_SUCCESS".
> + default:
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + return EFI_DEVICE_ERROR;
> + }
> +
> + } else {
> + DEBUG ((DEBUG_ERROR, "%a: unexpected reply\n", __FUNCTION__));
(30) We could also log Reply itself (%x), but that's just an idea.
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + return EFI_DEVICE_ERROR;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> //
> // Ext SCSI Pass Thru
> //
> @@ -218,7 +487,44 @@ 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);
> + Status = MptScsiPopulateRequest (Dev, *Target, Lun, Packet);
(31) Please add a comment that only the first byte of "Target" is used,
by design. (See PopulateRequest() in "PvScsi.c".)
> + if (EFI_ERROR (Status)) {
> + //
> + // MptScsiPopulateRequest modified packet according to the error
> + //
> + return Status;
> + }
> +
> + Status = MptScsiSendRequest (Dev, Packet);
> + if (EFI_ERROR (Status)) {
> + goto Fatal;
> + }
> +
> + Status = MptScsiGetReply (Dev, &Reply);
> + if (EFI_ERROR (Status)) {
> + goto Fatal;
> + }
> +
> + return MptScsiHandleReply (Dev, Reply, Packet);
> +
> +Fatal:
> + //
> + // We erred in the middle of a transaction, a very serious problem has occured
> + // and it's not clear if it's possible to recover without leaving the hardware
> + // in an inconsistent state. Perhaps we would want to reset the device...
> + //
> + DEBUG ((DEBUG_ERROR, "%a: fatal error in scsi request\n", __FUNCTION__));
> + Packet->InTransferLength = 0;
> + Packet->OutTransferLength = 0;
> + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
> + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_TASK_ABORTED;
> + Packet->SenseDataLength = 0;
> + return EFI_DEVICE_ERROR;
> }
(32) This logic looks OK to me; just please move it to a separate helper
function called ReportHostAdapterError(). Then please replace the "goto
Fatal" statements with "return ReportHostAdapterError()".
See the same function in "PvScsi.c". (I realize that TargetStatus is set
differently -- I'm OK with either value, I'd just like this to be a
separate function.)
>
> STATIC
> @@ -450,6 +756,7 @@ MptScsiControllerStart (
> {
> EFI_STATUS Status;
> MPT_SCSI_DEV *Dev;
> + UINTN BytesMapped;
>
> Dev = AllocateZeroPool (sizeof (*Dev));
> if (Dev == NULL) {
> @@ -494,9 +801,42 @@ MptScsiControllerStart (
> goto CloseProtocol;
> }
>
> + //
> + // Create buffers for data transfer
> + //
> + Status = Dev->PciIo->AllocateBuffer (
> + Dev->PciIo,
> + AllocateAnyPages,
> + EfiBootServicesData,
> + EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)),
> + (VOID **)&Dev->Dma,
> + EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> + );
> + if (EFI_ERROR (Status)) {
> + goto RestoreAttributes;
> + }
> +
> + BytesMapped = sizeof (*Dev->Dma);
(33) I think it's slightly more idiomatic to map all pages in full that
were allocated. In "PvScsi.c", a separate "Pages" helper variable (of
type UINTN) is used. So we could do:
UINTN Pages;
Pages = EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma));
AllocateBuffer ( ... Pages ... );
BytesMapped = EFI_PAGES_TO_SIZE (Pages);
Map ( ... &BytesMapped ...);
if (BytesMapped != EFI_PAGES_TO_SIZE (Pages)) {
...
}
(This would also simplify the FreeBuffer() call on the error path; you
could use "Pages" there at once.)
Feel free to disagree -- I'm certainly not saying the code is "wrong".
> + Status = Dev->PciIo->Map (
> + Dev->PciIo,
> + EfiPciIoOperationBusMasterCommonBuffer,
> + Dev->Dma,
> + &BytesMapped,
> + &Dev->DmaPhysical,
> + &Dev->DmaMapping
> + );
> + if (EFI_ERROR (Status)) {
> + goto FreeBuffer;
> + }
> +
> + if (BytesMapped != sizeof (*Dev->Dma)) {
> + Status = EFI_OUT_OF_RESOURCES;
> + goto Unmap;
> + }
> +
> Status = MptScsiInit (Dev);
> if (EFI_ERROR (Status)) {
> - goto RestorePciAttributes;
> + goto Unmap;
> }
>
> //
> @@ -531,6 +871,18 @@ 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)),
> + Dev->Dma
> + );
(34) The indentations of the arguments as well as the closing
parenthesis are wrong.
(35) Please insert an empty line too, before the subsequent
"RestoreAttributes" label.
> RestoreAttributes:
> Dev->PciIo->Attributes (
> Dev->PciIo,
> @@ -592,6 +944,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,
> EfiPciIoAttributeOperationEnable,
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 809f12173bb8..ef1f6a5ebb3a 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -34,3 +34,6 @@ [LibraryClasses]
> [Protocols]
> gEfiPciIoProtocolGuid ## TO_START
> gEfiExtScsiPassThruProtocolGuid ## BY_START
> +
> +[Pcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES
(36) General -- can you run "BaseTools/Scripts/SetupGit.py" in your edk2
clone? It will improve the file order in which changes are formatted
into patch files. A patch is easier to read if the INF, DEC, and header
file changes come first.
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 28030391cff2..7fa1581f2101 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -270,6 +270,9 @@ [PcdsFixedAtBuild]
> ## Number of page frames to use for storing grant table entries.
> gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33
>
> + ## Microseconds to stall between polling for MptScsi request result
> + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x39
> +
(37) Can you introduce this near "PcdPvScsiWaitForCmpStallInUsecs"? I
quite like that the virtio-scsi and PvScsi PCDs are grouped together;
keeping the MPT SCSI ones close would be nice.
> [PcdsDynamic, PcdsDynamicEx]
> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#57648): https://edk2.groups.io/g/devel/message/57648
Mute This Topic: https://groups.io/mt/73015391/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list