[PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Alex Williamson]

IOMMU drivers should account for the platform's susceptibility to
DMA triggered MSI interrupts in creating IOMMU groups.  Skip
devices when the IOMMU can't isolate MSI from DMA, but allow
an iommu=group_unsafe_msi option for opt-in.  This removes the
leap in logic for users that IOMMU_CAP_INTR_REMAP is required for
interrupt security when they may be running on a non-x86 platform
that does not have this dependency.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 Documentation/kernel-parameters.txt |    9 ++++++---
 arch/ia64/include/asm/iommu.h       |    2 ++
 arch/ia64/kernel/pci-dma.c          |    1 +
 arch/x86/include/asm/iommu.h        |    1 +
 arch/x86/kernel/pci-dma.c           |   12 ++++++++++++
 drivers/iommu/amd_iommu.c           |    7 +++++++
 drivers/iommu/intel-iommu.c         |    7 +++++++
 7 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 9d5ac6c..ffa43b7 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1059,9 +1059,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 		nomerge
 		forcesac
 		soft
-		pt		[x86, IA-64]
-		group_mf	[x86, IA-64]
-
+		pt			[x86, IA-64]
+		group_mf		[x86, IA-64]
+			Group multifunction devices together in IOMMU API.
+		group_unsafe_msi	[x86, IA-64]
+			Allow grouping of devices even when the platfom
+			does not provide MSI isolation in IOMMU API.
 
 	io7=		[HW] IO7 for Marvel based alpha systems
 			See comment before marvel_specify_io7 in
diff --git a/arch/ia64/include/asm/iommu.h b/arch/ia64/include/asm/iommu.h
index b6a809f..b726572 100644
--- a/arch/ia64/include/asm/iommu.h
+++ b/arch/ia64/include/asm/iommu.h
@@ -12,11 +12,13 @@ extern int force_iommu, no_iommu;
 extern int iommu_pass_through;
 extern int iommu_detected;
 extern int iommu_group_mf;
+extern int iommu_group_unsafe_msi;
 #else
 #define iommu_pass_through	(0)
 #define no_iommu		(1)
 #define iommu_detected		(0)
 #define iommu_group_mf		(0)
+#define iommu_group_unsafe_msi	(0)
 #endif
 extern void iommu_dma_init(void);
 extern void machvec_init(const char *name);
diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
index eb11757..63721ce 100644
--- a/arch/ia64/kernel/pci-dma.c
+++ b/arch/ia64/kernel/pci-dma.c
@@ -34,6 +34,7 @@ int force_iommu __read_mostly;
 
 int iommu_pass_through;
 int iommu_group_mf;
+int iommu_group_unsafe_msi;
 
 /* Dummy device used for NULL arguments (normally ISA). Better would
    be probably a smaller DMA mask, but this is bug-to-bug compatible
diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
index dffc38e..e3d289c 100644
--- a/arch/x86/include/asm/iommu.h
+++ b/arch/x86/include/asm/iommu.h
@@ -6,6 +6,7 @@ extern int force_iommu, no_iommu;
 extern int iommu_detected;
 extern int iommu_pass_through;
 extern int iommu_group_mf;
+extern int iommu_group_unsafe_msi;
 
 /* 10 seconds */
 #define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 1c4d769..80eb6b6 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -54,6 +54,16 @@ int iommu_pass_through __read_mostly;
  */
 int iommu_group_mf __read_mostly;
 
+/*
+ * For the iommu_device_group interface, we not only need to be concerned
+ * with DMA isolation between devices, but also DMA isolation that could
+ * affect the platform, including MSI isolation.  If a platform is
+ * susceptible to malicious DMA writes triggering MSI interrupts, the
+ * IOMMU driver can't reasonably group devices.  This allows the iommu
+ * driver to ignore MSI isolation when creating groups.
+ */
+int iommu_group_unsafe_msi __read_mostly;
+
 extern struct iommu_table_entry __iommu_table[], __iommu_table_end[];
 
 /* Dummy device used for NULL arguments (normally ISA). */
@@ -180,6 +190,8 @@ static __init int iommu_setup(char *p)
 			iommu_pass_through = 1;
 		if (!strncmp(p, "group_mf", 8))
 			iommu_group_mf = 1;
+		if (!strncmp(p, "group_unsafe_msi", 16))
+			iommu_group_unsafe_msi = 1;
 
 		gart_parse_options(p);
 
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index ad074dc..5f55e7a 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2794,6 +2794,13 @@ static int amd_iommu_device_group(struct device *dev, unsigned int *groupid)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	u16 devid;
 
+	if (!iommu_group_unsafe_msi) {
+		printk_once(KERN_NOTICE "%s: "
+			    "IOMMU device group disabled without interrupt remapping support",
+			    pci_bus_type.name);
+		return -ENODEV;
+	}
+
 	if (!dev_data)
 		return -ENODEV;
 
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index e918f72..f3fcd69 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4094,6 +4094,13 @@ static int intel_iommu_device_group(struct device *dev, unsigned int *groupid)
 		u32 group;
 	} id;
 
