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 F21FA21E11D90 for ; Wed, 9 Aug 2017 07:24:46 -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 98C50C1CAC9A; Wed, 9 Aug 2017 14:27:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 98C50C1CAC9A Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.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 C67388476B; Wed, 9 Aug 2017 14:27:02 +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-2-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: Date: Wed, 9 Aug 2017 16:27:01 +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-2-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.31]); Wed, 09 Aug 2017 14:27:04 +0000 (UTC) Subject: Re: [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL 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 14:24: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 extends VIRTIO_DEVICE_PROTOCOL to provide the following new > member functions: > > - AllocateSharedPages : allocate a memory region suitable for sharing > between guest and hypervisor (e.g ring buffer). > > - FreeSharedPages: free the memory allocated using AllocateSharedPages (). > > - MapSharedBuffer: map a host address to device address suitable to share > with device for bus master operations. > > - UnmapSharedBuffer: unmap the device address obtained through the > MapSharedBuffer(). > > Suggested-by: Laszlo Ersek > 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/Protocol/VirtioDevice.h | 121 ++++++++++++++++++++ > 1 file changed, 121 insertions(+) (1) I suggest the following subject: OvmfPkg: introduce IOMMU-like member functions to VIRTIO_DEVICE_PROTOCOL The current wording "new member functions" is a bit too generic IMO. > > diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h > index 910a4866e7ac..ea5272165389 100644 > --- a/OvmfPkg/Include/Protocol/VirtioDevice.h > +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h > @@ -5,6 +5,7 @@ > and should not be used outside of the EDK II tree. > > Copyright (c) 2013, ARM Ltd. All rights reserved.
> + 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 > @@ -31,6 +32,26 @@ > > typedef struct _VIRTIO_DEVICE_PROTOCOL VIRTIO_DEVICE_PROTOCOL; > > +// > +// VIRTIO Operation for Map > +// (2) "Map" can be made a bit more precise; please say either "VIRTIO_MAP_SHARED" or MapSharedBuffer(). > +typedef enum { > + // > + // A read operation from system memory by a bus master > + // > + EfiVirtIoOperationBusMasterRead, > + // > + // A write operation from system memory by a bus master > + // > + EfiVirtIoOperationBusMasterWrite, (3) s/from/to/ > + // > + // Provides both read and write access to system memory by both the > + // processor and a bus master > + // > + EfiVirtIoOperationBusMasterCommonBuffer, > + EfiVirtIoOperationMaximum > +} VIRTIO_MAP_OPERATION; > + (4) Please drop the "Efi" prefix. (5) Please replace the remaining "VirtIo" prefix with "Virtio". Although you can find both spellings in the source code, they are used in different contexts. In function names and enumeration constants, we've used Virtio* thus far, as far as I can see. I hope this won't cause too much churn for the series! > /** > > Read a word from the device-specific I/O region of the Virtio Header. > @@ -319,6 +340,100 @@ EFI_STATUS > IN UINT8 DeviceStatus > ); > > +/** > + > + Allocates pages that are suitable for sharing between guest and hypervisor. > + > + @param This The protocol instance pointer. > + @param Pages The number of pages to allocate. > + @param HostAddress A pointer to store the base system memory > + address of the allocated range. > + > + @retval EFI_SUCCESS The requested memory pages were allocated. > + @retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated. > + > +**/ > +typedef > +EFI_STATUS > +(EFIAPI *VIRTIO_ALLOCATE_SHARED)( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN Pages, > + IN OUT VOID **HostAddress > + ); The typedef / function prototype looks good. Some comments for the comment block: (6) s/base system memory address/system memory base address/, I'd think; but I could be convinced otherwise (7) Please annotate each @param with: [in], [out], or [in,out], as appropriate (see other examples in the same header file). (This affects the rest of the members added in this patch. It also likely affects all three implementations of this set of members (via copy-pasted comment blocks).) > + > +/** > + Frees memory that was allocated with SharedAllocateBuffer(). (8) s/SharedAllocateBuffer()/VIRTIO_ALLOCATE_SHARED/ > + > + @param This The protocol instance pointer. > + @param Pages The number of pages to free. > + @param HostAddress The base system memory address of the allocated range. (9) Whatever you do for comment (6), please apply it here as well. > + > +**/ > +typedef > +VOID > +(EFIAPI *VIRTIO_FREE_SHARED)( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN Pages, > + IN VOID *HostAddress > + ); > + > +/** > + Provides the shared addresses required to access system memory from a > + DMA bus master. (10) what do you think of s/shared addresses/device address/, singular? (Also, replace "shared" with "virtio device" or just "device" below?) Just an idea. > + > + @param This The protocol instance pointer. > + @param Operation Indicates if the bus master is going to > + read or write to system memory. > + @param HostAddress The system memory address to map to shared > + buffer address. (11) Please point out the connection between VirtioOperationBusMasterCommonBuffer, VIRTIO_ALLOCATE_SHARED, and HostAddress. It doesn't have to be extremely verbose (feel free to steal and adapt the woring of the PciIo chapter in the UEFI spec), but since this is a protocol header file, we should say something about that connection. It's also fine if you just add a pointer to the UEFI spec (spec version, chapter/section, interface), and say "this interface follows the same usage pattern". > + @param NumberOfBytes On input the number of bytes to map. > + On output the number of bytes that were > + mapped. > + @param DeviceAddress The resulting shared map address for the > + bus master to access the hosts HostAddress. > + @param Mapping A resulting value to pass to Unmap(). (12) What do you think of: "a resulting token to pass to VIRTIO_UNMAP_SHARED"? > + > + > + @retval EFI_SUCCESS The range was mapped for the returned > + NumberOfBytes. > + @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. > + @retval EFI_DEVICE_ERROR The system hardware could not map the > + requested address. > +**/ > + > +typedef > +EFI_STATUS > +(EFIAPI *VIRTIO_MAP_SHARED) ( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN VIRTIO_MAP_OPERATION Operation, > + IN VOID *HostAddress, > + IN OUT UINTN *NumberOfBytes, > + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, > + OUT VOID **Mapping > + ); > + Looks OK. > +/** > + Completes the Map() operation and releases any corresponding resources. (13) What do you think of s/Map()/VIRTIO_MAP_SHARED/? > + > + @param This The protocol instance pointer. > + @param Mapping The mapping value returned from Map(). (14) I suggest "the Mapping token output by VIRTIO_MAP_SHARED". > + > + @retval EFI_SUCCESS The range was unmapped. > + @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by > + Map(). (15) s/value/token/ (16) Please remind the reader that the implementation is not required to recognize and report all such cases. Invalid Mapping tokens can cause undefined behavior. > + @retval EFI_DEVICE_ERROR The data was not committed to the target > + system memory. > +**/ > +typedef > +EFI_STATUS > +(EFIAPI *VIRTIO_UNMAP_SHARED)( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN VOID *Mapping > + ); OK. (In general I'm not sure if I like EFI_STATUS as return type here. I would prefer VOID, I think -- my guess is that the rest of the patch set will ignore the return value anywayy: - EFI_SUCCESS fits VOID obviously. - EFI_INVALID_PARAMETER is fully preventable if the caller sticks with the valid use pattern (and if they don't they can get undefined behavior anyway). The only reason I think we should still keep the EFI_STATUS return type on the interface level is that Unmap() might do actual data movement after BusMasterWrite. Before client code consumes such data, the client code should really check whether Unmap() succeeded (i.e., see the EFI_DEVICE_ERROR case). So, I guess let's keep the EFI_STATUS return type. (And check the returns status everywhere in this series, wherever appropriate.) > > /// > /// This protocol provides an abstraction over the VirtIo transport layer > @@ -353,6 +468,12 @@ struct _VIRTIO_DEVICE_PROTOCOL { > // Functions to read/write Device Specific headers > VIRTIO_DEVICE_WRITE WriteDevice; > VIRTIO_DEVICE_READ ReadDevice; > + > + // Function to allocate, free, map and unmap shared buffer (17) s/Function/Functions/, plural (18) I realize the current comments in this structure don't conform to the coding style. Can you perhaps prepend a patch that fixes the comment style for the following: // VirtIo Specification Revision: ... /// VirtIo Specification Revision encoded with VIRTIO_SPEC_REVISION() /// From the Virtio Spec // Functions to read/write Device Specific headers They should all be spelled // // comment // (There is no general need to prefix or suffix such comments with additional empty lines -- use your judgement. In actual code, the empty lines are helpful; between fields of a structure, not necessarily.) (19) and then please adapt the new comment in this patch. (20) This is actually for the commit message, but I'm too lazy to renumber my points :) In the commit message, please state that: We're free to extend the protocol structure without changing the protocol GUID, or bumping any protocol version fields (of which we currently have none), because VIRTIO_DEVICE_PROTOCOL is internal to edk2 by design -- see the disclaimers in "VirtioDevice.h". Thanks! Laszlo > + VIRTIO_ALLOCATE_SHARED AllocateSharedPages; > + VIRTIO_FREE_SHARED FreeSharedPages; > + VIRTIO_MAP_SHARED MapSharedBuffer; > + VIRTIO_UNMAP_SHARED UnmapSharedBuffer; > }; > > extern EFI_GUID gVirtioDeviceProtocolGuid; >