From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.3853.1588240045428582318 for ; Thu, 30 Apr 2020 02:47:26 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ezC5mdtf; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1588240044; 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=Gu4xVdbZaeUde1nSyFwPuxmdDOPKKRI0hL8Bn+6lumE=; b=ezC5mdtfNZT8X8pTJT+W9xPBepHcw0vHvH7DiZb0+4fYEHmNHXZjOGUqSlYlkKzMaWAc// IPKkUVCU//GHDxm14vlHYfoTKaDKy5yEMZhcE1P+VI1F+3KKlmMMqDpd5VIkNyhCbaouow jlA5DLYuBusVjk8KBIVahw1WncNWlvQ= 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-236-_h-qQ45pOkutd0cSl9vhJw-1; Thu, 30 Apr 2020 05:47:19 -0400 X-MC-Unique: _h-qQ45pOkutd0cSl9vhJw-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 EFA8E45F; Thu, 30 Apr 2020 09:47:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-57.ams2.redhat.com [10.36.115.57]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2A9C7605DD; Thu, 30 Apr 2020 09:47:15 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v5 11/12] 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: <20200424175927.41210-1-nikita.leshchenko@oracle.com> <20200424175927.41210-12-nikita.leshchenko@oracle.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 30 Apr 2020 11:47:15 +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: <20200424175927.41210-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/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 > --- > .../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 > #include > #include > +#include > #include > #include > > @@ -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 (Well, (1) is a must, (2) through (4) are open for discussion, of course.) Thanks! Laszlo