+	if (!intr_remapping_enabled && !iommu_group_unsafe_msi) {
+		printk_once(KERN_NOTICE "%s: "
+			    "IOMMU device group disabled without interrupt remapping support",
+			    pci_bus_type.name);
+		return -ENODEV;
+	}
+
 	if (iommu_no_mapping(dev))
 		return -ENODEV;
 

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Kai Huang]

On Fri, Nov 18, 2011 at 1:09 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> IOMMU drivers should account for the platform's susceptibility to
> DMA triggered MSI interrupts in creating IOMMU groups.  Skip
> devices when the IOMMU can't isolate MSI from DMA, but allow
> an iommu=group_unsafe_msi option for opt-in.  This removes the
> leap in logic for users that IOMMU_CAP_INTR_REMAP is required for
> interrupt security when they may be running on a non-x86 platform
> that does not have this dependency.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
>  Documentation/kernel-parameters.txt |    9 ++++++---
>  arch/ia64/include/asm/iommu.h       |    2 ++
>  arch/ia64/kernel/pci-dma.c          |    1 +
>  arch/x86/include/asm/iommu.h        |    1 +
>  arch/x86/kernel/pci-dma.c           |   12 ++++++++++++
>  drivers/iommu/amd_iommu.c           |    7 +++++++
>  drivers/iommu/intel-iommu.c         |    7 +++++++
>  7 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 9d5ac6c..ffa43b7 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1059,9 +1059,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>                nomerge
>                forcesac
>                soft
> -               pt              [x86, IA-64]
> -               group_mf        [x86, IA-64]
> -
> +               pt                      [x86, IA-64]
> +               group_mf                [x86, IA-64]
> +                       Group multifunction devices together in IOMMU API.
> +               group_unsafe_msi        [x86, IA-64]
> +                       Allow grouping of devices even when the platfom
> +                       does not provide MSI isolation in IOMMU API.
>
>        io7=            [HW] IO7 for Marvel based alpha systems
>                        See comment before marvel_specify_io7 in
> diff --git a/arch/ia64/include/asm/iommu.h b/arch/ia64/include/asm/iommu.h
> index b6a809f..b726572 100644
> --- a/arch/ia64/include/asm/iommu.h
> +++ b/arch/ia64/include/asm/iommu.h
> @@ -12,11 +12,13 @@ extern int force_iommu, no_iommu;
>  extern int iommu_pass_through;
>  extern int iommu_detected;
>  extern int iommu_group_mf;
> +extern int iommu_group_unsafe_msi;
>  #else
>  #define iommu_pass_through     (0)
>  #define no_iommu               (1)
>  #define iommu_detected         (0)
>  #define iommu_group_mf         (0)
> +#define iommu_group_unsafe_msi (0)
>  #endif
>  extern void iommu_dma_init(void);
>  extern void machvec_init(const char *name);
> diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
> index eb11757..63721ce 100644
> --- a/arch/ia64/kernel/pci-dma.c
> +++ b/arch/ia64/kernel/pci-dma.c
> @@ -34,6 +34,7 @@ int force_iommu __read_mostly;
>
>  int iommu_pass_through;
>  int iommu_group_mf;
> +int iommu_group_unsafe_msi;
>
>  /* Dummy device used for NULL arguments (normally ISA). Better would
>    be probably a smaller DMA mask, but this is bug-to-bug compatible
> diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
> index dffc38e..e3d289c 100644
> --- a/arch/x86/include/asm/iommu.h
> +++ b/arch/x86/include/asm/iommu.h
> @@ -6,6 +6,7 @@ extern int force_iommu, no_iommu;
>  extern int iommu_detected;
>  extern int iommu_pass_through;
>  extern int iommu_group_mf;
> +extern int iommu_group_unsafe_msi;
>
>  /* 10 seconds */
>  #define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index 1c4d769..80eb6b6 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -54,6 +54,16 @@ int iommu_pass_through __read_mostly;
>  */
>  int iommu_group_mf __read_mostly;
>
> +/*
> + * For the iommu_device_group interface, we not only need to be concerned
> + * with DMA isolation between devices, but also DMA isolation that could
> + * affect the platform, including MSI isolation.  If a platform is
> + * susceptible to malicious DMA writes triggering MSI interrupts, the
> + * IOMMU driver can't reasonably group devices.  This allows the iommu
> + * driver to ignore MSI isolation when creating groups.
> + */
> +int iommu_group_unsafe_msi __read_mostly;
> +
>  extern struct iommu_table_entry __iommu_table[], __iommu_table_end[];
>
>  /* Dummy device used for NULL arguments (normally ISA). */
> @@ -180,6 +190,8 @@ static __init int iommu_setup(char *p)
>                        iommu_pass_through = 1;
>                if (!strncmp(p, "group_mf", 8))
>                        iommu_group_mf = 1;
> +               if (!strncmp(p, "group_unsafe_msi", 16))
> +                       iommu_group_unsafe_msi = 1;
>
>                gart_parse_options(p);
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index ad074dc..5f55e7a 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2794,6 +2794,13 @@ static int amd_iommu_device_group(struct device *dev, unsigned int *groupid)
>        struct pci_dev *pdev = to_pci_dev(dev);
>        u16 devid;
>
> +       if (!iommu_group_unsafe_msi) {
> +               printk_once(KERN_NOTICE "%s: "
> +                           "IOMMU device group disabled without interrupt remapping support",
> +                           pci_bus_type.name);
> +               return -ENODEV;
> +       }
> +
>        if (!dev_data)
>                return -ENODEV;
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index e918f72..f3fcd69 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4094,6 +4094,13 @@ static int intel_iommu_device_group(struct device *dev, unsigned int *groupid)
>                u32 group;
>        } id;
>
> +       if (!intr_remapping_enabled && !iommu_group_unsafe_msi) {
> +               printk_once(KERN_NOTICE "%s: "
> +                           "IOMMU device group disabled without interrupt remapping support",
> +                           pci_bus_type.name);
> +               return -ENODEV;
> +       }
> +

When intr_remapping_enabled == 0 and iommu_group_unsafe_msi == 1, is
it better to add an print msg that notice user we enabled the group by
allowing unsafe msi but without hardware interrupt remapping support?
Or have we added such msg at some other place?

-cody

>        if (iommu_no_mapping(dev))
>                return -ENODEV;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Alex Williamson]

On Fri, 2011-11-18 at 12:37 +0800, Kai Huang wrote:
> On Fri, Nov 18, 2011 at 1:09 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > IOMMU drivers should account for the platform's susceptibility to
> > DMA triggered MSI interrupts in creating IOMMU groups.  Skip
> > devices when the IOMMU can't isolate MSI from DMA, but allow
> > an iommu=group_unsafe_msi option for opt-in.  This removes the
> > leap in logic for users that IOMMU_CAP_INTR_REMAP is required for
> > interrupt security when they may be running on a non-x86 platform
> > that does not have this dependency.
> >
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >
> >  Documentation/kernel-parameters.txt |    9 ++++++---
> >  arch/ia64/include/asm/iommu.h       |    2 ++
> >  arch/ia64/kernel/pci-dma.c          |    1 +
> >  arch/x86/include/asm/iommu.h        |    1 +
> >  arch/x86/kernel/pci-dma.c           |   12 ++++++++++++
> >  drivers/iommu/amd_iommu.c           |    7 +++++++
> >  drivers/iommu/intel-iommu.c         |    7 +++++++
> >  7 files changed, 36 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index 9d5ac6c..ffa43b7 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -1059,9 +1059,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> >                nomerge
> >                forcesac
> >                soft
> > -               pt              [x86, IA-64]
> > -               group_mf        [x86, IA-64]
> > -
> > +               pt                      [x86, IA-64]
> > +               group_mf                [x86, IA-64]
> > +                       Group multifunction devices together in IOMMU API.
> > +               group_unsafe_msi        [x86, IA-64]
> > +                       Allow grouping of devices even when the platfom
> > +                       does not provide MSI isolation in IOMMU API.
> >
> >        io7=            [HW] IO7 for Marvel based alpha systems
> >                        See comment before marvel_specify_io7 in
> > diff --git a/arch/ia64/include/asm/iommu.h b/arch/ia64/include/asm/iommu.h
> > index b6a809f..b726572 100644
> > --- a/arch/ia64/include/asm/iommu.h
> > +++ b/arch/ia64/include/asm/iommu.h
> > @@ -12,11 +12,13 @@ extern int force_iommu, no_iommu;
> >  extern int iommu_pass_through;
> >  extern int iommu_detected;
> >  extern int iommu_group_mf;
> > +extern int iommu_group_unsafe_msi;
> >  #else
> >  #define iommu_pass_through     (0)
> >  #define no_iommu               (1)
> >  #define iommu_detected         (0)
> >  #define iommu_group_mf         (0)
> > +#define iommu_group_unsafe_msi (0)
> >  #endif
> >  extern void iommu_dma_init(void);
> >  extern void machvec_init(const char *name);
> > diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
> > index eb11757..63721ce 100644
> > --- a/arch/ia64/kernel/pci-dma.c
> > +++ b/arch/ia64/kernel/pci-dma.c
> > @@ -34,6 +34,7 @@ int force_iommu __read_mostly;
> >
> >  int iommu_pass_through;
> >  int iommu_group_mf;
> > +int iommu_group_unsafe_msi;
> >
> >  /* Dummy device used for NULL arguments (normally ISA). Better would
> >    be probably a smaller DMA mask, but this is bug-to-bug compatible
> > diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
> > index dffc38e..e3d289c 100644
> > --- a/arch/x86/include/asm/iommu.h
> > +++ b/arch/x86/include/asm/iommu.h
> > @@ -6,6 +6,7 @@ extern int force_iommu, no_iommu;
> >  extern int iommu_detected;
> >  extern int iommu_pass_through;
> >  extern int iommu_group_mf;
> > +extern int iommu_group_unsafe_msi;
> >
> >  /* 10 seconds */
> >  #define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
> > diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> > index 1c4d769..80eb6b6 100644
> > --- a/arch/x86/kernel/pci-dma.c
> > +++ b/arch/x86/kernel/pci-dma.c
> > @@ -54,6 +54,16 @@ int iommu_pass_through __read_mostly;
> >  */
> >  int iommu_group_mf __read_mostly;
> >
> > +/*
> > + * For the iommu_device_group interface, we not only need to be concerned
> > + * with DMA isolation between devices, but also DMA isolation that could
> > + * affect the platform, including MSI isolation.  If a platform is
> > + * susceptible to malicious DMA writes triggering MSI interrupts, the
> > + * IOMMU driver can't reasonably group devices.  This allows the iommu
> > + * driver to ignore MSI isolation when creating groups.
> > + */
> > +int iommu_group_unsafe_msi __read_mostly;
> > +
> >  extern struct iommu_table_entry __iommu_table[], __iommu_table_end[];
> >
> >  /* Dummy device used for NULL arguments (normally ISA). */
> > @@ -180,6 +190,8 @@ static __init int iommu_setup(char *p)
> >                        iommu_pass_through = 1;
> >                if (!strncmp(p, "group_mf", 8))
> >                        iommu_group_mf = 1;
> > +               if (!strncmp(p, "group_unsafe_msi", 16))
> > +                       iommu_group_unsafe_msi = 1;
> >
> >                gart_parse_options(p);
> >
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index ad074dc..5f55e7a 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -2794,6 +2794,13 @@ static int amd_iommu_device_group(struct device *dev, unsigned int *groupid)
> >        struct pci_dev *pdev = to_pci_dev(dev);
> >        u16 devid;
> >
> > +       if (!iommu_group_unsafe_msi) {
> > +               printk_once(KERN_NOTICE "%s: "
> > +                           "IOMMU device group disabled without interrupt remapping support",
> > +                           pci_bus_type.name);
> > +               return -ENODEV;
> > +       }
> > +
> >        if (!dev_data)
> >                return -ENODEV;
> >
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index e918f72..f3fcd69 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -4094,6 +4094,13 @@ static int intel_iommu_device_group(struct device *dev, unsigned int *groupid)
> >                u32 group;
> >        } id;
> >
> > +       if (!intr_remapping_enabled && !iommu_group_unsafe_msi) {
> > +               printk_once(KERN_NOTICE "%s: "
> > +                           "IOMMU device group disabled without interrupt remapping support",
> > +                           pci_bus_type.name);
> > +               return -ENODEV;
> > +       }
> > +
> 
> When intr_remapping_enabled == 0 and iommu_group_unsafe_msi == 1, is
> it better to add an print msg that notice user we enabled the group by
> allowing unsafe msi but without hardware interrupt remapping support?
> Or have we added such msg at some other place?

