Re: [edk2] [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL
On 08/09/2017 09:27 AM, Laszlo Ersek wrote:
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.
Will do.
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().
Will do
+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/
Will update it. thanks
+ //
+ // 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!
I was actually trying to follow the IOMMU protocol header file, but I will
drop the Efi Prefix and also fix the "Virtio" prefix.
Drivers don't use these macro's directly, I have a helper function in Patch 4
to map the shared buffers. Depending on your feedback on Patch #4, it may not
be bad change at all. If we don't want those helper functions from patch 4 then
I will anyway have to update the drivers. In summary, I will make the suggested
changes in v2.
/**
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 UINTNPages,
+ 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).)
I will fix those, I was doing copy/paste of these functions from
MdeModules/Include/Protocol/IoMmu.h hence missed updating the annotate of
each param.
+
+/**
+ Frees memory that was allocated with SharedAllocateBuffer().
(8) s/SharedAllocateBuffer()/VIRTIO_ALLOCATE_SHARED/
Will do thanks
+
+ @param This The protocol instance pointer.
+ @param Pag
Re: [edk2] [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL
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 UINTNPages,
> + 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 HostAddressThe 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 UINTNPages,
> + IN VOID *HostAddress
> + );
> +
> +/**
> + Provides the shared addresses required to access system memory from a
> + DMA bus master.
(10) what do you think of s/shar
[edk2] [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL
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(+)
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
+//
+typedef enum {
+ //
+ // A read operation from system memory by a bus master
+ //
+ EfiVirtIoOperationBusMasterRead,
+ //
+ // A write operation from system memory by a bus master
+ //
+ EfiVirtIoOperationBusMasterWrite,
+ //
+ // Provides both read and write access to system memory by both the
+ // processor and a bus master
+ //
+ EfiVirtIoOperationBusMasterCommonBuffer,
+ EfiVirtIoOperationMaximum
+} VIRTIO_MAP_OPERATION;
+
/**
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 UINTNPages,
+ IN OUT VOID **HostAddress
+ );
+
+/**
+ Frees memory that was allocated with SharedAllocateBuffer().
+
+ @param This The protocol instance pointer.
+ @param Pages The number of pages to free.
+ @param HostAddressThe base system memory address of the allocated range.
+
+**/
+typedef
+VOID
+(EFIAPI *VIRTIO_FREE_SHARED)(
+ IN VIRTIO_DEVICE_PROTOCOL *This,
+ IN UINTNPages,
+ IN VOID *HostAddress
+ );
+
+/**
+ Provides the shared addresses required to access system memory from a
+ DMA bus master.
+
+ @param ThisThe 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.
+ @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().
+
+
+ @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_RESOURCESThe request could not be completed due to
+ a lack of resources.
+ @retval EFI_DEVICE_ERRORThe 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
