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 <ler...@redhat.com>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Tom Lendacky <thomas.lenda...@amd.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
---
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.<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,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 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).)
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 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.
Will do
+
+**/
+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.
device address works for me, I will update it.
+
+ @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".
Again this was basically copy/paste from IoMmu.h hence I did not put anything
regarding the VirtioOperationBusMasterCommonBuffer, I will mention the UEFI
spec version and section.
+ @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"?
Works for me. thanks
+
+
+ @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/?
Works for me
+
+ @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".
Will do
+
+ @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/
Will do
(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.
Will do
+ @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.)
Okay lets make sure we check the status when required
///
/// 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
Will fix it
(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:
Sure will do
// 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".
Sure, I will use the message in commit.
Thanks
Brijesh
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
