Re: [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL

[Laszlo Ersek <lersek@redhat.com>]

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 <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@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.

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

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

> +
> +/**
> +  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  HostAddress    The 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  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.

> +
> +  @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".

> +  @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"?

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

> +
> +  @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".

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

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

> +  @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.)

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

(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:

// 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".

Thanks!
Laszlo

> +  VIRTIO_ALLOCATE_SHARED      AllocateSharedPages;
> +  VIRTIO_FREE_SHARED          FreeSharedPages;
> +  VIRTIO_MAP_SHARED           MapSharedBuffer;
> +  VIRTIO_UNMAP_SHARED         UnmapSharedBuffer;
>  };
>  
>  extern EFI_GUID gVirtioDeviceProtocolGuid;
>