My system prints "Enabled IRQ remapping in xapic mode" when interrupt
remapping is enabled.  So there is some evidence in the messages when
interrupt remapping is actually present.  The command line option to
opt-in to the override will also appear in "Kernel command line: ...
iommu=group_unsafe_msi ...".  So while there's not some kind of
"Enabling IOMMU device groups against the better judgment of the
hardware..." message, there are clues.  Do you think it's necessary to
be more verbose?  Thanks,

Alex

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Kai Huang]

On Fri, Nov 18, 2011 at 1:40 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Fri, 2011-11-18 at 12:37 +0800, Kai Huang wrote:
>> On Fri, Nov 18, 2011 at 1:09 AM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> > IOMMU drivers should account for the platform's susceptibility to
>> > DMA triggered MSI interrupts in creating IOMMU groups.  Skip
>> > devices when the IOMMU can't isolate MSI from DMA, but allow
>> > an iommu=group_unsafe_msi option for opt-in.  This removes the
>> > leap in logic for users that IOMMU_CAP_INTR_REMAP is required for
>> > interrupt security when they may be running on a non-x86 platform
>> > that does not have this dependency.
>> >
>> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> > ---
>> >
>> >  Documentation/kernel-parameters.txt |    9 ++++++---
>> >  arch/ia64/include/asm/iommu.h       |    2 ++
>> >  arch/ia64/kernel/pci-dma.c          |    1 +
>> >  arch/x86/include/asm/iommu.h        |    1 +
>> >  arch/x86/kernel/pci-dma.c           |   12 ++++++++++++
>> >  drivers/iommu/amd_iommu.c           |    7 +++++++
>> >  drivers/iommu/intel-iommu.c         |    7 +++++++
>> >  7 files changed, 36 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> > index 9d5ac6c..ffa43b7 100644
>> > --- a/Documentation/kernel-parameters.txt
>> > +++ b/Documentation/kernel-parameters.txt
>> > @@ -1059,9 +1059,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>> >                nomerge
>> >                forcesac
>> >                soft
>> > -               pt              [x86, IA-64]
>> > -               group_mf        [x86, IA-64]
>> > -
>> > +               pt                      [x86, IA-64]
>> > +               group_mf                [x86, IA-64]
>> > +                       Group multifunction devices together in IOMMU API.
>> > +               group_unsafe_msi        [x86, IA-64]
>> > +                       Allow grouping of devices even when the platfom
>> > +                       does not provide MSI isolation in IOMMU API.
>> >
>> >        io7=            [HW] IO7 for Marvel based alpha systems
>> >                        See comment before marvel_specify_io7 in
>> > diff --git a/arch/ia64/include/asm/iommu.h b/arch/ia64/include/asm/iommu.h
>> > index b6a809f..b726572 100644
>> > --- a/arch/ia64/include/asm/iommu.h
>> > +++ b/arch/ia64/include/asm/iommu.h
>> > @@ -12,11 +12,13 @@ extern int force_iommu, no_iommu;
>> >  extern int iommu_pass_through;
>> >  extern int iommu_detected;
>> >  extern int iommu_group_mf;
>> > +extern int iommu_group_unsafe_msi;
>> >  #else
>> >  #define iommu_pass_through     (0)
>> >  #define no_iommu               (1)
>> >  #define iommu_detected         (0)
>> >  #define iommu_group_mf         (0)
>> > +#define iommu_group_unsafe_msi (0)
>> >  #endif
>> >  extern void iommu_dma_init(void);
>> >  extern void machvec_init(const char *name);
>> > diff --git a/arch/ia64/kernel/pci-dma.c b/arch/ia64/kernel/pci-dma.c
>> > index eb11757..63721ce 100644
>> > --- a/arch/ia64/kernel/pci-dma.c
>> > +++ b/arch/ia64/kernel/pci-dma.c
>> > @@ -34,6 +34,7 @@ int force_iommu __read_mostly;
>> >
>> >  int iommu_pass_through;
>> >  int iommu_group_mf;
>> > +int iommu_group_unsafe_msi;
>> >
>> >  /* Dummy device used for NULL arguments (normally ISA). Better would
>> >    be probably a smaller DMA mask, but this is bug-to-bug compatible
>> > diff --git a/arch/x86/include/asm/iommu.h b/arch/x86/include/asm/iommu.h
>> > index dffc38e..e3d289c 100644
>> > --- a/arch/x86/include/asm/iommu.h
>> > +++ b/arch/x86/include/asm/iommu.h
>> > @@ -6,6 +6,7 @@ extern int force_iommu, no_iommu;
>> >  extern int iommu_detected;
>> >  extern int iommu_pass_through;
>> >  extern int iommu_group_mf;
>> > +extern int iommu_group_unsafe_msi;
>> >
>> >  /* 10 seconds */
>> >  #define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
>> > diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
>> > index 1c4d769..80eb6b6 100644
>> > --- a/arch/x86/kernel/pci-dma.c
>> > +++ b/arch/x86/kernel/pci-dma.c
>> > @@ -54,6 +54,16 @@ int iommu_pass_through __read_mostly;
>> >  */
>> >  int iommu_group_mf __read_mostly;
>> >
>> > +/*
>> > + * For the iommu_device_group interface, we not only need to be concerned
>> > + * with DMA isolation between devices, but also DMA isolation that could
>> > + * affect the platform, including MSI isolation.  If a platform is
>> > + * susceptible to malicious DMA writes triggering MSI interrupts, the
>> > + * IOMMU driver can't reasonably group devices.  This allows the iommu
>> > + * driver to ignore MSI isolation when creating groups.
>> > + */
>> > +int iommu_group_unsafe_msi __read_mostly;
>> > +
>> >  extern struct iommu_table_entry __iommu_table[], __iommu_table_end[];
>> >
>> >  /* Dummy device used for NULL arguments (normally ISA). */
>> > @@ -180,6 +190,8 @@ static __init int iommu_setup(char *p)
>> >                        iommu_pass_through = 1;
>> >                if (!strncmp(p, "group_mf", 8))
>> >                        iommu_group_mf = 1;
>> > +               if (!strncmp(p, "group_unsafe_msi", 16))
>> > +                       iommu_group_unsafe_msi = 1;
>> >
>> >                gart_parse_options(p);
>> >
>> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> > index ad074dc..5f55e7a 100644
>> > --- a/drivers/iommu/amd_iommu.c
>> > +++ b/drivers/iommu/amd_iommu.c
>> > @@ -2794,6 +2794,13 @@ static int amd_iommu_device_group(struct device *dev, unsigned int *groupid)
>> >        struct pci_dev *pdev = to_pci_dev(dev);
>> >        u16 devid;
>> >
>> > +       if (!iommu_group_unsafe_msi) {
>> > +               printk_once(KERN_NOTICE "%s: "
>> > +                           "IOMMU device group disabled without interrupt remapping support",
>> > +                           pci_bus_type.name);
>> > +               return -ENODEV;
>> > +       }
>> > +
>> >        if (!dev_data)
>> >                return -ENODEV;
>> >
>> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> > index e918f72..f3fcd69 100644
>> > --- a/drivers/iommu/intel-iommu.c
>> > +++ b/drivers/iommu/intel-iommu.c
>> > @@ -4094,6 +4094,13 @@ static int intel_iommu_device_group(struct device *dev, unsigned int *groupid)
>> >                u32 group;
>> >        } id;
>> >
>> > +       if (!intr_remapping_enabled && !iommu_group_unsafe_msi) {
>> > +               printk_once(KERN_NOTICE "%s: "
>> > +                           "IOMMU device group disabled without interrupt remapping support",
>> > +                           pci_bus_type.name);
>> > +               return -ENODEV;
>> > +       }
>> > +
>>
>> When intr_remapping_enabled == 0 and iommu_group_unsafe_msi == 1, is
>> it better to add an print msg that notice user we enabled the group by
>> allowing unsafe msi but without hardware interrupt remapping support?
>> Or have we added such msg at some other place?
>
> My system prints "Enabled IRQ remapping in xapic mode" when interrupt
> remapping is enabled.  So there is some evidence in the messages when
> interrupt remapping is actually present.  The command line option to
> opt-in to the override will also appear in "Kernel command line: ...
> iommu=group_unsafe_msi ...".  So while there's not some kind of
> "Enabling IOMMU device groups against the better judgment of the
> hardware..." message, there are clues.  Do you think it's necessary to
> be more verbose?  Thanks,

