(1) In my previous review (msgid <[email protected]>) 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 <[email protected]> > Cc: Ard Biesheuvel <[email protected]> > Cc: Jordan Justen <[email protected]> > Cc: Tom Lendacky <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <[email protected]> > --- > 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.<BR> > + Copyright (c) 2017, AMD Inc, All rights reserved.<BR> > > 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; > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
