Hi Nicolin,

On 1/11/25 4:32 AM, Nicolin Chen wrote:

From: Jason Gunthorpe jgg@nvidia.com

All the iommu cases simply want to override the MSI page's address with

those which translate MSIs

the IOVA that was mapped through the iommu. This doesn't need a cookie pointer, we just need to store the IOVA and its page size in the msi_desc.

Instead provide msi_desc_set_iommu_msi_iova() which allows the IOMMU side to specify the IOVA that the MSI page is placed during iommu_dma_prepare(). This is stored in the msi_desc and then

iommu_dma_prepare_msi()

iommu_dma_compose_msi_msg() is a simple inline that sets address_hi/lo.

The next patch will correct the naming.

This is done because we cannot correctly lock access to group->domain in the atomic context that iommu_dma_compose_msi_msg() is called under. Today the locking miss is tolerable because dma_iommu.c operates under an assumption that the domain does not change while a driver is probed.

However iommufd now permits the domain to change while the driver is probed and VFIO userspace can create races with IRQ changes calling iommu_dma_prepare/compose_msi_msg() and changing/freeing the iommu_domain.

and is it safe in iommu_dma_prepare_msi()?

Removing the pointer, and critically, the call to iommu_get_domain_for_dev() during compose resolves this race.

Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com

---

include/linux/iommu.h | 6 ------ include/linux/msi.h | 45 +++++++++++++++++++++++---------------- drivers/iommu/dma-iommu.c | 30 +++++--------------------- 3 files changed, 32 insertions(+), 49 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 318d27841130..3a4215966c1b 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -1513,7 +1513,6 @@ static inline void iommu_debugfs_setup(void) {} int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base); int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr); -void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg); #else /* CONFIG_IOMMU_DMA */ @@ -1529,11 +1528,6 @@ static inline int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_a { return 0; }

-static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg) -{ -}

#endif /* CONFIG_IOMMU_DMA */ /* diff --git a/include/linux/msi.h b/include/linux/msi.h index b10093c4d00e..d442b4a69d56 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -184,7 +184,8 @@ struct msi_desc { struct msi_msg msg; struct irq_affinity_desc *affinity; #ifdef CONFIG_IRQ_MSI_IOMMU

• const void *iommu_cookie;

you may add kernel doc comments above

• u64 iommu_msi_iova : 58;
• u64 iommu_msi_page_shift : 6;

#endif #ifdef CONFIG_SYSFS struct device_attribute *sysfs_attrs; @@ -285,28 +286,36 @@ struct msi_desc *msi_next_desc(struct device *dev, unsigned int domid, #define msi_desc_to_dev(desc) ((desc)->dev) -#ifdef CONFIG_IRQ_MSI_IOMMU -static inline const void *msi_desc_get_iommu_cookie(struct msi_desc *desc) -{

• return desc->iommu_cookie;

-}

-static inline void msi_desc_set_iommu_cookie(struct msi_desc *desc,

• 			     const void *iommu_cookie)
  

+static inline void msi_desc_set_iommu_msi_iova(struct msi_desc *desc,

• 			       u64 msi_iova,
  

• 			       unsigned int page_shift)
  

{

• desc->iommu_cookie = iommu_cookie;

-} -#else -static inline const void *msi_desc_get_iommu_cookie(struct msi_desc *desc) -{

• return NULL;

+#ifdef CONFIG_IRQ_MSI_IOMMU

• desc->iommu_msi_iova = msi_iova >> page_shift;
• desc->iommu_msi_page_shift = page_shift;

+#endif } -static inline void msi_desc_set_iommu_cookie(struct msi_desc *desc,

• 			     const void *iommu_cookie)
  

+/**

• • iommu_dma_compose_msi_msg() - Apply translation to an MSI message
• • @desc: MSI descriptor prepared by iommu_dma_prepare_msi()
• • @msg: MSI message containing target physical address
• */

+static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc,

• 			     struct msi_msg *msg)
  

{ -} +#ifdef CONFIG_IRQ_MSI_IOMMU

• if (desc->iommu_msi_page_shift) {

• u64 msi_iova = desc->iommu_msi_iova
  

• 	       << desc->iommu_msi_page_shift;
  

• msg->address_hi = upper_32_bits(msi_iova);
  

• msg->address_lo = lower_32_bits(msi_iova) |
  

• 		  (msg->address_lo &
  

• 		   ((1 << desc->iommu_msi_page_shift) - 1));
  

• }

#endif +} int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid, struct msi_desc *init_desc); diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 2a9fa0c8cc00..bf91e014d179 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -1815,7 +1815,7 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr) static DEFINE_MUTEX(msi_prepare_lock); /* see below */ if (!domain || !domain->iova_cookie) {

• desc->iommu_cookie = NULL;
  

• msi_desc_set_iommu_msi_iova(desc, 0, 0);
  

  return 0; }

@@ -1827,33 +1827,13 @@ int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr) mutex_lock(&msi_prepare_lock); msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain); mutex_unlock(&msi_prepare_lock);

• msi_desc_set_iommu_cookie(desc, msi_page);
• if (!msi_page) return -ENOMEM;
• return 0;

-} -/**

• • iommu_dma_compose_msi_msg() - Apply translation to an MSI message
• • @desc: MSI descriptor prepared by iommu_dma_prepare_msi()
• • @msg: MSI message containing target physical address
• */

-void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg) -{

• struct device *dev = msi_desc_to_dev(desc);
• const struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
• const struct iommu_dma_msi_page *msi_page;
• msi_page = msi_desc_get_iommu_cookie(desc);
• if (!domain || !domain->iova_cookie || WARN_ON(!msi_page))

• return;
  

• msg->address_hi = upper_32_bits(msi_page->iova);
• msg->address_lo &= cookie_msi_granule(domain->iova_cookie) - 1;
• msg->address_lo += lower_32_bits(msi_page->iova);

• msi_desc_set_iommu_msi_iova(

• desc, msi_page->iova,
  

• ilog2(cookie_msi_granule(domain->iova_cookie)));
  

• return 0;

} static int iommu_dma_init(void)

Hi,

On 1/23/25 7:48 PM, Jason Gunthorpe wrote:

On Thu, Jan 23, 2025 at 06:10:48PM +0100, Eric Auger wrote:

...

...

However iommufd now permits the domain to change while the driver is probed and VFIO userspace can create races with IRQ changes calling iommu_dma_prepare/compose_msi_msg() and changing/freeing the iommu_domain.

and is it safe in iommu_dma_prepare_msi()?

iommu_dma_prepare_msi() takes the group mutex:

int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr) { struct device *dev = msi_desc_to_dev(desc); struct iommu_group *group = dev->iommu_group;

mutex_lock(&group->mutex); if (group->domain && group->domain->sw_msi) ret = group->domain->sw_msi(group->domain, desc, msi_addr);

Which prevents changing domain attachments during execution.

For iommufd, if the domain attachment changes immediately after iommu_dma_prepare_msi() unlocks, then the information given to msi_desc_set_iommu_msi_iova() is still valid on the new domain.

This is because the iommufd implementation of sw_msi keeps the same IOVA for the same ITS page globally across all domains. Any racing change of domain will attach a new domain with the right ITS IOVA already mapped and populated. It is why this series stops using the domain pointer as a cookie inside the msi_desc, immediately after the group->mutex is unlocked a new domain can be attached and the old domain can be freed, which would UAF the domain pointer in the cookie.

OK thank you for the clarification

...

...

diff --git a/include/linux/msi.h b/include/linux/msi.h index b10093c4d00e..d442b4a69d56 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -184,7 +184,8 @@ struct msi_desc { struct msi_msg msg; struct irq_affinity_desc *affinity; #ifdef CONFIG_IRQ_MSI_IOMMU

• const void *iommu_cookie;

you may add kernel doc comments above

I wondered if internal stuff was not being documented as the old iommu_cookie didn't have a comment..

But sure:

• @iommu_msi_iova: Optional IOVA from the IOMMU to overide the msi_addr.

•              Only used if iommu_msi_page_shift != 0
  

• @iommu_msi_page_shift: Indicates how many bits of the original address

•                    should be preserved when using iommu_msi_iova.
  

Sounds good

Eric

Jason

On 1/11/25 4:32 AM, Nicolin Chen wrote:

From: Jason Gunthorpe jgg@nvidia.com

The new function is used to take in a u64 MSI address and store it in the msi_msg. If the iommu has provided an alternative address then that is replaced instead.

All callers have a tidy u64 already so this also consolidates the repeated low/high code into a small helper.

Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com

---

include/linux/msi.h | 18 ++++++++---------- drivers/irqchip/irq-gic-v2m.c | 5 +---- drivers/irqchip/irq-gic-v3-its.c | 13 +++---------- drivers/irqchip/irq-gic-v3-mbi.c | 12 ++++-------- drivers/irqchip/irq-ls-scfg-msi.c | 5 ++--- 5 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/include/linux/msi.h b/include/linux/msi.h index d442b4a69d56..f6369748fc6e 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -296,13 +296,8 @@ static inline void msi_desc_set_iommu_msi_iova(struct msi_desc *desc, #endif } -/**

• • iommu_dma_compose_msi_msg() - Apply translation to an MSI message
• • @desc: MSI descriptor prepared by iommu_dma_prepare_msi()
• • @msg: MSI message containing target physical address
• */

-static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc,

• 			     struct msi_msg *msg)
  

+static inline void msi_msg_set_msi_addr(struct msi_desc *desc,

• 			struct msi_msg *msg, u64 msi_addr)
  

nit: msi_msg_set_addr(ess) may be enough as the populated fields are address_lo/hi

{ #ifdef CONFIG_IRQ_MSI_IOMMU if (desc->iommu_msi_page_shift) { @@ -310,11 +305,14 @@ static inline void iommu_dma_compose_msi_msg(struct msi_desc *desc, << desc->iommu_msi_page_shift; msg->address_hi = upper_32_bits(msi_iova);

• msg->address_lo = lower_32_bits(msi_iova) |
  

• 		  (msg->address_lo &
  

• 		   ((1 << desc->iommu_msi_page_shift) - 1));
  

• msg->address_lo =
  

• 	lower_32_bits(msi_iova) |
  

• 	(msi_addr & ((1 << desc->iommu_msi_page_shift) - 1));
  

• return;
  

  }

#endif

• msg->address_hi = upper_32_bits(msi_addr);
• msg->address_lo = lower_32_bits(msi_addr);

} int msi_domain_insert_msi_desc(struct device *dev, unsigned int domid, diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c index be35c5349986..6599c56873ad 100644 --- a/drivers/irqchip/irq-gic-v2m.c +++ b/drivers/irqchip/irq-gic-v2m.c @@ -87,9 +87,6 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) struct v2m_data *v2m = irq_data_get_irq_chip_data(data); phys_addr_t addr = gicv2m_get_msi_addr(v2m, data->hwirq);

• msg->address_hi = upper_32_bits(addr);
• msg->address_lo = lower_32_bits(addr);
• if (v2m->flags & GICV2M_GRAVITON_ADDRESS_ONLY) msg->data = 0; else

@@ -97,7 +94,7 @@ static void gicv2m_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) if (v2m->flags & GICV2M_NEEDS_SPI_OFFSET) msg->data -= v2m->spi_offset;

• iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);

• msi_msg_set_msi_addr(irq_data_get_msi_desc(data), msg, addr);

} static struct irq_chip gicv2m_irq_chip = { diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 92244cfa0464..8c3ab7b471ca 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -1809,17 +1809,10 @@ static u64 its_irq_get_msi_base(struct its_device *its_dev) static void its_irq_compose_msi_msg(struct irq_data *d, struct msi_msg *msg) { struct its_device *its_dev = irq_data_get_irq_chip_data(d);

• struct its_node *its;
• u64 addr;
• its = its_dev->its;
• addr = its->get_msi_base(its_dev);
• msg->address_lo = lower_32_bits(addr);
• msg->address_hi = upper_32_bits(addr);
• msg->data = its_get_event_id(d);

• iommu_dma_compose_msi_msg(irq_data_get_msi_desc(d), msg);

• msg->data = its_get_event_id(d);
• msi_msg_set_msi_addr(irq_data_get_msi_desc(d), msg,

• 	     its_dev->its->get_msi_base(its_dev));
  

} static int its_irq_set_irqchip_state(struct irq_data *d, diff --git a/drivers/irqchip/irq-gic-v3-mbi.c b/drivers/irqchip/irq-gic-v3-mbi.c index 3fe870f8ee17..513479da9ee3 100644 --- a/drivers/irqchip/irq-gic-v3-mbi.c +++ b/drivers/irqchip/irq-gic-v3-mbi.c @@ -147,22 +147,18 @@ static const struct irq_domain_ops mbi_domain_ops = { static void mbi_compose_msi_msg(struct irq_data *data, struct msi_msg *msg) {

• msg[0].address_hi = upper_32_bits(mbi_phys_base + GICD_SETSPI_NSR);
• msg[0].address_lo = lower_32_bits(mbi_phys_base + GICD_SETSPI_NSR); msg[0].data = data->parent_data->hwirq;
• iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);

• msi_msg_set_msi_addr(irq_data_get_msi_desc(data), &msg[0],

• 	     mbi_phys_base + GICD_SETSPI_NSR);
  

} static void mbi_compose_mbi_msg(struct irq_data *data, struct msi_msg *msg) { mbi_compose_msi_msg(data, msg);

• msg[1].address_hi = upper_32_bits(mbi_phys_base + GICD_CLRSPI_NSR);
• msg[1].address_lo = lower_32_bits(mbi_phys_base + GICD_CLRSPI_NSR); msg[1].data = data->parent_data->hwirq;
• iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), &msg[1]);

• msi_msg_set_msi_addr(irq_data_get_msi_desc(data), &msg[1],

• 	     mbi_phys_base + GICD_CLRSPI_NSR);
  

} static bool mbi_init_dev_msi_info(struct device *dev, struct irq_domain *domain, diff --git a/drivers/irqchip/irq-ls-scfg-msi.c b/drivers/irqchip/irq-ls-scfg-msi.c index c0e1aafe468c..2ac6d89b4cb4 100644 --- a/drivers/irqchip/irq-ls-scfg-msi.c +++ b/drivers/irqchip/irq-ls-scfg-msi.c @@ -87,8 +87,6 @@ static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg) { struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(data);

• msg->address_hi = upper_32_bits(msi_data->msiir_addr);
• msg->address_lo = lower_32_bits(msi_data->msiir_addr); msg->data = data->hwirq;

if (msi_affinity_flag) { @@ -98,7 +96,8 @@ static void ls_scfg_msi_compose_msg(struct irq_data *data, struct msi_msg *msg) msg->data |= cpumask_first(mask); }

• iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data), msg);

