Re: [PATCH v1 04/14] OvmfPkg/VirtioLib: Add SharedBuffer helper functions

[Laszlo Ersek <lersek@redhat.com>]

On 08/07/17 13:58, Brijesh Singh wrote:
> Patch adds convenience helper functions to call VIRTIO_DEVICE_PROTOCOL
> AllocateSharedPages, FreeSharedPages, MapSharedBuffer and UnmapSharedBuffer
> member functions.
> 
> 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   | 143 +++++++++++++
>  OvmfPkg/Library/VirtioLib/VirtioLib.c | 220 ++++++++++++++++++++
>  2 files changed, 363 insertions(+)

I prefer to add functions to VirtioLib that actually buy us something.
So let me evaluate that for each function:

> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index 5badfb32917f..fa82734f1852 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,146 @@ Virtio10WriteFeatures (
>    IN OUT UINT8                  *DeviceStatus
>    );
>  
> +/**
> +  Helper function to allocate pages that is suitable for sharing with
> +  hypervisor.
> +
> +  @param[in]  VirtIo  The target virtio device to use. It must be valid.
> +
> +  @param[in]  Pages   The number of pages to allocate.
> +
> +  @param[out] Buffer  A pointer to store the base system memory address of
> +                      the allocated range.
> +
> +  return              Returns error code from VirtIo->AllocateSharedPages()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioAllocateSharedPages (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  UINTN                   NumPages,
> +  OUT VOID                    **Buffer
> +  );
> +
> +/**
> +  Helper function to free pages allocated using VirtioAllocateSharedPages().
> +
> +  @param[in]  VirtIo  The target virtio device to use. It must be valid.
> +
> +  @param[in]  Pages   The number of allocated pages.
> +
> +  @param[in]  Buffer  System memory address allocated from
> +                      VirtioAllocateSharedPages ().
> +**/
> +VOID
> +EFIAPI
> +VirtioFreeSharedPages (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  UINTN                   NumPages,
> +  IN  VOID                    *Buffer
> +  );
> +
> +/**
> +  A helper function to map a system memory to a shared bus master memory for
> +  read operation from DMA bus master.
> +
> +  @param[in]  VirtIo          The target virtio device to use. It must be
> +                              valid.
> +
> +  @param[in]  HostAddress     The system memory address to map to shared bus
> +                              master address.
> +
> +  @param[in]  NumberOfBytes   Number of bytes to be mapped.
> +
> +  @param[out] DeviceAddress   The resulting shared map address for the bus
> +                              master to access the hosts HostAddress.
> +
> +  @param[out] Mapping         A resulting value to pass to Unmap().
> +
> +  return                      Returns error code from
> +                              VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferRead (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS    *DeviceAddress,
> +  OUT VOID                    **Mapping
> +  );
> +
> +/**
> +  A helper function to map a system memory to a shared bus master memory for
> +  write operation from DMA bus master.
> +
> +  @param[in]  VirtIo          The target virtio device to use. It must be
> +                              valid.
> +
> +  @param[in]  HostAddress     The system memory address to map to shared bus
> +                              master address.
> +
> +  @param[in]  NumberOfBytes   Number of bytes to be mapped.
> +
> +  @param[out] DeviceAddress   The resulting shared map address for the bus
> +                              master to access the hosts HostAddress.
> +
> +  @param[out] Mapping         A resulting value to pass to Unmap().
> +
> +  return                      Returns error code from
> +                              VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferWrite (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS    *DeviceAddress,
> +  OUT VOID                    **Mapping
> +  );
> +
> +/**
> +  A helper function to map a system memory to a shared bus master memory for
> +  common operation from DMA bus master.
> +
> +  @param[in]  VirtIo          The target virtio device to use. It must be
> +                              valid.
> +
> +  @param[in]  HostAddress     The system memory address to map to shared bus
> +                              master address.
> +
> +  @param[in]  NumberOfBytes   Number of bytes to be mapped.
> +
> +  @param[out] Mapping         A resulting value to pass to Unmap().
> +
> +  return                      Returns error code from
> +                              VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferCommon (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT VOID                    **Mapping
> +  );
> +
> +/**
> +  A helper function to unmap shared bus master memory mapped using Map().
> +
> +  @param[in]  VirtIo          The target virtio device to use. It must be
> +                              valid.
> +
> +  @param[in] Mapping          A mapping value return from Map().
> +
> +  return                      Returns error code from
> +                              VirtIo->UnmapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioUnmapSharedBuffer (
> +  IN VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN VOID                    *Mapping
> +  );
>  #endif // _VIRTIO_LIB_H_
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 845f206369a3..f6b464b6cdf8 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,222 @@ Virtio10WriteFeatures (
>  
>    return Status;
>  }
> +
> +/**
> +  Helper function to allocate pages that is suitable for sharing with
> +  hypervisor.
> +
> +  @param[in]  VirtIo  The target virtio device to use. It must be valid.
> +
> +  @param[in]  Pages   The number of pages to allocate.
> +
> +  @param[out] Buffer  A pointer to store the base system memory address of
> +                      the allocated range.
> +
> +  return              Returns error code from VirtIo->AllocateSharedPages()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioAllocateSharedPages (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  UINTN                   NumPages,
> +  OUT VOID                    **Buffer
> +  )
> +{
> +  return VirtIo->AllocateSharedPages (VirtIo, NumPages, Buffer);
> +}

(1) I think this function buys us nothing. I suggest to drop it.

> +
> +/**
> +  Helper function to free pages allocated using VirtioAllocateSharedPages().
> +
> +  @param[in]  VirtIo  The target virtio device to use. It must be valid.
> +
> +  @param[in]  Pages   The number of allocated pages.
> +
> +  @param[in]  Buffer  System memory address allocated from
> +                      VirtioAllocateSharedPages ().
> +**/
> +VOID
> +EFIAPI
> +VirtioFreeSharedPages (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  UINTN                   NumPages,
> +  IN  VOID                    *Buffer
> +  )
> +{
> +  VirtIo->FreeSharedPages (VirtIo, NumPages, Buffer);
> +}

