This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
devices on ARM in addition to PCI. This is required in order to use the
Exynos SMMU, or ARM SMMU driver with VFIO_IOMMU_TYPE1.
Signed-off-by: Antonios Motakis <[email protected]>
---
drivers/vfio/Kconfig | 2 +-
drivers/vfio/vfio_iommu_type1.c | 5 ++++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index af7b204..3a598ed 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -11,7 +11,7 @@ config VFIO_IOMMU_SPAPR_TCE
menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
depends on IOMMU_API
- select VFIO_IOMMU_TYPE1 if X86
+ select VFIO_IOMMU_TYPE1 if X86 || ARM
select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES)
select ANON_INODES
help
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 1f90344..4dc989d 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -30,6 +30,7 @@
#include <linux/iommu.h>
#include <linux/module.h>
#include <linux/mm.h>
+#include <linux/pci.h> /* pci_bus_type */
#include <linux/rbtree.h>
#include <linux/sched.h>
#include <linux/slab.h>
@@ -721,13 +722,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
INIT_LIST_HEAD(&domain->group_list);
list_add(&group->next, &domain->group_list);
- if (!allow_unsafe_interrupts &&
+#ifdef CONFIG_PCI
+ if (bus == &pci_bus_type && !allow_unsafe_interrupts &&
!iommu_domain_has_cap(domain->domain, IOMMU_CAP_INTR_REMAP)) {
pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
__func__);
ret = -EPERM;
goto out_detach;
}
+#endif
if (iommu_domain_has_cap(domain->domain, IOMMU_CAP_CACHE_COHERENCY))
domain->prot |= IOMMU_CACHE;
--
1.8.3.2
On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
> devices on ARM in addition to PCI. This is required in order to use the
> Exynos SMMU, or ARM SMMU driver with VFIO_IOMMU_TYPE1.
>
> Signed-off-by: Antonios Motakis <[email protected]>
> ---
> drivers/vfio/Kconfig | 2 +-
> drivers/vfio/vfio_iommu_type1.c | 5 ++++-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index af7b204..3a598ed 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -11,7 +11,7 @@ config VFIO_IOMMU_SPAPR_TCE
> menuconfig VFIO
> tristate "VFIO Non-Privileged userspace driver framework"
> depends on IOMMU_API
> - select VFIO_IOMMU_TYPE1 if X86
> + select VFIO_IOMMU_TYPE1 if X86 || ARM
> select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES)
> select ANON_INODES
> help
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 1f90344..4dc989d 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -30,6 +30,7 @@
> #include <linux/iommu.h>
> #include <linux/module.h>
> #include <linux/mm.h>
> +#include <linux/pci.h> /* pci_bus_type */
> #include <linux/rbtree.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> @@ -721,13 +722,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> INIT_LIST_HEAD(&domain->group_list);
> list_add(&group->next, &domain->group_list);
>
> - if (!allow_unsafe_interrupts &&
> +#ifdef CONFIG_PCI
> + if (bus == &pci_bus_type && !allow_unsafe_interrupts &&
> !iommu_domain_has_cap(domain->domain, IOMMU_CAP_INTR_REMAP)) {
> pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
> __func__);
> ret = -EPERM;
> goto out_detach;
> }
> +#endif
>
> if (iommu_domain_has_cap(domain->domain, IOMMU_CAP_CACHE_COHERENCY))
> domain->prot |= IOMMU_CACHE;
This is not a PCI specific requirement. Anything that can support MSI
needs an IOMMU that can provide isolation for both DMA and interrupts.
I think the IOMMU should still be telling us that it has this feature.
Thanks,
Alex
Hi Alex,
On Mon, Apr 28, 2014 at 05:43:41PM +0100, Alex Williamson wrote:
> On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> > This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
> > devices on ARM in addition to PCI. This is required in order to use the
> > Exynos SMMU, or ARM SMMU driver with VFIO_IOMMU_TYPE1.
[...]
> > @@ -721,13 +722,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> > INIT_LIST_HEAD(&domain->group_list);
> > list_add(&group->next, &domain->group_list);
> >
> > - if (!allow_unsafe_interrupts &&
> > +#ifdef CONFIG_PCI
> > + if (bus == &pci_bus_type && !allow_unsafe_interrupts &&
> > !iommu_domain_has_cap(domain->domain, IOMMU_CAP_INTR_REMAP)) {
> > pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
> > __func__);
> > ret = -EPERM;
> > goto out_detach;
> > }
> > +#endif
> >
> > if (iommu_domain_has_cap(domain->domain, IOMMU_CAP_CACHE_COHERENCY))
> > domain->prot |= IOMMU_CACHE;
>
> This is not a PCI specific requirement. Anything that can support MSI
> needs an IOMMU that can provide isolation for both DMA and interrupts.
> I think the IOMMU should still be telling us that it has this feature.
Please excuse any ignorance on part here (I'm not at all familiar with the
Intel IOMMU), but shouldn't this really be a property of the interrupt
controller itself? On ARM with GICv3, there is a separate block called the
ITS (interrupt translation service) which is part of the interrupt
controller. The ITS provides a doorbell page which the SMMU can map into a
guest operating system to provide MSI for passthrough devices, but this
isn't something the SMMU is aware of -- it will just see the iommu_map
request for a non-cacheable mapping.
Will
On Mon, 2014-04-28 at 20:19 +0100, Will Deacon wrote:
> Hi Alex,
>
> On Mon, Apr 28, 2014 at 05:43:41PM +0100, Alex Williamson wrote:
> > On Mon, 2014-04-28 at 17:52 +0200, Antonios Motakis wrote:
> > > This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
> > > devices on ARM in addition to PCI. This is required in order to use the
> > > Exynos SMMU, or ARM SMMU driver with VFIO_IOMMU_TYPE1.
>
> [...]
>
> > > @@ -721,13 +722,15 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> > > INIT_LIST_HEAD(&domain->group_list);
> > > list_add(&group->next, &domain->group_list);
> > >
> > > - if (!allow_unsafe_interrupts &&
> > > +#ifdef CONFIG_PCI
> > > + if (bus == &pci_bus_type && !allow_unsafe_interrupts &&
> > > !iommu_domain_has_cap(domain->domain, IOMMU_CAP_INTR_REMAP)) {
> > > pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
> > > __func__);
> > > ret = -EPERM;
> > > goto out_detach;
> > > }
> > > +#endif
> > >
> > > if (iommu_domain_has_cap(domain->domain, IOMMU_CAP_CACHE_COHERENCY))
> > > domain->prot |= IOMMU_CACHE;
> >
> > This is not a PCI specific requirement. Anything that can support MSI
> > needs an IOMMU that can provide isolation for both DMA and interrupts.
> > I think the IOMMU should still be telling us that it has this feature.
>
> Please excuse any ignorance on part here (I'm not at all familiar with the
> Intel IOMMU), but shouldn't this really be a property of the interrupt
> controller itself? On ARM with GICv3, there is a separate block called the
> ITS (interrupt translation service) which is part of the interrupt
> controller. The ITS provides a doorbell page which the SMMU can map into a
> guest operating system to provide MSI for passthrough devices, but this
> isn't something the SMMU is aware of -- it will just see the iommu_map
> request for a non-cacheable mapping.
Hi Will,
I don't know the history of why this is an IOMMU domain capability on
x86, it's sort of a paradox. An MSI from a device is conceptually just
a DMA write and is therefore logically co-located in the IOMMU hardware,
but x86 doesn't allow it to be mapped via the IOMMU API interfaces. For
compatibility, interrupt remapping support is buried deep in the
request_irq interface and effectively invisible other than having this
path to query it. Therefore this flag is effectively just saying "MSI
isolation support is present and enabled". IOW, the host is protected
from interrupt injection attacks from malicious devices. If there is
some property of your platform that makes this always the case, then the
IOMMU driver can always export this capability as true.
With PCI, MSI is configured via spec defined configuration space
registers, so we emulate these registers and prevent user access to them
so that we don't need to allow the user a way to setup an interrupt
remapping entry. It's done for them via request_irq.
IIRC, the Freescale devices have a limited number of MSI pages and can
therefore create some instances with isolation while others may require
sharing. In that case I would expect this flag to indicate whether the
domain has an exclusive or shared page.
In any case, I suspect keying on the bus_type here is not the correct
way to go. Thanks,
Alex
On Mon, Apr 28, 2014 at 09:08:10PM +0100, Alex Williamson wrote:
> On Mon, 2014-04-28 at 20:19 +0100, Will Deacon wrote:
> > Please excuse any ignorance on part here (I'm not at all familiar with the
> > Intel IOMMU), but shouldn't this really be a property of the interrupt
> > controller itself? On ARM with GICv3, there is a separate block called the
> > ITS (interrupt translation service) which is part of the interrupt
> > controller. The ITS provides a doorbell page which the SMMU can map into a
> > guest operating system to provide MSI for passthrough devices, but this
> > isn't something the SMMU is aware of -- it will just see the iommu_map
> > request for a non-cacheable mapping.
>
> I don't know the history of why this is an IOMMU domain capability on
> x86, it's sort of a paradox. An MSI from a device is conceptually just
> a DMA write and is therefore logically co-located in the IOMMU hardware,
> but x86 doesn't allow it to be mapped via the IOMMU API interfaces. For
> compatibility, interrupt remapping support is buried deep in the
> request_irq interface and effectively invisible other than having this
> path to query it. Therefore this flag is effectively just saying "MSI
> isolation support is present and enabled". IOW, the host is protected
> from interrupt injection attacks from malicious devices. If there is
> some property of your platform that makes this always the case, then the
> IOMMU driver can always export this capability as true.
Thanks for the explanation. On ARM, the SMMU does indeed see the MSI write
just like a normal write, so it can be mapped via iommu_map() to point at
the interrupt controller doorbell page. I guess that means we can enable
this capability for all MSI-capable devices upstream of the SMMU, providing
that the IRQ controller doesn't have any horrible quirks.
> With PCI, MSI is configured via spec defined configuration space
> registers, so we emulate these registers and prevent user access to them
> so that we don't need to allow the user a way to setup an interrupt
> remapping entry. It's done for them via request_irq.
>
> IIRC, the Freescale devices have a limited number of MSI pages and can
> therefore create some instances with isolation while others may require
> sharing. In that case I would expect this flag to indicate whether the
> domain has an exclusive or shared page.
>
> In any case, I suspect keying on the bus_type here is not the correct
> way to go. Thanks,
Agreed, I was more intrigued by the meaning of the flag.
Thanks,
Will