Re: [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support

[Brijesh Singh <brijesh.singh@amd.com>]

Hi Laszlo,


> 
> (5.3) All virtio driver code should treat VIRTIO_F_IOMMU_PLATFORM simply
> in parallel with VIRTIO_F_VERSION_1, and don't act upon
> VIRTIO_F_IOMMU_PLATFORM in any special shape or form. So basically this
> is just my point (3) from above.
> 
> 
> (5.4) There are three VIRTIO_DEVICE_PROTOCOL implementations in edk2:
> 
> - "OvmfPkg/VirtioPciDeviceDxe" binds legacy-only and transitional
>    virtio-pci devices, and offers virtio 0.9.5 semantics.
> 
> - "ArmVirtPkg/VirtioFdtDxe" (via "OvmfPkg/Library/VirtioMmioDeviceLib")
>    binds virtio-mmio devices, and offers virtio 0.9.5 semantics.
> 
> - "OvmfPkg/Virtio10Dxe" binds modern-only virtio-pci devices, and offers
>    virtio 1.0.0 semantics.
> 
> The first two drivers should implement the AllocateSharedPages() and
> FreeSharedPages() member functions simply with the corresponding
> MemoryAllocationLib functions (using BootServicesData type memory), and
> implement the MapSharedPages() and UnmapSharedPages() member functions
> as no-ops (return the input addresses transparently).
> 
> The third driver should implement all four new 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() -- with BusMasterCommonBuffer64 --
> - EFI_PCI_IO_PROTOCOL.Unmap()
> 

I have working to implement patch per your recommendation. I assume you mean
map the buffers with EfiPciIoOperationBusMasterCommonBuffer [1]. If so, I see
one issue with SEV guest. When SEV is active, IOMMU driver uses a bounce buffer
to map host address to a device address. While creating bounce buffer we can map
it either for EfiPciIoOperationBusMasterRead or EfiPciIoOperationBusMasterWrite
Operation. If caller wants to map EfiPciIoOperationBusMasterCommonBuffer then it
must allocate the buffer using EFI_PCI_IO_PROTOCOL.AllocateBuffer() [2] otherwise
we will fail to map. I see that PciRootBridgeIo.c has similar check when using
a bounce buffer for < 4GB use cases [3].

Do you see any issue if we use EfiPciIoOperationBusMasterRead or EfiPciIoOperationBusMasterWrite
instead of EfiPciIoOperationBusMasterCommonBuffer ?

[1] https://github.com/tianocore/edk2/blob/master/EdkCompatibilityPkg/Foundation/Efi/Protocol/PciIo/PciIo.h#L169
[2] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c#L1082
[3] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c#L1109

> The EFI_PCI_IO_PROTOCOL implementation will delegate these calls to the
> platform-specific PCI host bridge / root bridge driver, and *that*
> driver in turn is allowed to talk to an IOMMU protocol (if any).
> 
> (This last step is already covered by the following edk2 commits:
> - generally, c15da8eb3587 ("MdeModulePkg/PciHostBridge: Add IOMMU
> support.", 2017-04-29),
> - specifically for SEV in OVMF, c6ab9aecb71b ("OvmfPkg: update
> PciHostBridgeDxe to use PlatformHasIoMmuLib", 2017-07-06).)
> 
>