More verbose will not bring any harm:)  Anyway it's just an
suggestion, I am OK with current patch.

-cody

>
> Alex
>
>

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Joerg Roedel]

On Thu, Nov 17, 2011 at 10:09:26AM -0700, Alex Williamson wrote:
> IOMMU drivers should account for the platform's susceptibility to
> DMA triggered MSI interrupts in creating IOMMU groups.  Skip
> devices when the IOMMU can't isolate MSI from DMA, but allow
> an iommu=group_unsafe_msi option for opt-in.  This removes the
> leap in logic for users that IOMMU_CAP_INTR_REMAP is required for
> interrupt security when they may be running on a non-x86 platform
> that does not have this dependency.

I actually don't see the point in this. The iommu-group thing is to tell
user-space what devices the IOMMU can safely distinguish between. The
absence of interrupt-remapping changes nothing to that grouping. So why
remove it when interrupt remapping is not enabled?

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Alex Williamson]

On Fri, 2011-11-18 at 11:46 +0100, Joerg Roedel wrote:
> On Thu, Nov 17, 2011 at 10:09:26AM -0700, Alex Williamson wrote:
> > IOMMU drivers should account for the platform's susceptibility to
> > DMA triggered MSI interrupts in creating IOMMU groups.  Skip
> > devices when the IOMMU can't isolate MSI from DMA, but allow
> > an iommu=group_unsafe_msi option for opt-in.  This removes the
> > leap in logic for users that IOMMU_CAP_INTR_REMAP is required for
> > interrupt security when they may be running on a non-x86 platform
> > that does not have this dependency.
> 
> I actually don't see the point in this. The iommu-group thing is to tell
> user-space what devices the IOMMU can safely distinguish between. The
> absence of interrupt-remapping changes nothing to that grouping. So why
> remove it when interrupt remapping is not enabled?

