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 4CCAE21DFC871 for ; Wed, 16 Aug 2017 07:34:51 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BF841C053FC6; Wed, 16 Aug 2017 14:37:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com BF841C053FC6 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-59.phx2.redhat.com [10.3.116.59]) by smtp.corp.redhat.com (Postfix) with ESMTP id 07A92709F5; Wed, 16 Aug 2017 14:37:12 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1502710605-8058-1-git-send-email-brijesh.singh@amd.com> <1502710605-8058-7-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <817088c8-f655-4589-5a88-95d87d851b7d@redhat.com> Date: Wed, 16 Aug 2017 16:37:02 +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: <1502710605-8058-7-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 16 Aug 2017 14:37:16 +0000 (UTC) Subject: Re: [PATCH v2 06/23] OvmfPkg/Virtio: introduce IOMMU-like member functions to 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, 16 Aug 2017 14:34:51 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (1) In my previous review (msgid ) point (1), I suggested the following subject line: "OvmfPkg: introduce IOMMU-like member functions to VIRTIO_DEVICE_PROTOCOL" This subject was 72 characters long (within the 74 chars limit). Your current subject is: "OvmfPkg/Virtio: introduce IOMMU-like member functions to VIRTIO_DEVICE_PROTOCOL" It is too long (79 chars). So please drop the "/Virtio" string, as I requested. On 08/14/17 13:36, 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(). (2) You missed point (20) of my above-referenced v1 review. Again, please append the following to the commit message: --------- 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". --------- > > 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 | 143 ++++++++++++++++++++ > 1 file changed, 143 insertions(+) > > diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h > index edb20c18822c..14f980d7bf0a 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,25 @@ > > typedef struct _VIRTIO_DEVICE_PROTOCOL VIRTIO_DEVICE_PROTOCOL; > > +// > +// VIRTIO Operation for VIRTIO_MAP_SHARED > +// > +typedef enum { > + // > + // A read operation from system memory by a bus master > + // > + VirtioOperationBusMasterRead, > + // > + // A write operation to system memory by a bus master > + // > + VirtioOperationBusMasterWrite, > + // > + // Provides both read and write access to system memory by both the > + // processor and a bus master > + // > + VirtioOperationBusMasterCommonBuffer, > +} VIRTIO_MAP_OPERATION; > + > /** > > Read a word from the device-specific I/O region of the Virtio Header. > @@ -319,6 +339,121 @@ EFI_STATUS > IN UINT8 DeviceStatus > ); > > +/** > + > + Allocates pages that are suitable for an VirtioOperationBusMasterCommonBuffer > + mapping. This means that the buffer allocated by this function supports > + simultaneous access by both the processor and the bus master. The device > + address that the bus master uses to access the buffer must be retrieved with > + a call to VIRTIO_MAP_SHARED. > + > + @param[in] This The protocol instance pointer. > + > + @param[in] Pages The number of pages to allocate. > + > + @param[in,out] HostAddress A pointer to store the system memory base > + 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 > + ); > + > +/** > + Frees memory that was allocated with VIRTIO_ALLOCATE_SHARED. > + > + @param[in] This The protocol instance pointer. > + > + @param[in] Pages The number of pages to free. > + > + @param[in] HostAddress The system memory base address of the allocated > + range. > + > +**/ > +typedef > +VOID > +(EFIAPI *VIRTIO_FREE_SHARED)( > + IN VIRTIO_DEVICE_PROTOCOL *This, > + IN UINTN Pages, > + IN VOID *HostAddress > + ); > + > +/** > + 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) > + > + @param[in] This The protocol instance pointer. > + > + @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,out] NumberOfBytes On input the number of bytes to map. > + On output the number of bytes that were > + mapped. > + > + @param[out] DeviceAddress The resulting shared map address for the > + bus master to access the hosts HostAddress. > + > + @param[out] Mapping A resulting taken to pass to (3) s/taken/token/ The rest looks fine! Thanks, Laszlo > + 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 > + ); > + > +/** > + Completes the VIRTIO_MAP_SHARED operation and releases any corresponding > + resources. > + > + @param[in] This The protocol instance pointer. > + > + @param[in] Mapping The mapping token returned from > + VIRTIO_MAP_SHARED. > + > + @retval EFI_SUCCESS The range was unmapped. > + @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by > + VIRTIO_MAP_SHARED. Passing an invalid Mapping > + token 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 > + ); > > /** > > @@ -359,6 +494,14 @@ struct _VIRTIO_DEVICE_PROTOCOL { > // Functions to read/write Device Specific headers > VIRTIO_DEVICE_WRITE WriteDevice; > VIRTIO_DEVICE_READ ReadDevice; > + > + // > + // Functions to allocate, free, map and unmap shared buffer > + // > + VIRTIO_ALLOCATE_SHARED AllocateSharedPages; > + VIRTIO_FREE_SHARED FreeSharedPages; > + VIRTIO_MAP_SHARED MapSharedBuffer; > + VIRTIO_UNMAP_SHARED UnmapSharedBuffer; > }; > > extern EFI_GUID gVirtioDeviceProtocolGuid; >