From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web11.9163.1585308375851178987 for ; Fri, 27 Mar 2020 04:26:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VS7Q16Ta; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585308374; 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=0QgRenwFEDM0v2dk0yTLKpA1EviVcn/QqUpIopy31gk=; b=VS7Q16TaqjhVR+mZoRRtxeIKYbV/leaBhebPt0+bK+3WymICFyW3c15bcNOdJgSgUKVOHh wX/QTRjJIfA/Htx4xUGvhqbnwjBIhoSsFEQrtfJVeD4RQoFrJnnF8kDM7mNoyB949rZten 7TIGILiTwp5UbgV8C1Jso3A3ENd+a3c= 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-228-PJMW9a9yPy-QUon3mT3K8Q-1; Fri, 27 Mar 2020 07:26:09 -0400 X-MC-Unique: PJMW9a9yPy-QUon3mT3K8Q-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C0DC2A0CC1; Fri, 27 Mar 2020 11:26:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-36.ams2.redhat.com [10.36.114.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 01A4D5DA2C; Fri, 27 Mar 2020 11:26:05 +0000 (UTC) From: "Laszlo Ersek" Subject: Re: [edk2-devel] [PATCH v2 15/17] OvmfPkg/PvScsiDxe: Support sending SCSI request and receive response To: devel@edk2.groups.io, liran.alon@oracle.com Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200325161005.16743-1-liran.alon@oracle.com> <20200325161005.16743-16-liran.alon@oracle.com> Message-ID: Date: Fri, 27 Mar 2020 12:26:05 +0100 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: <20200325161005.16743-16-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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 03/25/20 17:10, Liran Alon wrote: > Implement EXT_SCSI_PASS_THRU.PassThru(). > > Machines should be able to boot after this commit. > Tested with Ubuntu 16.04 guest. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567 > Signed-off-by: Liran Alon > --- > OvmfPkg/OvmfPkg.dec | 6 + > OvmfPkg/PvScsiDxe/PvScsi.c | 442 +++++++++++++++++++++++++++++++- > OvmfPkg/PvScsiDxe/PvScsi.h | 1 + > OvmfPkg/PvScsiDxe/PvScsiDxe.inf | 5 +- > 4 files changed, 451 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index a04aee5c2cd4..ff49ec0e9e6a 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -130,6 +130,12 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit|64|UINT8|0x36 > gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit|0|UINT8|0x37 > > + ## After PvScsiDxe sends a SCSI request to the device, it waits for > + # the request completion in a polling loop. > + # This constant defines how many micro-seconds to wait between each > + # polling loop iteration. > + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x38 > + > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index 928984099520..de4122e39a81 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -31,6 +31,27 @@ > // Ext SCSI Pass Thru utilities > // > > +/** > + Reads a 32-bit value into BAR0 using MMIO > +**/ > +STATIC > +EFI_STATUS > +PvScsiMmioRead32 ( > + IN CONST PVSCSI_DEV *Dev, > + IN UINT64 Offset, > + OUT UINT32 *Value > + ) > +{ > + return Dev->PciIo->Mem.Read ( > + Dev->PciIo, > + EfiPciIoWidthUint32, > + PCI_BAR_IDX0, > + Offset, > + 1, // Count > + Value > + ); > +} > + > /** > Writes a 32-bit value into BAR0 using MMIO > **/ > @@ -109,6 +130,352 @@ PvScsiWriteCmdDesc ( > return EFI_SUCCESS; > } > > +/** > + Returns if PVSCSI request ring is full > +**/ > +STATIC > +BOOLEAN > +PvScsiIsReqRingFull ( > + IN CONST PVSCSI_DEV *Dev > + ) > +{ > + PVSCSI_RINGS_STATE *RingsState; > + UINT32 ReqNumEntries; > + > + RingsState = Dev->RingDesc.RingState; > + ReqNumEntries = 1U << RingsState->ReqNumEntriesLog2; > + return (RingsState->ReqProdIdx - RingsState->CmpConsIdx) >= ReqNumEntries; > +} (Just some thoughts, not a request for changing the code.) Normally I prefer accessing buffers shared with the device though volatile-qualified pointers. Meaning, in this case, that every "PCI host" pointer (i.e., each pointer that is associated with a PVSCSI_DMA_DESC) would have to be volatile-qualified. In particular: - in patch#13, PVSCSI_RING_DESC would have to be updated like this: > typedef struct { > volatile PVSCSI_RINGS_STATE *RingState; > PVSCSI_DMA_DESC RingStateDmaDesc; > > volatile PVSCSI_RING_REQ_DESC *RingReqs; > PVSCSI_DMA_DESC RingReqsDmaDesc; > > volatile PVSCSI_RING_CMP_DESC *RingCmps; > PVSCSI_DMA_DESC RingCmpsDmaDesc; > } PVSCSI_RING_DESC; - in patch#14, PVSCSI_DEV would change as follows: > typedef struct { > UINT32 Signature; > EFI_PCI_IO_PROTOCOL *PciIo; > EFI_EVENT ExitBoot; > UINT64 OriginalPciAttributes; > PVSCSI_RING_DESC RingDesc; > volatile PVSCSI_DMA_BUFFER *DmaBuf; > PVSCSI_DMA_DESC DmaBufDmaDesc; > UINT8 MaxTarget; > UINT8 MaxLun; > UINTN WaitForCmpStallInUsecs; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; > } PVSCSI_DEV; After these changes, the compiler would (justifiedly) flag a bunch of code locations casting away the volatile qualification -- for example, in the above function, in the assignment to the "RingsState" local variable. Clearly, most of these compilation errors would have to be fixed (not suppressed), because they would be valid. Meaning: - you'd have to volatile-qualify the "RingsState" local variable in all of PvScsiIsReqRingFull(), PvScsiGetCurrentRequest(), PvScsiWaitForRequestCompletion(); - you'd also have to volatile-qualify the return types of PvScsiGetCurrentRequest() and PvScsiWaitForRequestCompletion(); - you'd have to update PopulateRequest() and HandleResponse() too; and the most annoying part of that would be that you could no longer use CopyMem() and ZeroMem() -- because those functions take pointer-to-void parameters, rather than pointer-to-volatile-void ones. (FWIW, we wouldn't have to change the PvScsiFreeSharedPages() prototype -- it would be OK to cast away volatile in those calls, as we wouldn't dereference the pointers in that case.) So... the reason I'm not actually requesting these volatile-qualifications is that (a) your use of MemoryFence() seems mostly OK, and (b) the UEFI Driver Writer's guide recommends *either* volatile *or* MemoryFence(). Of course using both techniques at the same time is not a problem -- and in code I write I actually like to use both at the same time --, but just one suffices too. (See section 4.2.6 "Memory ordering" in the DWG.) The reason I'm writing this up here is because I want the "record" (the mailing list archive) to show that we have considered this topic explicitly. Back to your patch: On 03/25/20 17:10, Liran Alon wrote: > + > +/** > + Returns pointer to current request descriptor to produce > +**/ > +STATIC > +PVSCSI_RING_REQ_DESC * > +PvScsiGetCurrentRequest ( > + IN CONST PVSCSI_DEV *Dev > + ) > +{ > + PVSCSI_RINGS_STATE *RingState; > + UINT32 ReqNumEntries; > + > + RingState = Dev->RingDesc.RingState; > + ReqNumEntries = 1U << RingState->ReqNumEntriesLog2; > + return Dev->RingDesc.RingReqs + > + (RingState->ReqProdIdx & (ReqNumEntries - 1)); > +} > + > +/** > + Returns pointer to current completion descriptor to consume > +**/ > +STATIC > +PVSCSI_RING_CMP_DESC * > +PvScsiGetCurrentResponse ( > + IN CONST PVSCSI_DEV *Dev > + ) > +{ > + PVSCSI_RINGS_STATE *RingState; > + UINT32 CmpNumEntries; > + > + RingState = Dev->RingDesc.RingState; > + CmpNumEntries = 1U << RingState->CmpNumEntriesLog2; > + return Dev->RingDesc.RingCmps + > + (RingState->CmpConsIdx & (CmpNumEntries - 1)); > +} > + > +/** > + Wait for device to signal completion of submitted requests > +**/ > +STATIC > +EFI_STATUS > +PvScsiWaitForRequestCompletion ( > + IN CONST PVSCSI_DEV *Dev > + ) > +{ > + EFI_STATUS Status; > + UINT32 IntrStatus; > + > + // > + // Note: We don't yet support Timeout according to > + // EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET.Timeout. > + // > + // This is consistent with some other Scsi PassThru drivers > + // such as VirtioScsi. > + // > + for (;;) { > + Status = PvScsiMmioRead32 (Dev, PvScsiRegOffsetIntrStatus, &IntrStatus); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // PVSCSI_INTR_CMPL_MASK is set if device completed submitted requests > + // > + if ((IntrStatus & PVSCSI_INTR_CMPL_MASK) != 0) { > + break; (1) Wrong indentation. > + } > + > + gBS->Stall (Dev->WaitForCmpStallInUsecs); > + } > + > + // > + // Acknowledge PVSCSI_INTR_CMPL_MASK in device interrupt-status register > + // > + return PvScsiMmioWrite32 ( > + Dev, > + PvScsiRegOffsetIntrStatus, > + PVSCSI_INTR_CMPL_MASK > + ); > +} > + > +/** > + Populate a PVSCSI request descriptor from the Extended SCSI Pass Thru > + Protocol packet. > +**/ > +STATIC > +EFI_STATUS > +PopulateRequest ( > + IN CONST PVSCSI_DEV *Dev, > + IN UINT8 *Target, > + IN UINT64 Lun, > + IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet, > + OUT PVSCSI_RING_REQ_DESC *Request > + ) > +{ > + UINT8 TargetValue; > + > + // > + // We only use first byte of target identifer > + // > + TargetValue = *Target; > + > + // > + // Check for unsupported requests > + // > + if ( > + // > + // Bidirectional transfer was requested > + // > + (Packet->InTransferLength > 0 && Packet->OutTransferLength > 0) || > + (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_BIDIRECTIONAL) || > + // > + // Command Descriptor Block bigger than this constant should be considered > + // out-of-band. We currently don't support these CDBs. > + // > + (Packet->CdbLength > PVSCSI_CDB_MAX_SIZE) > + ) { > + > + // > + // This error code doesn't require updates to the Packet output fields > + // > + return EFI_UNSUPPORTED; > + } > + > + // > + // Check for invalid parameters > + // > + if ( > + // > + // Addressed invalid device > + // > + (TargetValue > Dev->MaxTarget) || (Lun > Dev->MaxLun) || > + // > + // Invalid direction (there doesn't seem to be a macro for the "no data > + // transferred" "direction", eg. for TEST UNIT READY) > + // > + (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) > + ) > + ) > + ) { > + > + // > + // This error code doesn't require updates to the Packet output fields > + // > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // Check for input/output buffer too large for DMA communication buffer > + // > + if (Packet->InTransferLength > sizeof (Dev->DmaBuf->Data)) { > + Packet->InTransferLength = sizeof (Dev->DmaBuf->Data); > + return EFI_BAD_BUFFER_SIZE; > + } > + if (Packet->OutTransferLength > sizeof (Dev->DmaBuf->Data)) { > + Packet->OutTransferLength = sizeof (Dev->DmaBuf->Data); > + return EFI_BAD_BUFFER_SIZE; > + } > + > + // > + // Encode PVSCSI request > + // > + ZeroMem (Request, sizeof (*Request)); > + > + Request->Bus = 0; > + Request->Target = TargetValue; > + // > + // This cast is safe as MaxLun is defined as UINT8 > + // > + Request->Lun[1] = (UINT8)Lun; > + Request->SenseLen = Packet->SenseDataLength; Ah, *now* I understand why you chose MAX_UINT8 as the size of "PVSCSI_DMA_BUFFER.SenseData". Because, "Packet->SenseDataLength" has type UINT8, and this way you guarantee that the SCSI client's "Packet->SenseDataLength" will always fit in the DMA buffer. Good solution, but it *absolutely* needs to be documented in patch#14 ("OvmfPkg/PvScsiDxe: Introduce DMA communication buffer") -- in fact, see my question (4) under patch#14. (2) Also, please add a comment here that a "Dev->DmaBuf->SenseData" overflow is not possible due to "Packet->SenseDataLength" having type UINT8. This would be a comment in the same vein as the "MaxLun" reference just above -- I find *that* comment very helpful, too. > + Request->SenseAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, SenseData); (3) Please drop the (UINT64) cast -- see my earlier comment about EFI_PHYSICAL_ADDRESS being a typedef to UINT64. > + Request->CdbLen = Packet->CdbLength; > + CopyMem (Request->Cdb, Packet->Cdb, Packet->CdbLength); Right, this is safe, due to the PVSCSI_CDB_MAX_SIZE near the top of the function (PVSCSI_CDB_MAX_SIZE is 16, and "sizeof PVSCSI_RING_REQ_DESC.Cdb" is also 16). > + Request->VcpuHint = 0; > + Request->Tag = PVSCSI_SIMPLE_QUEUE_TAG; > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) { > + Request->Flags = PVSCSI_FLAG_CMD_DIR_TOHOST; > + Request->DataLen = Packet->InTransferLength; > + } else { > + Request->Flags = PVSCSI_FLAG_CMD_DIR_TODEVICE; > + Request->DataLen = Packet->OutTransferLength; > + CopyMem ( > + Dev->DmaBuf->Data, > + Packet->OutDataBuffer, > + Packet->OutTransferLength > + ); > + } > + Request->DataAddr = (UINT64)PVSCSI_DMA_BUF_DEV_ADDR (Dev, Data); (4) Same as (3). > + > + return EFI_SUCCESS; > +} > + > +/** > + Handle the PVSCSI device response: > + - Copy returned data from DMA communication buffer. > + - Update fields in Extended SCSI Pass Thru Protocol packet as required. > + - Translate response code to EFI status code and host adapter status. > +**/ > +STATIC > +EFI_STATUS > +HandleResponse ( > + IN PVSCSI_DEV *Dev, > + IN OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet, > + IN CONST PVSCSI_RING_CMP_DESC *Response > + ) > +{ > + // > + // Check if device returned sense data > + // > + if (Response->ScsiStatus == EFI_EXT_SCSI_STATUS_TARGET_CHECK_CONDITION) { > + // > + // Fix SenseDataLength to amount of data returned > + // > + if (Packet->SenseDataLength > Response->SenseLen) { > + Packet->SenseDataLength = (UINT8)Response->SenseLen; > + } > + // > + // Copy sense data from DMA communication buffer > + // > + CopyMem ( > + Packet->SenseData, > + Dev->DmaBuf->SenseData, > + Packet->SenseDataLength > + ); > + } else { > + // > + // Signal no sense data returned > + // > + Packet->SenseDataLength = 0; > + } > + > + // > + // Copy device output from DMA communication buffer > + // > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) { > + CopyMem (Packet->InDataBuffer, Dev->DmaBuf->Data, Packet->InTransferLength); > + } I'm unfamilar with the PVSCSI device model, but I think this is not general enough. The "PVSCSI_RING_CMP_DESC.DataLen" field suggests that short reads are possible at least in theory. (5) If a short read occurs (Response->DataLen < Packet->InTransferLength), then we should adjust "Packet->InTransferLength", and also copy that many bytes only. (6) I think it would be prudent to update "Packet->OutTransferLength" too, for short writes. > + > + // > + // Report target status > + // > + Packet->TargetStatus = Response->ScsiStatus; > + > + // > + // Host adapter status and function return value depend on > + // device response's host status > + // > + switch (Response->HostStatus) { > + case PvScsiBtStatSuccess: > + case PvScsiBtStatLinkedCommandCompleted: > + case PvScsiBtStatLinkedCommandCompletedWithFlag: > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OK; > + return EFI_SUCCESS; > + > + case PvScsiBtStatSelTimeout: > + Packet->HostAdapterStatus = > + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_SELECTION_TIMEOUT; > + return EFI_TIMEOUT; > + > + case PvScsiBtStatDatarun: > + case PvScsiBtStatDataUnderrun: > + // > + // Report residual data in overrun/underrun > + // > + if (Packet->DataDirection == EFI_EXT_SCSI_DATA_DIRECTION_READ) { > + Packet->InTransferLength = Response->DataLen; > + } else { > + Packet->OutTransferLength = Response->DataLen; > + } OK, if we are sure that (a) the device will always report short reads/writes like this, and that (b) the above assignments will never cause InTransferLength / OutTransferLength to *grow*, then the InTransferLength / OutTransferLength adjustments are sufficiently covered. Still: (7) Please cast DataLen to UINT32, otherwise Visual Studio might whine. (8) The CopyMem() call above should not copy garbage (at the tail). Honestly, *if* the PVSCSI device model always sets "Response->DataLen", then I would prefer if: - we always updated InTransferLength / OutTransferLength (regardless of "Response->HostStatus"), - and we only used these case labels (PvScsiBtStatDatarun / PvScsiBtStatDataUnderrun) for setting "Packet->HostAdapterStatus". > + Packet->HostAdapterStatus = > + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_DATA_OVERRUN_UNDERRUN; > + return EFI_BAD_BUFFER_SIZE; I think EFI_BAD_BUFFER_SIZE is invalid here. According to the UEFI spec, EFI_BAD_BUFFER_SIZE means "The SCSI Request Packet was not executed". But that's not the case here -- we do have a partially completed transfer. (9) Thus I feel we should use a "break" here. > + > + case PvScsiBtStatBusFree: > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_FREE; > + break; > + > + case PvScsiBtStatInvPhase: > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PHASE_ERROR; > + break; > + > + case PvScsiBtStatSensFailed: > + Packet->HostAdapterStatus = > + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_REQUEST_SENSE_FAILED; > + break; > + > + case PvScsiBtStatTagReject: > + case PvScsiBtStatBadMsg: > + Packet->HostAdapterStatus = > + EFI_EXT_SCSI_STATUS_HOST_ADAPTER_MESSAGE_REJECT; > + break; > + > + case PvScsiBtStatBusReset: > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_BUS_RESET; > + break; > + > + case PvScsiBtStatHaTimeout: > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_TIMEOUT; > + return EFI_TIMEOUT; > + > + case PvScsiBtStatScsiParity: > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_PARITY_ERROR; > + break; > + > + default: > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > + break; > + } > + > + return EFI_DEVICE_ERROR; > +} > + > /** > Check if Target argument to EXT_SCSI_PASS_THRU.GetNextTarget() and > EXT_SCSI_PASS_THRU.GetNextTargetLun() is initialized > @@ -129,6 +496,23 @@ IsTargetInitialized ( > return FALSE; > } > > +/** > + Create a fake host adapter error > +**/ > +STATIC > +EFI_STATUS > +ReportHostAdapterError ( > + OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet > + ) > +{ > + 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_GOOD; > + return EFI_DEVICE_ERROR; > +} > + > // > // Ext SCSI Pass Thru > // > @@ -144,7 +528,62 @@ PvScsiPassThru ( > IN EFI_EVENT Event OPTIONAL > ) > { > - return EFI_UNSUPPORTED; > + PVSCSI_DEV *Dev; > + EFI_STATUS Status; > + PVSCSI_RING_REQ_DESC *Request; > + PVSCSI_RING_CMP_DESC *Response; > + > + Dev = PVSCSI_FROM_PASS_THRU (This); > + > + if (PvScsiIsReqRingFull (Dev)) { > + return EFI_NOT_READY; > + } > + > + Request = PvScsiGetCurrentRequest (Dev); > + > + Status = PopulateRequest (Dev, Target, Lun, Packet, Request); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // Writes to Request must be globally visible before making request > + // available to device > + // > + MemoryFence (); > + Dev->RingDesc.RingState->ReqProdIdx++; > + (10) Please insert another MemoryFence () here. > + Status = PvScsiMmioWrite32 (Dev, PvScsiRegOffsetKickRwIo, 0); > + if (EFI_ERROR (Status)) { > + // > + // If kicking the host fails, we must fake a host adapter error. > + // EFI_NOT_READY would save us the effort, but it would also suggest that > + // the caller retry. > + // > + return ReportHostAdapterError (Packet); > + } > + > + Status = PvScsiWaitForRequestCompletion (Dev); > + if (EFI_ERROR (Status)) { > + // > + // If waiting for request completion fails, we must fake a host adapter > + // error. EFI_NOT_READY would save us the effort, but it would also suggest > + // that the caller retry. > + // > + return ReportHostAdapterError (Packet); > + } > + (11) Please insert a MemoryFence() here. > + Response = PvScsiGetCurrentResponse (Dev); > + Status = HandleResponse (Dev, Packet, Response); > + > + // > + // Reads from response must complete before releasing completion entry > + // to device > + // > + MemoryFence (); > + Dev->RingDesc.RingState->CmpConsIdx++; > + > + return Status; > } > > STATIC > @@ -685,6 +1124,7 @@ PvScsiInit ( > // > Dev->MaxTarget = PcdGet8 (PcdPvScsiMaxTargetLimit); > Dev->MaxLun = PcdGet8 (PcdPvScsiMaxLunLimit); > + Dev->WaitForCmpStallInUsecs = PcdGet32 (PcdPvScsiWaitForCmpStallInUsecs); > > // > // Set PCI Attributes > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h > index 7f91d70fec79..08e876b75930 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.h > +++ b/OvmfPkg/PvScsiDxe/PvScsi.h > @@ -47,6 +47,7 @@ typedef struct { > PVSCSI_DMA_DESC DmaBufDmaDesc; > UINT8 MaxTarget; > UINT8 MaxLun; > + UINTN WaitForCmpStallInUsecs; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; > } PVSCSI_DEV; > diff --git a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf > index fcffc90d46c8..ae6d8753623f 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsiDxe.inf > +++ b/OvmfPkg/PvScsiDxe/PvScsiDxe.inf > @@ -38,5 +38,6 @@ > gEfiPciIoProtocolGuid ## TO_START > > [Pcd] > - gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit ## CONSUMES > - gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit ## CONSUMES > + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxLunLimit ## CONSUMES > + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiMaxTargetLimit ## CONSUMES > + gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs ## CONSUMES > Thanks, Laszlo