[PATCH iommufd 0/9] Remove IOMMU_CAP_INTR_REMAP

[PATCH iommufd 0/9] Remove IOMMU_CAP_INTR_REMAP

[Jason Gunthorpe]

[ This would be for v6.3, the series depends on a bunch of stuff in
linux-next. I would be happy to merge it through the iommfd tree ]

Currently the kernel has two ways to signal the "secure MSI" concept that
IOMMU_CAP_INTR_REMAP and irq_domain_check_msi_remap() both lay claim to.

Harmonize these into a single irq_domain based check under
msi_device_has_secure_msi().

In real HW "secure MSI" is implemented in a few different ways:

 - x86 uses "interrupt remapping" which is a block that sits between
   the device and APIC, that can "remap" the MSI MemWr using per-RID
   tables. Part of the remapping is discarding, the per-RID tables
   will not contain vectors that have not been enabled for the device.

 - ARM GICv3 ITS integrates the concept of an out-of-band "device ID"
   directly into the interrupt controller logic. The tables the GIC checks
   that determine how to deliver the interrupt through the ITS device table
   and interrupt translation tables allow limiting which interrupts device
   IDs can trigger.

 - S390 has unconditionally claimed it has secure MSI through the iommu
   driver. I'm not sure how it works, or if it even does. Perhaps
   zpci_set_airq() pushes the "zdev->gias" to the hypervisor which
   limits a device's MSI to only certain KVM contexts (though if true
   this would be considered insecure by VFIO)

After this series the "secure MSI" is tagged based only on the irq_domains
that the interrupt travels through. For x86 enabling interrupt remapping
causes IR irq_domains to be installed in the path, and they can carry the
IRQ_DOMAIN_FLAG_SECURE_MSI. For ARM the GICv3 ITS itself already sets the
flag when it is running in a secure mode, and S390 simply sets it always
through an arch hook since it doesn't use irq_domains at all.

This removes the intrusion of entirely IRQ subsystem information into the
iommu layer. Linux's iommu_domains abstraction has no bearing at all on
the security of MSI. Even if HW linked to the IOMMU may implement the
security on x86 implementations, Linux models that HW through the
irq_domain, not the iommu_domain.

This is on github: https://github.com/jgunthorpe/linux/commits/secure_msi

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Jason Gunthorpe (9):
  irq: Add msi_device_has_secure_msi()
  vfio/type1: Check that every device supports IOMMU_CAP_INTR_REMAP
  vfio/type1: Convert to msi_device_has_secure_msi()
  iommufd: Convert to msi_device_has_secure_msi()
  irq: Remove unused irq_domain_check_msi_remap() code
  irq: Rename MSI_REMAP to SECURE_MSI
  iommu/x86: Replace IOMMU_CAP_INTR_REMAP with
    IRQ_DOMAIN_FLAG_SECURE_MSI
  irq/s390: Add arch_is_secure_msi() for s390
  iommu: Remove IOMMU_CAP_INTR_REMAP

 arch/s390/include/asm/msi.h         | 12 +++++++++
 drivers/iommu/amd/iommu.c           |  5 ++--
 drivers/iommu/intel/iommu.c         |  2 --
 drivers/iommu/intel/irq_remapping.c |  3 ++-
 drivers/iommu/iommufd/device.c      |  5 ++--
 drivers/iommu/s390-iommu.c          |  2 --
 drivers/irqchip/irq-gic-v3-its.c    |  4 +--
 drivers/vfio/vfio_iommu_type1.c     | 16 ++++++------
 include/linux/iommu.h               |  1 -
 include/linux/irqdomain.h           | 27 ++------------------
 include/linux/msi.h                 | 17 +++++++++++++
 kernel/irq/irqdomain.c              | 39 -----------------------------
 kernel/irq/msi.c                    | 25 ++++++++++++++++++
 13 files changed, 73 insertions(+), 85 deletions(-)
 create mode 100644 arch/s390/include/asm/msi.h


base-commit: 644f4ef9a6ea0e0c65f949bd6b80857d4223c476
-- 
2.38.1

[PATCH iommufd 1/9] irq: Add msi_device_has_secure_msi()

[Jason Gunthorpe]

This will replace irq_domain_check_msi_remap() in following patches.

The new API makes it more clear what "msi_remap" actually means from a
functional perspective instead of identifying an implementation specific
HW feature.

Secure MSI means that an irq_domain on the path from the initiating device
to the CPU will validate that the MSI message specifies an interrupt
number that the initiating device is authorized to trigger. Secure MSI
must block devices from triggering interrupts they are not authorized to
trigger. Currently authorization means the MSI vector is one assigned to
the device.

This determination is interesting for VMs where assigning a device to VM A
should not allow VM A to trigger interrupts on VM B or the host via rouge
MSI operations, eg by mimicking MSI using PCI MemWr DMA.

As this is actually modeled as a per-irq_domain property, not a global
platform property, correct the interface to accept the device parameter
and scan through only the part of the irq_domains hierarchy originating
from the source device.

Locate the new code in msi.c as it naturally only works with
CONFIG_GENERIC_MSI_IRQ, which also requires CONFIG_IRQ_DOMAIN and
IRQ_DOMAIN_HIERARCHY.

Cc: Eric Auger <eric.auger@redhat.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Tomasz Nowicki <tomasz.nowicki@caviumnetworks.com>
Cc: Bharat Bhushan <bharat.bhushan@nxp.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 include/linux/msi.h | 13 +++++++++++++
 kernel/irq/msi.c    | 25 +++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/linux/msi.h b/include/linux/msi.h
