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 C815D209589D1 for ; Wed, 9 Aug 2017 17:44:13 -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 EA5811076E8; Thu, 10 Aug 2017 00:46:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EA5811076E8 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.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 7E2066C53E; Thu, 10 Aug 2017 00:46:30 +0000 (UTC) From: Laszlo Ersek 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> <82b54f74-f1ec-732f-4757-f7061e536406@redhat.com> Message-ID: Date: Thu, 10 Aug 2017 02:46:29 +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: <82b54f74-f1ec-732f-4757-f7061e536406@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.25]); Thu, 10 Aug 2017 00:46:32 +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:44:14 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/10/17 02:25, Laszlo Ersek wrote: > 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. Something else: please don't forget about the VIRTIO_F_IOMMU_PLATFORM flag -- pls see point (5.3) in . After this conversion is complete, we can claim that our virtio device drivers are "IOMMU aware", whenever we negotiate VIRTIO_F_VERSION_1. (It's a different question what IOMMU drivers we have.) 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; >> + } >> } >> >> >> >