Re: [RFC PATCH v3 04/14] iommu/riscv: Reject live S2 replacement with forwarded IRQs

[Gong Shuai]

Hi Drew,

> MSI forwarding state is tied to the S2 domain MSI table. Replacing a
> device's S2 domain while one of its IRQs is forwarded would leave the
> IRQ state referring to the detached table.
> 
> Reject direct S2-to-S2 replacement when the moving device has forwarded
> IRQs. Introduce a per-device nr_forwarded_irqs counter, rather than
> domain-wide accounting, so other devices in the same IOMMU group can
> still move and can be rolled back if a later device fails.
> 
> Later patches which introduce interrupt remapping support will manage
> the newly introduced nr_forwarded_irqs counter.
> 
> Signed-off-by: Andrew Jones <andrew.jones@xxxxxxxxxxxxxxxx>
> ---
>  drivers/iommu/riscv/iommu.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> index 57f2884dec42..d48667112cd9 100644
> --- a/drivers/iommu/riscv/iommu.c
> +++ b/drivers/iommu/riscv/iommu.c
> @@ -872,6 +872,7 @@ PT_IOMMU_CHECK_DOMAIN(struct riscv_iommu_domain, riscvpt.iommu, domain);
>  /* Private IOMMU data for managed devices, dev_iommu_priv_* */
>  struct riscv_iommu_info {
>  	struct riscv_iommu_domain *domain;
> +	unsigned int nr_forwarded_irqs;
>  };
>  
>  static struct riscv_iommu_msi_table *riscv_iommu_domain_msi_table(struct iommu_domain *iommu_domain)
> @@ -1432,6 +1433,36 @@ static int riscv_iommu_msi_table_alloc(struct riscv_iommu_domain *domain,
>  	return 0;
>  }
>  
> +static bool riscv_iommu_can_attach_paging_domain(struct iommu_domain *iommu_domain,
> +						 struct device *dev,
> +						 struct iommu_domain *old)
> +{
> +	struct riscv_iommu_domain *domain = iommu_domain_to_riscv(iommu_domain);
> +	struct riscv_iommu_info *info = dev_iommu_priv_get(dev);
> +	bool new_is_s2 = domain->gscid;
> +	struct riscv_iommu_msi_table *new_msi_table, *old_msi_table;
> +
> +	if (iommu_domain == old)
> +		return true;
> +
> +	new_msi_table = riscv_iommu_domain_msi_table(iommu_domain);
> +	old_msi_table = riscv_iommu_domain_msi_table(old);
> +
> +	if (new_msi_table)
> +		lockdep_assert_held(&new_msi_table->lock);
> +	if (old_msi_table)
> +		lockdep_assert_held(&old_msi_table->lock);
> +
> +	/*
> +	 * Per-device accounting allows other devices in the same IOMMU group
> +	 * to move or roll back while this device has forwarded interrupts.
> +	 */
> +	if (new_is_s2 && old_msi_table && info->nr_forwarded_irqs)

This check might miss one case. old_msi_table comes from this
attach's old domain, so only a direct S2 to S2 swap is caught.
A device with live forwarded IRQs can still be moved to blocking
and then to a different S2 domain, and on that second attach old
is the blocking domain, so old_msi_table is NULL and the check
passes. This is only from reading the code, I have no test program
that triggers it.

If you think this is a real gap, one idea would be to record the
table the forwarding was set up on in riscv_iommu_info(a pointer to
riscv_iommu_msi_table), set when the first IRQ of the device is
forwarded and cleared when the last one is unforwarded. The pointer
survives the blocking hop, since attaching to blocking domain only
clears info->domain. Then can_attach here will reject when the new
domain's table differs from it. That covers both the direct and the
blocking case, and the PCI reset path still works because the device
returns to the same S2 domain. The unforward path can use the pointer
instead of info->domain. Note that the S2 domain owning the table
must not be freed while a device still points at that table.

Thanks,
Shuai

> +		return false;
> +
> +	return true;
> +}
> +
>  static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
>  					    struct device *dev,
>  					    struct iommu_domain *old)
> @@ -1477,6 +1508,11 @@ static int riscv_iommu_attach_paging_domain(struct iommu_domain *iommu_domain,
>  	bond->dev = dev;
>  
>  	flags = riscv_iommu_msi_tables_lock(old, iommu_domain);
> +	if (!riscv_iommu_can_attach_paging_domain(iommu_domain, dev, old)) {
> +		riscv_iommu_msi_tables_unlock(old, iommu_domain, flags);
> +		kfree(bond);
> +		return -EBUSY;
> +	}
>  	riscv_iommu_bond_link(domain, bond);
>  	riscv_iommu_iodir_update(iommu, dev, &dc);
>  	riscv_iommu_bond_unlink(info->domain, dev);
> -- 
> 2.43.0