From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web09.6468.1583372122682533287 for ; Wed, 04 Mar 2020 17:35:22 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@oracle.com header.s=corp-2020-01-29 header.b=KrrL3NnV; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 0251S7qZ120941; Thu, 5 Mar 2020 01:35:21 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2020-01-29; bh=YndSzG4noOpCxqAN12PO9XtQqRSVPu0CkqzZuI5d6tM=; b=KrrL3NnVXL6XKeifNNhEQ58pF1WTMlhes7BOyCY3ML+Tc23tkLXLvFjOuaNuPyR2lKyM HPd6iqDeMZw7UNf/nl5QCBciR8YnXu+YWu1EpbgskbcZTKU2djR8asJPgDTNg6VU5uLA z1LZ8a5Ds0Xr8X5I/ivRc90+qwF2vcvx5uWbBWwO/KZMANU21h6XJ42O/fRHDoW11hZw sZT/lfV/PBoB+a8C/GJqtDsh+CUOr6sJ7/719vVEhVTv2HXW8Xi06MGwX3YSDVDPZZnr 9QA9TfJ9aMS63BBkEKI9nOf7TFtYQQ5dfipEYj2/mVaUgktOERBR6k2dFnyh5LStyy+j nA== Received: from userp3020.oracle.com (userp3020.oracle.com [156.151.31.79]) by aserp2120.oracle.com with ESMTP id 2yffwr2427-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 05 Mar 2020 01:35:21 +0000 Received: from pps.filterd (userp3020.oracle.com [127.0.0.1]) by userp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 0251SX4t073602; Thu, 5 Mar 2020 01:35:21 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3020.oracle.com with ESMTP id 2yg1p92d5r-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 05 Mar 2020 01:35:20 +0000 Received: from abhmp0019.oracle.com (abhmp0019.oracle.com [141.146.116.25]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 0251ZJp8010085; Thu, 5 Mar 2020 01:35:19 GMT Received: from [192.168.14.112] (/79.177.218.220) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 04 Mar 2020 17:35:18 -0800 Subject: Re: [PATCH v3 12/13] OvmfPkg/MptScsiDxe: Implement the PassThru method To: Nikita Leshenko , devel@edk2.groups.io Cc: aaron.young@oracle.com, jordan.l.justen@intel.com, lersek@redhat.com, ard.biesheuvel@linaro.org References: <20200304192257.96736-1-nikita.leshchenko@oracle.com> <20200304192257.96736-13-nikita.leshchenko@oracle.com> From: Liran Alon Message-ID: <7939b20e-2b43-661e-4476-3b8574586098@oracle.com> Date: Thu, 5 Mar 2020 03:35:15 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200304192257.96736-13-nikita.leshchenko@oracle.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9550 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0 mlxlogscore=999 mlxscore=0 spamscore=0 adultscore=0 bulkscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2003050005 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9550 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 lowpriorityscore=0 spamscore=0 impostorscore=0 malwarescore=0 mlxlogscore=999 mlxscore=0 suspectscore=0 phishscore=0 clxscore=1015 bulkscore=0 adultscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2003050005 X-MIME-Autoconverted: from 8bit to quoted-printable by aserp2120.oracle.com id 0251S7qZ120941 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 04/03/2020 21:22, Nikita Leshenko wrote: > Machines should be able to boot after this commit. Tested with differen= t > Linux distributions (Ubuntu, CentOS) and different Windows > versions (Windows 7, Windows 10, Server 2016). > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2390 > Signed-off-by: Nikita Leshenko > --- > .../Include/IndustryStandard/FusionMptScsi.h | 18 + > OvmfPkg/MptScsiDxe/MptScsi.c | 344 +++++++++++++++++= - > OvmfPkg/MptScsiDxe/MptScsiDxe.inf | 3 + > OvmfPkg/OvmfPkg.dec | 3 + > 4 files changed, 365 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h b/OvmfPkg= /Include/IndustryStandard/FusionMptScsi.h > index 1ce2432bd3c2..e793f4856d0b 100644 > --- a/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > +++ b/OvmfPkg/Include/IndustryStandard/FusionMptScsi.h > @@ -50,6 +50,12 @@ > =20 > #define MPT_IOC_WHOINIT_ROM_BIOS 0x02 > =20 > +#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_IO_ERROR_IOCSTATUS_DEVICE_NOT_THERE 0x0043 > + > // > // Device structures > // > @@ -109,6 +115,10 @@ typedef struct { > UINT32 Length: 24; > UINT32 EndOfList: 1; > UINT32 Is64BitAddress: 1; > + // > + // True when the buffer contains data to be transfered. Otherwise it= 's the > + // destination buffer > + // Why is this comment added on this patch and not the previous one which=20 introduced the struct? > UINT32 BufferContainsData: 1; > UINT32 LocalAddress: 1; > UINT32 ElementType: 2; > @@ -141,4 +151,12 @@ typedef struct { > UINT64 Uint64; // 8 byte alignment required by HW > } MPT_SCSI_IO_ERROR_REPLY; > =20 > +typedef union { > + struct { > + MPT_SCSI_IO_REQUEST Header; > + MPT_SG_ENTRY_SIMPLE Sg; > + } Data; > + 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 37f1ea4b3506..0985be07bc8e 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsi.c > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > #include > =20 > // > @@ -35,6 +36,13 @@ > // Runtime Structures > // > =20 > +typedef struct { > + MPT_SCSI_IO_ERROR_REPLY IoErrorReply; > + MPT_SCSI_REQUEST_WITH_SG IoRequest; > + UINT8 Sense[MAX_UINT8]; > + UINT8 Data[0x2000]; > +} MPT_SCSI_DMA_BUFFER; > + > #define MPT_SCSI_DEV_SIGNATURE SIGNATURE_32 ('M','P','T','S') > typedef struct { > UINT32 Signature; > @@ -42,11 +50,18 @@ 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; > } MPT_SCSI_DEV; > =20 > #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \ > CR (PassThruPtr, MPT_SCSI_DEV, PassThru, MPT_SCSI_DEV_SIGNATURE) > =20 > +#define MPT_SCSI_DMA_ADDR(Dev, MemberName) \ > + (Dev->DmaPhysical + OFFSET_OF(MPT_SCSI_DMA_BUFFER, MemberName)) > + > // > // Hardware functions > // > @@ -147,6 +162,8 @@ MptScsiInit ( > UINT8 *ReplyBytes; > UINT32 Reply32; > =20 > + Dev->StallPerPollUsec =3D PcdGet32 (PcdMptScsiStallPerPollUsec); > + > Status =3D MptScsiReset (Dev); > if (EFI_ERROR (Status)) { > return Status; > @@ -205,6 +222,227 @@ MptScsiInit ( > return Status; > } > =20 > + // > + // Put one free reply frame on the reply queue, the hardware may use= it to > + // report an error to us. > + // > + Status =3D Out32 (Dev, MPT_REG_REP_Q, MPT_SCSI_DMA_ADDR (Dev, IoErro= rReply)); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + 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 =3D &Dev->Dma->IoRequest; > + > + if (Packet->DataDirection =3D=3D EFI_EXT_SCSI_DATA_DIRECTION_BIDIREC= TIONAL || > + Packet->CdbLength > sizeof (Request->Data.Header.CDB)) { > + return EFI_UNSUPPORTED; > + } According to VirtioScsi implementation, you should also check for=20 bidirectional direction by adding the following condition: "(Packet->InTransferLength > 0 && Packet->OutTransferLength > 0). > + > + if (Target > 0 || Lun > 0) { > + return EFI_INVALID_PARAMETER; > + } According to VirtioScsi implementation, you should also check for the=20 following contradicting parameters (I don't know if it's really=20 required...): =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Trying to receive, but destination poi= nter is NULL, or=20 contradicting =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // transfer direction =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((Packet->InTransferLength > 0) && =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((Packet->InDataBuffer =3D=3D NULL)= || =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (Packet->DataDirection =3D=3D= EFI_EXT_SCSI_DATA_DIRECTION_WRITE) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ) || =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // Trying to send, but source pointer is = NULL, or contradicting =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // transfer direction =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((Packet->OutTransferLength > 0) && =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((Packet->OutDataBuffer =3D=3D NULL= ) || =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (Packet->DataDirection =3D=3D= EFI_EXT_SCSI_DATA_DIRECTION_READ) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ) =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ) > + > + if (Packet->InTransferLength > sizeof (Dev->Dma->Data)) { > + Packet->InTransferLength =3D sizeof (Dev->Dma->Data); > + return EFI_BAD_BUFFER_SIZE; > + } > + if (Packet->OutTransferLength > sizeof (Dev->Dma->Data)) { > + Packet->OutTransferLength =3D sizeof (Dev->Dma->Data); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + ZeroMem (Request, sizeof (*Request)); > + Request->Data.Header.TargetID =3D Target; > + // > + // It's 1 and not 0, for some reason... > + // > + Request->Data.Header.LUN[1] =3D Lun; > + Request->Data.Header.Function =3D MPT_MESSAGE_HDR_FUNCTION_SCSI_IO_R= EQUEST; > + Request->Data.Header.MessageContext =3D 1; // We handle one request = at a time > + > + Request->Data.Header.CDBLength =3D Packet->CdbLength; > + CopyMem (Request->Data.Header.CDB, Packet->Cdb, Packet->CdbLength); > + > + // > + // SenseDataLength is UINT8, Sense[] is MAX_UINT8, so we can't overf= low > + // > + ZeroMem (&Dev->Dma->Sense, Packet->SenseDataLength); > + Request->Data.Header.SenseBufferLength =3D Packet->SenseDataLength; > + Request->Data.Header.SenseBufferLowAddress =3D MPT_SCSI_DMA_ADDR (De= v, Sense); > + > + Request->Data.Sg.EndOfList =3D 1; > + Request->Data.Sg.EndOfBuffer =3D 1; > + Request->Data.Sg.LastElement =3D 1; > + Request->Data.Sg.ElementType =3D MPT_SG_ENTRY_TYPE_SIMPLE; > + Request->Data.Sg.DataBufferAddress =3D MPT_SCSI_DMA_ADDR (Dev, Data)= ; > + > + Request->Data.Header.Control =3D MPT_SCSIIO_REQUEST_CONTROL_TXDIR_NO= NE; Why explicitly init Control field to 0? You have already=20 ZeroMem(Request). Seems redundant. > + switch (Packet->DataDirection) > + { > + case EFI_EXT_SCSI_DATA_DIRECTION_READ: > + if (Packet->InTransferLength =3D=3D 0) { > + break; > + } > + Request->Data.Header.DataLength =3D Packet->InTransferLength; > + Request->Data.Sg.Length =3D Packet->InTransferLength; > + Request->Data.Header.Control =3D MPT_SCSIIO_REQUEST_CONTROL_TXDIR_= READ; > + break; > + case EFI_EXT_SCSI_DATA_DIRECTION_WRITE: > + if (Packet->OutTransferLength =3D=3D 0) { > + break; > + } > + Request->Data.Header.DataLength =3D Packet->OutTransferLength; > + Request->Data.Sg.Length =3D Packet->OutTransferLength; > + Request->Data.Header.Control =3D MPT_SCSIIO_REQUEST_CONTROL_TXDIR_= WRITE; > + > + CopyMem (Dev->Dma->Data, Packet->OutDataBuffer, Packet->OutTransfe= rLength); > + Request->Data.Sg.BufferContainsData =3D 1; > + break; > + } > + > + 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; > + > + // > + // Make sure Request is fully written > + // > + MemoryFence (); MemoryFence() shouldn't be required here and thus should be removed. EDK2 IOPort and MMIO accessors should be responsible for performing=20 required compiler-barrier and memory-barriers (Based on architecture).=20 If that's not the case, then this is a bug in EDK2 that should be fixed. On x86 specific (As we are talking about IOPorts which are x86=20 specific), IOPort instruction (e.g. "outl") guarantees it is completed=20 only after all previous reads/writes were completed and performed on=20 memory (E.g. Store-buffer is flushed). In addition, writes to IOSpace=20 are guaranteed to never be buffered. See AMD System Programming Manual=20 Volume 2 section 7.5 Buffering and Combining Memory Writes. Specifically, it can be seen in=20 MdePkg/Library/BaseIoLibIntrinsic/IoLibMsc.c that IoWrite32() use=20 _ReadWriteBarrier() to perform required compiler-barrier. In contrast,=20 MdePkg/Library/BaseIoLibIntrinsic/IoLibGcc.c IoWrite32() seems to only=20 execute "outl" using gcc inline asm. It seems a bit bizzare to me that=20 it doesn't specify the "memory" clobber to perform required=20 compiler-barrier. Lazlo, is this a bug? If so, I can submit a trivial=20 commit to fix it. > + > + Status =3D Out32 (Dev, MPT_REG_REQ_Q, MPT_SCSI_DMA_ADDR (Dev, IoRequ= est)); > + if (EFI_ERROR (Status)) { > + // > + // We couldn't enqueue the request, report it as an adapter error > + // > + Packet->InTransferLength =3D 0; > + Packet->OutTransferLength =3D 0; > + Packet->HostAdapterStatus =3D EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTH= ER; > + Packet->TargetStatus =3D EFI_EXT_SCSI_STATUS_TARGET_GOOD; > + Packet->SenseDataLength =3D 0; > + 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; > + Nit: Add a comment here that this loop is infinite as you currently=20 don't support EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET Timeout. > + for (;;) { > + Status =3D 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 =3D In32 (Dev, MPT_REG_REP_Q, Reply); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // The driver is supposed to fetch replies until 0xffffffff is retur= ned, which > + // will reset the interrupt status. We put only one request, so we e= xpect the > + // next read reply to be the last. > + // > + Status =3D In32 (Dev, MPT_REG_REP_Q, &EmptyReply); > + if (EFI_ERROR (Status) || EmptyReply !=3D 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 > + ) > +{ > + EFI_STATUS Status; > + > + CopyMem (Packet->SenseData, Dev->Dma->Sense, Packet->SenseDataLength= ); > + if (Packet->DataDirection =3D=3D EFI_EXT_SCSI_DATA_DIRECTION_READ) { > + CopyMem (Packet->InDataBuffer, Dev->Dma->Data, Packet->InTransferL= ength); > + } You should populate Packet->TargetStatus with=20 Dev->Dma->IoErrorReply.Data.SCSIStatus. In theory, you should've also updated Packet->SenseDataLength according=20 to Dev->Dma->IoErrorReply.Data.SenseCount, and=20 Packet->InTransferLength/OutTransferLength according to=20 Dev->Dma->IoErrorReply.Data.TransferCount. This is documented in these=20 EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET documentation. However, examining VirtualBox implementation for this device=20 (https://www.virtualbox.org/browser/vbox/trunk/src/VBox/Devices/Storage/D= evLsiLogicSCSI.cpp)=20 seems to reveal that it only updates SenseCount and TransferCount fields=20 on I/O error. Therefore, it seems that the above suggestion won't work=20 on this device emulation. I would result to at least documenting why you don't update these fields=20 if this is the case. i.e. To preserve working on buggy device emulation=20 implementations. > + > + if (Reply =3D=3D Dev->Dma->IoRequest.Data.Header.MessageContext) { > + // > + // Everything is good > + // > + Packet->HostAdapterStatus =3D EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK; > + Packet->TargetStatus =3D EFI_EXT_SCSI_STATUS_TARGET_GOOD; > + > + } else if (Reply & (1 << 31)) { > + DEBUG ((DEBUG_ERROR, "%a: request failed\n", __FUNCTION__)); > + // > + // When reply MSB is set, it's an error frame. > + // > + > + switch (Dev->Dma->IoErrorReply.Data.IOCStatus) { > + case MPT_SCSI_IO_ERROR_IOCSTATUS_DEVICE_NOT_THERE: > + Packet->HostAdapterStatus =3D > + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT; > + break; > + default: > + Packet->HostAdapterStatus =3D EFI_EXT_SCSI_STATUS_HOST_ADAPTER_O= THER; > + break; > + } > + > + // > + // Resubmit the reply frame to the reply queue > + // > + Status =3D Out32 (Dev, MPT_REG_REP_Q, MPT_SCSI_DMA_ADDR (Dev, IoEr= rorReply)); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + } else { > + DEBUG ((DEBUG_ERROR, "%a: unexpected reply\n", __FUNCTION__)); > + return EFI_DEVICE_ERROR; > + } > + > return EFI_SUCCESS; > } > =20 > @@ -223,7 +461,50 @@ MptScsiPassThru ( > IN EFI_EVENT Event OPTIONA= L > ) > { > - return EFI_UNSUPPORTED; > + EFI_STATUS Status; > + MPT_SCSI_DEV *Dev; > + UINT32 Reply; > + > + Dev =3D MPT_SCSI_FROM_PASS_THRU (This); > + Status =3D MptScsiPopulateRequest (Dev, *Target, Lun, Packet); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status =3D MptScsiSendRequest (Dev, Packet); > + if (EFI_ERROR (Status)) { Nit: I suggest adding here a comment that in case of failure,=20 MptScsiSendRequest() have modified Packet fields accordingly. As it's=20 not immediately obvious. > + return Status; > + } > + > + Packet->HostAdapterStatus =3D EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK; > + > + Status =3D MptScsiGetReply (Dev, &Reply); > + if (EFI_ERROR (Status)) { > + goto Fatal; > + } > + > + Status =3D MptScsiHandleReply (Dev, Reply, Packet); > + if (EFI_ERROR (Status)) { > + goto Fatal; > + } > + > + return Status; > + > +Fatal: > + // > + // We erred in the middle of a transaction, a very serious problem h= as occured > + // and it's not clear if it's possible to recover without leaving th= e hardware > + // in an inconsistent state. Perhaps we would want to reset the devi= ce... > + // > + DEBUG ((DEBUG_ERROR, "%a: fatal error in scsi request\n", __FUNCTION= __)); > + Packet->InTransferLength =3D 0; > + Packet->OutTransferLength =3D 0; > + if (Packet->HostAdapterStatus =3D=3D EFI_EXT_SCSI_STATUS_HOST_ADAPTE= R_OK) { > + Packet->HostAdapterStatus =3D EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTH= ER; > + } > + Packet->TargetStatus =3D EFI_EXT_SCSI_STATUS_TARGET_TASK_ABORTE= D; > + Packet->SenseDataLength =3D 0; > + return EFI_DEVICE_ERROR; > } > =20 > STATIC > @@ -453,6 +734,7 @@ MptScsiControllerStart ( > { > EFI_STATUS Status; > MPT_SCSI_DEV *Dev; > + UINTN BytesMapped; > =20 > Dev =3D AllocateZeroPool (sizeof (*Dev)); > if (Dev =3D=3D NULL) { > @@ -497,9 +779,42 @@ MptScsiControllerStart ( > goto CloseProtocol; > } > =20 > + // > + // Create buffers for data transfer > + // > + Status =3D 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 =3D sizeof (*Dev->Dma); > + Status =3D Dev->PciIo->Map ( > + Dev->PciIo, > + EfiPciIoOperationBusMasterCommonBuffer, > + Dev->Dma, > + &BytesMapped, > + &Dev->DmaPhysical, > + &Dev->DmaMapping > + ); > + if (EFI_ERROR (Status)) { > + goto FreeBuffer; > + } > + > + if (BytesMapped !=3D sizeof (*Dev->Dma)) { > + Status =3D EFI_OUT_OF_RESOURCES; > + goto Unmap; > + } > + Nit: I suggest introducing the DMA communication buffer in a separate=20 patch. It ease review and makes code easier to bisect. > Status =3D MptScsiInit (Dev); > if (EFI_ERROR (Status)) { > - goto CloseProtocol; > + goto Unmap; > } > =20 > // > @@ -526,11 +841,23 @@ MptScsiControllerStart ( > &Dev->PassThru > ); > if (EFI_ERROR (Status)) { > - goto RestoreAttributes; > + goto Unmap; > } > =20 > return EFI_SUCCESS; > =20 > +Unmap: > + Dev->PciIo->Unmap ( > + Dev->PciIo, > + Dev->DmaMapping > + ); > + > +FreeBuffer: > + Dev->PciIo->FreeBuffer ( > + Dev->PciIo, > + EFI_SIZE_TO_PAGES (sizeof (*Dev->Dma)), > + Dev->Dma > + ); > RestoreAttributes: > Dev->PciIo->Attributes ( > Dev->PciIo, > @@ -590,6 +917,17 @@ MptScsiControllerStop ( > =20 > MptScsiReset (Dev); > =20 > + 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/Mpt= ScsiDxe.inf > index 8f366b92eb72..1ba65f2fbbdf 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf > +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf > @@ -40,3 +40,6 @@ [LibraryClasses] > [Protocols] > gEfiPciIoProtocolGuid ## TO_START > gEfiExtScsiPassThruProtocolGuid ## BY_START > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec ## CONSUMES > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 4c5b6511cb97..7e8097f9952e 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -228,6 +228,9 @@ [PcdsFixedAtBuild] > ## Number of page frames to use for storing grant table entries. > gUefiOvmfPkgTokenSpaceGuid.PcdXenGrantFrames|4|UINT32|0x33 > =20 > + ## Microseconds to stall between polling for MptScsi request result > + gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiStallPerPollUsec|5|UINT32|0x36 > + > [PcdsDynamic, PcdsDynamicEx] > gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEA= N|0x10