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

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 
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 @@
+// 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

(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.
+  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

+  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

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

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.
+  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.
+  IN  VOID                      *Mapping
+  );


(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

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.

edk2-devel mailing list

Reply via email to