index a112b913fff949..75c2c4e71fc34c 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -649,6 +649,19 @@ int platform_msi_device_domain_alloc(struct irq_domain *domain, unsigned int vir
 void platform_msi_device_domain_free(struct irq_domain *domain, unsigned int virq,
 				     unsigned int nvec);
 void *platform_msi_get_host_data(struct irq_domain *domain);
+
+bool msi_device_has_secure_msi(struct device *dev);
+#else /* CONFIG_GENERIC_MSI_IRQ */
+static inline bool msi_device_has_secure_msi(struct device *dev)
+{
+	/*
+	 * Arguably if the platform does not enable MSI support then it has
+	 * "secure MSI", as an interrupt controller that cannot receive MSIs is
+	 * inherently secure by our definition. As nobody seems to needs this be
+	 * conservative and return false anyhow.
+	 */
+	return false;
+}
 #endif /* CONFIG_GENERIC_MSI_IRQ */
 
 /* PCI specific interfaces */
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index bd4d4dd626b4bd..7a7d9f969001c7 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -1622,3 +1622,28 @@ struct msi_domain_info *msi_get_domain_info(struct irq_domain *domain)
 {
 	return (struct msi_domain_info *)domain->host_data;
 }
+
+/**
+ * msi_device_has_secure_msi - True if the device has secure MSI
+ * @dev: The device to check
+ *
+ * Secure MSI means that an irq_domain on the path from the initiating device to
+ * the CPU will validate that the MSI message specifies an interrupt number that
+ * the device is authorized to trigger. This must block devices from triggering
+ * interrupts they are not authorized to trigger. Currently authorization means
+ * the MSI vector is one assigned to the device.
+ *
+ * This is interesting for VMs where assigning device to a VM A should not allow
+ * VM A to trigger interrupts on VM B via rouge MSI operations, eg by mimicking
+ * MSI using PCI MemWr DMA.
+ */
+bool msi_device_has_secure_msi(struct device *dev)
+{
+	struct irq_domain *domain = dev_get_msi_domain(dev);
+
+	for (; domain; domain = domain->parent)
+		if (domain->flags & IRQ_DOMAIN_FLAG_MSI_REMAP)
+			return true;
+	return false;
+}
+EXPORT_SYMBOL_GPL(msi_device_has_secure_msi);
-- 
2.38.1

Re: [PATCH iommufd 1/9] irq: Add msi_device_has_secure_msi()

[Marc Zyngier]

On Thu, 08 Dec 2022 20:26:28 +0000,
Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> This will replace irq_domain_check_msi_remap() in following patches.
> 
> The new API makes it more clear what "msi_remap" actually means from a
> functional perspective instead of identifying an implementation specific
> HW feature.
> 
> Secure MSI means that an irq_domain on the path from the initiating device

irq_domain is a SW construct, and you are trying to validate something
that is HW property.

"Secure" is also a terribly overloaded term that means very different
things in non-x86 circles. When I read this, I see an ARM system with
a device generating an MSI with the "secure" bit set as part of the
transaction and identifying the memory access as being part of the
"secure" domain.

But that's not what you mean at all.

> to the CPU will validate that the MSI message specifies an interrupt
> number that the initiating device is authorized to trigger. Secure MSI
> must block devices from triggering interrupts they are not authorized to
> trigger. Currently authorization means the MSI vector is one assigned to
> the device.

What you are describing here is a *device isolation* property, and I'd
rather we stay away from calling that "secure". If anything, I'd
rather call everything else "broken".

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH iommufd 1/9] irq: Add msi_device_has_secure_msi()

[Jason Gunthorpe]

On Fri, Dec 09, 2022 at 01:59:35PM +0000, Marc Zyngier wrote:
> On Thu, 08 Dec 2022 20:26:28 +0000,
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> > 
> > This will replace irq_domain_check_msi_remap() in following patches.
> > 
> > The new API makes it more clear what "msi_remap" actually means from a
> > functional perspective instead of identifying an implementation specific
> > HW feature.
> > 
> > Secure MSI means that an irq_domain on the path from the initiating device
> 
> irq_domain is a SW construct, and you are trying to validate something
> that is HW property.

Sure, but the SW constructs model the HW functions, so yes this is
trying to say that the irq_domain is modeling HW that does this.

> "Secure" is also a terribly overloaded term that means very different
> things in non-x86 circles. 

Here it is being used as a software property - it is security safe to
allow device operation outside the kernel.

> When I read this, I see an ARM system with
> a device generating an MSI with the "secure" bit set as part of the
> transaction and identifying the memory access as being part of the
> "secure" domain.

Is that secure meaning "confidential" or some other ARM thing?

> > number that the initiating device is authorized to trigger. Secure MSI
> > must block devices from triggering interrupts they are not authorized to
> > trigger. Currently authorization means the MSI vector is one assigned to
> > the device.
> 
> What you are describing here is a *device isolation* property, and I'd
> rather we stay away from calling that "secure". If anything, I'd
> rather call everything else "broken".

Sure, so

msi_device_isolated_interrupts() 

And related ?

Jason

Re: [PATCH iommufd 1/9] irq: Add msi_device_has_secure_msi()

[Marc Zyngier]

On Fri, 09 Dec 2022 14:10:54 +0000,
Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> On Fri, Dec 09, 2022 at 01:59:35PM +0000, Marc Zyngier wrote:
> > On Thu, 08 Dec 2022 20:26:28 +0000,
> > Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > 
> > > This will replace irq_domain_check_msi_remap() in following patches.
> > > 
> > > The new API makes it more clear what "msi_remap" actually means from a
> > > functional perspective instead of identifying an implementation specific
> > > HW feature.
> > > 
> > > Secure MSI means that an irq_domain on the path from the initiating device
> > 
> > irq_domain is a SW construct, and you are trying to validate something
> > that is HW property.
> 
> Sure, but the SW constructs model the HW functions, so yes this is
> trying to say that the irq_domain is modeling HW that does this.
> 
> > "Secure" is also a terribly overloaded term that means very different
> > things in non-x86 circles. 
> 
> Here it is being used as a software property - it is security safe to
> allow device operation outside the kernel.
> 
> > When I read this, I see an ARM system with
> > a device generating an MSI with the "secure" bit set as part of the
> > transaction and identifying the memory access as being part of the
> > "secure" domain.
> 
> Is that secure meaning "confidential" or some other ARM thing?

In ARM parlance, "secure" denotes the secure *physical address space*,
which is a totally disconnected PA space from the "normal" PA space.

If on top of that you have had an unhealthy helping of the
"confidential computing" kool-aid, you get another 2 extra physical
address spaces ("root" and "realm").

> 
> > > number that the initiating device is authorized to trigger. Secure MSI
> > > must block devices from triggering interrupts they are not authorized to
> > > trigger. Currently authorization means the MSI vector is one assigned to
> > > the device.
> > 
> > What you are describing here is a *device isolation* property, and I'd
> > rather we stay away from calling that "secure". If anything, I'd
> > rather call everything else "broken".
> 
> Sure, so
> 
> msi_device_isolated_interrupts() 
> 
> And related ?

Sure.

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH iommufd 2/9] vfio/type1: Check that every device supports IOMMU_CAP_INTR_REMAP

[Jason Gunthorpe]

iommu_group_for_each_dev() exits the loop at the first callback that
returns 1 - thus returning 1 fails to check the rest of the devices in the
group.

msi_remap (aka secure MSI) requires that all the devices in the group
support it, not just any one. This is only a theoretical problem as no
current drivers will have different secure MSI properties within a group.

Make vfio_iommu_device_secure_msi() reduce AND across all the devices.

Fixes: eed20c782aea ("vfio/type1: Simplify bus_type determination")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 23c24fe98c00d4..3025b4e643c135 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2160,10 +2160,12 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
 	list_splice_tail(iova_copy, iova);
 }
 
-/* Redundantly walks non-present capabilities to simplify caller */
-static int vfio_iommu_device_capable(struct device *dev, void *data)
+static int vfio_iommu_device_secure_msi(struct device *dev, void *data)
 {
-	return device_iommu_capable(dev, (enum iommu_cap)data);
+	bool *secure_msi = data;
+
+	*secure_msi &= device_iommu_capable(dev, IOMMU_CAP_INTR_REMAP);
+	return 0;
 }
 
 static int vfio_iommu_domain_alloc(struct device *dev, void *data)
@@ -2278,9 +2280,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	INIT_LIST_HEAD(&domain->group_list);
 	list_add(&group->next, &domain->group_list);
 
