(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 <ler...@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.si...@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
> };
>
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
