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 8029821DFE931 for ; Wed, 9 Aug 2017 16:49:04 -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 870DF5109C; Wed, 9 Aug 2017 23:51:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 870DF5109C Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.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 D4C0618871; Wed, 9 Aug 2017 23:51:20 +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-7-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <01220a3b-565b-8a36-406d-cfcc63265fb7@redhat.com> Date: Thu, 10 Aug 2017 01:51:19 +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-7-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.28]); Wed, 09 Aug 2017 23:51:22 +0000 (UTC) Subject: Re: [PATCH v1 06/14] OvmfPkg/VirtioLib: Add functions to map/unmap VRING 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: Wed, 09 Aug 2017 23:49:04 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit First some superficial comments, then some more serious ones. On 08/07/17 13:58, Brijesh Singh wrote: > Add functions to map and unmap the ring buffer with BusMasterCommonBuffer. > > 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/Include/Library/VirtioLib.h | 41 +++++++++++++++ > OvmfPkg/Library/VirtioLib/VirtioLib.c | 52 ++++++++++++++++++++ > 2 files changed, 93 insertions(+) > > diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h > index 610654225de7..877961af0514 100644 > --- a/OvmfPkg/Include/Library/VirtioLib.h > +++ b/OvmfPkg/Include/Library/VirtioLib.h > @@ -62,6 +62,47 @@ VirtioRingInit ( > > /** > > + Map the ring buffer so that it can be accessed equally by both guest > + and hypervisor. > + > + @param[in] VirtIo The virtio device instance. > + > + @param[in] Ring The virtio ring to map. > + > + @param[out] Mapping A resulting value to pass to Unmap(). (1) Please say "token" here (to be consistent with my earlier request). (2) Please replace Unmap() with VirtIo->UnmapSharedBuffer(). > + > + @retval Value returned from VirtIo->MapSharedBuffer() (3) This should be @return, not @retval. @retval is for concrete constants. Also I would suggest "Status code" rather than "Value". (4) Please sync the comment block in the C file. > +**/ > +EFI_STATUS > +EFIAPI > +VirtioRingMap ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VRING *Ring, > + OUT VOID **Mapping > + ); > + > +/** > + > + Unmap the ring buffer mapped using VirtioRingMap() > + > + @param[in] VirtIo The virtio device instance. > + > + @param[in] Ring The virtio ring to unmap. > + > + @param[in] Mapping A value obtained through Map(). > + > + @retval Value returned from VirtIo->UnmapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioRingUnmap ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VRING *Ring, > + IN VOID *Mapping > + ); (5) Please drop this function. It doesn't save us anything. (The Ring parameter isn't even used in the implementation.) > + > +/** > + > Tear down the internal resources of a configured virtio ring. > > The caller is responsible to stop the host from using this ring before > diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c > index 50e5ec28ea1b..09a3f9b7f2e5 100644 > --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c > +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c > @@ -640,3 +640,55 @@ VirtioUnmapSharedBuffer ( > { > return VirtIo->UnmapSharedBuffer (VirtIo, Mapping); > } > + > +/** > + > + Map the ring buffer so that it can be accessed equally by both guest > + and hypervisor. > + > + @param[in] VirtIo The virtio device instance. > + > + @param[in] Ring The virtio ring to map. > + > + @param[out] Mapping A resulting value to pass to Unmap(). > + > + @retval Value returned from VirtIo->MapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioRingMap ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VRING *Ring, > + OUT VOID **Mapping > + ) > +{ > + UINTN NumberOfBytes; > + > + NumberOfBytes = Ring->NumPages * EFI_PAGE_SIZE; (6) "EFI_PAGES_TO_SIZE (Ring->NumPages)" is more idiomatic. > + > + return VirtioMapSharedBufferCommon (VirtIo, Ring->Base, > + NumberOfBytes, Mapping); (7) So, based on my earlier request, this should become a call to VirtioMapAllBytesInSharedBuffer(). Please break every argument to a separate line. Now, my important comments for this patch. :) VirtioMapAllBytesInSharedBuffer() will give you a DeviceAddress. Here's what we should do with it: (8) Please introduce a new patch that modifies the VIRTIO_SET_QUEUE_ADDRESS typedef, in "OvmfPkg/Include/Protocol/VirtioDevice.h". Namely, please add a third parameter: IN UINT64 RingBaseShift In this initial patch, simply update all call sites to pass in 0. In parallel (in the same initial patch), update all three implementations: - VirtioMmioSetQueueAddress() [OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c] - VirtioPciSetQueueAddress() [OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c] - Virtio10SetQueueAddress() [OvmfPkg/Virtio10Dxe/Virtio10.c] as follows: - accept the new parameter (obviously), - simply assert that it is zero, at the top of the function. (9) Modify Virtio10SetQueueAddress() as follows, in another separate patch: - every time after the local variable "Address" is assigned, please insert: Address += RingBaseShift; This is automatically going to happen in UINT64 arithmetic, which has well-defined wrap-around semantics. Three such insertions will be necessary. - remove the ASSERT added in (8) (10) Then please include the present patch, with the following update (beyond the changes requested above): - Add the following output parameter to VirtioRingMap(): OUT UINT64 *RingBaseShift - In the implementation, assign it like this, if VirtioMapAllBytesInSharedBuffer() succeeds: *RingBaseShift = DeviceAddress - (UINT64)(UINTN)Ring->Base; (DeviceAddress has type EFI_PHYSICAL_ADDRESS, which is equivalent to UINT64, on the spec level.) (11) Please make sure that no output parameter is modified before we can guarantee returning EFI_SUCCESS. (12) In the individual virtio device drivers, whenever you insert the VirtioRingMap() function call, carry "RingBaseShift" from VirtioRingMap() to VirtIo->SetQueueAddress() in a new local UINT64 variable. End result: - the MMIO and legacy PCI transport implementations will always set DeviceAddress to HostAddress in their no-op MapSharedBuffer() member functions, - therefore VirtioRingMap() will set RingBaseShift to zero, - the ASSERT()s in the SetQueueAddress() members of those same transport implementations will be satisfied, - with the modern-only (1.0) transport, RingBaseShift will also be zero (because of the SEV IOMMU that we have), and "Address += 0" will have no effect, - if we ever add an IOMMU driver that translates addresses, the translation will happen automatically in the modern-only transport. Thanks, Laszlo > +} > + > +/** > + > + Unmap the ring buffer mapped using VirtioRingMap() > + > + @param[in] VirtIo The virtio device instance. > + > + @param[in] Ring The virtio ring to unmap. > + > + @param[in] Mapping A value obtained through Map(). > + > + @retval Value returned from VirtIo->UnmapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioRingUnmap ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VRING *Ring, > + IN VOID *Mapping > + ) > +{ > + return VirtioUnmapSharedBuffer (VirtIo, Mapping); > +} >