• msi_msg_set_msi_addr(irq_data_get_msi_desc(data), msg,

• 	     msi_data->msiir_addr);
  

} static int ls_scfg_msi_set_affinity(struct irq_data *irq_data,

Hi,

On 1/11/25 4:32 AM, Nicolin Chen wrote:

From: Jason Gunthorpe jgg@nvidia.com

SW_MSI supports IOMMU to translate an MSI message before the MSI message is delivered to the interrupt controller. On such systems the iommu_domain must have a translation for the MSI message for interrupts to work.

The IRQ subsystem will call into IOMMU to request that a physical page be setup to receive MSI message, and the IOMMU then sets an IOVA that maps to that physical page. Ultimately the IOVA is programmed into the device via the msi_msg.

Generalize this to allow the iommu_domain owner to provide its own implementation of this mapping. Add a function pointer to struct iommu_domain to allow the domain owner to provide an implementation.

Have dma-iommu supply its implementation for IOMMU_DOMAIN_DMA types during the iommu_get_dma_cookie() path. For IOMMU_DOMAIN_UNMANAGED types used by VFIO (and iommufd for now), have the same iommu_dma_sw_msi set as well in the iommu_get_msi_cookie() path.

Hold the group mutex while in iommu_dma_prepare_msi() to ensure the domain doesn't change or become freed while running. Races with IRQ operations from VFIO and domain changes from iommufd are possible here.

this was my question in previous comments

Rreplace the msi_prepare_lock with a lockdep assertion for the group mutex

Replace

as documentation. For the dma_iommu.c each iommu_domain unique to a

is?

group.

Signed-off-by: Jason Gunthorpe jgg@nvidia.com [nicolinc: move iommu_domain_set_sw_msi() from iommu_dma_init_domain() to iommu_dma_init_domain(); add in iommu_put_dma_cookie() an sw_msi test] Signed-off-by: Nicolin Chen nicolinc@nvidia.com

---

include/linux/iommu.h | 44 ++++++++++++++++++++++++++------------- drivers/iommu/dma-iommu.c | 33 +++++++++++++---------------- drivers/iommu/iommu.c | 29 ++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 33 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 3a4215966c1b..423fdfa6b3bb 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -44,6 +44,8 @@ struct iommu_dma_cookie; struct iommu_fault_param; struct iommufd_ctx; struct iommufd_viommu; +struct msi_desc; +struct msi_msg; #define IOMMU_FAULT_PERM_READ (1 << 0) /* read */ #define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */ @@ -216,6 +218,12 @@ struct iommu_domain { struct iommu_domain_geometry geometry; struct iommu_dma_cookie *iova_cookie; int (*iopf_handler)(struct iopf_group *group);

+#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)

• int (*sw_msi)(struct iommu_domain *domain, struct msi_desc *desc,

•       phys_addr_t msi_addr);
  

+#endif

• void *fault_data; union { struct {

@@ -234,6 +242,16 @@ struct iommu_domain { }; }; +static inline void iommu_domain_set_sw_msi(

• struct iommu_domain *domain,
• int (*sw_msi)(struct iommu_domain *domain, struct msi_desc *desc,

•       phys_addr_t msi_addr))
  

+{ +#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU)

• domain->sw_msi = sw_msi;

+#endif +}

static inline bool iommu_is_dma_domain(struct iommu_domain *domain) { return domain->type & __IOMMU_DOMAIN_DMA_API; @@ -1475,6 +1493,18 @@ static inline ioasid_t iommu_alloc_global_pasid(struct device *dev) static inline void iommu_free_global_pasid(ioasid_t pasid) {} #endif /* CONFIG_IOMMU_API */ +#ifdef CONFIG_IRQ_MSI_IOMMU +#ifdef CONFIG_IOMMU_API +int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr); +#else +static inline int iommu_dma_prepare_msi(struct msi_desc *desc,

• 			phys_addr_t msi_addr)
  

+{

• return 0;

+} +#endif /* CONFIG_IOMMU_API */ +#endif /* CONFIG_IRQ_MSI_IOMMU */

#if IS_ENABLED(CONFIG_LOCKDEP) && IS_ENABLED(CONFIG_IOMMU_API) void iommu_group_mutex_assert(struct device *dev); #else @@ -1508,26 +1538,12 @@ static inline void iommu_debugfs_setup(void) {} #endif #ifdef CONFIG_IOMMU_DMA -#include <linux/msi.h>

int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base);

-int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr);

#else /* CONFIG_IOMMU_DMA */

-struct msi_desc; -struct msi_msg;

static inline int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base) { return -ENODEV; }

-static inline int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr) -{

• return 0;

-} #endif /* CONFIG_IOMMU_DMA */ /* diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index bf91e014d179..3b58244e6344 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -24,6 +24,7 @@ #include <linux/memremap.h> #include <linux/mm.h> #include <linux/mutex.h> +#include <linux/msi.h> #include <linux/of_iommu.h> #include <linux/pci.h> #include <linux/scatterlist.h> @@ -102,6 +103,9 @@ static int __init iommu_dma_forcedac_setup(char *str) } early_param("iommu.forcedac", iommu_dma_forcedac_setup); +static int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,