-	msi_remap = irq_domain_check_msi_remap() ||
-		    iommu_group_for_each_dev(iommu_group, (void *)IOMMU_CAP_INTR_REMAP,
-					     vfio_iommu_device_capable);
+	msi_remap = irq_domain_check_msi_remap();
+	if (!msi_remap) {
+		msi_remap = true;
+		iommu_group_for_each_dev(iommu_group, &msi_remap,
+					 vfio_iommu_device_secure_msi);
+	}
 
 	if (!allow_unsafe_interrupts && !msi_remap) {
 		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
-- 
2.38.1

Re: [PATCH iommufd 2/9] vfio/type1: Check that every device supports IOMMU_CAP_INTR_REMAP

[Alex Williamson]

On Thu,  8 Dec 2022 16:26:29 -0400
Jason Gunthorpe <jgg@nvidia.com> wrote:

> iommu_group_for_each_dev() exits the loop at the first callback that
> returns 1 - thus returning 1 fails to check the rest of the devices in the
> group.
> 
> msi_remap (aka secure MSI) requires that all the devices in the group
> support it, not just any one. This is only a theoretical problem as no
> current drivers will have different secure MSI properties within a group.

Which is exactly how Robin justified the behavior in the referenced
commit:

  As with domains, any capability must in practice be consistent for
  devices in a given group - and after all it's still the same
  capability which was expected to be consistent across an entire bus!
  - so there's no need for any complicated validation.

That suggests to me that it's intentional that we break if any device
supports the capability and therefore this isn't so much a "Fixes:", as
it is a refactoring expressly to support msi_device_has_secure_msi(),
which cannot make these sort of assumptions as a non-group API.  Thanks,

Alex

> Make vfio_iommu_device_secure_msi() reduce AND across all the devices.
> 
> Fixes: eed20c782aea ("vfio/type1: Simplify bus_type determination")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 23c24fe98c00d4..3025b4e643c135 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -2160,10 +2160,12 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
>  	list_splice_tail(iova_copy, iova);
>  }
>  
> -/* Redundantly walks non-present capabilities to simplify caller */
> -static int vfio_iommu_device_capable(struct device *dev, void *data)
> +static int vfio_iommu_device_secure_msi(struct device *dev, void *data)
>  {
> -	return device_iommu_capable(dev, (enum iommu_cap)data);
> +	bool *secure_msi = data;
> +
> +	*secure_msi &= device_iommu_capable(dev, IOMMU_CAP_INTR_REMAP);
> +	return 0;
>  }
>  
>  static int vfio_iommu_domain_alloc(struct device *dev, void *data)
> @@ -2278,9 +2280,12 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	INIT_LIST_HEAD(&domain->group_list);
>  	list_add(&group->next, &domain->group_list);
>  
> -	msi_remap = irq_domain_check_msi_remap() ||
> -		    iommu_group_for_each_dev(iommu_group, (void *)IOMMU_CAP_INTR_REMAP,
> -					     vfio_iommu_device_capable);
> +	msi_remap = irq_domain_check_msi_remap();
> +	if (!msi_remap) {
> +		msi_remap = true;
> +		iommu_group_for_each_dev(iommu_group, &msi_remap,
> +					 vfio_iommu_device_secure_msi);
> +	}
>  
>  	if (!allow_unsafe_interrupts && !msi_remap) {
>  		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",

Re: [PATCH iommufd 2/9] vfio/type1: Check that every device supports IOMMU_CAP_INTR_REMAP

[Jason Gunthorpe]

On Thu, Dec 08, 2022 at 02:48:25PM -0700, Alex Williamson wrote:
> On Thu,  8 Dec 2022 16:26:29 -0400
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > iommu_group_for_each_dev() exits the loop at the first callback that
> > returns 1 - thus returning 1 fails to check the rest of the devices in the
> > group.
> > 
> > msi_remap (aka secure MSI) requires that all the devices in the group
> > support it, not just any one. This is only a theoretical problem as no
> > current drivers will have different secure MSI properties within a group.
> 
> Which is exactly how Robin justified the behavior in the referenced
> commit:
> 
>   As with domains, any capability must in practice be consistent for
>   devices in a given group - and after all it's still the same
>   capability which was expected to be consistent across an entire bus!
>   - so there's no need for any complicated validation.
> 
> That suggests to me that it's intentional that we break if any device
> supports the capability and therefore this isn't so much a "Fixes:", as
> it is a refactoring expressly to support msi_device_has_secure_msi(),
> which cannot make these sort of assumptions as a non-group API.  Thanks,

Sure, lets drop the fixes and your analysis seems correct

Jason

Re: [PATCH iommufd 2/9] vfio/type1: Check that every device supports IOMMU_CAP_INTR_REMAP

[Robin Murphy]

On 2022-12-09 00:44, Jason Gunthorpe wrote:
> On Thu, Dec 08, 2022 at 02:48:25PM -0700, Alex Williamson wrote:
>> On Thu,  8 Dec 2022 16:26:29 -0400
>> Jason Gunthorpe <jgg@nvidia.com> wrote:
>>
>>> iommu_group_for_each_dev() exits the loop at the first callback that
>>> returns 1 - thus returning 1 fails to check the rest of the devices in the
>>> group.
>>>
>>> msi_remap (aka secure MSI) requires that all the devices in the group
>>> support it, not just any one. This is only a theoretical problem as no
>>> current drivers will have different secure MSI properties within a group.
>>
>> Which is exactly how Robin justified the behavior in the referenced
>> commit:
>>
>>    As with domains, any capability must in practice be consistent for
>>    devices in a given group - and after all it's still the same
>>    capability which was expected to be consistent across an entire bus!
>>    - so there's no need for any complicated validation.
>>
>> That suggests to me that it's intentional that we break if any device
>> supports the capability and therefore this isn't so much a "Fixes:", as
>> it is a refactoring expressly to support msi_device_has_secure_msi(),
>> which cannot make these sort of assumptions as a non-group API.  Thanks,
> 
> Sure, lets drop the fixes and your analysis seems correct

Yup, Alex is spot on - the fact is that all the IOMMU drivers supporting 
this cap have always returned global values, and continue to do so right 
up until we remove it later, so there's really no benefit in pretending 
otherwise. I'd say just fold this into patch #3 to keep it even simpler.

Thanks,
Robin.

[PATCH iommufd 3/9] vfio/type1: Convert to msi_device_has_secure_msi()

[Jason Gunthorpe]

Use the new API. VFIO can just slot this in the existing iteration over
every device in the group.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio_iommu_type1.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 3025b4e643c135..a954b58d606766 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -37,7 +37,7 @@
 #include <linux/vfio.h>
 #include <linux/workqueue.h>
 #include <linux/notifier.h>
-#include <linux/irqdomain.h>
+#include <linux/msi.h>
 #include "vfio.h"
 
 #define DRIVER_VERSION  "0.2"
@@ -2164,6 +2164,8 @@ static int vfio_iommu_device_secure_msi(struct device *dev, void *data)
 {
 	bool *secure_msi = data;
 
+	if (msi_device_has_secure_msi(dev))
+		return 0;
 	*secure_msi &= device_iommu_capable(dev, IOMMU_CAP_INTR_REMAP);
 	return 0;
 }
@@ -2280,12 +2282,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	INIT_LIST_HEAD(&domain->group_list);
 	list_add(&group->next, &domain->group_list);
 
-	msi_remap = irq_domain_check_msi_remap();
-	if (!msi_remap) {
-		msi_remap = true;
-		iommu_group_for_each_dev(iommu_group, &msi_remap,
-					 vfio_iommu_device_secure_msi);
-	}
+	msi_remap = true;
+	iommu_group_for_each_dev(iommu_group, &msi_remap,
+				 vfio_iommu_device_secure_msi);
 
 	if (!allow_unsafe_interrupts && !msi_remap) {
 		pr_warn("%s: No interrupt remapping support.  Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
-- 
2.38.1

[PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Jason Gunthorpe]

iommufd already has a struct device, so trivial conversion.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 29ff97f756bc42..13ad737d50b18c 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -4,7 +4,7 @@
 #include <linux/iommufd.h>
 #include <linux/slab.h>
 #include <linux/iommu.h>
-#include <linux/irqdomain.h>
+#include <linux/msi.h>
 
 #include "io_pagetable.h"
 #include "iommufd_private.h"
@@ -170,7 +170,7 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev,
 	 * interrupt outside this iommufd context.
 	 */
 	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP) &&
-	    !irq_domain_check_msi_remap()) {
+	    !msi_device_has_secure_msi(idev->dev)) {
 		if (!allow_unsafe_interrupts)
 			return -EPERM;
 
-- 
2.38.1

RE: [PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, December 9, 2022 4:27 AM
>
> @@ -170,7 +170,7 @@ static int iommufd_device_setup_msi(struct
> iommufd_device *idev,
>  	 * interrupt outside this iommufd context.
>  	 */
>  	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP)
> &&
> -	    !irq_domain_check_msi_remap()) {
> +	    !msi_device_has_secure_msi(idev->dev)) {
>  		if (!allow_unsafe_interrupts)
>  			return -EPERM;
> 

this is where iommufd and vfio diverge.

vfio has a check to ensure all devices in the group has secure_msi.

but iommufd only imposes the check per device.

Re: [PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Jason Gunthorpe]

On Fri, Dec 09, 2022 at 06:01:14AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Friday, December 9, 2022 4:27 AM
> >
> > @@ -170,7 +170,7 @@ static int iommufd_device_setup_msi(struct
> > iommufd_device *idev,
> >  	 * interrupt outside this iommufd context.
> >  	 */
> >  	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP)
> > &&
> > -	    !irq_domain_check_msi_remap()) {
> > +	    !msi_device_has_secure_msi(idev->dev)) {
> >  		if (!allow_unsafe_interrupts)
> >  			return -EPERM;
> > 
> 
> this is where iommufd and vfio diverge.
> 
> vfio has a check to ensure all devices in the group has secure_msi.
> 
> but iommufd only imposes the check per device.

Ah, that is an interesting, though pedantic point.

So, let us do this and address the other point about vfio as well:

+++ b/drivers/iommu/iommu.c
@@ -941,6 +941,28 @@ static bool iommu_is_attach_deferred(struct device *dev)
        return false;
 }
 
+static int iommu_group_add_device_list(struct iommu_group *group,
+                                      struct group_device *group_dev)
+{
+       struct group_device *existing;
+
+       lockdep_assert_held(&group->mutex);
+
+       existing = list_first_entry_or_null(&group->devices,
+                                           struct group_device, list);
+
+       /*
+        * It is a driver bug to create groups with different irq_domain
+        * properties.
+        */
+       if (existing && msi_device_has_isolated_msi(existing->dev) !=
+                               msi_device_has_isolated_msi(group_dev->dev))
+               return -EINVAL;
+
+       list_add_tail(&group_dev->list, &group->devices);
+       return 0;
+}
+
 /**
  * iommu_group_add_device - add a device to an iommu group
  * @group: the group into which to add the device (reference should be held)
@@ -992,7 +1014,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
        dev->iommu_group = group;
 
        mutex_lock(&group->mutex);
-       list_add_tail(&device->list, &group->devices);
+       iommu_group_add_device_list(group, device);
        if (group->domain  && !iommu_is_attach_deferred(dev))
                ret = __iommu_attach_device(group->domain, dev);
        mutex_unlock(&group->mutex);

Re: [PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Robin Murphy]

On 2022-12-09 14:47, Jason Gunthorpe wrote:
> On Fri, Dec 09, 2022 at 06:01:14AM +0000, Tian, Kevin wrote:
>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>> Sent: Friday, December 9, 2022 4:27 AM
>>>
>>> @@ -170,7 +170,7 @@ static int iommufd_device_setup_msi(struct
>>> iommufd_device *idev,
>>>   	 * interrupt outside this iommufd context.
>>>   	 */
>>>   	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP)
>>> &&
>>> -	    !irq_domain_check_msi_remap()) {
>>> +	    !msi_device_has_secure_msi(idev->dev)) {
>>>   		if (!allow_unsafe_interrupts)
>>>   			return -EPERM;
>>>
>>
>> this is where iommufd and vfio diverge.
>>
>> vfio has a check to ensure all devices in the group has secure_msi.
>>
>> but iommufd only imposes the check per device.
> 
> Ah, that is an interesting, though pedantic point.
> 
> So, let us do this and address the other point about vfio as well:
> 
> +++ b/drivers/iommu/iommu.c
> @@ -941,6 +941,28 @@ static bool iommu_is_attach_deferred(struct device *dev)
>          return false;
>   }
>   
> +static int iommu_group_add_device_list(struct iommu_group *group,
> +                                      struct group_device *group_dev)
> +{
> +       struct group_device *existing;
> +
> +       lockdep_assert_held(&group->mutex);
> +
> +       existing = list_first_entry_or_null(&group->devices,
> +                                           struct group_device, list);
> +
> +       /*
> +        * It is a driver bug to create groups with different irq_domain
> +        * properties.
> +        */
> +       if (existing && msi_device_has_isolated_msi(existing->dev) !=
> +                               msi_device_has_isolated_msi(group_dev->dev))
> +               return -EINVAL;

Isn't the problem with this that it's super-early, and a device's MSI 
domain may not actually be resolved until someone starts requesting MSIs 
for it? Maybe Thomas' ongoing per-device stuff changes that, but I'm not 
sure :/

Furthermore, even if the system does have a topology with multiple 
heterogeneous MSI controllers reachable by devices behind the same 
IOMMU, and the IRQ layer decides to choose each device's MSI parent at 
random, that's hardly the IOMMU layer's problem, and shouldn't prevent 
the devices being usable by kernel drivers for whom MSI isolation 
doesn't matter.

Thanks,
Robin.

> +
> +       list_add_tail(&group_dev->list, &group->devices);
> +       return 0;
> +}
> +
>   /**
>    * iommu_group_add_device - add a device to an iommu group
>    * @group: the group into which to add the device (reference should be held)
> @@ -992,7 +1014,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>          dev->iommu_group = group;
>   
>          mutex_lock(&group->mutex);
> -       list_add_tail(&device->list, &group->devices);
> +       iommu_group_add_device_list(group, device);
>          if (group->domain  && !iommu_is_attach_deferred(dev))
>                  ret = __iommu_attach_device(group->domain, dev);
>          mutex_unlock(&group->mutex);

Re: [PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Jason Gunthorpe]

On Fri, Dec 09, 2022 at 04:44:06PM +0000, Robin Murphy wrote:

> Isn't the problem with this that it's super-early, and a device's MSI domain
> may not actually be resolved until someone starts requesting MSIs for it?
> Maybe Thomas' ongoing per-device stuff changes that, but I'm not
> sure :/

Yes, this looks correct, OK, so I will do Kevin's thought

Thanks!

> Furthermore, even if the system does have a topology with multiple
> heterogeneous MSI controllers reachable by devices behind the same
> IOMMU,

Sure, but this doesn't exist and my thinking was to put a big red flag
here in case someone actually wants to try to do it - most likely it
is a bug not a real thing

I re-did things to use this new function, iommufd and vfio just
trivially call it

+/**
+ * iommu_group_has_isolated_msi() - Compute msi_device_has_isolated_msi()
+ *       for a group
+ * @group: Group to query
+ *
+ * IOMMU groups should not have differing values of
+ * msi_device_has_isolated_msi() for devices in a group. However nothing
+ * directly prevents this, so ensure mistakes don't result in isolation failures
+ * by checking that all the devices are the same.
+ */
+bool iommu_group_has_isolated_msi(struct iommu_group *group)
+{
+	struct group_device *group_dev;
+	bool ret = true;
+
+	mutex_lock(&group->mutex);
+	list_for_each_entry(group_dev, &group->devices, list)
+		ret &= msi_device_has_isolated_msi(group_dev->dev) ||
+		       device_iommu_capable(group_dev->dev,
+					    IOMMU_CAP_INTR_REMAP);
+	mutex_unlock(&group->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_group_has_isolated_msi);

Re: [PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Thomas Gleixner]

On Fri, Dec 09 2022 at 13:38, Jason Gunthorpe wrote:
> On Fri, Dec 09, 2022 at 04:44:06PM +0000, Robin Murphy wrote:
>
>> Isn't the problem with this that it's super-early, and a device's MSI domain
>> may not actually be resolved until someone starts requesting MSIs for it?
>> Maybe Thomas' ongoing per-device stuff changes that, but I'm not
>> sure :/
>
> Yes, this looks correct, OK, so I will do Kevin's thought

The device MSI domain has to be valid before a device can be probed, at
least that's the case for PCI/MSI devices. The pointer is established
during PCI discovery.

The per device MSI domains do not change that requirement. The only
difference is that device->msi.domain now points to the MSI parent
domain.

In the "global" PCI/MSI domain case the hierarchy walk will (on x86)
start at the PCI/MSI domain and end up either at the remapping unit,
which has the protection property, or at the vector (root) domain, which
does not.

For the per device domain case the walk will start at the parent domain,
which is (on x86) either the remapping unit or the vector (root) domain.

The same is true for ARM(64) and other hierarchy users, just the naming
conventions and possible scenarios are different.

So for both scenarios (global and per-device) searching for that
protection property down the hierarchy starting from device->msi.domain
is correct.

Obvioulsy unless it's done somewhere early in the PCI discovery,
i.e. before the discovery associated the domain pointer.

Thanks,

        tglx

Re: [PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Jason Gunthorpe]

On Mon, Dec 12, 2022 at 04:17:58PM +0100, Thomas Gleixner wrote:

> Obvioulsy unless it's done somewhere early in the PCI discovery,
> i.e. before the discovery associated the domain pointer.

I thought the problem is more that the iommu drivers change the
assigned irq_domain:

void intel_irq_remap_add_device(struct dmar_pci_notify_info *info)
{
	if (!irq_remapping_enabled || pci_dev_has_special_msi_domain(info->dev))
		return;

	dev_set_msi_domain(&info->dev->dev, map_dev_to_ir(info->dev));
}

Which is ultimately called by 

	bus_register_notifier(&pci_bus_type, &dmar_pci_bus_nb);

And that compares with the iommu setup which is also done from a
bus notifier:

		nb[i].notifier_call = iommu_bus_notifier;
		bus_register_notifier(iommu_buses[i], &nb[i]);

So, I think, there is not reliable ordering between these two things.

At least that is why I was convinced we should not do the idea I
shared :)