As you say, the interface is to tell userspace what devices the IOMMU
can safely distinguish between.  On x86, the IOMMU cannot distinguish
DMA to the interrupt region between devices without interrupt remapping.
Therefore, to only expose devices we can safely distinguish between, we
shouldn't expose any devices by default if we don't provide this
isolation.  The interrupt remapping capability of the IOMMU isn't
exposed to userspace, nor should userspace need to make the leap in
understanding how MSIs are generated on a specific platform and whether
interrupt remapping is required to provide isolation.  Thanks,

Alex

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Joerg Roedel]

On Fri, Nov 18, 2011 at 07:56:30AM -0700, Alex Williamson wrote:
> As you say, the interface is to tell userspace what devices the IOMMU
> can safely distinguish between.  On x86, the IOMMU cannot distinguish
> DMA to the interrupt region between devices without interrupt remapping.
> Therefore, to only expose devices we can safely distinguish between, we
> shouldn't expose any devices by default if we don't provide this
> isolation.  The interrupt remapping capability of the IOMMU isn't
> exposed to userspace, nor should userspace need to make the leap in
> understanding how MSIs are generated on a specific platform and whether
> interrupt remapping is required to provide isolation.

But the isolation domains are different. The iommu-group interface is
about device isolation and interrupt remapping is about DMA address
space isolation (MSI range vs. the rest). The iommu can still
distinguish between devices when it can not distinguish between DMA and
MSI interrupts.
So what you want to do belongs to the VFIO driver or (as it is today)
into the KVM device assignment code and not into the iommu-group
interface.

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Alex Williamson]

On Fri, 2011-11-18 at 16:27 +0100, Joerg Roedel wrote:
> On Fri, Nov 18, 2011 at 07:56:30AM -0700, Alex Williamson wrote:
> > As you say, the interface is to tell userspace what devices the IOMMU
> > can safely distinguish between.  On x86, the IOMMU cannot distinguish
> > DMA to the interrupt region between devices without interrupt remapping.
> > Therefore, to only expose devices we can safely distinguish between, we
> > shouldn't expose any devices by default if we don't provide this
> > isolation.  The interrupt remapping capability of the IOMMU isn't
> > exposed to userspace, nor should userspace need to make the leap in
> > understanding how MSIs are generated on a specific platform and whether
> > interrupt remapping is required to provide isolation.
> 
> But the isolation domains are different. The iommu-group interface is
> about device isolation and interrupt remapping is about DMA address
> space isolation (MSI range vs. the rest). The iommu can still
> distinguish between devices when it can not distinguish between DMA and
> MSI interrupts.

I guess I fail to see the difference.  We group devices behind certain
bridges together because we can't distinguish DMA from those devices.
MSI presents an address window across all devices for which we
potentially can't distinguish between any of them.  If instead of MSI
the window was VGA and the IOMMU could differentiate all devices, so
long as they don't do DMA to 0xa0000-0xc0000, what would we do?  I think
we'd have to exclude such an IOMMU from providing grouping information.

> So what you want to do belongs to the VFIO driver or (as it is today)
> into the KVM device assignment code and not into the iommu-group
> interface.

The trouble is that interrupt remapping closing a hole in DMA isolation
is a platform issue.  Is vfio supposed to know that on architecture foo
we don't have such a hole and we don't need to look for interrupt
remapping.  Or maybe that platform bar solved it differently and we need
to instead check flag MSI_OK.  Current KVM doesn't care about this
because it only does device assignment on x86.

Another way to solve this might be to expose a more generic capability
for the IOMMU.  Instead of exposing "interrupt remapping support" it
could expose "full DMA isolation", then amd_iommu/intel-iommu could set
it based on interrupt remapping capability.  The trouble is that then we
expose grouping information for devices, but can't figure out the domain
capabilities until we actually create a domain and attach those devices,
which is kind of a crappy experience for a vfio user to find out they
don't actually get to use those device.

Instead, it seems like the device-group interface should only provide
grouping information for devices with full DMA isolation, and hide the
rest.  Then we know that if iommu_device_group() returns an error, we
can't safely use that device.  Thanks,

Alex

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Joerg Roedel]

On Fri, Nov 18, 2011 at 09:32:36AM -0700, Alex Williamson wrote:
> I guess I fail to see the difference.  We group devices behind certain
> bridges together because we can't distinguish DMA from those devices.
> MSI presents an address window across all devices for which we
> potentially can't distinguish between any of them.

With an IOMMU the address window is per-device and not shared between
all devices. A MSI message is nothing more than a DMA write transaction
to a specific address. This message has a requestor-id so an IOMMU can
distinguish between devices. The AMD IOMMU for example uses that to
implement per-device remapping tables.

> The trouble is that interrupt remapping closing a hole in DMA isolation
> is a platform issue.  Is vfio supposed to know that on architecture foo
> we don't have such a hole and we don't need to look for interrupt
> remapping.  Or maybe that platform bar solved it differently and we need
> to instead check flag MSI_OK.  Current KVM doesn't care about this
> because it only does device assignment on x86.

>From device standpoint a MSI transaction is always a DMA memory write
to a given address range. The IOMMU-API should export a feature flag
whether it supports filtering on those transaction or not. We have that
today with the IOMMU_CAP_INTR_REMAP. I agree that the interface to get
this information is ugly because a domain is needed. But the interface
can be fixed. While doing this I suggest to rename that feature
IOMMU_CAP_INTR_ISOLATION or something like that.
VFIO can then check for this flag on module-load and refuse to load if
it is not available.

Regards,

	Joerg

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Kai Huang]

On Sun, Nov 20, 2011 at 8:00 PM, Joerg Roedel <joro@8bytes.org> wrote:
> On Fri, Nov 18, 2011 at 09:32:36AM -0700, Alex Williamson wrote:
>> I guess I fail to see the difference.  We group devices behind certain
>> bridges together because we can't distinguish DMA from those devices.
>> MSI presents an address window across all devices for which we
>> potentially can't distinguish between any of them.
>
> With an IOMMU the address window is per-device and not shared between
> all devices. A MSI message is nothing more than a DMA write transaction
> to a specific address. This message has a requestor-id so an IOMMU can
> distinguish between devices. The AMD IOMMU for example uses that to
> implement per-device remapping tables.

In my understanding the Interrupt Remapping provides device isolation
from the security perspective. The VFIO framework is designed to
"expose direct device access to userspace, in a secure, IOMMU
protected environment" (copied from vfio.txt, I am not familiar with
vfio). Without Interrupt Remapping, an spurious interrupt issued by
user space driver may crash other groups or the whole system entirely.
>From this point, I think it is reasonable to disable group without
Interrupt Remapping support in IOMMU, or at least give user a notice.

-cody