(2) Same here.

> +
> +STATIC
> +EFI_STATUS
> +VirtioMapSharedBuffer (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VIRTIO_MAP_OPERATION    Operation,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS    *DeviceAddress,
> +  OUT VOID                    **Mapping
> +  )

(3) Please add a comment block above the function. Basically you can
copy the MapSharedBuffer() documentation, just point out the extra logic
for NumberOfBytes.

In fact, please rename this function to:

  VirtioMapAllBytesInSharedBuffer

> +{
> +  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;
> +}

Now, I think this helper function does actually buy us convenience -- it
helps us centralize error handling, and it allows callers to pass in
constants or other evaluated expressions for NumberOfBytes.

I've given quite a bit of thought to our "hardliner" handling of the
case when NumberOfBytes is decreased on output. The PciIo spec says that
in such cases the transfer should be broken up into smaller chunks:

> In all mapping requests the resulting NumberOfBytes actually mapped
> may be less than the requested amount. In this case, the DMA operation
> will have to be broken up into smaller chunks. The Map() function will
> map as much of the DMA operation as it can at one time. The caller may
> have to loop on Map() and Unmap() in order to complete a large DMA
> transfer.

While generally speaking that could be a valid idea, I think it doesn't
apply well to virtio, for the following reasons:

- I don't think that CommonBuffer areas are possible to split up in any
sensible way (e.g., rings),

- Even for unidirectional transfers, the request framing is usually
dictated on the virtio request level (i.e., request framing is usually
matched closely by the individual virtio descriptors that constitute the
descriptor chain that *is* the virtio request). If PciIo (ultimately:
the IOMMU) "suggests" that we split the request into smaller chunks,
that could affect the descriptors / descriptor chaining too, and I
simply don't want to go there.

- Turning a single "outer request" (like EFI_BLOCK_IO_PROTOCOL read or
write request) into a series of "inner virtio requests" is a mess as
well -- what if you get a failure from the hypervisor mid-loop?

IMO, we should forward the outer (higher level) request as transparently
over virtio as possible. If the IOMMU limits that for whatever reason,
then we should fail the request immediately, and propagate the error to
the client of the higher level protocol.

So, IMO, the handling of NumberOfBytes is valid here, and this function
is useful.

(4) Please make this function extern (add it to the header file too).

