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