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 E81052095B9C9 for ; Wed, 23 Aug 2017 12:43:15 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 982FE882FB; Wed, 23 Aug 2017 19:45:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 982FE882FB 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-43.phx2.redhat.com [10.3.116.43]) by smtp.corp.redhat.com (Postfix) with ESMTP id EF0385D753; Wed, 23 Aug 2017 19:45:47 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1503490967-5559-1-git-send-email-brijesh.singh@amd.com> <1503490967-5559-6-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 23 Aug 2017 21:45:46 +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: <1503490967-5559-6-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 23 Aug 2017 19:45:49 +0000 (UTC) Subject: Re: [PATCH v3 05/23] OvmfPkg/VirtioLib: add VirtioMapAllBytesInSharedBuffer() helper function 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, 23 Aug 2017 19:43:16 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit There are two trivial omissions in this patch: On 08/23/17 14:22, Brijesh Singh wrote: > The function can be used for mapping the system physical address to virtio > device address using VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer (). The > function helps with centralizing error handling, and it allows the caller > to pass in constant or other evaluated expressions for NumberOfBytes. > > 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 | 52 ++++++++++++ > OvmfPkg/Library/VirtioLib/VirtioLib.c | 85 ++++++++++++++++++++ > 2 files changed, 137 insertions(+) > > diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h > index 5badfb32917f..9ec9b91b59bb 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,55 @@ Virtio10WriteFeatures ( > IN OUT UINT8 *DeviceStatus > ); > > +/** > + Provides the virtio device address required to access system memory from a > + DMA bus master. > + > + The interface follows the same usage pattern as defined in UEFI spec 2.6 > + (Section 13.2 PCI Root Bridge I/O Protocol) > + > + The VirtioMapAllBytesInSharedBuffer() is similar to VIRTIO_MAP_SHARED > + with exception that NumberOfBytes is IN-only parameter. The function > + maps all the bytes specified in NumberOfBytes param in one consecutive > + range. > + > + @param[in] This The virtio device for which the mapping is > + requested. (1) The parameter is called "VirtIo", not "This". (In my previous review, I proposed a replacement that covered both the parameter name and the parameter documentation, and I think you updated only the documentation.) > + > + @param[in] Operation Indicates if the bus master is going to > + read or write to system memory. > + > + @param[in] HostAddress The system memory address to map to shared > + buffer address. > + > + @param[in] NumberOfBytes Number of bytes to map. > + > + @param[out] DeviceAddress The resulting shared map address for the > + bus master to access the hosts HostAddress. > + > + @param[out] Mapping A resulting token to pass to > + VIRTIO_UNMAP_SHARED. > + > + > + @retval EFI_SUCCESS The NumberOfBytes is succesfully mapped. > + @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a > + common buffer. > + @retval EFI_INVALID_PARAMETER One or more parameters are invalid. > + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to > + a lack of resources. This includes the case > + when NumberOfBytes bytes cannot be mapped > + in one consecutive range. > + @retval EFI_DEVICE_ERROR The system hardware could not map the > + requested address. > +**/ > +EFI_STATUS > +EFIAPI > +VirtioMapAllBytesInSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VIRTIO_MAP_OPERATION Operation, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ); > #endif // _VIRTIO_LIB_H_ > diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c > index 845f206369a3..c85cd8dba569 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,87 @@ Virtio10WriteFeatures ( > > return Status; > } > + > +/** > + Provides the virtio device address required to access system memory from a > + DMA bus master. > + > + The interface follows the same usage pattern as defined in UEFI spec 2.6 > + (Section 13.2 PCI Root Bridge I/O Protocol) > + > + The VirtioMapAllBytesInSharedBuffer() is similar to VIRTIO_MAP_SHARED > + with exception that NumberOfBytes is IN-only parameter. The function > + maps all the bytes specified in NumberOfBytes param in one consecutive > + range. > + > + @param[in] This The virtio device for which the mapping is > + requested. (2) Same as (1), the parameter is called "VirtIo", not "This". My plan is to test and push the initial sequence of this series (if the review progresses far enough without more serious remarks), so my plan is to fix these up on commit myself. With the above fixed (by you or by me; we'll see): Reviewed-by: Laszlo Ersek Thanks Laszlo > + > + @param[in] Operation Indicates if the bus master is going to > + read or write to system memory. > + > + @param[in] HostAddress The system memory address to map to shared > + buffer address. > + > + @param[in] NumberOfBytes Number of bytes to map. > + > + @param[out] DeviceAddress The resulting shared map address for the > + bus master to access the hosts HostAddress. > + > + @param[out] Mapping A resulting token to pass to > + VIRTIO_UNMAP_SHARED. > + > + > + @retval EFI_SUCCESS The NumberOfBytes is succesfully mapped. > + @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a > + common buffer. > + @retval EFI_INVALID_PARAMETER One or more parameters are invalid. > + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to > + a lack of resources. This includes the case > + when NumberOfBytes bytes cannot be mapped > + in one consecutive range. > + @retval EFI_DEVICE_ERROR The system hardware could not map the > + requested address. > +**/ > +EFI_STATUS > +EFIAPI > +VirtioMapAllBytesInSharedBuffer ( > + IN VIRTIO_DEVICE_PROTOCOL *VirtIo, > + IN VIRTIO_MAP_OPERATION Operation, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ) > +{ > + 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; > +} >