Re: [PATCH 3/5] iommu: Introduce the domain op enforce_cache_coherency()

[Jason Gunthorpe via iommu <iommu@lists.linux-foundation.org>]

On Tue, Apr 05, 2022 at 01:50:36PM -0600, Alex Williamson wrote:
> >  
> > +static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
> > +{
> > +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > +
> > +	if (!dmar_domain->iommu_snooping)
> > +		return false;
> > +	dmar_domain->enforce_no_snoop = true;
> > +	return true;
> > +}
> 
> Don't we have issues if we try to set DMA_PTE_SNP on DMARs that don't
> support it, ie. reserved register bit set in pte faults?  

The way the Intel driver is setup that is not possible. Currently it
does:

 static bool intel_iommu_capable(enum iommu_cap cap)
 {
	if (cap == IOMMU_CAP_CACHE_COHERENCY)
		return domain_update_iommu_snooping(NULL);

Which is a global property unrelated to any device.

Thus either all devices and all domains support iommu_snooping, or
none do.

It is unclear because for some reason the driver recalculates this
almost constant value on every device attach..

> There's also a disconnect, maybe just in the naming or documentation,
> but if I call enforce_cache_coherency for a domain, that seems like the
> domain should retain those semantics regardless of how it's
> modified,

Right, this is how I would expect it to work.

> ie. "enforced".  For example, if I tried to perform the above operation,
> I should get a failure attaching the device that brings in the less
> capable DMAR because the domain has been set to enforce this
> feature.

We don't have any code causing a failure like this because no driver
needs it.

> Maybe this should be something like set_no_snoop_squashing with the
> above semantics, it needs to be re-applied whenever the domain:device
> composition changes?  Thanks,

If we get a real driver that needs non-uniformity here we can revisit
what to do. There are a couple of good options depending on exactly
what the HW behavior is.

Is it more clear if I fold in the below? It helps show that the
decision to use DMA_PTE_SNP is a global choice based on
domain_update_iommu_snooping():

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e5062461ab0640..fc789a9d955645 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -641,7 +641,6 @@ static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
 static void domain_update_iommu_cap(struct dmar_domain *domain)
 {
 	domain_update_iommu_coherency(domain);
-	domain->iommu_snooping = domain_update_iommu_snooping(NULL);
 	domain->iommu_superpage = domain_update_iommu_superpage(domain, NULL);
 
 	/*
@@ -4283,7 +4282,6 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
 	domain->agaw = width_to_agaw(adjust_width);
 
 	domain->iommu_coherency = false;
-	domain->iommu_snooping = false;
 	domain->iommu_superpage = 0;
 	domain->max_addr = 0;
 
@@ -4549,7 +4547,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
 {
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 
-	if (!dmar_domain->iommu_snooping)
+	if (!domain_update_iommu_snooping(NULL))
 		return false;
 	dmar_domain->enforce_no_snoop = true;
 	return true;
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 1f930c0c225d94..bc39f633efdf03 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -539,7 +539,6 @@ struct dmar_domain {
 
 	u8 has_iotlb_device: 1;
 	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
-	u8 iommu_snooping: 1;		/* indicate snooping control feature */
 	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
 
 	struct list_head devices;	/* all devices' list */
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu