From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.7218.1587403847322951864 for ; Mon, 20 Apr 2020 10:30:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=gQyZTZPq; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587403846; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=oB4rB2M9+KryJJKq52DINFXiymPIqUYbzMZMShHqRJg=; b=gQyZTZPqSC9PvHDQ98G8f9KMYmY91AJOvy8mPAiqxZEOaMJlXcf7+JZ53i5X8Xe4JqWZYy +4y7+Icq0BJcAAQh2/rqvVJBuORTpd5nYQIMh8nhobD1EsF8UtZZdK9vBq7XK44vmBzfES Rr+d72OaN9UnqwaPT3wGvSsvNlgT6Q8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-185-MYWc8FCqMge2NmAQkC8o6w-1; Mon, 20 Apr 2020 13:30:39 -0400 X-MC-Unique: MYWc8FCqMge2NmAQkC8o6w-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D5F9E8048EF; Mon, 20 Apr 2020 17:30:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-228.ams2.redhat.com [10.36.114.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id C9A8018A85; Mon, 20 Apr 2020 17:30:25 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 11/13] OvmfPkg/MptScsiDxe: Implement the PassThru method To: devel@edk2.groups.io, nikita.leshchenko@oracle.com Cc: liran.alon@oracle.com, aaron.young@oracle.com, Jordan Justen , Ard Biesheuvel References: <20200414173813.7715-1-nikita.leshchenko@oracle.com> <20200414173813.7715-12-nikita.leshchenko@oracle.com> From: "Laszlo Ersek" Message-ID: <6be4886a-85e8-a343-0480-a758273cee5e@redhat.com> Date: Mon, 20 Apr 2020 19:30:24 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200414173813.7715-12-nikita.leshchenko@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > --- > .../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 > #include > #include > +#include > #include > #include > > @@ -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