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 7CF1A208F7AD4 for ; Wed, 9 Aug 2017 17:22:47 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C8AE0E30D4; Thu, 10 Aug 2017 00:25:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C8AE0E30D4 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-102.phx2.redhat.com [10.3.116.102]) by smtp.corp.redhat.com (Postfix) with ESMTP id DD11D7C8A3; Thu, 10 Aug 2017 00:25:03 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502107139-412-1-git-send-email-brijesh.singh@amd.com> <1502107139-412-15-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <82b54f74-f1ec-732f-4757-f7061e536406@redhat.com> Date: Thu, 10 Aug 2017 02:25:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1502107139-412-15-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 10 Aug 2017 00:25:05 +0000 (UTC) Subject: Re: [PATCH v1 14/14] OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors 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, 10 Aug 2017 00:22:47 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/07/17 13:58, Brijesh Singh wrote: > The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() > to map system memory to device address and programs the vring descriptors > with device addresses. > > 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 | 47 +++++++++++++++++--- > 2 files changed, 43 insertions(+), 5 deletions(-) I'm skipping all the other device driver conversion patches for now (v1 8-13), because I think that this is the simplest driver, and we already have enough changes queued from this review. I suggest that for v2, you please update and test all of the drivers (so that we can be sure that my requests are feasible), but move this driver patch in front of the others. I'll say a number of generalities: * Please never roll-back earlier actions directly within an error-catching "if" -- instead, insert a new error handling label at the appropriate place, and jump to it with "goto". Sooner or later we'll grow yet more actions, and then we'll have to convert those "if" bodies to goto's anyway. * NULL for RingMap is a valid value () -- see also point (8) in my v1 03/14 feedback -- and device driver logic should not depend on it. Instead, use additional (precise) labels -- such as "UnmapQueue" and "UnmapBuffer" -- and matching goto statements. If a VirtIo->MapSharedBuffer() implementation gives you RingMap=NULL (on success), then VirtIo->UnmapSharedBuffer() will also take RingMap=NULL -- you simply don't have to be aware of the value. * In most cases, the fact that you have a live exit-boot-services callback implies that your device is "live" as well. So no need for additional checks before unmapping the queue. (There are exceptions of course, such as virtio-net -- IIRC the SimpleNetworkProtocol has an internal state flag that affects this question.) For example, in the virtio-rng case, the exit-boot-services notification function is installed in VirtioRngDriverBindingStart() *after* the call to VirtioRngInit(). In addition, it is uninstalled -- its associated event is closed -- in VirtioRngDriverBindingStop(), *before* the call to VirtioRngUninit(). So whenever the notification function can run, the device is guaranteed to be live. Thanks, Laszlo > 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; > > #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \ > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c > index e20602ac7225..5ff54867616a 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c > @@ -140,11 +140,15 @@ VirtioRngGetRNG ( > UINT32 Len; > UINT32 BufferSize; > EFI_STATUS Status; > + UINTN NumPages; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *Mapping; > > if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) { > return EFI_INVALID_PARAMETER; > } > > + Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); > // > // We only support the raw algorithm, so reject requests for anything else > // > @@ -153,12 +157,18 @@ VirtioRngGetRNG ( > return EFI_UNSUPPORTED; > } > > - Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength); > - if (Buffer == NULL) { > + NumPages = EFI_SIZE_TO_PAGES (RNGValueLength); > + Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages, (VOID *)&Buffer); > + if (EFI_ERROR (Status)) { > return EFI_DEVICE_ERROR; > } > > - Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); > + Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *)Buffer, > + RNGValueLength, &DeviceAddress, &Mapping); > + if (EFI_ERROR (Status)) { > + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer); > + return EFI_DEVICE_ERROR; > + } > > // > // The Virtio RNG device may return less data than we asked it to, and can > @@ -170,7 +180,7 @@ VirtioRngGetRNG ( > > VirtioPrepare (&Dev->Ring, &Indices); > VirtioAppendDesc (&Dev->Ring, > - (UINTN)Buffer + Index, > + (UINTN)DeviceAddress + Index, > BufferSize, > VRING_DESC_F_WRITE, > &Indices); > @@ -178,19 +188,22 @@ VirtioRngGetRNG ( > if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != > EFI_SUCCESS) { > Status = EFI_DEVICE_ERROR; > + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping); > goto FreeBuffer; > } > ASSERT (Len > 0); > ASSERT (Len <= BufferSize); > } > > + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping); > + > for (Index = 0; Index < RNGValueLength; Index++) { > RNGValue[Index] = Buffer[Index]; > } > Status = EFI_SUCCESS; > > FreeBuffer: > - FreePool ((VOID *)Buffer); > + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer); > return Status; > } > > @@ -281,6 +294,11 @@ VirtioRngInit ( > goto Failed; > } > > + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &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. > @@ -332,6 +350,11 @@ VirtioRngInit ( > return EFI_SUCCESS; > > ReleaseQueue: > + if (Dev->RingMap != NULL) { > + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); > + Dev->RingMap = NULL; > + } > + > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > Failed: > @@ -359,6 +382,11 @@ VirtioRngUninit ( > // the old comms area. > // > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + if (Dev->RingMap != NULL) { > + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); > + } > + > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > } > > @@ -385,6 +413,15 @@ VirtioRngExitBoot ( > // > Dev = Context; > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + // > + // If Ring mapping exist then umap it so that hypervisor will not able to > + // get readable data after device reset. > + // > + if (Dev->RingMap != NULL) { > + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); > + Dev->RingMap = NULL; > + } > } > > >