Re: [PATCH v2 06/23] OvmfPkg/Virtio: introduce IOMMU-like member functions to VIRTIO_DEVICE_PROTOCOL

[Laszlo Ersek <lersek@redhat.com>]

(1) In my previous review (msgid
<f953ec03-b160-726b-b9ed-5e3122642815@redhat.com>) 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 <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 | 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;
>