Re: [PATCH v1 02/14] OvmfPkg/Virtio10Dxe: Implement new member functions

[Laszlo Ersek <lersek@redhat.com>]

(1) For the subject, I suggest again

    OvmfPkg/Virtio10Dxe: Implement IOMMU-like member functions

On 08/07/17 13:58, Brijesh Singh wrote:
> The patch implements the newly added member functions by respectively
> delegating the job to:
>
> - EFI_PCI_IO_PROTOCOL.AllocateBuffer() -- with BootServicesData
> - EFI_PCI_IO_PROTOCOL.FreeBuffer()
> - EFI_PCI_IO_PROTOCOL.Map()
> - EFI_PCI_IO_PROTOCOL.Unmap()

(2) looks good, but please spell out the new virtio protocol member
    names as well. (This will become more important in the commit
    message of the next patch, where the reader has to guess what member
    functions exactly get the "no-op" implementation.)

    And then we should be consistent in the commit messages.

>
> 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/Virtio10Dxe/Virtio10.c | 114 +++++++++++++++++++-
>  1 file changed, 113 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c
> index a8a6a58c3f1d..5bc8f1c7ee27 100644
> --- a/OvmfPkg/Virtio10Dxe/Virtio10.c
> +++ b/OvmfPkg/Virtio10Dxe/Virtio10.c
> @@ -2,6 +2,7 @@
>    A non-transitional driver for VirtIo 1.0 PCI devices.
>
>    Copyright (C) 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
> @@ -772,6 +773,113 @@ Virtio10ReadDevice (
>    return Status;
>  }
>
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10AllocateSharedPages (
> +  IN     VIRTIO_DEVICE_PROTOCOL  *This,
> +  IN     UINTN                   Pages,
> +  IN OUT VOID                    **HostAddress
> +  )
> +{
> +  VIRTIO_1_0_DEV *Dev;
> +  EFI_STATUS     Status;
> +
> +  Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> +  Status = Dev->PciIo->AllocateBuffer (
> +                         Dev->PciIo,
> +                         AllocateAnyPages,
> +                         EfiBootServicesData,
> +                         Pages,
> +                         HostAddress,
> +                         0

(3) We should pass Attributes=EFI_PCI_ATTRIBUTE_MEMORY_CACHED, even if
    only for cosmetic purposes (at the moment).

(4) Independently, this makes me think about
    EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE.

    * PciIoAllocateBuffer() [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c]
      sets the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute
      internally, before calling out to the PCI Root Bridge IO Protocol,
      *if* "PciIoDevice->Attributes" has
      EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE set.

      In turn:

      - If there is an IOMMU protocol in the system, then
        RootBridgeIoAllocateBuffer()
        [MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c] passes
        EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE through to the IOMMU
        protocol's AllocateBuffer() member *if* "RootBridge->DmaAbove4G"
        is set. Otherwise the DUAL_ADDRESS_CYCLE hint is cleared.

        (EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE has the same numeric
        value, 0x8000, as EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE.)

      - If there is no IOMMU protocol in the system, then
        RootBridgeIoAllocateBuffer() itself allocates memory, again
        obeying  EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE and
        "RootBridge->DmaAbove4G".

      So regardless of whether there is an IOMMU protocol in the system
      or not, on the PCI Root Bridge IO level, 64-bit AllocateBuffer()
      is be permitted according to device-level DUAL_ADDRESS_CYCLE *AND*
      root-bridge-level DmaAbove4G.

    * Let's consider what's going to happen if we convert the virtio
      drivers to the new virtio proto member functions, and (in
      addition) Virtio10Dxe is converted to explicit
      PciIo.AllocateBuffer():

      - In OvmfPkg, allocations that used to be plain AllocatePool() or
        AllocatePages(), will be restricted to 32-bit, due to two
        reasons (either of which is sufficient in this regard):

        - In "OvmfPkg/Library/PciHostBridgeLib", we clear DmaAbove4G,
          for hysterical raisins.

        - In Virtio10Dxe, we don't set
          EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE.

      - In ArmVirtPkg, allocations that used to be plain AllocatePool()
        or AllocatePages(), will also be restricted to 32-bit. While
        "ArmVirtPkg/Library/FdtPciHostBridgeLib" *sets* DmaAbove4G, lack
        of EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in Virtio10Dxe alone
        is suffices to trigger the restriction.

    * Therefore, please extend this patch with the following: in the
      Virtio10BindingStart() function [OvmfPkg/Virtio10Dxe/Virtio10.c],
      please replace

        SetAttributes = 0;

      with

        SetAttributes = EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE;

      It will not help OvmfPkg, but it will keep the 4GB restriction out
      of ArmVirtPkg. (I'm unsure if we can simply flip "DmaAbove4G" to
      TRUE in OvmfPkg.)

    * (In the past, Ard wrote a number of similar patches for physical
      device drivers:

        167c3fb45674 MdeModulePkg/EhciDxe: enable 64-bit PCI DMA
        4e28ea2c29e0 MdeModulePkg/NvmExpressDxe: enable 64-bit PCI DMA
        df0a0e4b6fae MdeModulePkg/SdMmcPciHcDxe: enable 64-bit PCI DMA
        5c1b371a8839 MdeModulePkg/XhciDxe: enable 64-bit PCI DMA
        e58a71d9c50b MdeModulePkg/PciHostBridgeDxe: restrict 64-bit DMA
                     to devices that support it
        4c0b2d25c61c ArmVirtPkg/FdtPciHostBridgeLib: enable 64-bit PCI
                     DMA

      and that's what we should do here, in separate patches, for the
      virtio-pci transport drivers.)

(5) Looking more at the PciIo attribute setting in both virtio-PCI
    transport drivers, it seems that I failed to set
    EFI_PCI_IO_ATTRIBUTE_BUS_MASTER in both. Virtio devices read and
    write guest RAM (they don't just decode their IO and/or MMIO BARs),
    which translates to "bus master".

    So, please clean up my mess :) , and prepend two separate patches to
    the series:

    - "OvmfPkg/VirtioPciDeviceDxe: supply missing BUS_MASTER attribute"

      Please change

        EFI_PCI_IO_ATTRIBUTE_IO

      to

        EFI_PCI_IO_ATTRIBUTE_IO | EFI_PCI_IO_ATTRIBUTE_BUS_MASTER

      in VirtioPciDeviceBindingStart()
      [OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c].

    - "OvmfPkg/Virtio10Dxe: supply missing BUS_MASTER attribute"

      Please change

        SetAttributes = 0;

      to

        SetAttributes = EFI_PCI_IO_ATTRIBUTE_BUS_MASTER;

      in Virtio10BindingStart() [OvmfPkg/Virtio10Dxe/Virtio10.c].

      (And then do point (4) on top of that, separately.)

Okay, summary of points (3), (4) and (5):

- please prepend *two* patches, setting EFI_PCI_IO_ATTRIBUTE_BUS_MASTER
  in each virtio-pci transport driver -- point (5)

- please set EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE in the modern-only
  driver, in this patch -- point (4)

- please pass Attributes=EFI_PCI_ATTRIBUTE_MEMORY_CACHED to
  Dev->PciIo->AllocateBuffer(), in this patch -- point (3).

Phew, this is complex. :)

> +                         );
> +  return Status;
> +}
> +
> +STATIC
> +VOID
> +EFIAPI
> +Virtio10FreeSharedPages (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *This,
> +  IN  UINTN                   Pages,
> +  IN  VOID                    *HostAddress
> +  )
> +{
> +  VIRTIO_1_0_DEV *Dev;
> +
> +  Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> +  Dev->PciIo->FreeBuffer (
> +                Dev->PciIo,
> +                Pages,
> +                HostAddress
> +                );
> +}

Looks OK.

> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10MapSharedBuffer (
> +  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
> +  )
> +{
> +  EFI_STATUS                    Status;
> +  VIRTIO_1_0_DEV                *Dev;
> +  EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation;
> +
> +  Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> +  //
> +  // Map VIRTIO_MAP_OPERATION to EFI_PCI_IO_PROTOCOL_OPERATION
> +  //
> +  if (Operation == EfiVirtIoOperationBusMasterRead) {
> +    PciIoOperation = EfiPciIoOperationBusMasterRead;
> +  } else if (Operation == EfiVirtIoOperationBusMasterWrite) {
> +    PciIoOperation = EfiPciIoOperationBusMasterWrite;
> +  } else if (Operation == EfiVirtIoOperationBusMasterCommonBuffer) {
> +    PciIoOperation = EfiPciIoOperationBusMasterCommonBuffer;
> +  } else {
> +    return EFI_UNSUPPORTED;
> +  }

(6) This mapping lends itself to simplification by virtue of a switch
    statement. (Please remember that the "case" labels are indented
    *zero* positions relative to the "switch" keyword.)

(7) Rather than EFI_UNSUPPORTED, EFI_INVALID_PARAMETER should be
    returned ("One or more parameters are invalid.")

(8) You don't seem to be using "VirtioOperationMaximum" for range
    checking (that's fine, with the switch we can check each value
    individually, and then add a "default" case) -- please consider
    removing VirtioOperationMaximum from the protocol interface then.


Regarding 64-bit transparency: this code is good, as long as we cover
point (4) above:

- EFI_PCI_IO_PROTOCOL_OPERATION has no separate 32-bit and 64-bit
  constants,

- EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION does make such a
  distinction,

- PciIoMap() in "MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c" shifts the
  former op constants up to the 64-bit latter op constants, if
  EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE is enabled for the device
  (hence my reference to (4)).

The rest also looks good.

Thanks,
Laszlo

> +
> +  Status = Dev->PciIo->Map (
> +                         Dev->PciIo,
> +                         PciIoOperation,
> +                         HostAddress,
> +                         NumberOfBytes,
> +                         DeviceAddress,
> +                         Mapping
> +                         );
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +Virtio10UnmapSharedBuffer (
> +  IN  VIRTIO_DEVICE_PROTOCOL  *This,
> +  IN  VOID                    *Mapping
> +  )
> +{
> +  EFI_STATUS      Status;
> +  VIRTIO_1_0_DEV  *Dev;
> +
> +  Dev = VIRTIO_1_0_FROM_VIRTIO_DEVICE (This);
> +
> +  Status = Dev->PciIo->Unmap (
> +                         Dev->PciIo,
> +                         Mapping
> +                         );
> +
> +  return Status;
> +}
>
>  STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
>    VIRTIO_SPEC_REVISION (1, 0, 0),
> @@ -788,7 +896,11 @@ STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
>    Virtio10GetDeviceStatus,
>    Virtio10SetDeviceStatus,
>    Virtio10WriteDevice,
> -  Virtio10ReadDevice
> +  Virtio10ReadDevice,
> +  Virtio10AllocateSharedPages,
> +  Virtio10FreeSharedPages,
> +  Virtio10MapSharedBuffer,
> +  Virtio10UnmapSharedBuffer
>  };
>
>
>