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 AFBCF21D0A27D for ; Mon, 21 Aug 2017 07:03:19 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C776361467; Mon, 21 Aug 2017 14:05:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C776361467 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-165.phx2.redhat.com [10.3.116.165]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6345877BEC; Mon, 21 Aug 2017 14:05:49 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502710605-8058-1-git-send-email-brijesh.singh@amd.com> <1502710605-8058-17-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Mon, 21 Aug 2017 16:05:48 +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: <1502710605-8058-17-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Mon, 21 Aug 2017 14:05:51 +0000 (UTC) Subject: Re: [PATCH v2 16/23] OvmfPkg/VirtioRngDxe: map host address to device address 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: Mon, 21 Aug 2017 14:03:19 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/14/17 13:36, Brijesh Singh wrote: > patch maps the host address to a device address for buffers (including > rings, device specifc request and response pointed by vring descriptor, > and any further memory reference by those request and response). > > 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/VirtioRngDxe/VirtioRng.h | 1 + > OvmfPkg/VirtioRngDxe/VirtioRng.c | 65 +++++++++++++++++--- > 2 files changed, 58 insertions(+), 8 deletions(-) > > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h b/OvmfPkg/VirtioRngDxe/VirtioRng.h > index 998f9fae48c2..b372d84c1ebc 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.h > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h > @@ -38,6 +38,7 @@ typedef struct { > EFI_EVENT ExitBoot; // DriverBindingStart 0 > VRING Ring; // VirtioRingInit 2 > EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1 > + VOID *RingMap; // VirtioRngInit 1 > } VIRTIO_RNG_DEV; (1) Please change the "depth" value from 1 to 2 on the right-hand side. "RingMap" has the same initialization depth as "Ring". Also, please change the function name from "VirtioRngInit" to "VirtioRingMap". > > #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \ > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c > index 0abca488e6cd..fc01f1996654 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c > @@ -140,6 +140,8 @@ VirtioRngGetRNG ( > UINT32 Len; > UINT32 BufferSize; > EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *Mapping; > > if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) { > return EFI_INVALID_PARAMETER; > @@ -159,6 +161,20 @@ VirtioRngGetRNG ( > } > > Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); > + // > + // Map the Buffers system phyiscal address to device address > + // (2) s/the Buffers/Buffer's/ > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterWrite, > + (VOID *)Buffer, > + RNGValueLength, > + &DeviceAddress, > + &Mapping > + ); > + if (EFI_ERROR (Status)) { > + goto FreeBuffer; > + } > > // > // The Virtio RNG device may return less data than we asked it to, and can > @@ -170,7 +186,7 @@ VirtioRngGetRNG ( > > VirtioPrepare (&Dev->Ring, &Indices); > VirtioAppendDesc (&Dev->Ring, > - (UINTN)Buffer + Index, > + (UINTN)DeviceAddress + Index, (3) Please insert a new VirtioLib patch in the series, before this patch. The new patch should: (3a) change the "BufferPhysAddr" parameter of VirtioAppendDesc() from type UINTN to UINT64, (3b) rename the same parameter to "BufferDeviceAddress", (3c) replace the following parameter documentation string: "(Guest pseudo-physical)" with the string "Bus master device" Justification: UINTN is appropriate as long as we pass system memory references. After the introduction of this feature, that's no longer the case in general. Should we implement "real" IOMMU support at some point, UINTN could break in 32-bit builds of OVMF. The definition of VirtioAppendDesc() itself needs no update (beyond the parameter rename), because the VRING_DESC.Addr field already has type UINT64. (4) In this patch, please remove the UINTN cast, and just say DeviceAddress + Index Same for the rest of the patches. (I haven't seen them yet, but I assume the same pattern is repeated in them.) > BufferSize, > VRING_DESC_F_WRITE, > &Indices); > @@ -178,17 +194,30 @@ VirtioRngGetRNG ( > if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != > EFI_SUCCESS) { > Status = EFI_DEVICE_ERROR; > - goto FreeBuffer; > + goto UnmapBuffer; > } > ASSERT (Len > 0); > ASSERT (Len <= BufferSize); > } > > + // > + // Unmap the device buffer before accesing it. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + (5) In the following messages: - http://mid.mail-archive.com/f953ec03-b160-726b-b9ed-5e3122642815@redhat.com - http://mid.mail-archive.com/35b958de-ed97-6275-be96-1cbb93a99d97@amd.com we agreed that EFI_STATUS was the appropriate return type for VIRTIO_UNMAP_SHARED. We also agreed that we'd check the return value when necessary -- that is, when we are after a bus master write operation and want to consume the data produced by the device. So, please check the return status here, and jump to FreeBuffer if the unmap fails. (With VIRTIO_UNMAP_SHARED, we imitate EFI_PCI_IO_PROTOCOL.Unmap(). The specification says, "Any resources used for the mapping are freed" -- this is unconditional. The part that can fail is "committing the data to target system memory". So, if the UnmapSharedBuffer() call fails, we must skip consuming the data, but still proceed with freeing the buffer.) Ignoring the return value of UnmapSharedBuffer() is OK on error handling paths. > for (Index = 0; Index < RNGValueLength; Index++) { > RNGValue[Index] = Buffer[Index]; > } > Status = EFI_SUCCESS; > > + // > + // Buffer is already Unmaped(), goto free it. > + // > + goto FreeBuffer; (6) We restrict "goto" statements to error handling: https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html > 5.7.3.8 Goto Statements should not be used (in general) > [...] > It is common to use goto for error handling and thus, exiting a > routine in an error case is the only legal use of a goto. [...] So please open-code the FreePool() call and the "return" statement here. > + > +UnmapBuffer: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + (7) The function call is incorrectly indented. > FreeBuffer: > FreePool ((VOID *)Buffer); > return Status; > @@ -205,6 +234,7 @@ VirtioRngInit ( > EFI_STATUS Status; > UINT16 QueueSize; > UINT64 Features; > + UINT64 RingBaseShift; > > // > // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence. > @@ -281,26 +311,33 @@ VirtioRngInit ( > goto Failed; > } > (8) Please add a comment here: // // If anything fails from here on, we must release the ring resources. // > + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift, > + &Dev->RingMap); (9) Please break each argument to a separate line. (As much as I personally prefer the style visible above, it is not (one of) the edk2 coding style(s). :( Not yet, anyway.) > + 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. > // (10) Please replace the word "release" with "unmap", in the above comment. > 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); (11) Please break each argument to a separate line. > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -310,7 +347,7 @@ VirtioRngInit ( > Features &= ~(UINT64)VIRTIO_F_VERSION_1; > Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > } > > @@ -320,7 +357,7 @@ VirtioRngInit ( > NextDevStat |= VSTAT_DRIVER_OK; > Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -331,6 +368,9 @@ VirtioRngInit ( > > return EFI_SUCCESS; > > +UnmapQueue: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > + (12) The function call is incorrectly indented. > ReleaseQueue: > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > @@ -359,6 +399,9 @@ VirtioRngUninit ( > // the old comms area. > // > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > + > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > } > > @@ -385,6 +428,12 @@ VirtioRngExitBoot ( > // > Dev = Context; > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + // > + // Umap the ring buffer so that hypervisor will not able to get readable data (13) s/Umap/Unmap/; also s/will not able/will not be able/ (Please make sure the line doesn't become overlong.) > + // after device reset. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > } > > > The logic seems fine otherwise. Thanks! Laszlo