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

[Brijesh Singh <brijesh.singh@amd.com>]

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


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