linux-kernel - RE: [PATCH v2 2/3] iommu/dma: Support MSIs through nested domains

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BN9PR11MB52767A327C11601AEA7E33398CBA2@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Fri, 9 Aug 2024 07:34:53 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Robin Murphy <robin.murphy@....com>, Nicolin Chen <nicolinc@...dia.com>,
	"jgg@...dia.com" <jgg@...dia.com>
CC: "joro@...tes.org" <joro@...tes.org>, "will@...nel.org" <will@...nel.org>,
	"shuah@...nel.org" <shuah@...nel.org>, "iommu@...ts.linux.dev"
	<iommu@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>
Subject: RE: [PATCH v2 2/3] iommu/dma: Support MSIs through nested domains

> From: Robin Murphy <robin.murphy@....com>
> Sent: Thursday, August 8, 2024 8:39 PM
> 
> On 06/08/2024 9:25 am, Tian, Kevin wrote:
> >> From: Nicolin Chen <nicolinc@...dia.com>
> >> Sent: Saturday, August 3, 2024 8:32 AM
> >>
> >> From: Robin Murphy <robin.murphy@....com>
> >>
> >> 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

yes this was the consensus from previous discussion iirc. Just a bit
distracting by calling it old VFIO paradigm. 😊

> 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@....com>
> >> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> >> ---
> >>   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.

Then probably good to clarify it in below comment for
@get_msi_mapping_domain so driver developers could catch that
restriction easily w/o searching the related code.

> 
> 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
> >>
> >