Re: [PATCH v3 05/23] OvmfPkg/VirtioLib: add VirtioMapAllBytesInSharedBuffer() helper function

[Laszlo Ersek <lersek@redhat.com>]

There are two trivial omissions in this patch:

On 08/23/17 14:22, 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 with centralizing error handling, and it allows the caller
> to pass in constant or other evaluated expressions for NumberOfBytes.
> 
> 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   | 52 ++++++++++++
>  OvmfPkg/Library/VirtioLib/VirtioLib.c | 85 ++++++++++++++++++++
>  2 files changed, 137 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index 5badfb32917f..9ec9b91b59bb 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,55 @@ Virtio10WriteFeatures (
>    IN OUT UINT8                  *DeviceStatus
>    );
>  
> +/**
> +  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)
> +
> +  The VirtioMapAllBytesInSharedBuffer() is similar to VIRTIO_MAP_SHARED
> +  with exception that NumberOfBytes is IN-only parameter. The function
> +  maps all the bytes specified in NumberOfBytes param in one consecutive
> +  range.
> +
> +  @param[in]     This             The virtio device for which the mapping is
> +                                  requested.

(1) The parameter is called "VirtIo", not "This". (In my previous
review, I proposed a replacement that covered both the parameter name
and the parameter documentation, and I think you updated only the
documentation.)

> +
> +  @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]     NumberOfBytes    Number of bytes to map.
> +
> +  @param[out]    DeviceAddress    The resulting shared map address for the
> +                                  bus master to access the hosts HostAddress.
> +
> +  @param[out]    Mapping          A resulting token to pass to
> +                                  VIRTIO_UNMAP_SHARED.
> +
> +
> +  @retval EFI_SUCCESS             The NumberOfBytes is succesfully mapped.
> +  @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. 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..c85cd8dba569 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,87 @@ Virtio10WriteFeatures (
>  
>    return Status;
>  }
> +
> +/**
> +  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)
> +
> +  The VirtioMapAllBytesInSharedBuffer() is similar to VIRTIO_MAP_SHARED
> +  with exception that NumberOfBytes is IN-only parameter. The function
> +  maps all the bytes specified in NumberOfBytes param in one consecutive
> +  range.
> +
> +  @param[in]     This             The virtio device for which the mapping is
> +                                  requested.

(2) Same as (1), the parameter is called "VirtIo", not "This".

My plan is to test and push the initial sequence of this series (if the
review progresses far enough without more serious remarks), so my plan
is to fix these up on commit myself.

With the above fixed (by you or by me; we'll see):

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


> +
> +  @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]     NumberOfBytes    Number of bytes to map.
> +
> +  @param[out]    DeviceAddress    The resulting shared map address for the
> +                                  bus master to access the hosts HostAddress.
> +
> +  @param[out]    Mapping          A resulting token to pass to
> +                                  VIRTIO_UNMAP_SHARED.
> +
> +
> +  @retval EFI_SUCCESS             The NumberOfBytes is succesfully mapped.
> +  @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. 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
> +  )
> +{
> +  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);
> +  return EFI_OUT_OF_RESOURCES;
> +}
>