• 	    phys_addr_t msi_addr);
  

/* Number of entries per flush queue */ #define IOVA_DEFAULT_FQ_SIZE 256 #define IOVA_SINGLE_FQ_SIZE 32768 @@ -398,6 +402,7 @@ int iommu_get_dma_cookie(struct iommu_domain *domain) return -ENOMEM; mutex_init(&domain->iova_cookie->mutex);

• iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi); return 0;

} @@ -429,6 +434,7 @@ int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base) cookie->msi_iova = base; domain->iova_cookie = cookie;

• iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi); return 0;

} EXPORT_SYMBOL(iommu_get_msi_cookie); @@ -443,6 +449,9 @@ void iommu_put_dma_cookie(struct iommu_domain *domain) struct iommu_dma_cookie *cookie = domain->iova_cookie; struct iommu_dma_msi_page *msi, *tmp;

• if (domain->sw_msi != iommu_dma_sw_msi)

• return;
  

I don't get the above check. The comment says this is also called for a cookie prepared with iommu_get_dma_cookie(). Don't you need to do some cleanup for this latter?

if (!cookie) return; @@ -1800,33 +1809,19 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, return NULL; } -/**

• • iommu_dma_prepare_msi() - Map the MSI page in the IOMMU domain
• • @desc: MSI descriptor, will store the MSI page
• • @msi_addr: MSI target address to be mapped
• • Return: 0 on success or negative error code if the mapping failed.
• */

-int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr) +static int iommu_dma_sw_msi(struct iommu_domain *domain, struct msi_desc *desc,

• 	    phys_addr_t msi_addr)
  

{ struct device *dev = msi_desc_to_dev(desc);

• struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
• struct iommu_dma_msi_page *msi_page;
• static DEFINE_MUTEX(msi_prepare_lock); /* see below */

• const struct iommu_dma_msi_page *msi_page;

• if (!domain || !domain->iova_cookie) {

• if (!domain->iova_cookie) { msi_desc_set_iommu_msi_iova(desc, 0, 0); return 0; }

• /*

• * In fact the whole prepare operation should already be serialised by
  

• * irq_domain_mutex further up the callchain, but that's pretty subtle
  

• * on its own, so consider this locking as failsafe documentation...
  

• */
  

• mutex_lock(&msi_prepare_lock);

• iommu_group_mutex_assert(dev); msi_page = iommu_dma_get_msi_page(dev, msi_addr, domain);

• mutex_unlock(&msi_prepare_lock); if (!msi_page) return -ENOMEM;

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 599030e1e890..fbbbcdba8a4f 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3587,3 +3587,32 @@ int iommu_replace_group_handle(struct iommu_group *group, return ret; } EXPORT_SYMBOL_NS_GPL(iommu_replace_group_handle, "IOMMUFD_INTERNAL");

+#if IS_ENABLED(CONFIG_IRQ_MSI_IOMMU) +/**

• • iommu_dma_prepare_msi() - Map the MSI page in the IOMMU domain
• • @desc: MSI descriptor, will store the MSI page
• • @msi_addr: MSI target address to be mapped
• • The implementation of sw_msi() should take msi_addr and map it to
• • an IOVA in the domain and call msi_desc_set_iommu_msi_iova() with the
• • mapping information.
• • Return: 0 on success or negative error code if the mapping failed.
• */

+int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr) +{

• struct device *dev = msi_desc_to_dev(desc);
• struct iommu_group *group = dev->iommu_group;
• int ret = 0;
• if (!group)

• return 0;
  

• mutex_lock(&group->mutex);
• if (group->domain && group->domain->sw_msi)

• ret = group->domain->sw_msi(group->domain, desc, msi_addr);
  

• mutex_unlock(&group->mutex);
• return ret;

+} +#endif /* CONFIG_IRQ_MSI_IOMMU */

Thanks

Eric

Hi Jason,

On 1/23/25 7:16 PM, Jason Gunthorpe wrote:

On Thu, Jan 23, 2025 at 06:10:47PM +0100, Eric Auger wrote:

...

Hi,

On 1/11/25 4:32 AM, Nicolin Chen wrote:

...

From: Jason Gunthorpe jgg@nvidia.com

SW_MSI supports IOMMU to translate an MSI message before the MSI message is delivered to the interrupt controller. On such systems the iommu_domain must have a translation for the MSI message for interrupts to work.

The IRQ subsystem will call into IOMMU to request that a physical page be setup to receive MSI message, and the IOMMU then sets an IOVA that maps to that physical page. Ultimately the IOVA is programmed into the device via the msi_msg.

Generalize this to allow the iommu_domain owner to provide its own implementation of this mapping. Add a function pointer to struct iommu_domain to allow the domain owner to provide an implementation.

Have dma-iommu supply its implementation for IOMMU_DOMAIN_DMA types during the iommu_get_dma_cookie() path. For IOMMU_DOMAIN_UNMANAGED types used by VFIO (and iommufd for now), have the same iommu_dma_sw_msi set as well in the iommu_get_msi_cookie() path.

Hold the group mutex while in iommu_dma_prepare_msi() to ensure the domain doesn't change or become freed while running. Races with IRQ operations from VFIO and domain changes from iommufd are possible here.

this was my question in previous comments

Ah, well there is the answer :)

...

...

Rreplace the msi_prepare_lock with a lockdep assertion for the group mutex

Replace

...

as documentation. For the dma_iommu.c each iommu_domain unique to a

is?

...

group.

Yes

Replace the msi_prepare_lock with a lockdep assertion for the group mutex as documentation. For the dmau_iommu.c each iommu_domain is unique to a group.

...

...

@@ -443,6 +449,9 @@ void iommu_put_dma_cookie(struct iommu_domain *domain) struct iommu_dma_cookie *cookie = domain->iova_cookie; struct iommu_dma_msi_page *msi, *tmp;

• if (domain->sw_msi != iommu_dma_sw_msi)

• return;
  

I don't get the above check.

It is because of this:

void iommu_domain_free(struct iommu_domain *domain) { if (domain->type == IOMMU_DOMAIN_SVA) mmdrop(domain->mm); iommu_put_dma_cookie(domain);

iommufd may be using domain->sw_msi so iommu_put_dma_cookie() needs to be a NOP. Also, later we move cookie into a union so it is not reliably NULL anymore.

OK

...

The comment says this is also called for a cookie prepared with iommu_get_dma_cookie(). Don't you need to do some cleanup for this latter?

That seems seems OK, only two places set domain->iova_cookie:

int iommu_get_dma_cookie(struct iommu_domain *domain) { domain->iova_cookie = cookie_alloc(IOMMU_DMA_IOVA_COOKIE); iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);

and

int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base) { domain->iova_cookie = cookie; iommu_domain_set_sw_msi(domain, iommu_dma_sw_msi);

So (domain->sw_msi == iommu_dma_sw_msi) in iommu_put_dma_cookie() for both cases..

makes sense.

Thanks

Eric

Jason

On Thu, Jan 23, 2025 at 09:54:38AM +0000, Tian, Kevin wrote:

...

From: Nicolin Chen nicolinc@nvidia.com Sent: Saturday, January 11, 2025 11:32 AM

@@ -224,8 +224,10 @@ struct iommu_domain { phys_addr_t msi_addr); #endif

• void *fault_data;
• union {

• union { /* Pointer usable by owner of the domain */

• struct iommufd_hw_pagetable *iommufd_hwpt; /* iommufd
  

*/

• };
• union { /* Fault handler */

hmm is it better to rename it as "void *private;" and let the caller do type conversion?

I like the type safety, the union can hold other in-kernel users with their proper types and this discourages drivers from inventing weird things..

Jason

Hi Eric,

On Wed, Jan 29, 2025 at 01:40:54PM +0100, Eric Auger wrote:

On 1/11/25 4:32 AM, Nicolin Chen wrote:

...

A "fault_data" was added exclusively for the iommufd_fault_iopf_handler() used by IOPF/PRI use cases, along with the attach_handle. Now, the iommufd version of sw_msi function will resue the attach_handle and fault_data for

reuse

Ack.

...

diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index ce03c3804651..f7c0d7b214b6 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -402,10 +402,10 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd) } hwpt->fault = fault; hwpt->domain->iopf_handler = iommufd_fault_iopf_handler;

• hwpt->domain->fault_data = hwpt;
  

  refcount_inc(&fault->obj.users); iommufd_put_object(ucmd->ictx, &fault->obj); }

• hwpt->domain->iommufd_hwpt = hwpt;

don't we want to reset this somewhere on release path?

We do iommu_domain_free() entirely on HWPT's release path.

This basically sets the domain's "owner data" as Jason remarked: https://lore.kernel.org/linux-iommu/20250113164037.GO5556@nvidia.com/

Thanks Nicolin

On 2025/1/11 11:32, Nicolin Chen wrote:

"attach_handle" was added exclusively for the iommufd_fault_iopf_handler() used by IOPF/PRI use cases, along with the "fault_data". Now, the iommufd version of sw_msi function will resue the attach_handle and fault_data for a non-fault case.

Move the attach_handle part out of the fault.c file to make it generic for all cases. Simplify the remaining fault specific routine to attach/detach.

I guess you can send it separately since both of our series need it. :)

Signed-off-by: Nicolin Chen nicolinc@nvidia.com

drivers/iommu/iommufd/iommufd_private.h | 40 +------- drivers/iommu/iommufd/device.c | 105 +++++++++++++++++++++ drivers/iommu/iommufd/fault.c | 120 +++--------------------- 3 files changed, 122 insertions(+), 143 deletions(-)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index b6d706cf2c66..063c0a42f54f 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -472,42 +472,12 @@ void iommufd_fault_destroy(struct iommufd_object *obj); int iommufd_fault_iopf_handler(struct iopf_group *group); int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,

• 		    struct iommufd_device *idev);
  

• 		    struct iommufd_device *idev,
  

• 		    bool enable_iopf);
  

  void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,

• 		     struct iommufd_device *idev);
  

-int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,

• 		     struct iommufd_hw_pagetable *hwpt,
  

• 		     struct iommufd_hw_pagetable *old);
  

-static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,

• 			     struct iommufd_device *idev)
  

-{

• if (hwpt->fault)

• return iommufd_fault_domain_attach_dev(hwpt, idev);
  

• return iommu_attach_group(hwpt->domain, idev->igroup->group);

-}

-static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,

• 			      struct iommufd_device *idev)
  

-{

• if (hwpt->fault) {

• iommufd_fault_domain_detach_dev(hwpt, idev);
  

• return;
  

• }
• iommu_detach_group(hwpt->domain, idev->igroup->group);

-}

-static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,

• 			      struct iommufd_hw_pagetable *hwpt,
  

• 			      struct iommufd_hw_pagetable *old)
  

-{

• if (old->fault || hwpt->fault)

• return iommufd_fault_domain_replace_dev(idev, hwpt, old);
  

• return iommu_group_replace_domain(idev->igroup->group, hwpt->domain);

-}

• 		     struct iommufd_device *idev,
  

• 		     struct iommufd_attach_handle *handle,
  

• 		     bool disable_iopf);
  

static inline struct iommufd_viommu * iommufd_get_viommu(struct iommufd_ucmd *ucmd, u32 id) diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index dfd0898fb6c1..38b31b652147 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -352,6 +352,111 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev, return 0; } +/* The device attach/detach/replace helpers for attach_handle */

+static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,

• 		      struct iommufd_device *idev)
  

+{

• struct iommufd_attach_handle *handle;
• int rc;
• if (hwpt->fault) {

• rc = iommufd_fault_domain_attach_dev(hwpt, idev, true);
  

• if (rc)
  

• 	return rc;
  

• }
• handle = kzalloc(sizeof(*handle), GFP_KERNEL);
• if (!handle) {

• rc = -ENOMEM;
  

• goto out_fault_detach;
  

• }
• handle->idev = idev;
• rc = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,

• 		       &handle->handle);
  

• if (rc)

• goto out_free_handle;
  

• return 0;

+out_free_handle:

• kfree(handle);
• handle = NULL;

+out_fault_detach:

• if (hwpt->fault)

• iommufd_fault_domain_detach_dev(hwpt, idev, handle, true);
  

• return rc;

+}

+static struct iommufd_attach_handle * +iommufd_device_get_attach_handle(struct iommufd_device *idev) +{

• struct iommu_attach_handle *handle;
• handle =

• iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0);
  

• if (IS_ERR(handle))

• return NULL;
  

• return to_iommufd_handle(handle);

+}

+static void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,

• 		       struct iommufd_device *idev)
  

+{

• struct iommufd_attach_handle *handle;
• handle = iommufd_device_get_attach_handle(idev);
• iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
• if (hwpt->fault)

• iommufd_fault_domain_detach_dev(hwpt, idev, handle, true);
  

• kfree(handle);

+}

+static int iommufd_hwpt_replace_device(struct iommufd_device *idev,

• 		       struct iommufd_hw_pagetable *hwpt,
  

• 		       struct iommufd_hw_pagetable *old)
  

+{

• struct iommufd_attach_handle *old_handle =

• iommufd_device_get_attach_handle(idev);
  

• struct iommufd_attach_handle *handle;
• int rc;
• if (hwpt->fault) {

• rc = iommufd_fault_domain_attach_dev(hwpt, idev, !old->fault);
  

• if (rc)
  

• 	return rc;
  

• }
• handle = kzalloc(sizeof(*handle), GFP_KERNEL);
• if (!handle) {

• rc = -ENOMEM;
  

• goto out_fault_detach;
  

• }
• handle->idev = idev;
• rc = iommu_replace_group_handle(idev->igroup->group, hwpt->domain,

• 			&handle->handle);
  

• if (rc)

• goto out_free_handle;
  

• if (old->fault)

• iommufd_fault_domain_detach_dev(old, idev, old_handle,
  

• 				!hwpt->fault);
  

• kfree(old_handle);
• return 0;

+out_free_handle:

• kfree(handle);
• handle = NULL;

+out_fault_detach:

• if (hwpt->fault)

• iommufd_fault_domain_detach_dev(hwpt, idev, handle,
  

• 				!old->fault);
  

• return rc;

+}

• int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) {

diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index 06aa83a75e94..1d9bd3024b57 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -60,42 +60,17 @@ static void iommufd_fault_iopf_disable(struct iommufd_device *idev) mutex_unlock(&idev->iopf_lock); } -static int __fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,

• 		     struct iommufd_device *idev)
  

-{

• struct iommufd_attach_handle *handle;
• int ret;
• handle = kzalloc(sizeof(*handle), GFP_KERNEL);
• if (!handle)

• return -ENOMEM;
  

• handle->idev = idev;
• ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,

• 			&handle->handle);
  

• if (ret)

• kfree(handle);
  

• return ret;

-}

• int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,

• 		    struct iommufd_device *idev)
  

• 		    struct iommufd_device *idev,
  

• 		    bool enable_iopf)
  

  {

• int ret;

• int rc = 0;

if (!hwpt->fault) return -EINVAL;

• ret = iommufd_fault_iopf_enable(idev);
• if (ret)

• return ret;
  

• ret = __fault_domain_attach_dev(hwpt, idev);
• if (ret)

• iommufd_fault_iopf_disable(idev);
  

• return ret;

• if (enable_iopf)

• rc = iommufd_fault_iopf_enable(idev);
  

• return rc; }

static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt, @@ -127,86 +102,15 @@ static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt, mutex_unlock(&fault->mutex); } -static struct iommufd_attach_handle * -iommufd_device_get_attach_handle(struct iommufd_device *idev) -{

• struct iommu_attach_handle *handle;
• handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0);
• if (IS_ERR(handle))

• return NULL;
  

• return to_iommufd_handle(handle);

-}

• void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,

• 		     struct iommufd_device *idev)
  

• 		     struct iommufd_device *idev,
  

• 		     struct iommufd_attach_handle *handle,
  

• 		     bool disable_iopf)
  

  {

• struct iommufd_attach_handle *handle;
• handle = iommufd_device_get_attach_handle(idev);
• iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
• iommufd_auto_response_faults(hwpt, handle);
• iommufd_fault_iopf_disable(idev);
• kfree(handle);

-}

-static int __fault_domain_replace_dev(struct iommufd_device *idev,

• 		      struct iommufd_hw_pagetable *hwpt,
  

• 		      struct iommufd_hw_pagetable *old)
  

-{

• struct iommufd_attach_handle *handle, *curr = NULL;
• int ret;
• if (old->fault)

• curr = iommufd_device_get_attach_handle(idev);
  

• if (hwpt->fault) {

• handle = kzalloc(sizeof(*handle), GFP_KERNEL);
  

• if (!handle)
  

• 	return -ENOMEM;
  

• handle->idev = idev;
  

• ret = iommu_replace_group_handle(idev->igroup->group,
  

• 				 hwpt->domain, &handle->handle);
  

• } else {

• ret = iommu_replace_group_handle(idev->igroup->group,
  

• 				 hwpt->domain, NULL);
  

• }
• if (!ret && curr) {

• iommufd_auto_response_faults(old, curr);
  

• kfree(curr);
  

• }
• return ret;

-}

-int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,

• 		     struct iommufd_hw_pagetable *hwpt,
  

• 		     struct iommufd_hw_pagetable *old)
  

-{

• bool iopf_off = !hwpt->fault && old->fault;
• bool iopf_on = hwpt->fault && !old->fault;
• int ret;
• if (iopf_on) {

• ret = iommufd_fault_iopf_enable(idev);
  

• if (ret)
  

• 	return ret;
  

• }
• ret = __fault_domain_replace_dev(idev, hwpt, old);
• if (ret) {

• if (iopf_on)
  

• 	iommufd_fault_iopf_disable(idev);
  

• return ret;
  

• }
• if (iopf_off)

• if (handle)

• iommufd_auto_response_faults(hwpt, handle);
  

no need to check handle. After this patch, both the non-fault and the fault path will allocate handle. It cannot be used to isolate fault and non-fault path. Also, the callers of iommufd_fault_domain_detach_dev() will check hwpt->fault before calling it. So just call iommufd_auto_response_faults().

• if (disable_iopf) iommufd_fault_iopf_disable(idev);

• return 0; }

void iommufd_fault_destroy(struct iommufd_object *obj)

Hi,

On 1/11/25 4:32 AM, Nicolin Chen wrote:

"attach_handle" was added exclusively for the iommufd_fault_iopf_handler() used by IOPF/PRI use cases, along with the "fault_data". Now, the iommufd version of sw_msi function will resue the attach_handle and fault_data for

reuse

a non-fault case.

Move the attach_handle part out of the fault.c file to make it generic for all cases. Simplify the remaining fault specific routine to attach/detach.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com

drivers/iommu/iommufd/iommufd_private.h | 40 +------- drivers/iommu/iommufd/device.c | 105 +++++++++++++++++++++ drivers/iommu/iommufd/fault.c | 120 +++--------------------- 3 files changed, 122 insertions(+), 143 deletions(-)

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index b6d706cf2c66..063c0a42f54f 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -472,42 +472,12 @@ void iommufd_fault_destroy(struct iommufd_object *obj); int iommufd_fault_iopf_handler(struct iopf_group *group); int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,

• 		    struct iommufd_device *idev);
  

• 		    struct iommufd_device *idev,
  

• 		    bool enable_iopf);
  

void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,

• 		     struct iommufd_device *idev);
  

-int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,

• 		     struct iommufd_hw_pagetable *hwpt,
  

• 		     struct iommufd_hw_pagetable *old);
  

-static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,

• 			     struct iommufd_device *idev)
  

-{

• if (hwpt->fault)

• return iommufd_fault_domain_attach_dev(hwpt, idev);
  

• return iommu_attach_group(hwpt->domain, idev->igroup->group);

-}

-static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,

• 			      struct iommufd_device *idev)
  

-{

• if (hwpt->fault) {

• iommufd_fault_domain_detach_dev(hwpt, idev);
  

• return;
  

• }
• iommu_detach_group(hwpt->domain, idev->igroup->group);

-}

-static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,

• 			      struct iommufd_hw_pagetable *hwpt,
  

• 			      struct iommufd_hw_pagetable *old)
  

-{

• if (old->fault || hwpt->fault)

• return iommufd_fault_domain_replace_dev(idev, hwpt, old);
  

• return iommu_group_replace_domain(idev->igroup->group, hwpt->domain);

-}

• 		     struct iommufd_device *idev,
  

• 		     struct iommufd_attach_handle *handle,
  

• 		     bool disable_iopf);
  

static inline struct iommufd_viommu * iommufd_get_viommu(struct iommufd_ucmd *ucmd, u32 id) diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index dfd0898fb6c1..38b31b652147 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -352,6 +352,111 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev, return 0; } +/* The device attach/detach/replace helpers for attach_handle */

+static int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,

• 		      struct iommufd_device *idev)
  

+{

• struct iommufd_attach_handle *handle;
• int rc;
• if (hwpt->fault) {

• rc = iommufd_fault_domain_attach_dev(hwpt, idev, true);
  

why don't we simply call iommufd_fault_iopf_enable(idev) also it looks there is a redundant check of hwpt_fault here and in

iommufd_fault_domain_attach_dev

Besides the addition of enable_iopf param is not documented anywhere

• if (rc)
  

• 	return rc;
  

• }
• handle = kzalloc(sizeof(*handle), GFP_KERNEL);
• if (!handle) {

• rc = -ENOMEM;
  

• goto out_fault_detach;
  

• }
• handle->idev = idev;
• rc = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,

• 		       &handle->handle);
  

• if (rc)

• goto out_free_handle;
  

• return 0;

+out_free_handle:

• kfree(handle);
• handle = NULL;

+out_fault_detach:

• if (hwpt->fault)

• iommufd_fault_domain_detach_dev(hwpt, idev, handle, true);
  

• return rc;

+}

+static struct iommufd_attach_handle * +iommufd_device_get_attach_handle(struct iommufd_device *idev) +{

• struct iommu_attach_handle *handle;
• handle =

• iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0);
  

• if (IS_ERR(handle))

• return NULL;
  

• return to_iommufd_handle(handle);

+}

+static void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,

• 		       struct iommufd_device *idev)
  

+{

• struct iommufd_attach_handle *handle;
• handle = iommufd_device_get_attach_handle(idev);
• iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
• if (hwpt->fault)

• iommufd_fault_domain_detach_dev(hwpt, idev, handle, true);
  

same here, pretty difficult to understand what this

iommufd_fault_domain_detach_dev does To me calling iommufd_auto_response_faults and iommufd_fault_iopf_disable would be more readable or rename iommufd_fault_domain_detach_dev(). Also compared to the original code, there is a new check on handle. Why is it necessary.

Globally I feel that patch pretty hard to read. Would be nice to split if possible to ease the review process.

Thanks

Eric

• kfree(handle);

+}

+static int iommufd_hwpt_replace_device(struct iommufd_device *idev,

• 		       struct iommufd_hw_pagetable *hwpt,
  

• 		       struct iommufd_hw_pagetable *old)
  

+{

• struct iommufd_attach_handle *old_handle =

• iommufd_device_get_attach_handle(idev);
  

• struct iommufd_attach_handle *handle;
• int rc;
• if (hwpt->fault) {

• rc = iommufd_fault_domain_attach_dev(hwpt, idev, !old->fault);
  

• if (rc)
  

• 	return rc;
  

• }
• handle = kzalloc(sizeof(*handle), GFP_KERNEL);
• if (!handle) {

• rc = -ENOMEM;
  

• goto out_fault_detach;
  

• }
• handle->idev = idev;
• rc = iommu_replace_group_handle(idev->igroup->group, hwpt->domain,

• 			&handle->handle);
  

• if (rc)

• goto out_free_handle;
  

• if (old->fault)

• iommufd_fault_domain_detach_dev(old, idev, old_handle,
  

• 				!hwpt->fault);
  

• kfree(old_handle);
• return 0;

+out_free_handle:

• kfree(handle);
• handle = NULL;

+out_fault_detach:

• if (hwpt->fault)

• iommufd_fault_domain_detach_dev(hwpt, idev, handle,
  

• 				!old->fault);
  

• return rc;

+}

int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt, struct iommufd_device *idev) { diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c index 06aa83a75e94..1d9bd3024b57 100644 --- a/drivers/iommu/iommufd/fault.c +++ b/drivers/iommu/iommufd/fault.c @@ -60,42 +60,17 @@ static void iommufd_fault_iopf_disable(struct iommufd_device *idev) mutex_unlock(&idev->iopf_lock); } -static int __fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,

• 		     struct iommufd_device *idev)
  

-{

• struct iommufd_attach_handle *handle;
• int ret;
• handle = kzalloc(sizeof(*handle), GFP_KERNEL);
• if (!handle)

• return -ENOMEM;
  

• handle->idev = idev;
• ret = iommu_attach_group_handle(hwpt->domain, idev->igroup->group,

• 			&handle->handle);
  

• if (ret)

• kfree(handle);
  

• return ret;

-}

int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,

• 		    struct iommufd_device *idev)
  

• 		    struct iommufd_device *idev,
  

• 		    bool enable_iopf)
  

{

• int ret;

• int rc = 0;

if (!hwpt->fault) return -EINVAL;

• ret = iommufd_fault_iopf_enable(idev);
• if (ret)

• return ret;
  

• ret = __fault_domain_attach_dev(hwpt, idev);
• if (ret)

• iommufd_fault_iopf_disable(idev);
  

• return ret;

• if (enable_iopf)

• rc = iommufd_fault_iopf_enable(idev);
  

• return rc;

} static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt, @@ -127,86 +102,15 @@ static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt, mutex_unlock(&fault->mutex); } -static struct iommufd_attach_handle * -iommufd_device_get_attach_handle(struct iommufd_device *idev) -{

• struct iommu_attach_handle *handle;
• handle = iommu_attach_handle_get(idev->igroup->group, IOMMU_NO_PASID, 0);
• if (IS_ERR(handle))

• return NULL;
  

• return to_iommufd_handle(handle);

-}

void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,

• 		     struct iommufd_device *idev)
  

• 		     struct iommufd_device *idev,
  

• 		     struct iommufd_attach_handle *handle,
  

