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 3714621E11D34 for ; Sun, 27 Aug 2017 15:03:09 -0700 (PDT) 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 mx1.redhat.com (Postfix) with ESMTPS id 4EF31804E3; Sun, 27 Aug 2017 22:05:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4EF31804E3 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-92.phx2.redhat.com [10.3.116.92]) by smtp.corp.redhat.com (Postfix) with ESMTP id ABCB95D98A; Sun, 27 Aug 2017 22:05:45 +0000 (UTC) From: Laszlo Ersek To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Ard Biesheuvel , Tom Lendacky References: <1503697414-6830-1-git-send-email-brijesh.singh@amd.com> <1503697414-6830-3-git-send-email-brijesh.singh@amd.com> <61e3119c-0e51-ea05-96e6-55c4908dadd4@redhat.com> Message-ID: <1d6bade7-578e-c0e7-5aa6-2ca6af1185e6@redhat.com> Date: Mon, 28 Aug 2017 00:05:44 +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: <61e3119c-0e51-ea05-96e6-55c4908dadd4@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Sun, 27 Aug 2017 22:05:47 +0000 (UTC) Subject: Re: [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk 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: Sun, 27 Aug 2017 22:03:09 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/27/17 22:40, Laszlo Ersek wrote: > This patch is functionally correct. I'd like to request three stylistic > improvements: > > On 08/25/17 23:43, 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. >> >> - 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/VirtioBlkDxe/VirtioBlk.c | 157 ++++++++++++++++++-- >> 1 file changed, 143 insertions(+), 14 deletions(-) >> >> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> index 663ba281ab73..ab69cb08a625 100644 >> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> @@ -232,7 +232,8 @@ VerifyReadWriteRequest ( >> >> @retval EFI_DEVICE_ERROR Failed to notify host side via VirtIo write, or >> unable to parse host response, or host response >> - is not VIRTIO_BLK_S_OK. >> + is not VIRTIO_BLK_S_OK or failed to map Buffer >> + for a bus master operation. >> >> **/ >> >> @@ -249,8 +250,16 @@ SynchronousRequest ( >> { >> UINT32 BlockSize; >> volatile VIRTIO_BLK_REQ Request; >> - volatile UINT8 HostStatus; >> + volatile UINT8 *HostStatus; >> + VOID *HostStatusBuffer; >> DESC_INDICES Indices; >> + VOID *RequestMapping; >> + VOID *StatusMapping; >> + VOID *BufferMapping; >> + EFI_PHYSICAL_ADDRESS BufferDeviceAddress; >> + EFI_PHYSICAL_ADDRESS HostStatusDeviceAddress; >> + EFI_PHYSICAL_ADDRESS RequestDeviceAddress; >> + EFI_STATUS Status, Ret; > > (1) Please rename the "Ret" variable to "UnmapStatus", and define it > separately. > >> >> BlockSize = Dev->BlockIoMedia.BlockSize; >> >> @@ -278,9 +287,89 @@ SynchronousRequest ( >> VirtioPrepare (&Dev->Ring, &Indices); >> >> // >> + // Host status is bi-directional (we preset with a value and expect the >> + // device to update it). Allocate a host status buffer which can be mapped >> + // to access equally by both processor and the device. >> + // >> + Status = Dev->VirtIo->AllocateSharedPages ( >> + Dev->VirtIo, >> + EFI_SIZE_TO_PAGES (sizeof *HostStatus), >> + &HostStatusBuffer >> + ); >> + if (EFI_ERROR (Status)) { >> + return EFI_DEVICE_ERROR; >> + } >> + >> + // >> + // Map virtio-blk request header (must be done after request header is >> + // populated) >> + // >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterRead, >> + (VOID *) &Request, >> + sizeof Request, >> + &RequestDeviceAddress, >> + &RequestMapping >> + ); >> + if (EFI_ERROR (Status)) { >> + Status = EFI_DEVICE_ERROR; >> + goto FreeHostStatusBuffer; >> + } >> + >> + // >> + // Map data buffer >> + // >> + if (BufferSize > 0) { >> + if (RequestIsWrite) { >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterRead, >> + (VOID *) Buffer, >> + BufferSize, >> + &BufferDeviceAddress, >> + &BufferMapping >> + ); >> + } else { >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterWrite, >> + (VOID *) Buffer, >> + BufferSize, >> + &BufferDeviceAddress, >> + &BufferMapping >> + ); >> + } > > (2) Please squash these two branches into one, and determine the > Operation argument like this: > > (RequestIsWrite ? > VirtioOperationBusMasterRead : > VirtioOperationBusMasterWrite), > > (The conditional operator (? :) should be used sparingly, but when it > improves readability, it should be used.) > >> + >> + if (EFI_ERROR (Status)) { >> + Status = EFI_DEVICE_ERROR; >> + goto UnmapRequestBuffer; >> + } >> + } >> + >> + // >> + // Map the Status Buffer with VirtioOperationBusMasterCommonBuffer so that >> + // both processor and device can access it. >> + // >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterCommonBuffer, >> + HostStatusBuffer, >> + sizeof *HostStatus, >> + &HostStatusDeviceAddress, >> + &StatusMapping >> + ); >> + if (EFI_ERROR (Status)) { >> + Status = EFI_DEVICE_ERROR; >> + goto UnmapDataBuffer; >> + } >> + >> + HostStatus = HostStatusBuffer; >> + >> + // >> // preset a host status for ourselves that we do not accept as success >> // >> - HostStatus = VIRTIO_BLK_S_IOERR; >> + *HostStatus = VIRTIO_BLK_S_IOERR; (4) I think we should be careful with BusMasterCommonBuffer operations similarly to BusMasterWrite operations -- populate first, map second. This is to avoid exposing any stale data, even for a short window, to the device. Speaking in SEV terms, IoMmuMap() will decrypt NumberOfBytes in-place -- which is by-design, but then it should decrypt what we just put there, not whatever used to be there (until we overwrite it). IOW, please move the mapping just under the *HostStatus assignment. ... I've now checked all the VirtioOperationBusMasterCommonBuffer mappings in the tree, and the rest is fine -- in fact there is only one (outside of this patch) at the moment: in VirtioRingMap(). And that is fine, because VirtioRingInit() zeroes out the entire ring, after allocating it. Probably a good idea to attend to this in the other drivers (wherever they use BusMasterCommonBuffer). Thanks! Laszlo >> >> // >> // ensured by VirtioBlkInit() -- this predicate, in combination with the >> @@ -291,8 +380,13 @@ SynchronousRequest ( >> // >> // virtio-blk header in first desc >> // >> - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, >> - VRING_DESC_F_NEXT, &Indices); >> + VirtioAppendDesc ( >> + &Dev->Ring, >> + RequestDeviceAddress, >> + sizeof Request, >> + VRING_DESC_F_NEXT, >> + &Indices >> + ); >> >> // >> // data buffer for read/write in second desc >> @@ -311,27 +405,62 @@ SynchronousRequest ( >> // >> // VRING_DESC_F_WRITE is interpreted from the host's point of view. >> // >> - VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, >> + VirtioAppendDesc ( >> + &Dev->Ring, >> + BufferDeviceAddress, >> + (UINT32) BufferSize, >> VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), >> - &Indices); >> + &Indices >> + ); >> } >> >> // >> // host status in last (second or third) desc >> // >> - VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, >> - VRING_DESC_F_WRITE, &Indices); >> + VirtioAppendDesc ( >> + &Dev->Ring, >> + HostStatusDeviceAddress, >> + sizeof *HostStatus, >> + VRING_DESC_F_WRITE, >> + &Indices >> + ); >> >> // >> // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D). >> // >> - if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, >> - NULL) == EFI_SUCCESS && >> - HostStatus == VIRTIO_BLK_S_OK) { >> - return EFI_SUCCESS; >> + Status = VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, NULL); >> + >> + // >> + // Unmap the HostStatus buffer before accessing it >> + // >> + Ret = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping); > > (3) Okay, so here you will assign UnmapStatus. And then, the rest: > >> + if (EFI_ERROR (Ret)) { >> + Status = EFI_DEVICE_ERROR; >> + } >> + >> + if (!EFI_ERROR (Status) && >> + *HostStatus == VIRTIO_BLK_S_OK) { >> + Status = EFI_SUCCESS; >> + } else { >> + Status = EFI_DEVICE_ERROR; >> } > > should be written like this: > > if (EFI_ERROR (Status) || EFI_ERROR (UnmapStatus) || > *HostStatus != VIRTIO_BLK_S_OK) { > Status = EFI_DEVICE_ERROR; > } > > Namely, > > - this block will ensure that we only look at *HostStatus when we're > allowed to (i.e., after both VirtioFlush() and Unmap succeeded), > > - If VirtioFlush() fails and sets Status to some error code, then we > coerce Status to EFI_DEVICE_ERROR, > > - If the entire condition evaluates to FALSE, then Status is already > EFI_SUCCESS, so no need to touch it. > > > Regarding the VirtioScsiDxe driver... in this patch, we get away with > the above shortcut (without making a mess of the code), but in the > VirtioScsiDxe driver, we won't. In that driver, the bus master device > can produce *two* output buffers, so you will have to unmap at least one > of those under an error-handling label -- when you mapped the first > successfully, and failed to map the second. > > (In this patch you managed to unmap StatusMapping in shared code, but > that only worked because you had only one output buffer, and you could > put its mapping last.) > > And once you unmap an output buffer on both the success path and the > failure path, things get more complex. I think that's OK, we should just > be consistent about it. So, for VirtioScsiDxe, I suggest the pattern I > laid out in > . > > Thank you, > Laszlo > >> >> - return EFI_DEVICE_ERROR; >> +UnmapDataBuffer: >> + if (BufferSize > 0) { >> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); >> + } >> + >> +UnmapRequestBuffer: >> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); >> + >> +FreeHostStatusBuffer: >> + Dev->VirtIo->FreeSharedPages ( >> + Dev->VirtIo, >> + EFI_SIZE_TO_PAGES (sizeof *HostStatus), >> + HostStatusBuffer >> + ); >> + >> + return Status; >> } >> >> >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >