Re: [PATCH v2 07/23] OvmfPkg/Virtio10Dxe: implement IOMMU-like member functions

[Laszlo Ersek <lersek@redhat.com>]

On 08/14/17 13:36, Brijesh Singh wrote:
> The patch implements the newly added IOMMU-like member functions by
> respectively delegating the job to:
> 
> - VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() ->
>     EFI_PCI_IO_PROTOCOL.AllocateBuffer()
> 
> - VIRTIO_DEVICE_PROTOCOL.FreeSharedPages() ->
>     EFI_PCI_IO_PROTOCOL.FreeBuffer()
> 
> - VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() ->
>     EFI_PCI_IO_PROTOCOL.Map()
> 
> - VIRTIO_DEVICE_PROTOCOL.UnmapSharedBuffer() ->
>     EFI_PCI_IO_PROTOCOL.Unmap()
> 
> 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 | 121 +++++++++++++++++++-
>  1 file changed, 119 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c
> index 43dcfd7cb2a4..413ffa06cf35 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
> @@ -15,6 +16,7 @@
>  #include <IndustryStandard/Pci.h>
>  #include <IndustryStandard/Virtio.h>
>  #include <Protocol/PciIo.h>
> +#include <Protocol/PciRootBridgeIo.h>
>  #include <Protocol/VirtioDevice.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
> @@ -772,6 +774,117 @@ 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,
> +                         EFI_PCI_ATTRIBUTE_MEMORY_CACHED
> +                         );
> +  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
> +                );
> +}
> +
> +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
> +  //
> +  switch (Operation) {
> +  case VirtioOperationBusMasterRead:
> +    PciIoOperation = EfiPciIoOperationBusMasterRead;
> +    break;
> +  case VirtioOperationBusMasterWrite:
> +    PciIoOperation = EfiPciIoOperationBusMasterWrite;
> +    break;
> +  case VirtioOperationBusMasterCommonBuffer:
> +    PciIoOperation = EfiPciIoOperationBusMasterCommonBuffer;
> +    break;
> +  default:
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  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 +901,11 @@ STATIC CONST VIRTIO_DEVICE_PROTOCOL mVirtIoTemplate = {
>    Virtio10GetDeviceStatus,
>    Virtio10SetDeviceStatus,
>    Virtio10WriteDevice,
> -  Virtio10ReadDevice
> +  Virtio10ReadDevice,
> +  Virtio10AllocateSharedPages,
> +  Virtio10FreeSharedPages,
> +  Virtio10MapSharedBuffer,
> +  Virtio10UnmapSharedBuffer
>  };
>  
>  
> @@ -906,7 +1023,7 @@ Virtio10BindingStart (
>      goto ClosePciIo;
>    }
>  
> -  SetAttributes =  EFI_PCI_IO_ATTRIBUTE_BUS_MASTER;
> +  SetAttributes =  EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE;
>    UpdateAttributes (&Device->CommonConfig, &SetAttributes);
>    UpdateAttributes (&Device->NotifyConfig, &SetAttributes);
>    UpdateAttributes (&Device->SpecificConfig, &SetAttributes);
> 

(1) This patch is almost complete in my opinion, except the last hunk is
incorrect -- I think it may be due to an incorrect conflict resolution
during rebase.

Namely, we shouldn't assign "SetAttributes" the value

  EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE

but the value

  (EFI_PCI_IO_ATTRIBUTE_BUS_MASTER |
   EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE)

IOW, adding EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE is good, but
EFI_PCI_IO_ATTRIBUTE_BUS_MASTER should be preserved too.

Please refer to the following sentence in my v1 review
<http://mid.mail-archive.com/659fe2f7-0d17-cde7-c3d3-36e541ff7805@redhat.com>,
point (5):

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

Thank you!
Laszlo