• 		     bool disable_iopf)
  

{

• struct iommufd_attach_handle *handle;
• handle = iommufd_device_get_attach_handle(idev);
• iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
• iommufd_auto_response_faults(hwpt, handle);
• iommufd_fault_iopf_disable(idev);
• kfree(handle);

-}

-static int __fault_domain_replace_dev(struct iommufd_device *idev,

• 		      struct iommufd_hw_pagetable *hwpt,
  

• 		      struct iommufd_hw_pagetable *old)
  

-{

• struct iommufd_attach_handle *handle, *curr = NULL;
• int ret;
• if (old->fault)

• curr = iommufd_device_get_attach_handle(idev);
  

• if (hwpt->fault) {

• handle = kzalloc(sizeof(*handle), GFP_KERNEL);
  

• if (!handle)
  

• 	return -ENOMEM;
  

• handle->idev = idev;
  

• ret = iommu_replace_group_handle(idev->igroup->group,
  

• 				 hwpt->domain, &handle->handle);
  

• } else {

• ret = iommu_replace_group_handle(idev->igroup->group,
  

• 				 hwpt->domain, NULL);
  

• }
• if (!ret && curr) {

• iommufd_auto_response_faults(old, curr);
  

• kfree(curr);
  

• }
• return ret;

-}

-int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,

• 		     struct iommufd_hw_pagetable *hwpt,
  

• 		     struct iommufd_hw_pagetable *old)
  

-{

• bool iopf_off = !hwpt->fault && old->fault;
• bool iopf_on = hwpt->fault && !old->fault;
• int ret;
• if (iopf_on) {

• ret = iommufd_fault_iopf_enable(idev);
  

• if (ret)
  

• 	return ret;
  

• }
• ret = __fault_domain_replace_dev(idev, hwpt, old);
• if (ret) {

• if (iopf_on)
  

• 	iommufd_fault_iopf_disable(idev);
  

• return ret;
  

• }
• if (iopf_off)

• if (handle)

• iommufd_auto_response_faults(hwpt, handle);
  

• if (disable_iopf) iommufd_fault_iopf_disable(idev);

• return 0;

} void iommufd_fault_destroy(struct iommufd_object *obj)

On Thu, Jan 23, 2025 at 10:07:13AM +0000, Tian, Kevin wrote:

...

From: Nicolin Chen nicolinc@nvidia.com Sent: Saturday, January 11, 2025 11:32 AM

@@ -294,7 +294,9 @@ struct iommu_ioas_unmap {

/**

• enum iommufd_option - ioctl(IOMMU_OPTION_RLIMIT_MODE) and

• •                   ioctl(IOMMU_OPTION_HUGE_PAGES)
    

• •                   ioctl(IOMMU_OPTION_HUGE_PAGES) and
    

• •                   ioctl(IOMMU_OPTION_SW_MSI_START) and
    

• •                   ioctl(IOMMU_OPTION_SW_MSI_SIZE)
    

  • @IOMMU_OPTION_RLIMIT_MODE:
  • Change how RLIMIT_MEMLOCK accounting works. The caller must have

privilege

• to invoke this. Value 0 (default) is user based accounting, 1 uses process

@@ -304,10 +306,24 @@ struct iommu_ioas_unmap {

• iommu mappings. Value 0 disables combining, everything is mapped to
• PAGE_SIZE. This can be useful for benchmarking. This is a per-IOAS
• option, the object_id must be the IOAS ID.

• • @IOMMU_OPTION_SW_MSI_START:
• • Change the base address of the IOMMU mapping region for MSI

doorbell(s).

• • It must be set this before attaching a device to an IOAS/HWPT,

remove 'this'

Ack.

...

otherwise

• • this option will be not effective on that IOAS/HWPT. User can

Do we want to explicitly check this instead of leaving it no effect silently?

So, the idea here is: If this option is unset, use the default SW_MSI from the driver If this option is set, use it over the default SW_MSI from the driver

That's what the following statement "User can choose to let.." means.

...

choose to

• • let kernel pick a base address, by simply ignoring this option or setting
• • a value 0 to IOMMU_OPTION_SW_MSI_SIZE. Global option, object_id

must be 0

• • @IOMMU_OPTION_SW_MSI_SIZE:
• • Change the size of the IOMMU mapping region for MSI doorbell(s). It

must

• • be set this before attaching a device to an IOAS/HWPT, otherwise it

won't

• • be effective on that IOAS/HWPT. The value is in MB, and the minimum

value

• • is 1 MB. A value 0 (default) will invalidate the MSI doorbell base address
• • value set to IOMMU_OPTION_SW_MSI_START. Global option, object_id

must be 0

hmm there is no check on the minimal value and enable the effect of value 0 in this patch.

Well, it's somewhat enforced by __aligned_u64 since it can't be any value between 0 (disable) and 1 (minimal)?

And the override code checks "ctx->sw_msi_size".

...

iommufd_device_attach_reserved_iova(struct iommufd_device *idev, struct iommufd_hwpt_paging *hwpt_paging) {

• struct iommufd_ctx *ictx = idev->ictx; int rc;

  lockdep_assert_held(&idev->igroup->lock);

• /* Override it with a user-programmed SW_MSI region */

• if (ictx->sw_msi_size && ictx->sw_msi_start != PHYS_ADDR_MAX)

• idev->igroup->sw_msi_start = ictx->sw_msi_start;
  

  rc = iopt_table_enforce_dev_resv_regions(&hwpt_paging->ioas->iopt, idev->dev, &idev->igroup-

...

sw_msi_start);

what about moving above additions into iopt_table_enforce_dev_resv_regions() which is all about finding a sw_msi address and can check the user setting internally?

We could. Probably would be cleaner by doing that in one place.

...

diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c index 8a790e597e12..5d7f5ca1eecf 100644 --- a/drivers/iommu/iommufd/io_pagetable.c +++ b/drivers/iommu/iommufd/io_pagetable.c @@ -1446,7 +1446,9 @@ int iopt_table_enforce_dev_resv_regions(struct io_pagetable *iopt, if (sw_msi_start && resv->type == IOMMU_RESV_MSI) num_hw_msi++; if (sw_msi_start && resv->type == IOMMU_RESV_SW_MSI) {

• 	*sw_msi_start = resv->start;
  

• 	/* Bypass the driver-defined SW_MSI region, if preset
  

*/

• 	if (*sw_msi_start == PHYS_ADDR_MAX)
  

• 		*sw_msi_start = resv->start;
  

the code is not about bypass. Instead it's to use the driver-defined region if user doesn't set it.

Ack: /* If being unset, Use the default IOMMU_RESV_SW_MSI */

Thanks Nicolin