Re: [PATCH iommufd v2 8/9] irq/s390: Add arch_is_isolated_msi() for s390

[Matthew Rosato <mjrosato@xxxxxxxxxxxxx>]

On 12/12/22 1:46 PM, Jason Gunthorpe wrote:
> s390 doesn't use irq_domains, so it has no place to set
> IRQ_DOMAIN_FLAG_ISOLATED_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 msi_device_has_isolated() to
> return true.
> 
> Remove IOMMU_CAP_INTR_REMAP from the s390 iommu driver.
> 
> Cc: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
> Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> Cc: Eric Farman <farman@xxxxxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>

Reviewed-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>

Also sanity-tested on s390 (needed the previously-mentioned #include <linux/msi.h> added to drivers/iommu/iommu.c)

> ---
>  arch/s390/include/asm/msi.h | 17 +++++++++++++++++
>  drivers/iommu/s390-iommu.c  |  2 --
>  include/linux/msi.h         |  6 +++++-
>  kernel/irq/msi.c            |  2 +-
>  4 files changed, 23 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..399343ed9ffbc6
> --- /dev/null
> +++ b/arch/s390/include/asm/msi.h
> @@ -0,0 +1,17 @@
> +/* 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_ISOLATED_MSI. See for an explanation how it works:
> + *
> + * https://lore.kernel.org/r/31af8174-35e9-ebeb-b9ef-74c90d4bfd93@xxxxxxxxxxxxx/
> + *
> + * Note this is less isolated than the ARM/x86 versions as userspace can trigger
> + * MSI belonging to kernel devices within the same gisa.
> + */
> +#define arch_is_isolated_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 e8a3f3a8a7f427..5cbe6a9d27efd6 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_isolated_msi
> +#define arch_is_isolated_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_isolated_msi(struct device *dev)
>  	 * is inherently isolated by our definition. As nobody seems to needs
>  	 * this be conservative and return false anyhow.
>  	 */
> -	return false;
> +	return arch_is_isolated_msi();
>  }
>  #endif /* CONFIG_GENERIC_MSI_IRQ */
>  
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 7c5579d3ea4f79..3e46420a4f1a9f 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -1646,6 +1646,6 @@ bool msi_device_has_isolated_msi(struct device *dev)
>  	for (; domain; domain = domain->parent)
>  		if (domain->flags & IRQ_DOMAIN_FLAG_ISOLATED_MSI)
>  			return true;
> -	return false;
> +	return arch_is_isolated_msi();
>  }
>  EXPORT_SYMBOL_GPL(msi_device_has_isolated_msi);