Thanks,
Jason

Re: [PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Thomas Gleixner]

On Mon, Dec 12 2022 at 11:47, Jason Gunthorpe wrote:
> On Mon, Dec 12, 2022 at 04:17:58PM +0100, Thomas Gleixner wrote:
>> Obvioulsy unless it's done somewhere early in the PCI discovery,
>> i.e. before the discovery associated the domain pointer.
>
> I thought the problem is more that the iommu drivers change the
> assigned irq_domain:

Yes they do.

> void intel_irq_remap_add_device(struct dmar_pci_notify_info *info)
> {
> 	if (!irq_remapping_enabled || pci_dev_has_special_msi_domain(info->dev))
> 		return;
>
> 	dev_set_msi_domain(&info->dev->dev, map_dev_to_ir(info->dev));
> }
>
> Which is ultimately called by 
>
> 	bus_register_notifier(&pci_bus_type, &dmar_pci_bus_nb);
>
> And that compares with the iommu setup which is also done from a
> bus notifier:
>
> 		nb[i].notifier_call = iommu_bus_notifier;
> 		bus_register_notifier(iommu_buses[i], &nb[i]);
>
> So, I think, there is not reliable ordering between these two things.

Bah. Notifiers are a complete disaster vs. ordering. This really wants
reliable ordering IMO.

Thanks,

        tglx

[PATCH iommufd 5/9] irq: Remove unused irq_domain_check_msi_remap() code

[Jason Gunthorpe]

After converting the users of irq_domain_check_msi_remap() it and the
helpers are no longer needed.

The new version does not require all the #ifdef helpers and inlines
because CONFIG_GENERIC_MSI_IRQ always requires CONFIG_IRQ_DOMAIN and
IRQ_DOMAIN_HIERARCHY.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 include/linux/irqdomain.h | 23 -----------------------
 kernel/irq/irqdomain.c    | 39 ---------------------------------------
 2 files changed, 62 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index a372086750ca55..b04ce03d3bb69f 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -276,7 +276,6 @@ struct irq_domain *irq_domain_create_legacy(struct fwnode_handle *fwnode,
 					    void *host_data);
 extern struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
 						   enum irq_domain_bus_token bus_token);
-extern bool irq_domain_check_msi_remap(void);
 extern void irq_set_default_host(struct irq_domain *host);
 extern struct irq_domain *irq_get_default_host(void);
 extern int irq_domain_alloc_descs(int virq, unsigned int nr_irqs,
@@ -559,13 +558,6 @@ static inline bool irq_domain_is_msi(struct irq_domain *domain)
 	return domain->flags & IRQ_DOMAIN_FLAG_MSI;
 }
 
-static inline bool irq_domain_is_msi_remap(struct irq_domain *domain)
-{
-	return domain->flags & IRQ_DOMAIN_FLAG_MSI_REMAP;
-}
-
-extern bool irq_domain_hierarchical_is_msi_remap(struct irq_domain *domain);
-
 static inline bool irq_domain_is_msi_parent(struct irq_domain *domain)
 {
 	return domain->flags & IRQ_DOMAIN_FLAG_MSI_PARENT;
@@ -611,17 +603,6 @@ static inline bool irq_domain_is_msi(struct irq_domain *domain)
 	return false;
 }
 
-static inline bool irq_domain_is_msi_remap(struct irq_domain *domain)
-{
-	return false;
-}
-
-static inline bool
-irq_domain_hierarchical_is_msi_remap(struct irq_domain *domain)
-{
-	return false;
-}
-
 static inline bool irq_domain_is_msi_parent(struct irq_domain *domain)
 {
 	return false;
@@ -641,10 +622,6 @@ static inline struct irq_domain *irq_find_matching_fwnode(
 {
 	return NULL;
 }
-static inline bool irq_domain_check_msi_remap(void)
-{
-	return false;
-}
 #endif /* !CONFIG_IRQ_DOMAIN */
 
 #endif /* _LINUX_IRQDOMAIN_H */
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8fe1da9614ee8d..10495495158210 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -436,31 +436,6 @@ struct irq_domain *irq_find_matching_fwspec(struct irq_fwspec *fwspec,
 }
 EXPORT_SYMBOL_GPL(irq_find_matching_fwspec);
 
-/**
- * irq_domain_check_msi_remap - Check whether all MSI irq domains implement
- * IRQ remapping
- *
- * Return: false if any MSI irq domain does not support IRQ remapping,
- * true otherwise (including if there is no MSI irq domain)
- */
-bool irq_domain_check_msi_remap(void)
-{
-	struct irq_domain *h;
-	bool ret = true;
-
-	mutex_lock(&irq_domain_mutex);
-	list_for_each_entry(h, &irq_domain_list, link) {
-		if (irq_domain_is_msi(h) &&
-		    !irq_domain_hierarchical_is_msi_remap(h)) {
-			ret = false;
-			break;
-		}
-	}
-	mutex_unlock(&irq_domain_mutex);
-	return ret;
-}
-EXPORT_SYMBOL_GPL(irq_domain_check_msi_remap);
-
 /**
  * irq_set_default_host() - Set a "default" irq domain
  * @domain: default domain pointer
@@ -1815,20 +1790,6 @@ static void irq_domain_check_hierarchy(struct irq_domain *domain)
 	if (domain->ops->alloc)
 		domain->flags |= IRQ_DOMAIN_FLAG_HIERARCHY;
 }
-
-/**
- * irq_domain_hierarchical_is_msi_remap - Check if the domain or any
- * parent has MSI remapping support
- * @domain: domain pointer
- */
-bool irq_domain_hierarchical_is_msi_remap(struct irq_domain *domain)
-{
-	for (; domain; domain = domain->parent) {
-		if (irq_domain_is_msi_remap(domain))
-			return true;
-	}
-	return false;
-}
 #else	/* CONFIG_IRQ_DOMAIN_HIERARCHY */
 /**
  * irq_domain_get_irq_data - Get irq_data associated with @virq and @domain
-- 
2.38.1

[PATCH iommufd 6/9] irq: Rename MSI_REMAP to SECURE_MSI

[Jason Gunthorpe]

What x86 calls "interrupt remapping" is one way to achieve secure MSI,
make it clear this is talking about secure MSI, no matter how it is
achieved. This matches the new driver facing API name of
msi_device_has_secure_msi()

No functional change.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 4 ++--
 include/linux/irqdomain.h        | 4 ++--
 kernel/irq/msi.c                 | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 973ede0197e36f..bb1aa5367ada6a 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -4692,7 +4692,7 @@ static bool __maybe_unused its_enable_quirk_socionext_synquacer(void *data)
 		}
 
 		/* the pre-ITS breaks isolation, so disable MSI remapping */
-		its->msi_domain_flags &= ~IRQ_DOMAIN_FLAG_MSI_REMAP;
+		its->msi_domain_flags &= ~IRQ_DOMAIN_FLAG_SECURE_MSI;
 		return true;
 	}
 	return false;
@@ -5074,7 +5074,7 @@ static int __init its_probe_one(struct resource *res,
 	its->cmd_write = its->cmd_base;
 	its->fwnode_handle = handle;
 	its->get_msi_base = its_irq_get_msi_base;
-	its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP;
+	its->msi_domain_flags = IRQ_DOMAIN_FLAG_SECURE_MSI;
 
 	its_enable_quirks(its);
 
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index b04ce03d3bb69f..cc6238bfa9ed06 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -192,8 +192,8 @@ enum {
 	/* Irq domain implements MSIs */
 	IRQ_DOMAIN_FLAG_MSI		= (1 << 4),
 
-	/* Irq domain implements MSI remapping */
-	IRQ_DOMAIN_FLAG_MSI_REMAP	= (1 << 5),
+	/* Irq domain implements secure MSI, see msi_device_has_secure_msi() */
+	IRQ_DOMAIN_FLAG_SECURE_MSI	= (1 << 5),
 
 	/* Irq domain doesn't translate anything */
 	IRQ_DOMAIN_FLAG_NO_MAP		= (1 << 6),
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 7a7d9f969001c7..18264bddf63b89 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -1642,7 +1642,7 @@ bool msi_device_has_secure_msi(struct device *dev)
 	struct irq_domain *domain = dev_get_msi_domain(dev);
 
 	for (; domain; domain = domain->parent)
-		if (domain->flags & IRQ_DOMAIN_FLAG_MSI_REMAP)
+		if (domain->flags & IRQ_DOMAIN_FLAG_SECURE_MSI)
 			return true;
 	return false;
 }
-- 
2.38.1

[PATCH iommufd 7/9] iommu/x86: Replace IOMMU_CAP_INTR_REMAP with IRQ_DOMAIN_FLAG_SECURE_MSI

[Jason Gunthorpe]

On x86 platforms when the HW can support interrupt remapping the iommu
driver creates an irq_domain for the IR hardware and creates a child MSI
irq_domain.

When the global irq_remapping_enabled is set, the IR MSI domain is
assigned to the PCI devices (by intel_irq_remap_add_device(), or
amd_iommu_set_pci_msi_domain()) making those devices have the secure MSI
property.

Due to how interrupt domains work, setting IRQ_DOMAIN_FLAG_SECURE_MSI on
the parent IR domain will cause all struct devices attached to it to
return true from msi_device_has_secure_msi(). This replaces the
IOMMU_CAP_INTR_REMAP flag as all places using IOMMU_CAP_INTR_REMAP also
call msi_device_has_secure_msi()

Set the flag and delete the cap.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c           | 5 ++---
 drivers/iommu/intel/iommu.c         | 2 --
 drivers/iommu/intel/irq_remapping.c | 3 ++-
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8d37d9087fab28..a8ddcf42dd15c1 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2272,8 +2272,6 @@ static bool amd_iommu_capable(struct device *dev, enum iommu_cap cap)
 	switch (cap) {
 	case IOMMU_CAP_CACHE_COHERENCY:
 		return true;
-	case IOMMU_CAP_INTR_REMAP:
-		return (irq_remapping_enabled == 1);
 	case IOMMU_CAP_NOEXEC:
 		return false;
 	case IOMMU_CAP_PRE_BOOT_PROTECTION:
@@ -3672,7 +3670,8 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu)
 	}
 
 	irq_domain_update_bus_token(iommu->ir_domain,  DOMAIN_BUS_AMDVI);
-	iommu->ir_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
+	iommu->ir_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT |
+				   IRQ_DOMAIN_FLAG_SECURE_MSI;
 
 	if (amd_iommu_np_cache)
 		iommu->ir_domain->msi_parent_ops = &virt_amdvi_msi_parent_ops;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index ebe44a07c4b00e..8037a599ade0d6 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4453,8 +4453,6 @@ static bool intel_iommu_capable(struct device *dev, enum iommu_cap cap)
 	switch (cap) {
 	case IOMMU_CAP_CACHE_COHERENCY:
 		return true;
-	case IOMMU_CAP_INTR_REMAP:
-		return irq_remapping_enabled == 1;
 	case IOMMU_CAP_PRE_BOOT_PROTECTION:
 		return dmar_platform_optin();
 	case IOMMU_CAP_ENFORCE_CACHE_COHERENCY:
diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c
index a723f53ba472f9..0972f47e6ec166 100644
--- a/drivers/iommu/intel/irq_remapping.c
+++ b/drivers/iommu/intel/irq_remapping.c
@@ -576,7 +576,8 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu)
 	}
 
 	irq_domain_update_bus_token(iommu->ir_domain,  DOMAIN_BUS_DMAR);
-	iommu->ir_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
+	iommu->ir_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT |
+				   IRQ_DOMAIN_FLAG_SECURE_MSI;
 
 	if (cap_caching_mode(iommu->cap))
 		iommu->ir_domain->msi_parent_ops = &virt_dmar_msi_parent_ops;
-- 
2.38.1

[PATCH iommufd 8/9] irq/s390: Add arch_is_secure_msi() for s390

[Jason Gunthorpe]

s390 doesn't use irq_domains, so it has no place to set
IRQ_DOMAIN_FLAG_SECURE_MSI. Instead of continuing to abuse the iommu
subsystem to convey this information add a simple define which s390 can
make statically true. The define will cause irq_device_has_secure_msi() to
return true.

I do not know if S390 meets the definition of "Secure MSI" if someone can
explain how it works I will update the comment in the arch/msi.h to
explain it. Please consider updating S390 to use the modern IRQ
infrastructure.

Remove IOMMU_CAP_INTR_REMAP from the s390 iommu driver.

Cc: Matthew Rosato <mjrosato@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Eric Farman <farman@linux.ibm.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 arch/s390/include/asm/msi.h | 12 ++++++++++++
 drivers/iommu/s390-iommu.c  |  2 --
 include/linux/msi.h         |  6 +++++-
 kernel/irq/msi.c            |  2 +-
 4 files changed, 18 insertions(+), 4 deletions(-)
 create mode 100644 arch/s390/include/asm/msi.h

diff --git a/arch/s390/include/asm/msi.h b/arch/s390/include/asm/msi.h
new file mode 100644
index 00000000000000..e3522bde3e9c90
--- /dev/null
+++ b/arch/s390/include/asm/msi.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_S390_MSI_H
+#define _ASM_S390_MSI_H
+#include <asm-generic/msi.h>
+
+/*
+ * Work around S390 not using irq_domain at all so we can't set
+ * IRQ_DOMAIN_FLAG_SECURE_MSI
+ */
+#define arch_is_secure_msi() true
+
+#endif
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 3c071782f6f16d..c80f4728c0f307 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -44,8 +44,6 @@ static bool s390_iommu_capable(struct device *dev, enum iommu_cap cap)
 	switch (cap) {
 	case IOMMU_CAP_CACHE_COHERENCY:
 		return true;
-	case IOMMU_CAP_INTR_REMAP:
-		return true;
 	default:
 		return false;
 	}
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 75c2c4e71fc34c..8efbf34a86f247 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -48,6 +48,10 @@ typedef struct arch_msi_msg_data {
 } __attribute__ ((packed)) arch_msi_msg_data_t;
 #endif
 
+#ifndef arch_is_secure_msi
+#define arch_is_secure_msi() false
+#endif
+
 /**
  * msi_msg - Representation of a MSI message
  * @address_lo:		Low 32 bits of msi message address
@@ -660,7 +664,7 @@ static inline bool msi_device_has_secure_msi(struct device *dev)
 	 * inherently secure by our definition. As nobody seems to needs this be
 	 * conservative and return false anyhow.
 	 */
-	return false;
+	return arch_is_secure_msi();
 }
 #endif /* CONFIG_GENERIC_MSI_IRQ */
 
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 18264bddf63b89..aba4f12df190b7 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -1644,6 +1644,6 @@ bool msi_device_has_secure_msi(struct device *dev)
 	for (; domain; domain = domain->parent)
 		if (domain->flags & IRQ_DOMAIN_FLAG_SECURE_MSI)
 			return true;
-	return false;
+	return arch_is_secure_msi();
 }
 EXPORT_SYMBOL_GPL(msi_device_has_secure_msi);
-- 
2.38.1

[PATCH iommufd 9/9] iommu: Remove IOMMU_CAP_INTR_REMAP

[Jason Gunthorpe]

No iommu driver implements this any more, get rid of it.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/device.c  | 3 +--
 drivers/vfio/vfio_iommu_type1.c | 4 +---
 include/linux/iommu.h           | 1 -
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 13ad737d50b18c..76a658bf9baa9c 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -169,8 +169,7 @@ static int iommufd_device_setup_msi(struct iommufd_device *idev,
 	 * operation from the device (eg a simple DMA) cannot trigger an
 	 * interrupt outside this iommufd context.
 	 */
-	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP) &&
-	    !msi_device_has_secure_msi(idev->dev)) {
+	if (!msi_device_has_secure_msi(idev->dev)) {
 		if (!allow_unsafe_interrupts)
 			return -EPERM;
 
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a954b58d606766..380297334842cf 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2164,9 +2164,7 @@ static int vfio_iommu_device_secure_msi(struct device *dev, void *data)
 {
 	bool *secure_msi = data;
 
-	if (msi_device_has_secure_msi(dev))
-		return 0;
-	*secure_msi &= device_iommu_capable(dev, IOMMU_CAP_INTR_REMAP);
+	*secure_msi &= msi_device_has_secure_msi(dev);
 	return 0;
 }
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1690c334e51631..0075b110d17998 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -120,7 +120,6 @@ static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
 
 enum iommu_cap {
 	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU_CACHE is supported */
-	IOMMU_CAP_INTR_REMAP,		/* IOMMU supports interrupt isolation */
 	IOMMU_CAP_NOEXEC,		/* IOMMU_NOEXEC flag */
 	IOMMU_CAP_PRE_BOOT_PROTECTION,	/* Firmware says it used the IOMMU for
 					   DMA protection and we should too */
-- 
2.38.1

Re: [PATCH iommufd 0/9] Remove IOMMU_CAP_INTR_REMAP

[Matthew Rosato]

On 12/8/22 3:26 PM, Jason Gunthorpe wrote:

>  - S390 has unconditionally claimed it has secure MSI through the iommu
>    driver. I'm not sure how it works, or if it even does. Perhaps
>    zpci_set_airq() pushes the "zdev->gias" to the hypervisor which
>    limits a device's MSI to only certain KVM contexts (though if true
>    this would be considered insecure by VFIO)
> 

There are a few layers here.  Interrupt isolation and mapping on s390 is accomplished via a mapping table used by a layer of firmware (and can be shared by a hypervisor e.g. qemu/kvm) that sits between the device and the kernel/driver (s390 linux always runs on at least this 'bare-metal hypervisor' firmware layer).  Indeed the initial relationship is established via zpci_set_airq -- the "zdev->fh" identifies the device, the "zdev->gisa" (if applicable) identifies the single KVM context that is eligible to receive interrupts related to the specified device as well as the single KVM context allowed to access the device via any zPCI instructions (e.g. config space access).  The aibv/noi indicate the vector mappings that are authorized for that device; firmware will typically route the interrupts to the guest without hypervisor involvement once this is established, but the table is shared by the hypervisor so that it can be tapped to complete delivery when necessary.  This registration process enables a firmware intermediary that will only pass along MSI from the device that has an associated, previously-authorized vector, associated with either the 'bare-metal hypervisor' (gisa = 0) and/or a specific VM (gisa != 0), depending what was registered as zpci_set_airq.

Re: [PATCH iommufd 0/9] Remove IOMMU_CAP_INTR_REMAP

[Jason Gunthorpe]

On Thu, Dec 08, 2022 at 06:37:42PM -0500, Matthew Rosato wrote:
> On 12/8/22 3:26 PM, Jason Gunthorpe wrote:
> 
> >  - S390 has unconditionally claimed it has secure MSI through the iommu
> >    driver. I'm not sure how it works, or if it even does. Perhaps
> >    zpci_set_airq() pushes the "zdev->gias" to the hypervisor which
> >    limits a device's MSI to only certain KVM contexts (though if true
> >    this would be considered insecure by VFIO)
> > 
> 
> There are a few layers here.  Interrupt isolation and mapping on
> s390 is accomplished via a mapping table used by a layer of firmware
> (and can be shared by a hypervisor e.g. qemu/kvm) that sits between
> the device and the kernel/driver (s390 linux always runs on at least
> this 'bare-metal hypervisor' firmware layer).  Indeed the initial
> relationship is established via zpci_set_airq -- the "zdev->fh"
> identifies the device, the "zdev->gisa" (if applicable) identifies
> the single KVM context that is eligible to receive interrupts
> related to the specified device as well as the single KVM context
> allowed to access the device via any zPCI instructions (e.g. config
> space access).  The aibv/noi indicate the vector mappings that are
> authorized for that device; firmware will typically route the
> interrupts to the guest without hypervisor involvement once this is
> established, but the table is shared by the hypervisor so that it
> can be tapped to complete delivery when necessary.  This
> registration process enables a firmware intermediary that will only
> pass along MSI from the device that has an associated,
> previously-authorized vector, associated with either the 'bare-metal
> hypervisor' (gisa = 0) and/or a specific VM (gisa != 0), depending
> what was registered as zpci_set_airq.

I suspected something like this - it technically isn't the same
"secure msi" thing since a VFIO userspace with no KVM can trigger
bogus MSIs against the kernel.

But it has been this way for a long time, let's just document it and
leave it be.

Jason

RE: [PATCH iommufd 0/9] Remove IOMMU_CAP_INTR_REMAP

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, December 9, 2022 4:26 AM
> 
> In real HW "secure MSI" is implemented in a few different ways:
> 
>  - x86 uses "interrupt remapping" which is a block that sits between
>    the device and APIC, that can "remap" the MSI MemWr using per-RID
>    tables. Part of the remapping is discarding, the per-RID tables
>    will not contain vectors that have not been enabled for the device.
> 

per-RID tables is true for AMD.

However for Intel VT-d it's per-IOMMU remapping table.

Re: [PATCH iommufd 0/9] Remove IOMMU_CAP_INTR_REMAP

[Jason Gunthorpe]

On Fri, Dec 09, 2022 at 05:54:46AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Friday, December 9, 2022 4:26 AM
> > 
> > In real HW "secure MSI" is implemented in a few different ways:
> > 
> >  - x86 uses "interrupt remapping" which is a block that sits between
> >    the device and APIC, that can "remap" the MSI MemWr using per-RID
> >    tables. Part of the remapping is discarding, the per-RID tables
> >    will not contain vectors that have not been enabled for the device.
> > 
> 
> per-RID tables is true for AMD.
> 
> However for Intel VT-d it's per-IOMMU remapping table.

Sorry, what exactly does that mean?

Doesn't the HW inspect the RID to determine what to do with the MSI?

Jason

Re: [PATCH iommufd 0/9] Remove IOMMU_CAP_INTR_REMAP

[Jason Gunthorpe]

On Fri, Dec 09, 2022 at 10:38:29AM -0400, Jason Gunthorpe wrote:
> On Fri, Dec 09, 2022 at 05:54:46AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Friday, December 9, 2022 4:26 AM
> > > 
> > > In real HW "secure MSI" is implemented in a few different ways:
> > > 
> > >  - x86 uses "interrupt remapping" which is a block that sits between
> > >    the device and APIC, that can "remap" the MSI MemWr using per-RID
> > >    tables. Part of the remapping is discarding, the per-RID tables
> > >    will not contain vectors that have not been enabled for the device.
> > > 
> > 
> > per-RID tables is true for AMD.
> > 
> > However for Intel VT-d it's per-IOMMU remapping table.
> 
> Sorry, what exactly does that mean?
> 
> Doesn't the HW inspect the RID to determine what to do with the MSI?

Okay, I get it:

 - x86 uses "interrupt remapping" which is a block that sits between
   the device and APIC, that can "remap" the MSI MemWr. AMD uses per-RID
   tables to implement isolation while Intel stores the authorized RID in
   each IRTE entry. Part of the remapping is discarding, HW will not
   forward MSIs that don't positively match the tables.

Jason

Re: [PATCH iommufd 0/9] Remove IOMMU_CAP_INTR_REMAP

[Thomas Gleixner]

On Thu, Dec 08 2022 at 16:26, Jason Gunthorpe wrote:
> [ This would be for v6.3, the series depends on a bunch of stuff in
> linux-next. I would be happy to merge it through the iommfd tree ]

I'll have a look next week.