* [PATCH v2 0/4] OvmfPkg/VirtioScsiDxe: map host address to device address @ 2017-08-30 20:44 Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 1/4] OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap() Brijesh Singh ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Brijesh Singh @ 2017-08-30 20:44 UTC (permalink / raw) To: edk2-devel Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky, Laszlo Ersek The patch updates VirtioScsiDxe to use IOMMU-like member functions to map the system physical address to device address for buffers (including vring, device specific request and response pointed by vring descriptor, and any furter memory reference by those request and response). Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Laszlo Ersek <lersek@redhat.com> Repo: https://github.com/codomania/edk2 Branch: virtio-scsi-2 Changes since v1: * changes to address v1 feedback Brijesh Singh (4): OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap() OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM OvmfPkg/VirtioScsiDxe/VirtioScsi.h | 1 + OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 301 +++++++++++++++++--- 2 files changed, 269 insertions(+), 33 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/4] OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap() 2017-08-30 20:44 [PATCH v2 0/4] OvmfPkg/VirtioScsiDxe: map host address to device address Brijesh Singh @ 2017-08-30 20:45 ` Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error Brijesh Singh ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Brijesh Singh @ 2017-08-30 20:45 UTC (permalink / raw) To: edk2-devel Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky, Laszlo Ersek When device is behind the IOMMU then driver need to pass the device address when programing the bus master. The patch uses VirtioRingMap() to map the VRING system physical address to device address. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Reviewed-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/VirtioScsiDxe/VirtioScsi.h | 1 + OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 47 +++++++++++++++----- 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h index 6d00567e8cb8..05a6bf567263 100644 --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h @@ -60,6 +60,7 @@ typedef struct { VRING Ring; // VirtioRingInit 2 EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; // VirtioScsiInit 1 EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; // VirtioScsiInit 1 + VOID *RingMap; // VirtioRingMap 2 } VSCSI_DEV; #define VIRTIO_SCSI_FROM_PASS_THRU(PassThruPointer) \ diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c index a983b3df7b9c..5e72b1a24b59 100644 --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c @@ -707,7 +707,7 @@ VirtioScsiInit ( { UINT8 NextDevStat; EFI_STATUS Status; - + UINT64 RingBaseShift; UINT64 Features; UINT16 MaxChannel; // for validation only UINT32 NumQueues; // for validation only @@ -839,25 +839,42 @@ VirtioScsiInit ( } // + // If anything fails from here on, we must release the ring resources + // + Status = VirtioRingMap ( + Dev->VirtIo, + &Dev->Ring, + &RingBaseShift, + &Dev->RingMap + ); + if (EFI_ERROR (Status)) { + goto ReleaseQueue; + } + + // // Additional steps for MMIO: align the queue appropriately, and set the - // size. If anything fails from here on, we must release the ring resources. + // size. If anything fails from here on, we must unmap the ring resources. // Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } // // step 4c -- Report GPFN (guest-physical frame number) of queue. // - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0); + Status = Dev->VirtIo->SetQueueAddress ( + Dev->VirtIo, + &Dev->Ring, + RingBaseShift + ); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } // @@ -867,7 +884,7 @@ VirtioScsiInit ( Features &= ~(UINT64)VIRTIO_F_VERSION_1; Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } } @@ -877,11 +894,11 @@ VirtioScsiInit ( // Status = VIRTIO_CFG_WRITE (Dev, CdbSize, VIRTIO_SCSI_CDB_SIZE); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } // @@ -890,7 +907,7 @@ VirtioScsiInit ( NextDevStat |= VSTAT_DRIVER_OK; Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } // @@ -926,6 +943,9 @@ VirtioScsiInit ( return EFI_SUCCESS; +UnmapQueue: + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); + ReleaseQueue: VirtioRingUninit (Dev->VirtIo, &Dev->Ring); @@ -965,6 +985,7 @@ VirtioScsiUninit ( Dev->MaxLun = 0; Dev->MaxSectors = 0; + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); VirtioRingUninit (Dev->VirtIo, &Dev->Ring); SetMem (&Dev->PassThru, sizeof Dev->PassThru, 0x00); @@ -995,6 +1016,12 @@ VirtioScsiExitBoot ( // Dev = Context; Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); + + // + // Unmap the ring buffer so that hypervisor will not be able to get + // readable data after device reset. + // + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); } -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error 2017-08-30 20:44 [PATCH v2 0/4] OvmfPkg/VirtioScsiDxe: map host address to device address Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 1/4] OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap() Brijesh Singh @ 2017-08-30 20:45 ` Brijesh Singh 2017-08-31 11:19 ` Laszlo Ersek 2017-08-30 20:45 ` [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 4/4] OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh 3 siblings, 1 reply; 9+ messages in thread From: Brijesh Singh @ 2017-08-30 20:45 UTC (permalink / raw) To: edk2-devel Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky, Laszlo Ersek When virtio request fails we return EFI_DEVICE_ERROR, as per the spec EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() member function is required to implement elaborated error reporting. The patch refactors out entire block of the code that creates the host adapter error into a separate helper function (ReportHostAdapterError). Suggested-by: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 27 +++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c index 5e72b1a24b59..cac213129409 100644 --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c @@ -387,6 +387,26 @@ ParseResponse ( return EFI_DEVICE_ERROR; } +/** + * The function can be used to create a fake host adapter error. + * When VirtioScsiPassThru is failed due to some reasons then this function + * can be called to contstruct a host adapter error. + * + */ +STATIC +EFI_STATUS +ReportHostAdapterError ( + OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet + ) +{ + Packet->InTransferLength = 0; + Packet->OutTransferLength = 0; + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; + Packet->SenseDataLength = 0; + return EFI_DEVICE_ERROR; +} + // // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL @@ -472,12 +492,7 @@ VirtioScsiPassThru ( // if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, &Indices, NULL) != EFI_SUCCESS) { - Packet->InTransferLength = 0; - Packet->OutTransferLength = 0; - Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; - Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; - Packet->SenseDataLength = 0; - return EFI_DEVICE_ERROR; + return ReportHostAdapterError (Packet); } return ParseResponse (Packet, &Response); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error 2017-08-30 20:45 ` [PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error Brijesh Singh @ 2017-08-31 11:19 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-08-31 11:19 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel On 08/30/17 22:45, Brijesh Singh wrote: > When virtio request fails we return EFI_DEVICE_ERROR, as per the spec > EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() member function is required > to implement elaborated error reporting. > > The patch refactors out entire block of the code that creates the host > adapter error into a separate helper function (ReportHostAdapterError). > > Suggested-by: Laszlo Ersek <lersek@redhat.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 27 +++++++++++++++----- > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > index 5e72b1a24b59..cac213129409 100644 > --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > @@ -387,6 +387,26 @@ ParseResponse ( > return EFI_DEVICE_ERROR; > } OK, I'm glad that you found the best place to introduce the new function (just below ParseResponse()). > +/** > + * The function can be used to create a fake host adapter error. > + * When VirtioScsiPassThru is failed due to some reasons then this function > + * can be called to contstruct a host adapter error. > + * > + */ (1) This comment block should be formatted according to the edk2 coding style, and it should document the Packet parameter, and the return value: ------- /** The function can be used to create a fake host adapter error. When VirtioScsiPassThru() is failed due to some reasons then this function can be called to construct a host adapter error. @param[out] Packet The Extended SCSI Pass Thru Protocol packet that the host adapter error shall be placed in. @retval EFI_DEVICE_ERROR The function returns this status code unconditionally, to be propagated by VirtioScsiPassThru(). **/ ------- There's no need to resubmit just because of this; if more serious updates are not needed, I can take care of it. (2) Another documentation update: pls see the following comment block (higher up in the source code): // UEFI Spec 2.3.1 + Errata C, 14.7 Extended SCSI Pass Thru Protocol specifies // the PassThru() interface. Beside returning a status code, the function must // set some fields in the EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET in/out // parameter on return. The following is a full list of those fields, for // easier validation of PopulateRequest(), ParseResponse(), and // VirtioScsiPassThru() below. After this patch, VirtioScsiPassThru() will no longer massage Packet->xxx fields directly, only via helper functions. So please replace the "VirtioScsiPassThru()" reference in the comment block with "ReportHostAdapterError()". I can take care of this as well, if v3 is not necessary otherwise. With these documentation updates, Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks, Laszlo > +STATIC > +EFI_STATUS > +ReportHostAdapterError ( > + OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet > + ) > +{ > + Packet->InTransferLength = 0; > + Packet->OutTransferLength = 0; > + Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > + Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > + Packet->SenseDataLength = 0; > + return EFI_DEVICE_ERROR; > +} > + > > // > // The next seven functions implement EFI_EXT_SCSI_PASS_THRU_PROTOCOL > @@ -472,12 +492,7 @@ VirtioScsiPassThru ( > // > if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, > &Indices, NULL) != EFI_SUCCESS) { > - Packet->InTransferLength = 0; > - Packet->OutTransferLength = 0; > - Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > - Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > - Packet->SenseDataLength = 0; > - return EFI_DEVICE_ERROR; > + return ReportHostAdapterError (Packet); > } > > return ParseResponse (Packet, &Response); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers 2017-08-30 20:44 [PATCH v2 0/4] OvmfPkg/VirtioScsiDxe: map host address to device address Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 1/4] OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap() Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error Brijesh Singh @ 2017-08-30 20:45 ` Brijesh Singh 2017-08-31 13:23 ` Laszlo Ersek 2017-08-30 20:45 ` [PATCH v2 4/4] OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh 3 siblings, 1 reply; 9+ messages in thread From: Brijesh Singh @ 2017-08-30 20:45 UTC (permalink / raw) To: edk2-devel Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky, Laszlo Ersek When device is behind the IOMMU, driver is require to pass the device address of virtio request, response and any memory referenced by those request/response to the bus master. The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to map request and response buffers system physical address to the device address. - If the buffer need to be accessed by both the processor and a bus master then map with BusMasterCommonBuffer. - If the buffer need to be accessed for a write operation by a bus master then map with BusMasterWrite. - If the buffer need to be accessed for a read operation by a bus master then map with BusMasterRead. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 225 ++++++++++++++++++-- 1 file changed, 209 insertions(+), 16 deletions(-) diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c index cac213129409..3e04097ddd11 100644 --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c @@ -429,26 +429,161 @@ VirtioScsiPassThru ( UINT16 TargetValue; EFI_STATUS Status; volatile VIRTIO_SCSI_REQ Request; - volatile VIRTIO_SCSI_RESP Response; + volatile VIRTIO_SCSI_RESP *Response; + VOID *ResponseBuffer; DESC_INDICES Indices; + VOID *RequestMapping; + VOID *ResponseMapping; + VOID *InDataMapping; + VOID *OutDataMapping; + EFI_PHYSICAL_ADDRESS RequestDeviceAddress; + EFI_PHYSICAL_ADDRESS ResponseDeviceAddress; + EFI_PHYSICAL_ADDRESS InDataDeviceAddress; + EFI_PHYSICAL_ADDRESS OutDataDeviceAddress; + VOID *InDataBuffer; + UINTN InDataNumPages; + BOOLEAN InDataBufferIsMapped; + BOOLEAN OutDataBufferIsMapped; ZeroMem ((VOID*) &Request, sizeof (Request)); - ZeroMem ((VOID*) &Response, sizeof (Response)); Dev = VIRTIO_SCSI_FROM_PASS_THRU (This); CopyMem (&TargetValue, Target, sizeof TargetValue); + InDataBuffer = NULL; + InDataBufferIsMapped = FALSE; + OutDataBufferIsMapped = FALSE; + InDataNumPages = 0; + Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request); if (EFI_ERROR (Status)) { return Status; } - VirtioPrepare (&Dev->Ring, &Indices); + // + // Map the virtio-scsi Request header buffer + // + Status = VirtioMapAllBytesInSharedBuffer ( + Dev->VirtIo, + VirtioOperationBusMasterRead, + (VOID *) &Request, + sizeof Request, + &RequestDeviceAddress, + &RequestMapping); + if (EFI_ERROR (Status)) { + return ReportHostAdapterError (Packet); + } + + // + // Map the input buffer + // + if (Packet->InTransferLength > 0) { + // + // Allocate a intermediate input buffer. This is mainly to handle the + // following case: + // * caller submits a bi-directional request + // * we perform the request fine + // * but we fail to unmap the "InDataMapping" + // + // In that case simply returing the EFI_DEVICE_ERROR is not sufficient. + // In addition to the error code we also need to update Packet-xxx fields + // accordingly so that we report the full loss of the incoming transfer. + // + // We allocate a temporary buffer and map it with BusMasterCommon. If the + // Virtio request is successful then we copy the data from temporary + // buffer into Packet->InDataBuffer. + // + InDataNumPages = EFI_SIZE_TO_PAGES (Packet->InTransferLength); + Status = Dev->VirtIo->AllocateSharedPages ( + Dev->VirtIo, + InDataNumPages, + &InDataBuffer + ); + if (EFI_ERROR (Status)) { + Status = ReportHostAdapterError (Packet); + goto UnmapRequestBuffer; + } + + ZeroMem (InDataBuffer, Packet->InTransferLength); + + Status = VirtioMapAllBytesInSharedBuffer ( + Dev->VirtIo, + VirtioOperationBusMasterCommonBuffer, + InDataBuffer, + Packet->InTransferLength, + &InDataDeviceAddress, + &InDataMapping + ); + if (EFI_ERROR (Status)) { + Status = ReportHostAdapterError (Packet); + goto FreeInDataBuffer; + } + + InDataBufferIsMapped = TRUE; + } + + // + // Map the output buffer + // + if (Packet->OutTransferLength > 0) { + Status = VirtioMapAllBytesInSharedBuffer ( + Dev->VirtIo, + VirtioOperationBusMasterRead, + Packet->OutDataBuffer, + Packet->OutTransferLength, + &OutDataDeviceAddress, + &OutDataMapping + ); + if (EFI_ERROR (Status)) { + Status = ReportHostAdapterError (Packet); + goto UnmapInDataBuffer; + } + + OutDataBufferIsMapped = TRUE; + } + + // + // Response header is bi-direction (we preset with host status and expect + // the device to update it). Allocate a response buffer which can be mapped + // to access equally by both processor and device. + // + Status = Dev->VirtIo->AllocateSharedPages ( + Dev->VirtIo, + EFI_SIZE_TO_PAGES (sizeof *Response), + &ResponseBuffer + ); + if (EFI_ERROR (Status)) { + Status = ReportHostAdapterError (Packet); + goto UnmapOutDataBuffer; + } + + Response = ResponseBuffer; + + ZeroMem ((VOID *)Response, sizeof (*Response)); // // preset a host status for ourselves that we do not accept as success // - Response.Response = VIRTIO_SCSI_S_FAILURE; + Response->Response = VIRTIO_SCSI_S_FAILURE; + + // + // Map the response buffer with BusMasterCommonBuffer so that response + // buffer can be accessed by both host and device. + // + Status = VirtioMapAllBytesInSharedBuffer ( + Dev->VirtIo, + VirtioOperationBusMasterCommonBuffer, + ResponseBuffer, + sizeof (*Response), + &ResponseDeviceAddress, + &ResponseMapping + ); + if (EFI_ERROR (Status)) { + Status = ReportHostAdapterError (Packet); + goto FreeResponseBuffer; + } + + VirtioPrepare (&Dev->Ring, &Indices); // // ensured by VirtioScsiInit() -- this predicate, in combination with the @@ -459,31 +594,49 @@ VirtioScsiPassThru ( // // enqueue Request // - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, - VRING_DESC_F_NEXT, &Indices); + VirtioAppendDesc ( + &Dev->Ring, + RequestDeviceAddress, + sizeof Request, + VRING_DESC_F_NEXT, + &Indices + ); // // enqueue "dataout" if any // if (Packet->OutTransferLength > 0) { - VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer, - Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices); + VirtioAppendDesc ( + &Dev->Ring, + OutDataDeviceAddress, + Packet->OutTransferLength, + VRING_DESC_F_NEXT, + &Indices + ); } // // enqueue Response, to be written by the host // - VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response, - VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? - VRING_DESC_F_NEXT : 0), - &Indices); + VirtioAppendDesc ( + &Dev->Ring, + ResponseDeviceAddress, + sizeof *Response, + VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0), + &Indices + ); // // enqueue "datain" if any, to be written by the host // if (Packet->InTransferLength > 0) { - VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer, - Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices); + VirtioAppendDesc ( + &Dev->Ring, + InDataDeviceAddress, + Packet->InTransferLength, + VRING_DESC_F_WRITE, + &Indices + ); } // If kicking the host fails, we must fake a host adapter error. @@ -492,10 +645,50 @@ VirtioScsiPassThru ( // if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, &Indices, NULL) != EFI_SUCCESS) { - return ReportHostAdapterError (Packet); + Status = ReportHostAdapterError (Packet); + goto UnmapResponseBuffer; } - return ParseResponse (Packet, &Response); + Status = ParseResponse (Packet, Response); + + // + // If virtio request was successful and it was a CPU read request then we + // have used an intermediate buffer. Copy the data from intermediate buffer + // to the final buffer. + // + if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) { + CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength); + } + +UnmapResponseBuffer: + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping); + +FreeResponseBuffer: + Dev->VirtIo->FreeSharedPages ( + Dev->VirtIo, + EFI_SIZE_TO_PAGES (sizeof *Response), + ResponseBuffer + ); + +UnmapOutDataBuffer: + if (OutDataBufferIsMapped == TRUE) { + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping); + } + +UnmapInDataBuffer: + if (InDataBufferIsMapped == TRUE) { + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping); + } + +FreeInDataBuffer: + if (InDataBuffer != NULL) { + Dev->VirtIo->FreeSharedPages (Dev->VirtIo, InDataNumPages, InDataBuffer); + } + +UnmapRequestBuffer: + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); + + return Status; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers 2017-08-30 20:45 ` [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers Brijesh Singh @ 2017-08-31 13:23 ` Laszlo Ersek 2017-08-31 13:49 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2017-08-31 13:23 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel (1) should have noticed it in my previous review: the subject should be changed like this ("pkg" -> "Pkg"): -Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers +OvmfPkg/VirtioScsiDxe: map virtio-scsi request and response buffers No need to repost just because of this. On 08/30/17 22:45, Brijesh Singh wrote: > When device is behind the IOMMU, driver is require to pass the device > address of virtio request, response and any memory referenced by those > request/response to the bus master. > > The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to > map request and response buffers system physical address to the device > address. > > - If the buffer need to be accessed by both the processor and a bus > master then map with BusMasterCommonBuffer. > > - If the buffer need to be accessed for a write operation by a bus master > then map with BusMasterWrite. (2) We no longer use BusMasterWrite in this patch -- as I requested, so that's good --, so I suggest that we continue this paragraph as follows: "However, after a BusMasterWrite Unmap() failure, error reporting via EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET would be very complex, therefore we map such buffers too with BusMasterCommonBuffer." I can update the commit message. > > - If the buffer need to be accessed for a read operation by a bus master > then map with BusMasterRead. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 225 ++++++++++++++++++-- > 1 file changed, 209 insertions(+), 16 deletions(-) > > diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > index cac213129409..3e04097ddd11 100644 > --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > @@ -429,26 +429,161 @@ VirtioScsiPassThru ( > UINT16 TargetValue; > EFI_STATUS Status; > volatile VIRTIO_SCSI_REQ Request; > - volatile VIRTIO_SCSI_RESP Response; > + volatile VIRTIO_SCSI_RESP *Response; > + VOID *ResponseBuffer; > DESC_INDICES Indices; > + VOID *RequestMapping; > + VOID *ResponseMapping; > + VOID *InDataMapping; > + VOID *OutDataMapping; > + EFI_PHYSICAL_ADDRESS RequestDeviceAddress; > + EFI_PHYSICAL_ADDRESS ResponseDeviceAddress; > + EFI_PHYSICAL_ADDRESS InDataDeviceAddress; > + EFI_PHYSICAL_ADDRESS OutDataDeviceAddress; > + VOID *InDataBuffer; > + UINTN InDataNumPages; > + BOOLEAN InDataBufferIsMapped; > + BOOLEAN OutDataBufferIsMapped; > > ZeroMem ((VOID*) &Request, sizeof (Request)); > - ZeroMem ((VOID*) &Response, sizeof (Response)); > > Dev = VIRTIO_SCSI_FROM_PASS_THRU (This); > CopyMem (&TargetValue, Target, sizeof TargetValue); > > + InDataBuffer = NULL; > + InDataBufferIsMapped = FALSE; > + OutDataBufferIsMapped = FALSE; > + InDataNumPages = 0; > + > Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request); > if (EFI_ERROR (Status)) { > return Status; > } > > - VirtioPrepare (&Dev->Ring, &Indices); > + // > + // Map the virtio-scsi Request header buffer > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + (VOID *) &Request, > + sizeof Request, > + &RequestDeviceAddress, > + &RequestMapping); > + if (EFI_ERROR (Status)) { > + return ReportHostAdapterError (Packet); > + } > + > + // > + // Map the input buffer > + // > + if (Packet->InTransferLength > 0) { > + // > + // Allocate a intermediate input buffer. This is mainly to handle the > + // following case: > + // * caller submits a bi-directional request > + // * we perform the request fine > + // * but we fail to unmap the "InDataMapping" > + // > + // In that case simply returing the EFI_DEVICE_ERROR is not sufficient. > + // In addition to the error code we also need to update Packet-xxx fields (3) Please replace "Packet-xxx" with just "Packet" > + // accordingly so that we report the full loss of the incoming transfer. > + // > + // We allocate a temporary buffer and map it with BusMasterCommon. If the (4) s/BusMasterCommon/BusMasterCommonBuffer/ Please don't forget to rewrap the text to 79 or 80 columns. > + // Virtio request is successful then we copy the data from temporary > + // buffer into Packet->InDataBuffer. > + // (5) The last two paragraphs in the comment should be un-indented one column. > + InDataNumPages = EFI_SIZE_TO_PAGES (Packet->InTransferLength); (6) "Packet->InTransferLength" has type (UINT32), but EFI_SIZE_TO_PAGES() takes UINTN. Please cast the argument to (UINTN). > + Status = Dev->VirtIo->AllocateSharedPages ( > + Dev->VirtIo, > + InDataNumPages, > + &InDataBuffer > + ); > + if (EFI_ERROR (Status)) { > + Status = ReportHostAdapterError (Packet); > + goto UnmapRequestBuffer; > + } > + > + ZeroMem (InDataBuffer, Packet->InTransferLength); > + > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + InDataBuffer, > + Packet->InTransferLength, > + &InDataDeviceAddress, > + &InDataMapping > + ); > + if (EFI_ERROR (Status)) { > + Status = ReportHostAdapterError (Packet); > + goto FreeInDataBuffer; > + } > + > + InDataBufferIsMapped = TRUE; > + } > + > + // > + // Map the output buffer > + // > + if (Packet->OutTransferLength > 0) { > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + Packet->OutDataBuffer, > + Packet->OutTransferLength, > + &OutDataDeviceAddress, > + &OutDataMapping > + ); > + if (EFI_ERROR (Status)) { > + Status = ReportHostAdapterError (Packet); > + goto UnmapInDataBuffer; > + } > + > + OutDataBufferIsMapped = TRUE; > + } > + > + // > + // Response header is bi-direction (we preset with host status and expect > + // the device to update it). Allocate a response buffer which can be mapped > + // to access equally by both processor and device. > + // > + Status = Dev->VirtIo->AllocateSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *Response), > + &ResponseBuffer > + ); > + if (EFI_ERROR (Status)) { > + Status = ReportHostAdapterError (Packet); > + goto UnmapOutDataBuffer; > + } > + > + Response = ResponseBuffer; > + > + ZeroMem ((VOID *)Response, sizeof (*Response)); > > // > // preset a host status for ourselves that we do not accept as success > // > - Response.Response = VIRTIO_SCSI_S_FAILURE; > + Response->Response = VIRTIO_SCSI_S_FAILURE; > + > + // > + // Map the response buffer with BusMasterCommonBuffer so that response > + // buffer can be accessed by both host and device. > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + ResponseBuffer, > + sizeof (*Response), > + &ResponseDeviceAddress, > + &ResponseMapping > + ); > + if (EFI_ERROR (Status)) { > + Status = ReportHostAdapterError (Packet); > + goto FreeResponseBuffer; > + } > + > + VirtioPrepare (&Dev->Ring, &Indices); > > // > // ensured by VirtioScsiInit() -- this predicate, in combination with the > @@ -459,31 +594,49 @@ VirtioScsiPassThru ( > // > // enqueue Request > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, > - VRING_DESC_F_NEXT, &Indices); > + VirtioAppendDesc ( > + &Dev->Ring, > + RequestDeviceAddress, > + sizeof Request, > + VRING_DESC_F_NEXT, > + &Indices > + ); > > // > // enqueue "dataout" if any > // > if (Packet->OutTransferLength > 0) { > - VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer, > - Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices); > + VirtioAppendDesc ( > + &Dev->Ring, > + OutDataDeviceAddress, > + Packet->OutTransferLength, > + VRING_DESC_F_NEXT, > + &Indices > + ); > } > > // > // enqueue Response, to be written by the host > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response, > - VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? > - VRING_DESC_F_NEXT : 0), > - &Indices); > + VirtioAppendDesc ( > + &Dev->Ring, > + ResponseDeviceAddress, > + sizeof *Response, > + VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0), > + &Indices > + ); > > // > // enqueue "datain" if any, to be written by the host > // > if (Packet->InTransferLength > 0) { > - VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer, > - Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices); > + VirtioAppendDesc ( > + &Dev->Ring, > + InDataDeviceAddress, > + Packet->InTransferLength, > + VRING_DESC_F_WRITE, > + &Indices > + ); > } > > // If kicking the host fails, we must fake a host adapter error. > @@ -492,10 +645,50 @@ VirtioScsiPassThru ( > // > if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, > &Indices, NULL) != EFI_SUCCESS) { > - return ReportHostAdapterError (Packet); > + Status = ReportHostAdapterError (Packet); > + goto UnmapResponseBuffer; > } > > - return ParseResponse (Packet, &Response); > + Status = ParseResponse (Packet, Response); > + > + // > + // If virtio request was successful and it was a CPU read request then we > + // have used an intermediate buffer. Copy the data from intermediate buffer > + // to the final buffer. > + // > + if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) { > + CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength); > + } (7) The comment is exactly right, but the condition that you check after is incorrect. The right thing to do is to call CopyMem() *unconditionally*. Namely, at this point we are past ParseResponse(). As I wrote before, ParseResponse() updates the Packet->... fields in every case, even if it reports an EFI_STATUS that is different from EFI_SUCCESS. And whatever we expose to the caller through "Packet->InTransferLength" *must* be reflected in "Packet->InDataBuffer" regardless of return status. Therefore the Status check must be dropped. And then we need not check (Packet->InTransferLength>0) either, because the CopyMem() will deal with it internally. Think of it like this: the "worst" that can happen, on error, is that "Packet->InTransferLength" is unchanged from its "input" value, and we overwrite the caller's "Packet->InDataBuffer" entirely. What is the data we are going to put there? It's all zeroes, from your ZeroMem (InDataBuffer, Packet->InTransferLength); higher up. So, again, this CopyMem() needs to be unconditional -- as the comment says, if the *virtio* request was successful (== we talked to the virtio-scsi adapter), then we have to copy the data, even if the *SCSI* request produced an error status in ParseResponse. > + > +UnmapResponseBuffer: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping); > + > +FreeResponseBuffer: > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *Response), > + ResponseBuffer > + ); > + > +UnmapOutDataBuffer: > + if (OutDataBufferIsMapped == TRUE) { (8) Just say "if (OutDataBufferIsMapped)" > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping); > + } > + > +UnmapInDataBuffer: > + if (InDataBufferIsMapped == TRUE) { (9) The "InDataBufferIsMapped" variable can be eliminated safely. Instead, just say if (InDataBuffer != NULL) Because, if you reach this error label, then InDataBuffer!=NULL guarantees that you have mapped it as well, and InDataBuffer==NULL guarantees that you have not mapped it either. Put differently, if InDataBuffer!=NULL, then we must have attempted to map it (because Packet->InTransferLength was positive on input). If we succeeded in mapping it, then we have to unmap it. If we failed to map it, then we're not here! Because we jumped to FreeInDataBuffer, just below. > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping); > + } > + > +FreeInDataBuffer: > + if (InDataBuffer != NULL) { > + Dev->VirtIo->FreeSharedPages (Dev->VirtIo, InDataNumPages, InDataBuffer); > + } > + > +UnmapRequestBuffer: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); > + > + return Status; > } > > > Otherwise, the logic is very good in this patch; all the updates I'm requesting are localized tweaks, and the only control flow change is point (7) -- which is also very tight. For the updates above, I must ask you for v3, but I trust that v3 is going to be the final version. Thank you! Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers 2017-08-31 13:23 ` Laszlo Ersek @ 2017-08-31 13:49 ` Laszlo Ersek 2017-08-31 14:44 ` Brijesh Singh 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2017-08-31 13:49 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel On 08/31/17 15:23, Laszlo Ersek wrote: > On 08/30/17 22:45, Brijesh Singh wrote: >> @@ -492,10 +645,50 @@ VirtioScsiPassThru ( >> // >> if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, >> &Indices, NULL) != EFI_SUCCESS) { >> - return ReportHostAdapterError (Packet); >> + Status = ReportHostAdapterError (Packet); >> + goto UnmapResponseBuffer; >> } >> >> - return ParseResponse (Packet, &Response); >> + Status = ParseResponse (Packet, Response); >> + >> + // >> + // If virtio request was successful and it was a CPU read request then we >> + // have used an intermediate buffer. Copy the data from intermediate buffer >> + // to the final buffer. >> + // >> + if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) { >> + CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength); >> + } > > (7) The comment is exactly right, but the condition that you check > after is incorrect. > > The right thing to do is to call CopyMem() *unconditionally*. > > Namely, at this point we are past ParseResponse(). As I wrote before, > ParseResponse() updates the Packet->... fields in every case, even if > it reports an EFI_STATUS that is different from EFI_SUCCESS. And > whatever we expose to the caller through "Packet->InTransferLength" > *must* be reflected in "Packet->InDataBuffer" regardless of return > status. > > Therefore the Status check must be dropped. And then we need not check > (Packet->InTransferLength>0) either, because the CopyMem() will deal > with it internally. > > Think of it like this: the "worst" that can happen, on error, is that > "Packet->InTransferLength" is unchanged from its "input" value, and we > overwrite the caller's "Packet->InDataBuffer" entirely. What is the > data we are going to put there? It's all zeroes, from your > > ZeroMem (InDataBuffer, Packet->InTransferLength); > > higher up. > > So, again, this CopyMem() needs to be unconditional -- as the comment > says, if the *virtio* request was successful (== we talked to the > virtio-scsi adapter), then we have to copy the data, even if the > *SCSI* request produced an error status in ParseResponse. I have to correct myself a little bit -- although I think you would have caught me anyway :) --, namely we should keep the "if", but the condition should be: InDataBuffer != NULL Admittedly, it is likely that none of the CopyMem() implementations would have problems with a NULL "SourceBuffer", if "Length" was zero. Nonetheless, the interface contract in MdePkg/Include/Library/BaseMemoryLib.h does not mark SourceBuffer OPTIONAL -- neither does the UEFI spec, for the similar gBS->CopyMem() boot service --, for the case when Length==0, so we should do an explicit check: if (InDataBuffer != NULL) { CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength); } Thank you, Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers 2017-08-31 13:49 ` Laszlo Ersek @ 2017-08-31 14:44 ` Brijesh Singh 0 siblings, 0 replies; 9+ messages in thread From: Brijesh Singh @ 2017-08-31 14:44 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Jordan Justen, Tom Lendacky, Ard Biesheuvel Thanks for quick feedback Laszlo. I am addressing all your review comments and submitting the v3 soon -Brijesh On 08/31/2017 08:49 AM, Laszlo Ersek wrote: > On 08/31/17 15:23, Laszlo Ersek wrote: >> On 08/30/17 22:45, Brijesh Singh wrote: > >>> @@ -492,10 +645,50 @@ VirtioScsiPassThru ( >>> // >>> if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, >>> &Indices, NULL) != EFI_SUCCESS) { >>> - return ReportHostAdapterError (Packet); >>> + Status = ReportHostAdapterError (Packet); >>> + goto UnmapResponseBuffer; >>> } >>> >>> - return ParseResponse (Packet, &Response); >>> + Status = ParseResponse (Packet, Response); >>> + >>> + // >>> + // If virtio request was successful and it was a CPU read request then we >>> + // have used an intermediate buffer. Copy the data from intermediate buffer >>> + // to the final buffer. >>> + // >>> + if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) { >>> + CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength); >>> + } >> >> (7) The comment is exactly right, but the condition that you check >> after is incorrect. >> >> The right thing to do is to call CopyMem() *unconditionally*. >> >> Namely, at this point we are past ParseResponse(). As I wrote before, >> ParseResponse() updates the Packet->... fields in every case, even if >> it reports an EFI_STATUS that is different from EFI_SUCCESS. And >> whatever we expose to the caller through "Packet->InTransferLength" >> *must* be reflected in "Packet->InDataBuffer" regardless of return >> status. >> >> Therefore the Status check must be dropped. And then we need not check >> (Packet->InTransferLength>0) either, because the CopyMem() will deal >> with it internally. >> >> Think of it like this: the "worst" that can happen, on error, is that >> "Packet->InTransferLength" is unchanged from its "input" value, and we >> overwrite the caller's "Packet->InDataBuffer" entirely. What is the >> data we are going to put there? It's all zeroes, from your >> >> ZeroMem (InDataBuffer, Packet->InTransferLength); >> >> higher up. >> >> So, again, this CopyMem() needs to be unconditional -- as the comment >> says, if the *virtio* request was successful (== we talked to the >> virtio-scsi adapter), then we have to copy the data, even if the >> *SCSI* request produced an error status in ParseResponse. > > I have to correct myself a little bit -- although I think you would have > caught me anyway :) --, namely we should keep the "if", but the > condition should be: > > InDataBuffer != NULL > > Admittedly, it is likely that none of the CopyMem() implementations > would have problems with a NULL "SourceBuffer", if "Length" was zero. > > Nonetheless, the interface contract in > > MdePkg/Include/Library/BaseMemoryLib.h > > does not mark SourceBuffer OPTIONAL -- neither does the UEFI spec, for > the similar gBS->CopyMem() boot service --, for the case when Length==0, > so we should do an explicit check: > > if (InDataBuffer != NULL) { > CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength); > } > > Thank you, > Laszlo > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 4/4] OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM 2017-08-30 20:44 [PATCH v2 0/4] OvmfPkg/VirtioScsiDxe: map host address to device address Brijesh Singh ` (2 preceding siblings ...) 2017-08-30 20:45 ` [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers Brijesh Singh @ 2017-08-30 20:45 ` Brijesh Singh 3 siblings, 0 replies; 9+ messages in thread From: Brijesh Singh @ 2017-08-30 20:45 UTC (permalink / raw) To: edk2-devel Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky, Laszlo Ersek VirtioScsiDxe driver has been updated to use IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to translate the system physical address to device address. We do not need to do anything special when VIRTIO_F_IOMMU_PLATFORM bit is present hence treat it in parallel with VIRTIO_F_VERSION_1. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Reviewed-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c index 3e04097ddd11..4ec22ccd1abe 100644 --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c @@ -1009,7 +1009,8 @@ VirtioScsiInit ( goto Failed; } - Features &= VIRTIO_SCSI_F_INOUT | VIRTIO_F_VERSION_1; + Features &= VIRTIO_SCSI_F_INOUT | VIRTIO_F_VERSION_1 | + VIRTIO_F_IOMMU_PLATFORM; // // In virtio-1.0, feature negotiation is expected to complete before queue @@ -1089,7 +1090,7 @@ VirtioScsiInit ( // step 5 -- Report understood features and guest-tuneables. // if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) { - Features &= ~(UINT64)VIRTIO_F_VERSION_1; + Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM); Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); if (EFI_ERROR (Status)) { goto UnmapQueue; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-31 14:41 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-30 20:44 [PATCH v2 0/4] OvmfPkg/VirtioScsiDxe: map host address to device address Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 1/4] OvmfPkg/VirtioScsiDxe: map VRING using VirtioRingMap() Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 2/4] OvmfPkg/VirtioScsiDxe: add helper to create a fake host adapter error Brijesh Singh 2017-08-31 11:19 ` Laszlo Ersek 2017-08-30 20:45 ` [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers Brijesh Singh 2017-08-31 13:23 ` Laszlo Ersek 2017-08-31 13:49 ` Laszlo Ersek 2017-08-31 14:44 ` Brijesh Singh 2017-08-30 20:45 ` [PATCH v2 4/4] OvmfPkg/VirtioScsiDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox