Add a driver for the ACPI VIOT table, which provides topology
information for para-virtual IOMMUs. Enable virtio-iommu on
non-devicetree platforms, including x86.
Since v3 [1] I fixed a build bug for !CONFIG_IOMMU_API. Joerg offered to
take this series through the IOMMU tree, which requires Acks for patches
1-3.
You can find a QEMU implementation at [2], with extra support for
testing all VIOT nodes including MMIO-based endpoints and IOMMU.
This series is at [3].
[1] https://lore.kernel.org/linux-iommu/20210602154444.1077006-1-jean-philippe@linaro.org/
[2] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/acpi
[3] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/acpi
Jean-Philippe Brucker (6):
ACPI: arm64: Move DMA setup operations out of IORT
ACPI: Move IOMMU setup code out of IORT
ACPI: Add driver for the VIOT table
iommu/dma: Pass address limit rather than size to
iommu_setup_dma_ops()
iommu/dma: Simplify calls to iommu_setup_dma_ops()
iommu/virtio: Enable x86 support
drivers/acpi/Kconfig | 3 +
drivers/iommu/Kconfig | 4 +-
drivers/acpi/Makefile | 2 +
drivers/acpi/arm64/Makefile | 1 +
include/acpi/acpi_bus.h | 3 +
include/linux/acpi.h | 3 +
include/linux/acpi_iort.h | 14 +-
include/linux/acpi_viot.h | 19 ++
include/linux/dma-iommu.h | 4 +-
arch/arm64/mm/dma-mapping.c | 2 +-
drivers/acpi/arm64/dma.c | 50 +++++
drivers/acpi/arm64/iort.c | 129 ++-----------
drivers/acpi/bus.c | 2 +
drivers/acpi/scan.c | 78 +++++++-
drivers/acpi/viot.c | 364 +++++++++++++++++++++++++++++++++++
drivers/iommu/amd/iommu.c | 9 +-
drivers/iommu/dma-iommu.c | 17 +-
drivers/iommu/intel/iommu.c | 10 +-
drivers/iommu/virtio-iommu.c | 8 +
MAINTAINERS | 8 +
20 files changed, 580 insertions(+), 150 deletions(-)
create mode 100644 include/linux/acpi_viot.h
create mode 100644 drivers/acpi/arm64/dma.c
create mode 100644 drivers/acpi/viot.c
--
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
The ACPI Virtual I/O Translation Table describes topology of
para-virtual platforms, similarly to vendor tables DMAR, IVRS and IORT.
For now it describes the relation between virtio-iommu and the endpoints
it manages.
Three steps are needed to configure DMA of endpoints:
(1) acpi_viot_init(): parse the VIOT table, find or create the fwnode
associated to each vIOMMU device.
(2) When probing the vIOMMU device, the driver registers its IOMMU ops
within the IOMMU subsystem. This step doesn't require any
intervention from the VIOT driver.
(3) viot_iommu_configure(): before binding the endpoint to a driver,
find the associated IOMMU ops. Register them, along with the
endpoint ID, into the device's iommu_fwspec.
If step (3) happens before step (2), it is deferred until the IOMMU is
initialized, then retried.
Signed-off-by: Jean-Philippe Brucker <redacted>
---
drivers/acpi/Kconfig | 3 +
drivers/iommu/Kconfig | 1 +
drivers/acpi/Makefile | 2 +
include/linux/acpi_viot.h | 19 ++
drivers/acpi/bus.c | 2 +
drivers/acpi/scan.c | 3 +
drivers/acpi/viot.c | 364 ++++++++++++++++++++++++++++++++++++++
MAINTAINERS | 8 +
8 files changed, 402 insertions(+)
create mode 100644 include/linux/acpi_viot.h
create mode 100644 drivers/acpi/viot.c
Extract generic DMA setup code out of IORT, so it can be reused by VIOT.
Keep it in drivers/acpi/arm64 for now, since it could break x86
platforms that haven't run this code so far, if they have invalid
tables.
Signed-off-by: Jean-Philippe Brucker <redacted>
---
drivers/acpi/arm64/Makefile | 1 +
include/linux/acpi.h | 3 +++
include/linux/acpi_iort.h | 6 ++---
drivers/acpi/arm64/dma.c | 50 ++++++++++++++++++++++++++++++++++
drivers/acpi/arm64/iort.c | 54 ++++++-------------------------------
drivers/acpi/scan.c | 2 +-
6 files changed, 66 insertions(+), 50 deletions(-)
create mode 100644 drivers/acpi/arm64/dma.c
Extract the code that sets up the IOMMU infrastructure from IORT, since
it can be reused by VIOT. Move it one level up into a new
acpi_iommu_configure_id() function, which calls the IORT parsing
function which in turn calls the acpi_iommu_fwspec_init() helper.
Signed-off-by: Jean-Philippe Brucker <redacted>
---
include/acpi/acpi_bus.h | 3 ++
include/linux/acpi_iort.h | 8 ++---
drivers/acpi/arm64/iort.c | 75 +++++----------------------------------
drivers/acpi/scan.c | 73 ++++++++++++++++++++++++++++++++++++-
4 files changed, 87 insertions(+), 72 deletions(-)
Passing a 64-bit address width to iommu_setup_dma_ops() is valid on
virtual platforms, but isn't currently possible. The overflow check in
iommu_dma_init_domain() prevents this even when @dma_base isn't 0. Pass
a limit address instead of a size, so callers don't have to fake a size
to work around the check.
Signed-off-by: Jean-Philippe Brucker <redacted>
---
include/linux/dma-iommu.h | 4 ++--
arch/arm64/mm/dma-mapping.c | 2 +-
drivers/iommu/amd/iommu.c | 2 +-
drivers/iommu/dma-iommu.c | 12 ++++++------
drivers/iommu/intel/iommu.c | 2 +-
5 files changed, 11 insertions(+), 11 deletions(-)
@@ -1713,7 +1713,7 @@ static void amd_iommu_probe_finalize(struct device *dev)/* Domains are initialized for this device - have a look what we ended up with */domain=iommu_get_domain_for_dev(dev);if(domain->type==IOMMU_DOMAIN_DMA)-iommu_setup_dma_ops(dev,IOVA_START_PFN<<PAGE_SHIFT,0);+iommu_setup_dma_ops(dev,IOVA_START_PFN<<PAGE_SHIFT,U64_MAX);elseset_dma_ops(dev,NULL);}
@@ -346,7 +346,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,/* Check the domain allows at least some access to the device... */if(domain->geometry.force_aperture){if(base>domain->geometry.aperture_end||-base+size<=domain->geometry.aperture_start){+limit<domain->geometry.aperture_start){pr_warn("specified DMA range outside IOMMU capability\n");return-EFAULT;}
dma-iommu uses the address bounds described in domain->geometry during
IOVA allocation. The address size parameters of iommu_setup_dma_ops()
are useful for describing additional limits set by the platform
firmware, but aren't needed for drivers that call this function from
probe_finalize(). The base parameter can be zero because dma-iommu
already removes the first IOVA page, and the limit parameter can be
U64_MAX because it's only checked against the domain geometry. Simplify
calls to iommu_setup_dma_ops().
Signed-off-by: Jean-Philippe Brucker <redacted>
---
drivers/iommu/amd/iommu.c | 9 +--------
drivers/iommu/dma-iommu.c | 4 +++-
drivers/iommu/intel/iommu.c | 10 +---------
3 files changed, 5 insertions(+), 18 deletions(-)
@@ -1708,14 +1708,7 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)staticvoidamd_iommu_probe_finalize(structdevice*dev){-structiommu_domain*domain;--/* Domains are initialized for this device - have a look what we ended up with */-domain=iommu_get_domain_for_dev(dev);-if(domain->type==IOMMU_DOMAIN_DMA)-iommu_setup_dma_ops(dev,IOVA_START_PFN<<PAGE_SHIFT,U64_MAX);-else-set_dma_ops(dev,NULL);+iommu_setup_dma_ops(dev,0,U64_MAX);}staticvoidamd_iommu_release_device(structdevice*dev)
With the VIOT support in place, x86 platforms can now use the
virtio-iommu.
Because the other x86 IOMMU drivers aren't yet ready to use the
acpi_dma_setup() path, x86 doesn't implement arch_setup_dma_ops() at the
moment. Similarly to Vt-d and AMD IOMMU, call iommu_setup_dma_ops() from
probe_finalize().
Acked-by: Joerg Roedel <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jean-Philippe Brucker <redacted>
---
drivers/iommu/Kconfig | 3 ++-
drivers/iommu/dma-iommu.c | 1 +
drivers/iommu/virtio-iommu.c | 8 ++++++++
3 files changed, 11 insertions(+), 1 deletion(-)
Hi Rafael,
On Thu, Jun 10, 2021 at 09:51:27AM +0200, Jean-Philippe Brucker wrote:
Add a driver for the ACPI VIOT table, which provides topology
information for para-virtual IOMMUs. Enable virtio-iommu on
non-devicetree platforms, including x86.
Since v3 [1] I fixed a build bug for !CONFIG_IOMMU_API. Joerg offered to
take this series through the IOMMU tree, which requires Acks for patches
1-3.
I was wondering if you could take a look at patches 1-3, otherwise we'll
miss the mark for 5.14 since I won't be able to resend next week. The
series adds support for virtio-iommu on QEMU and cloud hypervisor.
Thanks,
Jean
You can find a QEMU implementation at [2], with extra support for
testing all VIOT nodes including MMIO-based endpoints and IOMMU.
This series is at [3].
[1] https://lore.kernel.org/linux-iommu/20210602154444.1077006-1-jean-philippe@linaro.org/
[2] https://jpbrucker.net/git/qemu/log/?h=virtio-iommu/acpi
[3] https://jpbrucker.net/git/linux/log/?h=virtio-iommu/acpi
Jean-Philippe Brucker (6):
ACPI: arm64: Move DMA setup operations out of IORT
ACPI: Move IOMMU setup code out of IORT
ACPI: Add driver for the VIOT table
iommu/dma: Pass address limit rather than size to
iommu_setup_dma_ops()
iommu/dma: Simplify calls to iommu_setup_dma_ops()
iommu/virtio: Enable x86 support
drivers/acpi/Kconfig | 3 +
drivers/iommu/Kconfig | 4 +-
drivers/acpi/Makefile | 2 +
drivers/acpi/arm64/Makefile | 1 +
include/acpi/acpi_bus.h | 3 +
include/linux/acpi.h | 3 +
include/linux/acpi_iort.h | 14 +-
include/linux/acpi_viot.h | 19 ++
include/linux/dma-iommu.h | 4 +-
arch/arm64/mm/dma-mapping.c | 2 +-
drivers/acpi/arm64/dma.c | 50 +++++
drivers/acpi/arm64/iort.c | 129 ++-----------
drivers/acpi/bus.c | 2 +
drivers/acpi/scan.c | 78 +++++++-
drivers/acpi/viot.c | 364 +++++++++++++++++++++++++++++++++++
drivers/iommu/amd/iommu.c | 9 +-
drivers/iommu/dma-iommu.c | 17 +-
drivers/iommu/intel/iommu.c | 10 +-
drivers/iommu/virtio-iommu.c | 8 +
MAINTAINERS | 8 +
20 files changed, 580 insertions(+), 150 deletions(-)
create mode 100644 include/linux/acpi_viot.h
create mode 100644 drivers/acpi/arm64/dma.c
create mode 100644 drivers/acpi/viot.c
--
2.31.1
From: Eric Auger <eric.auger@redhat.com> Date: 2021-06-16 09:37:02
Hi jean,
On 6/10/21 9:51 AM, Jean-Philippe Brucker wrote:
quoted hunk
Extract the code that sets up the IOMMU infrastructure from IORT, since
it can be reused by VIOT. Move it one level up into a new
acpi_iommu_configure_id() function, which calls the IORT parsing
function which in turn calls the acpi_iommu_fwspec_init() helper.
Signed-off-by: Jean-Philippe Brucker <redacted>
---
include/acpi/acpi_bus.h | 3 ++
include/linux/acpi_iort.h | 8 ++---
drivers/acpi/arm64/iort.c | 75 +++++----------------------------------
drivers/acpi/scan.c | 73 ++++++++++++++++++++++++++++++++++++-
4 files changed, 87 insertions(+), 72 deletions(-)
Previously we had:
if (!err) {
ops = iort_fwspec_iommu_ops(dev);
err = iort_add_device_replay(dev);
}
Please can you explain the transform? I see the
acpi_iommu_fwspec_ops call below but is it not straightforward to me.
Also the comment mentions replay. Unsure if it is still OK.
quoted hunk
+
+ /* Ignore all other errors apart from EPROBE_DEFER */
+ if (err == -EPROBE_DEFER) {
+ return ERR_PTR(err);
+ } else if (err) {
+ dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
+ return NULL;
+ }
+ return acpi_iommu_fwspec_ops(dev);
+}
+
+#else /* !CONFIG_IOMMU_API */
+
+int acpi_iommu_fwspec_init(struct device *dev, u32 id,
+ struct fwnode_handle *fwnode,
+ const struct iommu_ops *ops)
+{
+ return -ENODEV;
+}
+
+static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
+ const u32 *id_in)
+{
+ return NULL;
+}
+
+#endif /* !CONFIG_IOMMU_API */
+
/**
* acpi_dma_configure_id - Set-up DMA configuration for the device.
* @dev: The pointer to the device
From: Eric Auger <eric.auger@redhat.com> Date: 2021-06-16 09:37:23
Hi jean,
On 6/10/21 9:51 AM, Jean-Philippe Brucker wrote:
Extract generic DMA setup code out of IORT, so it can be reused by VIOT.
Keep it in drivers/acpi/arm64 for now, since it could break x86
platforms that haven't run this code so far, if they have invalid
tables.
Signed-off-by: Jean-Philippe Brucker <redacted>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
I tested the series on both aarch64 and x86_64 with qemu. It works for me.
Feel free to add my T-b.
Tested-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
From: Eric Auger <eric.auger@redhat.com> Date: 2021-06-16 13:27:56
Hi Jean,
On 6/10/21 9:51 AM, Jean-Philippe Brucker wrote:
quoted hunk
The ACPI Virtual I/O Translation Table describes topology of
para-virtual platforms, similarly to vendor tables DMAR, IVRS and IORT.
For now it describes the relation between virtio-iommu and the endpoints
it manages.
Three steps are needed to configure DMA of endpoints:
(1) acpi_viot_init(): parse the VIOT table, find or create the fwnode
associated to each vIOMMU device.
(2) When probing the vIOMMU device, the driver registers its IOMMU ops
within the IOMMU subsystem. This step doesn't require any
intervention from the VIOT driver.
(3) viot_iommu_configure(): before binding the endpoint to a driver,
find the associated IOMMU ops. Register them, along with the
endpoint ID, into the device's iommu_fwspec.
If step (3) happens before step (2), it is deferred until the IOMMU is
initialized, then retried.
Signed-off-by: Jean-Philippe Brucker <redacted>
---
drivers/acpi/Kconfig | 3 +
drivers/iommu/Kconfig | 1 +
drivers/acpi/Makefile | 2 +
include/linux/acpi_viot.h | 19 ++
drivers/acpi/bus.c | 2 +
drivers/acpi/scan.c | 3 +
drivers/acpi/viot.c | 364 ++++++++++++++++++++++++++++++++++++++
MAINTAINERS | 8 +
8 files changed, 402 insertions(+)
create mode 100644 include/linux/acpi_viot.h
create mode 100644 drivers/acpi/viot.c
@@ -0,0 +1,364 @@+// SPDX-License-Identifier: GPL-2.0+/*+*VirtualI/Otopology+*+*TheVirtualI/OTranslationTable(VIOT)describesthetopologyof+*para-virtualIOMMUsandtheendpointstheymanage.TheOSusesitto+*initializedevicesintherightorder,preventingendpointsfromissuingDMA+*beforetheirIOMMUisready.+*+*Whenbindingadrivertoadevice,beforecallingthedevicedriver'sprobe()+*method,thedriverinfrastructurecallsdma_configure().Atthatpointthe+*VIOTdriverlooksforanIOMMUassociatedtothedeviceintheVIOTtable.+*IfanIOMMUexistsandhasbeeninitialized,theVIOTdriverinitializesthe+*device'sIOMMUfwspec,allowingtheDMAinfrastructuretoinvoketheIOMMU+*opswhenthedevicedriverconfiguresDMAmappings.IfanIOMMUexistsand+*hasn'tyetbeeninitialized,VIOTreturns-EPROBE_DEFERtopostponeprobing+*thedeviceuntiltheIOMMUisavailable.+*/+#define pr_fmt(fmt) "ACPI: VIOT: " fmt++#include<linux/acpi_viot.h>+#include<linux/dma-iommu.h>+#include<linux/fwnode.h>+#include<linux/iommu.h>+#include<linux/list.h>+#include<linux/pci.h>+#include<linux/platform_device.h>++structviot_iommu{+/* Node offset within the table */+unsignedintoffset;+structfwnode_handle*fwnode;+structlist_headlist;+};++structviot_endpoint{+union{+/* PCI range */+struct{+u16segment_start;+u16segment_end;+u16bdf_start;+u16bdf_end;+};+/* MMIO */+u64address;+};+u32endpoint_id;+structviot_iommu*viommu;+structlist_headlist;+};++staticstructacpi_table_viot*viot;+staticLIST_HEAD(viot_iommus);+staticLIST_HEAD(viot_pci_ranges);+staticLIST_HEAD(viot_mmio_endpoints);++staticint__initviot_check_bounds(conststructacpi_viot_header*hdr)+{+structacpi_viot_header*start,*end,*hdr_end;++start=ACPI_ADD_PTR(structacpi_viot_header,viot,+max_t(size_t,sizeof(*viot),viot->node_offset));+end=ACPI_ADD_PTR(structacpi_viot_header,viot,viot->header.length);+hdr_end=ACPI_ADD_PTR(structacpi_viot_header,hdr,sizeof(*hdr));++if(hdr<start||hdr_end>end){+pr_err(FW_BUG"Node pointer overflows\n");+return-EOVERFLOW;+}+if(hdr->length<sizeof(*hdr)){+pr_err(FW_BUG"Empty node\n");+return-EINVAL;+}+return0;+}++staticint__initviot_get_pci_iommu_fwnode(structviot_iommu*viommu,+u16segment,u16bdf)+{+structpci_dev*pdev;+structfwnode_handle*fwnode;++pdev=pci_get_domain_bus_and_slot(segment,PCI_BUS_NUM(bdf),+bdf&0xff);+if(!pdev){+pr_err("Could not find PCI IOMMU\n");+return-ENODEV;+}++fwnode=pdev->dev.fwnode;+if(!fwnode){+/*+*PCIdevicesaren'tnecessarilydescribedbyACPI.Createa+*fwnodesotheIOMMUsubsystemcanidentifythisdevice.+*/+fwnode=acpi_alloc_fwnode_static();+if(!fwnode){+pci_dev_put(pdev);+return-ENOMEM;+}+set_primary_fwnode(&pdev->dev,fwnode);+}+viommu->fwnode=pdev->dev.fwnode;+pci_dev_put(pdev);+return0;+}++staticint__initviot_get_mmio_iommu_fwnode(structviot_iommu*viommu,+u64address)+{+structacpi_device*adev;+structresourceres={+.start=address,+.end=address,+.flags=IORESOURCE_MEM,+};++adev=acpi_resource_consumer(&res);+if(!adev){+pr_err("Could not find MMIO IOMMU\n");+return-EINVAL;+}+viommu->fwnode=&adev->fwnode;+return0;+}++staticstructviot_iommu*__initviot_get_iommu(unsignedintoffset)+{+intret;+structviot_iommu*viommu;+structacpi_viot_header*hdr=ACPI_ADD_PTR(structacpi_viot_header,+viot,offset);+union{+structacpi_viot_virtio_iommu_pcipci;+structacpi_viot_virtio_iommu_mmiommio;+}*node=(void*)hdr;++list_for_each_entry(viommu,&viot_iommus,list)+if(viommu->offset==offset)+returnviommu;++if(viot_check_bounds(hdr))+returnNULL;++viommu=kzalloc(sizeof(*viommu),GFP_KERNEL);+if(!viommu)+returnNULL;++viommu->offset=offset;+switch(hdr->type){+caseACPI_VIOT_NODE_VIRTIO_IOMMU_PCI:+if(hdr->length<sizeof(node->pci))+gotoerr_free;++ret=viot_get_pci_iommu_fwnode(viommu,node->pci.segment,+node->pci.bdf);+break;+caseACPI_VIOT_NODE_VIRTIO_IOMMU_MMIO:+if(hdr->length<sizeof(node->mmio))+gotoerr_free;++ret=viot_get_mmio_iommu_fwnode(viommu,+node->mmio.base_address);+break;+default:+ret=-EINVAL;+}+if(ret)+gotoerr_free;++list_add(&viommu->list,&viot_iommus);+returnviommu;++err_free:+kfree(viommu);+returnNULL;+}++staticint__initviot_parse_node(conststructacpi_viot_header*hdr)+{+intret=-EINVAL;+structlist_head*list;+structviot_endpoint*ep;+union{+structacpi_viot_mmiommio;+structacpi_viot_pci_rangepci;+}*node=(void*)hdr;++if(viot_check_bounds(hdr))+return-EINVAL;++if(hdr->type==ACPI_VIOT_NODE_VIRTIO_IOMMU_PCI||+hdr->type==ACPI_VIOT_NODE_VIRTIO_IOMMU_MMIO)+return0;++ep=kzalloc(sizeof(*ep),GFP_KERNEL);+if(!ep)+return-ENOMEM;++switch(hdr->type){+caseACPI_VIOT_NODE_PCI_RANGE:+if(hdr->length<sizeof(node->pci)){+pr_err(FW_BUG"Invalid PCI node size\n");+gotoerr_free;+}++ep->segment_start=node->pci.segment_start;+ep->segment_end=node->pci.segment_end;+ep->bdf_start=node->pci.bdf_start;+ep->bdf_end=node->pci.bdf_end;+ep->endpoint_id=node->pci.endpoint_start;+ep->viommu=viot_get_iommu(node->pci.output_node);+list=&viot_pci_ranges;+break;+caseACPI_VIOT_NODE_MMIO:+if(hdr->length<sizeof(node->mmio)){+pr_err(FW_BUG"Invalid MMIO node size\n");+gotoerr_free;+}++ep->address=node->mmio.base_address;+ep->endpoint_id=node->mmio.endpoint;+ep->viommu=viot_get_iommu(node->mmio.output_node);+list=&viot_mmio_endpoints;+break;+default:+pr_warn("Unsupported node %x\n",hdr->type);+ret=0;+gotoerr_free;+}++/*+*Tobecompatiblewithfutureversionsofthetablewhichmayinclude+*othernodetypes,keepparsing.+*/
nit: doesn't this comment rather apply to the default clause in the
switch. In case the PCI range node or the single MMIO endoint node does
not refer to any translation element, isn't it simply an error case?
quoted hunk
+ if (!ep->viommu) {
+ pr_warn("No IOMMU node found\n");
+ ret = 0;
+ goto err_free;
+ }
+
+ list_add(&ep->list, list);
+ return 0;
+
+err_free:
+ kfree(ep);
+ return ret;
+}
+
+/**
+ * acpi_viot_init - Parse the VIOT table
+ *
+ * Parse the VIOT table, prepare the list of endpoints to be used during DMA
+ * setup of devices.
+ */
+void __init acpi_viot_init(void)
+{
+ int i;
+ acpi_status status;
+ struct acpi_table_header *hdr;
+ struct acpi_viot_header *node;
+
+ status = acpi_get_table(ACPI_SIG_VIOT, 0, &hdr);
+ if (ACPI_FAILURE(status)) {
+ if (status != AE_NOT_FOUND) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err("Failed to get table, %s\n", msg);
+ }
+ return;
+ }
+
+ viot = (void *)hdr;
+
+ node = ACPI_ADD_PTR(struct acpi_viot_header, viot, viot->node_offset);
+ for (i = 0; i < viot->node_count; i++) {
+ if (viot_parse_node(node))
+ return;
+
+ node = ACPI_ADD_PTR(struct acpi_viot_header, node,
+ node->length);
+ }
+}
+
+static int viot_dev_iommu_init(struct device *dev, struct viot_iommu *viommu,
+ u32 epid)
+{
+ const struct iommu_ops *ops;
+
+ if (!viommu)
+ return -ENODEV;
+
+ /* We're not translating ourself */
+ if (viommu->fwnode == dev->fwnode)
+ return -EINVAL;
+
+ ops = iommu_ops_from_fwnode(viommu->fwnode);
+ if (!ops)
+ return IS_ENABLED(CONFIG_VIRTIO_IOMMU) ?
+ -EPROBE_DEFER : -ENODEV;
+
+ return acpi_iommu_fwspec_init(dev, epid, viommu->fwnode, ops);
+}
+
+static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data)
+{
+ u32 epid;
+ struct viot_endpoint *ep;
+ u32 domain_nr = pci_domain_nr(pdev->bus);
+
+ list_for_each_entry(ep, &viot_pci_ranges, list) {
+ if (domain_nr >= ep->segment_start &&
+ domain_nr <= ep->segment_end &&
+ dev_id >= ep->bdf_start &&
+ dev_id <= ep->bdf_end) {
+ epid = ((domain_nr - ep->segment_start) << 16) +
+ dev_id - ep->bdf_start + ep->endpoint_id;
+
+ /*
+ * If we found a PCI range managed by the viommu, we're
+ * the one that has to request ACS.
+ */
+ pci_request_acs();
+
+ return viot_dev_iommu_init(&pdev->dev, ep->viommu,
+ epid);
+ }
+ }
+ return -ENODEV;
+}
+
+static int viot_mmio_dev_iommu_init(struct platform_device *pdev)
+{
+ struct resource *mem;
+ struct viot_endpoint *ep;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem)
+ return -ENODEV;
+
+ list_for_each_entry(ep, &viot_mmio_endpoints, list) {
+ if (ep->address == mem->start)
+ return viot_dev_iommu_init(&pdev->dev, ep->viommu,
+ ep->endpoint_id);
+ }
+ return -ENODEV;
+}
+
+/**
+ * viot_iommu_configure - Setup IOMMU ops for an endpoint described by VIOT
+ * @dev: the endpoint
+ *
+ * Return: 0 on success, <0 on failure
+ */
+int viot_iommu_configure(struct device *dev)
+{
+ if (dev_is_pci(dev))
+ return pci_for_each_dma_alias(to_pci_dev(dev),
+ viot_pci_dev_iommu_init, NULL);
+ else if (dev_is_platform(dev))
+ return viot_mmio_dev_iommu_init(to_platform_device(dev));
+ return -ENODEV;
+}
From: Eric Auger <eric.auger@redhat.com> Date: 2021-06-16 15:30:38
Hi Jean,
On 6/10/21 9:51 AM, Jean-Philippe Brucker wrote:
quoted hunk
Passing a 64-bit address width to iommu_setup_dma_ops() is valid on
virtual platforms, but isn't currently possible. The overflow check in
iommu_dma_init_domain() prevents this even when @dma_base isn't 0. Pass
a limit address instead of a size, so callers don't have to fake a size
to work around the check.
Signed-off-by: Jean-Philippe Brucker <redacted>
---
include/linux/dma-iommu.h | 4 ++--
arch/arm64/mm/dma-mapping.c | 2 +-
drivers/iommu/amd/iommu.c | 2 +-
drivers/iommu/dma-iommu.c | 12 ++++++------
drivers/iommu/intel/iommu.c | 2 +-
5 files changed, 11 insertions(+), 11 deletions(-)
@@ -1713,7 +1713,7 @@ static void amd_iommu_probe_finalize(struct device *dev)/* Domains are initialized for this device - have a look what we ended up with */domain=iommu_get_domain_for_dev(dev);if(domain->type==IOMMU_DOMAIN_DMA)-iommu_setup_dma_ops(dev,IOVA_START_PFN<<PAGE_SHIFT,0);+iommu_setup_dma_ops(dev,IOVA_START_PFN<<PAGE_SHIFT,U64_MAX);elseset_dma_ops(dev,NULL);}
@@ -346,7 +346,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,/* Check the domain allows at least some access to the device... */if(domain->geometry.force_aperture){if(base>domain->geometry.aperture_end||-base+size<=domain->geometry.aperture_start){+limit<domain->geometry.aperture_start){pr_warn("specified DMA range outside IOMMU capability\n");return-EFAULT;}
From: Eric Auger <eric.auger@redhat.com> Date: 2021-06-16 15:53:19
Hi Jean,
On 6/10/21 9:51 AM, Jean-Philippe Brucker wrote:
dma-iommu uses the address bounds described in domain->geometry during
IOVA allocation. The address size parameters of iommu_setup_dma_ops()
are useful for describing additional limits set by the platform
firmware, but aren't needed for drivers that call this function from
probe_finalize(). The base parameter can be zero because dma-iommu
already removes the first IOVA page, and the limit parameter can be
U64_MAX because it's only checked against the domain geometry. Simplify
calls to iommu_setup_dma_ops().
Signed-off-by: Jean-Philippe Brucker <redacted>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
@@ -1708,14 +1708,7 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)staticvoidamd_iommu_probe_finalize(structdevice*dev){-structiommu_domain*domain;--/* Domains are initialized for this device - have a look what we ended up with */-domain=iommu_get_domain_for_dev(dev);-if(domain->type==IOMMU_DOMAIN_DMA)-iommu_setup_dma_ops(dev,IOVA_START_PFN<<PAGE_SHIFT,U64_MAX);-else-set_dma_ops(dev,NULL);+iommu_setup_dma_ops(dev,0,U64_MAX);}staticvoidamd_iommu_release_device(structdevice*dev)
From: Eric Auger <eric.auger@redhat.com> Date: 2021-06-16 15:54:25
Hi jean,
On 6/10/21 9:51 AM, Jean-Philippe Brucker wrote:
With the VIOT support in place, x86 platforms can now use the
virtio-iommu.
Because the other x86 IOMMU drivers aren't yet ready to use the
acpi_dma_setup() path, x86 doesn't implement arch_setup_dma_ops() at the
moment. Similarly to Vt-d and AMD IOMMU, call iommu_setup_dma_ops() from
probe_finalize().
Acked-by: Joerg Roedel <redacted>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jean-Philippe Brucker <redacted>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
From: Robin Murphy <robin.murphy@arm.com> Date: 2021-06-16 17:04:52
On 2021-06-10 08:51, Jean-Philippe Brucker wrote:
quoted hunk
dma-iommu uses the address bounds described in domain->geometry during
IOVA allocation. The address size parameters of iommu_setup_dma_ops()
are useful for describing additional limits set by the platform
firmware, but aren't needed for drivers that call this function from
probe_finalize(). The base parameter can be zero because dma-iommu
already removes the first IOVA page, and the limit parameter can be
U64_MAX because it's only checked against the domain geometry. Simplify
calls to iommu_setup_dma_ops().
Signed-off-by: Jean-Philippe Brucker <redacted>
---
drivers/iommu/amd/iommu.c | 9 +--------
drivers/iommu/dma-iommu.c | 4 +++-
drivers/iommu/intel/iommu.c | 10 +---------
3 files changed, 5 insertions(+), 18 deletions(-)
@@ -1708,14 +1708,7 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)staticvoidamd_iommu_probe_finalize(structdevice*dev){-structiommu_domain*domain;--/* Domains are initialized for this device - have a look what we ended up with */-domain=iommu_get_domain_for_dev(dev);-if(domain->type==IOMMU_DOMAIN_DMA)-iommu_setup_dma_ops(dev,IOVA_START_PFN<<PAGE_SHIFT,U64_MAX);-else-set_dma_ops(dev,NULL);+iommu_setup_dma_ops(dev,0,U64_MAX);}staticvoidamd_iommu_release_device(structdevice*dev)
I'm not keen on moving this here, since iommu-dma only knows that its
own ops are right for devices it *is* managing; it can't assume any
particular ops are appropriate for devices it isn't. The idea here is
that arch_setup_dma_ops() may have already set the appropriate ops for
the non-IOMMU case, so if the default domain type is passthrough then we
leave those in place.
For example, I do still plan to revisit my conversion of arch/arm
someday, at which point I'd have to undo this for that reason.
Simplifying the base and size arguments is of course fine, but TBH I'd
say rip the whole bloody lot out of the arch_setup_dma_ops() flow now.
It's a considerable faff passing them around for nothing but a tenuous
sanity check in iommu_dma_init_domain(), and now that dev->dma_range_map
is a common thing we should expect that to give us any relevant
limitations if we even still care.
That said, those are all things which can be fixed up later if the
series is otherwise ready to go and there's still a chance of landing it
for 5.14. If you do have any other reason to respin, then I think the
x86 probe_finalize functions simply want an unconditional
set_dma_ops(dev, NULL) before the iommu_setup_dma_ops() call.
Cheers,
Robin.
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2021-06-17 11:53:03
On Thu, Jun 10, 2021 at 10:03 AM Jean-Philippe Brucker
[off-list ref] wrote:
quoted hunk
The ACPI Virtual I/O Translation Table describes topology of
para-virtual platforms, similarly to vendor tables DMAR, IVRS and IORT.
For now it describes the relation between virtio-iommu and the endpoints
it manages.
Three steps are needed to configure DMA of endpoints:
(1) acpi_viot_init(): parse the VIOT table, find or create the fwnode
associated to each vIOMMU device.
(2) When probing the vIOMMU device, the driver registers its IOMMU ops
within the IOMMU subsystem. This step doesn't require any
intervention from the VIOT driver.
(3) viot_iommu_configure(): before binding the endpoint to a driver,
find the associated IOMMU ops. Register them, along with the
endpoint ID, into the device's iommu_fwspec.
If step (3) happens before step (2), it is deferred until the IOMMU is
initialized, then retried.
Signed-off-by: Jean-Philippe Brucker <redacted>
---
drivers/acpi/Kconfig | 3 +
drivers/iommu/Kconfig | 1 +
drivers/acpi/Makefile | 2 +
include/linux/acpi_viot.h | 19 ++
drivers/acpi/bus.c | 2 +
drivers/acpi/scan.c | 3 +
drivers/acpi/viot.c | 364 ++++++++++++++++++++++++++++++++++++++
MAINTAINERS | 8 +
8 files changed, 402 insertions(+)
create mode 100644 include/linux/acpi_viot.h
create mode 100644 drivers/acpi/viot.c
@@ -1339,6 +1340,7 @@ static int __init acpi_init(void)pci_mmcfg_late_init();acpi_iort_init();acpi_scan_init();+acpi_viot_init();
Is there a specific reason why to call it right here?
In particular, does it need to be called after acpi_scan_init()? And
does it need to be called before the subsequent functions? If so,
then why?
Oops, I need to remove this (and add -Werror to my tests.)
quoted
+static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
+ const u32 *id_in)
+{
+ int err;
+ const struct iommu_ops *ops;
+
+ /*
+ * If we already translated the fwspec there is nothing left to do,
+ * return the iommu_ops.
+ */
+ ops = acpi_iommu_fwspec_ops(dev);
+ if (ops)
+ return ops;
+
+ err = iort_iommu_configure_id(dev, id_in);
+
+ /*
+ * If we have reason to believe the IOMMU driver missed the initial
+ * add_device callback for dev, replay it to get things in order.
+ */
+ if (!err && dev->bus && !device_iommu_mapped(dev))
+ err = iommu_probe_device(dev);
Previously we had:
if (!err) {
ops = iort_fwspec_iommu_ops(dev);
err = iort_add_device_replay(dev);
}
Please can you explain the transform? I see the
acpi_iommu_fwspec_ops call below but is it not straightforward to me.
I figured that iort_add_device_replay() is only used once and is
sufficiently simple to be inlined manually (saving 10 lines). Then I
replaced the ops assignment with returns, which saves another line and may
be slightly clearer? I guess it's mostly a matter of taste, the behavior
should be exactly the same.
Also the comment mentions replay. Unsure if it is still OK.
The "replay" part is, but "add_device" isn't accurate because it has since
been replaced by probe_device. I'll refresh the comment.
Thanks,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Wed, Jun 16, 2021 at 03:26:08PM +0200, Eric Auger wrote:
quoted
+ default:
+ pr_warn("Unsupported node %x\n", hdr->type);
+ ret = 0;
+ goto err_free;
+ }
+
+ /*
+ * To be compatible with future versions of the table which may include
+ * other node types, keep parsing.
+ */
nit: doesn't this comment rather apply to the default clause in the
switch.
Yes, the comment doesn't accurately explain the code below, I'll tweak it.
/*
* A future version of the table may use the node for other purposes.
* Keep parsing.
*/
In case the PCI range node or the single MMIO endoint node does
not refer to any translation element, isn't it simply an error case?
It is permissible in my opinion. If a future version of the spec appends
new fields to the MMIO endpoint describing some PV property (I can't think
of a useful example), then the table can contain the vIOMMU topology as
usual plus one MMIO node that's only here to describe that property, and
doesn't have a translation element. If we encounter that I think we should
keep parsing.
quoted
+ if (!ep->viommu) {
+ pr_warn("No IOMMU node found\n");
+ ret = 0;
+ goto err_free;
+ }
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
@@ -1339,6 +1340,7 @@ static int __init acpi_init(void)pci_mmcfg_late_init();acpi_iort_init();acpi_scan_init();+acpi_viot_init();
Is there a specific reason why to call it right here?
In particular, does it need to be called after acpi_scan_init()? And
does it need to be called before the subsequent functions? If so,
then why?
It does need to be called after acpi_scan_init(), because it relies on
struct device and their fwnode to be initialized. In particular to find a
PCI device we call pci_get_domain_bus_and_slot(), which needs the PCI
topology made available by acpi_scan_init().
It does not need to be before the subsequent functions however, I can move
it at the end.
quoted
+void __init acpi_viot_init(void)
+{
+ int i;
+ acpi_status status;
+ struct acpi_table_header *hdr;
+ struct acpi_viot_header *node;
+
+ status = acpi_get_table(ACPI_SIG_VIOT, 0, &hdr);
+ if (ACPI_FAILURE(status)) {
+ if (status != AE_NOT_FOUND) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err("Failed to get table, %s\n", msg);
+ }
+ return;
+ }
+
+ viot = (void *)hdr;
+
+ node = ACPI_ADD_PTR(struct acpi_viot_header, viot, viot->node_offset);
+ for (i = 0; i < viot->node_count; i++) {
+ if (viot_parse_node(node))
+ return;
+
+ node = ACPI_ADD_PTR(struct acpi_viot_header, node,
+ node->length);
+ }
Do you still need the table after the above is complete? If not,
release the reference on it acquired above.
Oops, I need to remove this (and add -Werror to my tests.)
quoted
quoted
+static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
+ const u32 *id_in)
+{
+ int err;
+ const struct iommu_ops *ops;
+
+ /*
+ * If we already translated the fwspec there is nothing left to do,
+ * return the iommu_ops.
+ */
+ ops = acpi_iommu_fwspec_ops(dev);
+ if (ops)
+ return ops;
+
+ err = iort_iommu_configure_id(dev, id_in);
+
+ /*
+ * If we have reason to believe the IOMMU driver missed the initial
+ * add_device callback for dev, replay it to get things in order.
+ */
+ if (!err && dev->bus && !device_iommu_mapped(dev))
+ err = iommu_probe_device(dev);
Previously we had:
if (!err) {
ops = iort_fwspec_iommu_ops(dev);
err = iort_add_device_replay(dev);
}
Please can you explain the transform? I see the
acpi_iommu_fwspec_ops call below but is it not straightforward to me.
I figured that iort_add_device_replay() is only used once and is
sufficiently simple to be inlined manually (saving 10 lines). Then I
replaced the ops assignment with returns, which saves another line and may
be slightly clearer? I guess it's mostly a matter of taste, the behavior
should be exactly the same.
Right, IIRC the multiple assignments to ops were more of a haphazard
evolution inherited from the DT version, and looking at it now I think
the multiple-return is indeed a bit nicer.
Similarly, it looks like the factoring out of iort_add_device_replay()
was originally an attempt to encapsulate the IOMMU_API dependency, but
things have moved around a lot since then, so that seems like a sensible
simplification to make too.
Robin.
quoted
Also the comment mentions replay. Unsure if it is still OK.
The "replay" part is, but "add_device" isn't accurate because it has since
been replaced by probe_device. I'll refresh the comment.
Thanks,
Jean
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Because it's wrong, should be dma_base + size - 1. Thanks for catching it!
Thanks,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
I'm not keen on moving this here, since iommu-dma only knows that its own
ops are right for devices it *is* managing; it can't assume any particular
ops are appropriate for devices it isn't. The idea here is that
arch_setup_dma_ops() may have already set the appropriate ops for the
non-IOMMU case, so if the default domain type is passthrough then we leave
those in place.
For example, I do still plan to revisit my conversion of arch/arm someday,
at which point I'd have to undo this for that reason.
Makes sense, I'll remove this bit.
Simplifying the base and size arguments is of course fine, but TBH I'd say
rip the whole bloody lot out of the arch_setup_dma_ops() flow now. It's a
considerable faff passing them around for nothing but a tenuous sanity check
in iommu_dma_init_domain(), and now that dev->dma_range_map is a common
thing we should expect that to give us any relevant limitations if we even
still care.
So I started working on this but it gets too bulky for a preparatory
patch. Dropping the parameters from arch_setup_dma_ops() seems especially
complicated because arm32 does need the size parameter for IOMMU mappings
and that value falls back to the bus DMA mask or U32_MAX in the absence of
dma-ranges. I could try to dig into this for a separate series.
Even only dropping the parameters from iommu_setup_dma_ops() isn't
completely trivial (8 files changed, 55 insertions(+), 36 deletions(-)
because we still need the lower IOVA limit from dma_range_map), so I'd
rather send it separately and have it sit in -next for a while.
Thanks,
Jean
That said, those are all things which can be fixed up later if the series is
otherwise ready to go and there's still a chance of landing it for 5.14. If
you do have any other reason to respin, then I think the x86 probe_finalize
functions simply want an unconditional set_dma_ops(dev, NULL) before the
iommu_setup_dma_ops() call.
Cheers,
Robin.
I'm not keen on moving this here, since iommu-dma only knows that its own
ops are right for devices it *is* managing; it can't assume any particular
ops are appropriate for devices it isn't. The idea here is that
arch_setup_dma_ops() may have already set the appropriate ops for the
non-IOMMU case, so if the default domain type is passthrough then we leave
those in place.
For example, I do still plan to revisit my conversion of arch/arm someday,
at which point I'd have to undo this for that reason.
Makes sense, I'll remove this bit.
quoted
Simplifying the base and size arguments is of course fine, but TBH I'd say
rip the whole bloody lot out of the arch_setup_dma_ops() flow now. It's a
considerable faff passing them around for nothing but a tenuous sanity check
in iommu_dma_init_domain(), and now that dev->dma_range_map is a common
thing we should expect that to give us any relevant limitations if we even
still care.
So I started working on this but it gets too bulky for a preparatory
patch. Dropping the parameters from arch_setup_dma_ops() seems especially
complicated because arm32 does need the size parameter for IOMMU mappings
and that value falls back to the bus DMA mask or U32_MAX in the absence of
dma-ranges. I could try to dig into this for a separate series.
Even only dropping the parameters from iommu_setup_dma_ops() isn't
completely trivial (8 files changed, 55 insertions(+), 36 deletions(-)
because we still need the lower IOVA limit from dma_range_map), so I'd
rather send it separately and have it sit in -next for a while.
Oh, sure, I didn't mean to imply that the whole cleanup should be within
the scope of this series, just that we can shave off as much as we *do*
need to touch here (which TBH is pretty much what you're doing already),
and mainly to start taking the attitude that these arguments are now
superseded and increasingly vestigial.
I expected the cross-arch cleanup to be a bit fiddly, but I'd forgotten
that arch/arm was still actively using these values, so maybe I can
revisit this when I pick up my iommu-dma conversion again (I swear it's
not dead, just resting!)
Cheers,
Robin.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization