From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9299421EB5283 for ; Thu, 31 Aug 2017 06:20:20 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6DC5F65148; Thu, 31 Aug 2017 13:23:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6DC5F65148 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-89.phx2.redhat.com [10.3.116.89]) by smtp.corp.redhat.com (Postfix) with ESMTP id CECD491D19; Thu, 31 Aug 2017 13:23:01 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1504125903-29816-1-git-send-email-brijesh.singh@amd.com> <1504125903-29816-4-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <3e3e8174-616d-8f34-7682-02f6e492fb24@redhat.com> Date: Thu, 31 Aug 2017 15:23:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1504125903-29816-4-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 31 Aug 2017 13:23:03 +0000 (UTC) Subject: Re: [PATCH v2 3/4] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 Aug 2017 13:20:20 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (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 > Cc: Jordan Justen > Cc: Tom Lendacky > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > 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