>
>> The trouble is that interrupt remapping closing a hole in DMA isolation
>> is a platform issue.  Is vfio supposed to know that on architecture foo
>> we don't have such a hole and we don't need to look for interrupt
>> remapping.  Or maybe that platform bar solved it differently and we need
>> to instead check flag MSI_OK.  Current KVM doesn't care about this
>> because it only does device assignment on x86.
>
> From device standpoint a MSI transaction is always a DMA memory write
> to a given address range. The IOMMU-API should export a feature flag
> whether it supports filtering on those transaction or not. We have that
> today with the IOMMU_CAP_INTR_REMAP. I agree that the interface to get
> this information is ugly because a domain is needed. But the interface
> can be fixed. While doing this I suggest to rename that feature
> IOMMU_CAP_INTR_ISOLATION or something like that.
> VFIO can then check for this flag on module-load and refuse to load if
> it is not available.
>
> Regards,
>
>        Joerg
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Chris Wright]

* Joerg Roedel (joro@8bytes.org) wrote:
> >From device standpoint a MSI transaction is always a DMA memory write
> to a given address range. The IOMMU-API should export a feature flag
> whether it supports filtering on those transaction or not. We have that
> today with the IOMMU_CAP_INTR_REMAP. I agree that the interface to get
> this information is ugly because a domain is needed. But the interface
> can be fixed. While doing this I suggest to rename that feature
> IOMMU_CAP_INTR_ISOLATION or something like that.
> VFIO can then check for this flag on module-load and refuse to load if
> it is not available.

I can see that the native grouping (the typical pci bridge type) is
really more a property of the topology.

The isolation properties of a group (arguably the whole point of the
group) is subtly different.

Leaves the questions:

What is the value of a group w/out complete isolation?
Is there a practical problem w/ conflating the subtleties above?

thanks,
-chris

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Joerg Roedel]

On Mon, Nov 21, 2011 at 03:35:05PM -0800, Chris Wright wrote:

> What is the value of a group w/out complete isolation?

There is still isolation for DMA. This may be sufficient for non-KVM
use-cases like a device driver partially implemented in userspace. There
is no no guest then that can attack the host with wrong interrupts.

> Is there a practical problem w/ conflating the subtleties above?

Same argument as above. It ties the the iommu_group interface to the KVM
use case. Another more pratical impact of this patch is that a reboot is
required to re-enable iommu-groups. When the check happens in VFIO it is
a simple module-reload.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Chris Wright]

* Joerg Roedel (joerg.roedel@amd.com) wrote:
> On Mon, Nov 21, 2011 at 03:35:05PM -0800, Chris Wright wrote:
> 
> > What is the value of a group w/out complete isolation?
> 
> There is still isolation for DMA. This may be sufficient for non-KVM
> use-cases like a device driver partially implemented in userspace. There
> is no no guest then that can attack the host with wrong interrupts.

There is a userspace process that could though.  I think I'm missing
the distinction.  In either case there is unprivileged code that could
program the hw to generate PCI write transactions that negatively effect
the system.

> > Is there a practical problem w/ conflating the subtleties above?
> 
> Same argument as above. It ties the the iommu_group interface to the KVM
> use case.

I don't agree that it's the KVM use case.  It's the unprivileged code
owning a device use case.  The promise of SR-IOV + IOMMU + PASID shows
hw is trying to go there.

> Another more pratical impact of this patch is that a reboot is
> required to re-enable iommu-groups. When the check happens in VFIO it is
> a simple module-reload.

I suppose, however iommu itself is managed via kernel cmdline and
reboot...

I guess we agree that we need to be able to give the user some way of
managing the risk they're willing to take, and just not on where the
flag should go?

thanks,
-chris

Re: [PATCH] iommu: Include MSI susceptibility to DMA in creating iommu groups

[Alex Williamson]

On Mon, 2011-11-21 at 15:35 -0800, Chris Wright wrote:
> * Joerg Roedel (joro@8bytes.org) wrote:
> > >From device standpoint a MSI transaction is always a DMA memory write
> > to a given address range. The IOMMU-API should export a feature flag
> > whether it supports filtering on those transaction or not. We have that
> > today with the IOMMU_CAP_INTR_REMAP. I agree that the interface to get
> > this information is ugly because a domain is needed. But the interface
> > can be fixed. While doing this I suggest to rename that feature
> > IOMMU_CAP_INTR_ISOLATION or something like that.
> > VFIO can then check for this flag on module-load and refuse to load if
> > it is not available.
> 
> I can see that the native grouping (the typical pci bridge type) is
> really more a property of the topology.
> 
> The isolation properties of a group (arguably the whole point of the
> group) is subtly different.

Yes, there is a subtle difference there, maybe that's what we're
tripping over.  I see the group as an assertion by the iommu driver that
it can distinguish and isolate the set of devices within that group from
other groups and shared resources.  For instance, numerous systems
include hardware iommus that provide only translation and not isolation
(DMA is translated through the IOVA window or allowed direct to memory).
That doesn't mean they should implement a device_group callback that
statically returns a single groupid, that means they should not
implement device_group.  I'm afraid the meaning of a group will be lost
if we allow iommus to define groups, but then add flags saying "oh, but
we don't isolate ________".  I much prefer we enable a user to opt-in at
the iommu driver level than weaken the definition of a group.  Thanks,

Alex