> +
> +/**
> +  A helper function to map a system memory to a shared bus master memory for
> +  read operation from DMA bus master.
> +
> +  @param[in]  VirtIo          The target virtio device to use. It must be
> +                              valid.
> +
> +  @param[in]  HostAddress     The system memory address to map to shared bus
> +                              master address.
> +
> +  @param[in]  NumberOfBytes   Number of bytes to be mapped.
> +
> +  @param[out] DeviceAddress   The resulting shared map address for the bus
> +                              master to access the hosts HostAddress.
> +
> +  @param[out] Mapping         A resulting value to pass to Unmap().
> +
> +  return                      Returns error code from
> +                              VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferRead (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS    *DeviceAddress,
> +  OUT VOID                    **Mapping
> +  )
> +{
> +  return VirtioMapSharedBuffer (VirtIo, EfiVirtIoOperationBusMasterRead,
> +           HostAddress, NumberOfBytes, DeviceAddress, Mapping);
> +}

(5) I don't think this function buys us much. The only parameter you can
save at the call site is Operation, but the rest (together with the
saving of the return code) will require so many characters still that
you'll have to break it to multiple lines anyway. So I suggest to drop
this function.

> +
> +/**
> +  A helper function to map a system memory to a shared bus master memory for
> +  write operation from DMA bus master.
> +
> +  @param[in]  VirtIo          The target virtio device to use. It must be
> +                              valid.
> +
> +  @param[in]  HostAddress     The system memory address to map to shared bus
> +                              master address.
> +
> +  @param[in]  NumberOfBytes   Number of bytes to be mapped.
> +
> +  @param[out] DeviceAddress   The resulting shared map address for the bus
> +                              master to access the hosts HostAddress.
> +
> +  @param[out] Mapping         A resulting value to pass to Unmap().
> +
> +  return                      Returns error code from
> +                              VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferWrite (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT EFI_PHYSICAL_ADDRESS    *DeviceAddress,
> +  OUT VOID                    **Mapping
> +  )
> +{
> +  return VirtioMapSharedBuffer (VirtIo, EfiVirtIoOperationBusMasterWrite,
> +           HostAddress, NumberOfBytes, DeviceAddress, Mapping);
> +}

(6) Same as (5) here.

> +
> +/**
> +  A helper function to map a system memory to a shared bus master memory for
> +  common operation from DMA bus master.
> +
> +  @param[in]  VirtIo          The target virtio device to use. It must be
> +                              valid.
> +
> +  @param[in]  HostAddress     The system memory address to map to shared bus
> +                              master address.
> +
> +  @param[in]  NumberOfBytes   Number of bytes to be mapped.
> +
> +  @param[out] Mapping         A resulting value to pass to Unmap().
> +
> +  return                      Returns error code from
> +                              VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapSharedBufferCommon (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN  VOID                    *HostAddress,
> +  IN  UINTN                   NumberOfBytes,
> +  OUT VOID                    **Mapping
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  DeviceAddress;
> +
> +  Status = VirtioMapSharedBuffer (VirtIo,
> +             EfiVirtIoOperationBusMasterCommonBuffer, HostAddress,
> +             NumberOfBytes, &DeviceAddress, Mapping);
> +
> +  //
> +  // On Success, lets verify that DeviceAddress same as HostAddress
> +  //
> +  if (!EFI_ERROR (Status)) {
> +    ASSERT (DeviceAddress == (EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress);
> +  }
> +
> +  return Status;
> +}

So this function seems to save us two parameters:

(7) Operation -- I commented on that under (5).

(8) DeviceAddress -- I actually disagree with hiding that. In my
opinion, this is different from the NumberOfBytes question.

We can be inflexible with NumberOfBytes because the in-out NumberOfBytes
parameter of PciIo.Map() reflects an *accidental* trait of some IOMMUs
(namely that their map sizes may be limited). I think that in the virtio
scope we can safely say that we simply don't support IOMMU protocol
instances that can't accommodate our desired request framing.

(Case in point: versions of the virtio spec have presented limits on
virtio request sizes, and thus the virtio drivers already enforce
suitable (stricter) limits on the outer protocol interfaces. Search
"VirtioBlk.c" and "VirtioScsi.c" for SIZE_1GB, for example. Therefore
refusing oversized requests is nothing new, except the definition of
"oversized" might vary, dependent on IOMMU.)

However, DeviceAddress being (potentially) different from HostAddress is
the core idea (an *inherent* trait) of the IOMMU protocol. If we ever
implement a different IOMMU protocol -- e.g. for QEMU's vIOMMU --, I
wouldn't like this assert -- or function prototype -- to cause issues.

So, please drop this helper function, and update all call sites to pass
DeviceAddress to the virtio device.

(I'm aware that this might cause major churn, and more review work for
me, but after all, this is the entire point of the IOMMU abstraction.)

> +
> +/**
> +  A helper function to unmap shared bus master memory mapped using Map().
> +
> +  @param[in]  VirtIo          The target virtio device to use. It must be
> +                              valid.
> +
> +  @param[in] Mapping          A mapping value return from Map().
> +
> +  return                      Returns error code from
> +                              VirtIo->UnmapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioUnmapSharedBuffer (
> +  IN VIRTIO_DEVICE_PROTOCOL  *VirtIo,
> +  IN VOID                    *Mapping
> +  )
> +{
> +  return VirtIo->UnmapSharedBuffer (VirtIo, Mapping);
> +}
> 

(9) I think this function also doesn't buy us much, please drop it.

Thanks,
Laszlo