Re: [PATCH v2 2/3] iommu/dma: Support MSIs through nested domains

[Robin Murphy]

On 06/08/2024 9:25 am, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > Sent: Saturday, August 3, 2024 8:32 AM
> >
> > From: Robin Murphy <robin.murphy@xxxxxxx>
> >
> > Currently, iommu-dma is the only place outside of IOMMUFD and drivers
> > which might need to be aware of the stage 2 domain encapsulated within
> > a nested domain. This would be in the legacy-VFIO-style case where we're
>
> why is it a legacy-VFIO-style? We only support nested in IOMMUFD.

Because with proper nesting we ideally shouldn't need the host-managed 
MSI mess at all, which all stems from the old VFIO paradigm of 
completely abstracting interrupts from userspace. I'm still hoping 
IOMMUFD can grow its own interface for efficient MSI passthrough, where 
the VMM can simply map the physical MSI doorbell into whatever IPA (GPA) 
it wants it to appear at in the S2 domain, then whatever the guest does 
with S1 it can program the MSI address into the endpoint accordingly 
without us having to fiddle with it.

FWIW, apart from IOMMUFD, we may also end up wanting something along 
those lines for Arm CCA (if non-Secure proxying of T=1 MSIs becomes an 
unavoidable thing).

> > using host-managed MSIs with an identity mapping at stage 1, where it is
> > the underlying stage 2 domain which owns an MSI cookie and holds the
> > corresponding dynamic mappings. Hook up the new op to resolve what we
> > need from a nested domain.
> >
> > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > ---
> >   drivers/iommu/dma-iommu.c | 18 ++++++++++++++++--
> >   include/linux/iommu.h     |  4 ++++
> >   2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 7b1dfa0665df6..05e04934a5f81 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -1799,6 +1799,20 @@ static struct iommu_dma_msi_page
> > *iommu_dma_get_msi_page(struct device *dev,
> >   	return NULL;
> >   }
> >
> > +/*
> > + * Nested domains may not have an MSI cookie or accept mappings, but
> > they may
> > + * be related to a domain which does, so we let them tell us what they need.
> > + */
> > +static struct iommu_domain
> > *iommu_dma_get_msi_mapping_domain(struct device *dev)
> > +{
> > +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > +
> > +	if (domain && domain->type == IOMMU_DOMAIN_NESTED &&
> > +	    domain->ops->get_msi_mapping_domain)
>
> I'm not sure the core should restrict it to the NESTED type. Given
> there is a new domain ops any type restriction can be handled
> inside the callback. Anyway the driver should register the op
> for a domain only when there is a need.

Nested should remain the only case where domains are chained together 
such that the VFIO MSI cookie may be hiding somewhere else. This is 
effectively documenting that implementing the callback for any other 
domain type would be a bad smell. Much like how the mapping-related ops 
are explicitly short-circuited for non-paging domain types.

Thanks,
Robin.

> > +		domain = domain->ops->get_msi_mapping_domain(domain);
> > +	return domain;
> > +}
> > +
> >   /**
> >    * iommu_dma_prepare_msi() - Map the MSI page in the IOMMU domain
> >    * @desc: MSI descriptor, will store the MSI page
> > @@ -1809,7 +1823,7 @@ static struct iommu_dma_msi_page
> > *iommu_dma_get_msi_page(struct device *dev,
> >   int iommu_dma_prepare_msi(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_domain *domain =
> > iommu_dma_get_msi_mapping_domain(dev);
> >   	struct iommu_dma_msi_page *msi_page;
> >   	static DEFINE_MUTEX(msi_prepare_lock); /* see below */
> >
> > @@ -1842,7 +1856,7 @@ 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)
> >   {
> >   	struct device *dev = msi_desc_to_dev(desc);
> > -	const struct iommu_domain *domain =
> > iommu_get_domain_for_dev(dev);
> > +	const struct iommu_domain *domain =
> > iommu_dma_get_msi_mapping_domain(dev);
> >   	const struct iommu_dma_msi_page *msi_page;
> >
> >   	msi_page = msi_desc_get_iommu_cookie(desc);
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 4d47f2c333118..69ed76f9c3ec4 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -638,6 +638,8 @@ struct iommu_ops {
> >    * @enable_nesting: Enable nesting
> >    * @set_pgtable_quirks: Set io page table quirks (IO_PGTABLE_QUIRK_*)
> >    * @free: Release the domain after use.
> > + * @get_msi_mapping_domain: Return the related iommu_domain that
> > should hold the
> > + *                          MSI cookie and accept mapping(s).
> >    */
> >   struct iommu_domain_ops {
> >   	int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
> > @@ -668,6 +670,8 @@ struct iommu_domain_ops {
> >   				  unsigned long quirks);
> >
> >   	void (*free)(struct iommu_domain *domain);
> > +	struct iommu_domain *
> > +		(*get_msi_mapping_domain)(struct iommu_domain
> > *domain);
> >   };
> >
> >   /**
> > --
> > 2.43.0