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 C8020208AE3FF for ; Wed, 9 Aug 2017 13:28:25 -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 D719015E915; Wed, 9 Aug 2017 20:30:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D719015E915 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 11A7F5E243; Wed, 9 Aug 2017 20:30:41 +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-5-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <9e743ec8-f2fe-65f2-9594-4731d2481a88@redhat.com> Date: Wed, 9 Aug 2017 22:30:41 +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-5-git-send-email-brijesh.singh@amd.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.28]); Wed, 09 Aug 2017 20:30:44 +0000 (UTC) Subject: Re: [PATCH v1 04/14] OvmfPkg/VirtioLib: Add SharedBuffer helper functions 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 20:28:26 -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: > Patch adds convenience helper functions to call VIRTIO_DEVICE_PROTOCOL > AllocateSharedPages, FreeSharedPages, MapSharedBuffer and UnmapSharedBuffer > member functions. > > 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 | 143 +++++++++++++ > OvmfPkg/Library/VirtioLib/VirtioLib.c | 220 ++++++++++++++++++++ > 2 files changed, 363 insertions(+) I prefer to add functions to VirtioLib that actually buy us something. So let me evaluate that for each function: > diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h > index 5badfb32917f..fa82734f1852 100644 > --- a/OvmfPkg/Include/Library/VirtioLib.h > +++ b/OvmfPkg/Include/Library/VirtioLib.h > @@ -3,6 +3,7 @@ > Declarations of utility functions used by virtio device drivers. > > Copyright (C) 2012-2016, Red Hat, Inc. > + Copyright (C) 2017, AMD Inc, All rights reserved.
> > This program and the accompanying materials are licensed and made available > under the terms and conditions of the BSD License which accompanies this > @@ -235,4 +236,146 @@ Virtio10WriteFeatures ( > IN OUT UINT8 *DeviceStatus > ); > > +/** > + Helper function to allocate pages that is suitable for sharing with > + hypervisor. > + > + @param[in] VirtIo The target virtio device to use. It must be valid. > + > + @param[in] Pages The number of pages to allocate. > + > + @param[out] Buffer A pointer to store the base system memory address of > + the allocated range. > + > + return Returns error code from VirtIo->AllocateSharedPages() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioAllocateSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN UINTN NumPages, > + OUT VOID **Buffer > + ); > + > +/** > + Helper function to free pages allocated using VirtioAllocateSharedPages(). > + > + @param[in] VirtIo The target virtio device to use. It must be valid. > + > + @param[in] Pages The number of allocated pages. > + > + @param[in] Buffer System memory address allocated from > + VirtioAllocateSharedPages (). > +**/ > +VOID > +EFIAPI > +VirtioFreeSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN UINTN NumPages, > + IN VOID *Buffer > + ); > + > +/** > + A helper function to map a system memory to a shared bus master memory for > + read operation from DMA bus master. > + > + @param[in] VirtIo The target virtio device to use. It must be > + valid. > + > + @param[in] HostAddress The system memory address to map to shared bus > + master address. > + > + @param[in] NumberOfBytes Number of bytes to be mapped. > + > + @param[out] DeviceAddress The resulting shared map address for the bus > + master to access the hosts HostAddress. > + > + @param[out] Mapping A resulting value to pass to Unmap(). > + > + return Returns error code from > + VirtIo->MapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioMapSharedBufferRead ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ); > + > +/** > + A helper function to map a system memory to a shared bus master memory for > + write operation from DMA bus master. > + > + @param[in] VirtIo The target virtio device to use. It must be > + valid. > + > + @param[in] HostAddress The system memory address to map to shared bus > + master address. > + > + @param[in] NumberOfBytes Number of bytes to be mapped. > + > + @param[out] DeviceAddress The resulting shared map address for the bus > + master to access the hosts HostAddress. > + > + @param[out] Mapping A resulting value to pass to Unmap(). > + > + return Returns error code from > + VirtIo->MapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioMapSharedBufferWrite ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ); > + > +/** > + A helper function to map a system memory to a shared bus master memory for > + common operation from DMA bus master. > + > + @param[in] VirtIo The target virtio device to use. It must be > + valid. > + > + @param[in] HostAddress The system memory address to map to shared bus > + master address. > + > + @param[in] NumberOfBytes Number of bytes to be mapped. > + > + @param[out] Mapping A resulting value to pass to Unmap(). > + > + return Returns error code from > + VirtIo->MapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioMapSharedBufferCommon ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT VOID **Mapping > + ); > + > +/** > + A helper function to unmap shared bus master memory mapped using Map(). > + > + @param[in] VirtIo The target virtio device to use. It must be > + valid. > + > + @param[in] Mapping A mapping value return from Map(). > + > + return Returns error code from > + VirtIo->UnmapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioUnmapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VOID *Mapping > + ); > #endif // _VIRTIO_LIB_H_ > diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c > index 845f206369a3..f6b464b6cdf8 100644 > --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c > +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c > @@ -4,6 +4,7 @@ > > Copyright (C) 2012-2016, Red Hat, Inc. > Portion of Copyright (C) 2013, ARM Ltd. > + Copyright (C) 2017, AMD Inc, All rights reserved.
> > This program and the accompanying materials are licensed and made available > under the terms and conditions of the BSD License which accompanies this > @@ -414,3 +415,222 @@ Virtio10WriteFeatures ( > > return Status; > } > + > +/** > + Helper function to allocate pages that is suitable for sharing with > + hypervisor. > + > + @param[in] VirtIo The target virtio device to use. It must be valid. > + > + @param[in] Pages The number of pages to allocate. > + > + @param[out] Buffer A pointer to store the base system memory address of > + the allocated range. > + > + return Returns error code from VirtIo->AllocateSharedPages() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioAllocateSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN UINTN NumPages, > + OUT VOID **Buffer > + ) > +{ > + return VirtIo->AllocateSharedPages (VirtIo, NumPages, Buffer); > +} (1) I think this function buys us nothing. I suggest to drop it. > + > +/** > + Helper function to free pages allocated using VirtioAllocateSharedPages(). > + > + @param[in] VirtIo The target virtio device to use. It must be valid. > + > + @param[in] Pages The number of allocated pages. > + > + @param[in] Buffer System memory address allocated from > + VirtioAllocateSharedPages (). > +**/ > +VOID > +EFIAPI > +VirtioFreeSharedPages ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN UINTN NumPages, > + IN VOID *Buffer > + ) > +{ > + VirtIo->FreeSharedPages (VirtIo, NumPages, Buffer); > +} (2) Same here. > + > +STATIC > +EFI_STATUS > +VirtioMapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VIRTIO_MAP_OPERATION Operation, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ) (3) Please add a comment block above the function. Basically you can copy the MapSharedBuffer() documentation, just point out the extra logic for NumberOfBytes. In fact, please rename this function to: VirtioMapAllBytesInSharedBuffer > +{ > + EFI_STATUS Status; > + VOID *MapInfo; > + UINTN Size; > + EFI_PHYSICAL_ADDRESS PhysicalAddress; > + > + Size = NumberOfBytes; > + Status = VirtIo->MapSharedBuffer ( > + VirtIo, > + Operation, > + HostAddress, > + &Size, > + &PhysicalAddress, > + &MapInfo > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (Size < NumberOfBytes) { > + goto Failed; > + } > + > + *Mapping = MapInfo; > + *DeviceAddress = PhysicalAddress; > + > + return EFI_SUCCESS; > +Failed: > + VirtIo->UnmapSharedBuffer (VirtIo, MapInfo); > + return EFI_OUT_OF_RESOURCES; > +} Now, I think this helper function does actually buy us convenience -- it helps us centralize error handling, and it allows callers to pass in constants or other evaluated expressions for NumberOfBytes. I've given quite a bit of thought to our "hardliner" handling of the case when NumberOfBytes is decreased on output. The PciIo spec says that in such cases the transfer should be broken up into smaller chunks: > In all mapping requests the resulting NumberOfBytes actually mapped > may be less than the requested amount. In this case, the DMA operation > will have to be broken up into smaller chunks. The Map() function will > map as much of the DMA operation as it can at one time. The caller may > have to loop on Map() and Unmap() in order to complete a large DMA > transfer. While generally speaking that could be a valid idea, I think it doesn't apply well to virtio, for the following reasons: - I don't think that CommonBuffer areas are possible to split up in any sensible way (e.g., rings), - Even for unidirectional transfers, the request framing is usually dictated on the virtio request level (i.e., request framing is usually matched closely by the individual virtio descriptors that constitute the descriptor chain that *is* the virtio request). If PciIo (ultimately: the IOMMU) "suggests" that we split the request into smaller chunks, that could affect the descriptors / descriptor chaining too, and I simply don't want to go there. - Turning a single "outer request" (like EFI_BLOCK_IO_PROTOCOL read or write request) into a series of "inner virtio requests" is a mess as well -- what if you get a failure from the hypervisor mid-loop? IMO, we should forward the outer (higher level) request as transparently over virtio as possible. If the IOMMU limits that for whatever reason, then we should fail the request immediately, and propagate the error to the client of the higher level protocol. So, IMO, the handling of NumberOfBytes is valid here, and this function is useful. (4) Please make this function extern (add it to the header file too). > + > +/** > + A helper function to map a system memory to a shared bus master memory for > + read operation from DMA bus master. > + > + @param[in] VirtIo The target virtio device to use. It must be > + valid. > + > + @param[in] HostAddress The system memory address to map to shared bus > + master address. > + > + @param[in] NumberOfBytes Number of bytes to be mapped. > + > + @param[out] DeviceAddress The resulting shared map address for the bus > + master to access the hosts HostAddress. > + > + @param[out] Mapping A resulting value to pass to Unmap(). > + > + return Returns error code from > + VirtIo->MapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioMapSharedBufferRead ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ) > +{ > + return VirtioMapSharedBuffer (VirtIo, EfiVirtIoOperationBusMasterRead, > + HostAddress, NumberOfBytes, DeviceAddress, Mapping); > +} (5) I don't think this function buys us much. The only parameter you can save at the call site is Operation, but the rest (together with the saving of the return code) will require so many characters still that you'll have to break it to multiple lines anyway. So I suggest to drop this function. > + > +/** > + A helper function to map a system memory to a shared bus master memory for > + write operation from DMA bus master. > + > + @param[in] VirtIo The target virtio device to use. It must be > + valid. > + > + @param[in] HostAddress The system memory address to map to shared bus > + master address. > + > + @param[in] NumberOfBytes Number of bytes to be mapped. > + > + @param[out] DeviceAddress The resulting shared map address for the bus > + master to access the hosts HostAddress. > + > + @param[out] Mapping A resulting value to pass to Unmap(). > + > + return Returns error code from > + VirtIo->MapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioMapSharedBufferWrite ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ) > +{ > + return VirtioMapSharedBuffer (VirtIo, EfiVirtIoOperationBusMasterWrite, > + HostAddress, NumberOfBytes, DeviceAddress, Mapping); > +} (6) Same as (5) here. > + > +/** > + A helper function to map a system memory to a shared bus master memory for > + common operation from DMA bus master. > + > + @param[in] VirtIo The target virtio device to use. It must be > + valid. > + > + @param[in] HostAddress The system memory address to map to shared bus > + master address. > + > + @param[in] NumberOfBytes Number of bytes to be mapped. > + > + @param[out] Mapping A resulting value to pass to Unmap(). > + > + return Returns error code from > + VirtIo->MapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioMapSharedBufferCommon ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT VOID **Mapping > + ) > +{ > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + > + Status = VirtioMapSharedBuffer (VirtIo, > + EfiVirtIoOperationBusMasterCommonBuffer, HostAddress, > + NumberOfBytes, &DeviceAddress, Mapping); > + > + // > + // On Success, lets verify that DeviceAddress same as HostAddress > + // > + if (!EFI_ERROR (Status)) { > + ASSERT (DeviceAddress == (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress); > + } > + > + return Status; > +} So this function seems to save us two parameters: (7) Operation -- I commented on that under (5). (8) DeviceAddress -- I actually disagree with hiding that. In my opinion, this is different from the NumberOfBytes question. We can be inflexible with NumberOfBytes because the in-out NumberOfBytes parameter of PciIo.Map() reflects an *accidental* trait of some IOMMUs (namely that their map sizes may be limited). I think that in the virtio scope we can safely say that we simply don't support IOMMU protocol instances that can't accommodate our desired request framing. (Case in point: versions of the virtio spec have presented limits on virtio request sizes, and thus the virtio drivers already enforce suitable (stricter) limits on the outer protocol interfaces. Search "VirtioBlk.c" and "VirtioScsi.c" for SIZE_1GB, for example. Therefore refusing oversized requests is nothing new, except the definition of "oversized" might vary, dependent on IOMMU.) However, DeviceAddress being (potentially) different from HostAddress is the core idea (an *inherent* trait) of the IOMMU protocol. If we ever implement a different IOMMU protocol -- e.g. for QEMU's vIOMMU --, I wouldn't like this assert -- or function prototype -- to cause issues. So, please drop this helper function, and update all call sites to pass DeviceAddress to the virtio device. (I'm aware that this might cause major churn, and more review work for me, but after all, this is the entire point of the IOMMU abstraction.) > + > +/** > + A helper function to unmap shared bus master memory mapped using Map(). > + > + @param[in] VirtIo The target virtio device to use. It must be > + valid. > + > + @param[in] Mapping A mapping value return from Map(). > + > + return Returns error code from > + VirtIo->UnmapSharedBuffer() > +**/ > +EFI_STATUS > +EFIAPI > +VirtioUnmapSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VOID *Mapping > + ) > +{ > + return VirtIo->UnmapSharedBuffer (VirtIo, Mapping); > +} > (9) I think this function also doesn't buy us much, please drop it. Thanks, Laszlo