Re: [PATCH v2 10/23] OvmfPkg/VirtioLib: add VirtioMapAllBytesInSharedBuffer() helper function

[Laszlo Ersek <lersek@redhat.com>]

On 08/14/17 13:36, Brijesh Singh wrote:
> The function can be used for mapping the system physical address to virtio
> device address using VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer (). The
> function helps centralizing error handling, and it allows caller to pass

(1) some grammar bikeshedding:

- s/helps centralizing/helps with centralizing/

- s/allows caller/allows the caller/

> in constant or other evaluated expression for NumberOfBytes.

- s/expression/expressions/

> 
> 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/Library/VirtioLib.h   | 44 +++++++++++
>  OvmfPkg/Library/VirtioLib/VirtioLib.c | 78 ++++++++++++++++++++
>  2 files changed, 122 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index 5badfb32917f..d8f24a5d68b1 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -3,6 +3,7 @@
>    Declarations of utility functions used by virtio device drivers.
>  
>    Copyright (C) 2012-2016, Red Hat, Inc.
> +  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
> @@ -235,4 +236,47 @@ Virtio10WriteFeatures (
>    IN OUT UINT8                  *DeviceStatus
>    );
>  
> +/**
> +  Provides the virtio device address required to access system memory from a
> +  DMA bus master.
> +

This part is slightly out of synch with the protocol definition itself
(i.e., with VIRTIO_MAP_SHARED): the UEFI spec reference is missing.

However, that's fine: this function diverges slightly from the spec --
that's exactly where the extra convenience comes from. So this is OK.

> +  @param[in]     This             The protocol instance pointer.
> +

(2) This should say instead:

  @param[in] VirtIo  The virtio device for which the mapping is
                     requested.

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

(3) This is wrong. In my v1 review
<http://mid.mail-archive.com/9e743ec8-f2fe-65f2-9594-4731d2481a88@redhat.com>,
point (3), I wrote,

    Basically you can copy the MapSharedBuffer() documentation, just
    point out the extra logic for NumberOfBytes.

That description is missing -- NumberOfBytes is an IN-only parameter
here, and it is interpreted as a hard requirement.

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

(4) s/taken/token/, as discussed before

> +
> +
> +  @retval EFI_SUCCESS             The range was mapped for the returned
> +                                  NumberOfBytes.

(5) This too should be adapted to our particular NumberOfBytes
interpretation.

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

(6) Please add, "This includes the case when NumberOfBytes bytes cannot
be mapped in one consecutive range."

> +  @retval EFI_DEVICE_ERROR        The system hardware could not map the
> +                                  requested address.
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapAllBytesInSharedBuffer (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VIRTIO_MAP_OPERATION    Operation,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS    *DeviceAddress,
> +  OUT VOID                    **Mapping
> +  );
>  #endif // _VIRTIO_LIB_H_
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 845f206369a3..ac0ae97f3692 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -4,6 +4,7 @@
>  
>    Copyright (C) 2012-2016, Red Hat, Inc.
>    Portion of Copyright (C) 2013, ARM Ltd.
> +  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
> @@ -414,3 +415,80 @@ Virtio10WriteFeatures (
>  
>    return Status;
>  }
> +
> +/**
> +  Provides the virtio device address required to access system memory from a
> +  DMA bus master.
> +
> +  @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
> +                                  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.
> +**/

(7) Please sync this with points (2) through (6) above.

> +EFI_STATUS
> +EFIAPI
> +VirtioMapAllBytesInSharedBuffer (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VIRTIO_MAP_OPERATION    Operation,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS    *DeviceAddress,
> +  OUT VOID                    **Mapping
> +  )
> +{
> +  EFI_STATUS            Status;
> +  VOID                  *MapInfo;
> +  UINTN                 Size;
> +  EFI_PHYSICAL_ADDRESS  PhysicalAddress;
> +
> +  Size = NumberOfBytes;
> +  Status = VirtIo->MapSharedBuffer (
> +                     VirtIo,
> +                     Operation,
> +                     HostAddress,
> +                     &Size,
> +                     &PhysicalAddress,
> +                     &MapInfo
> +                     );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (Size < NumberOfBytes) {
> +    goto Failed;
> +  }
> +
> +  *Mapping = MapInfo;
> +  *DeviceAddress = PhysicalAddress;
> +
> +  return EFI_SUCCESS;
> +
> +Failed:
> +  VirtIo->UnmapSharedBuffer (VirtIo, MapInfo);
> +  *Mapping = NULL;
> +  return EFI_OUT_OF_RESOURCES;
> +}
> 

(8) The logic differs slightly from the previous version: in this
version you set Mapping to NULL on error.

I liked the previous version more; IMO we should not touch output
parameters on error (that's why you introduced MapInfo earlier, to begin
with).

So I suggest dropping the NULL-ing on error.

Thanks
Laszlo