On 2023/10/17 19:22, Joao Martins wrote:
On 17/10/2023 03:08, Baolu Lu wrote:
On 10/17/23 12:00 AM, Joao Martins wrote:
The iommu_dirty_bitmap is defined in iommu core. The iommu driver has no
need to understand it and check its member anyway.
(...) The iommu driver has no need to understand it. iommu_dirty_bitmap_record()
already makes those checks in case there's no iova_bitmap to set bits to.
This is all true but the reason I am checking iommu_dirty_bitmap::bitmap is to
essentially not record anything in the iova bitmap and just clear the dirty bits
from the IOPTEs, all when dirty tracking is technically disabled. This is done
internally only when starting dirty tracking, and thus to ensure that we cleanup
all dirty bits before we enable dirty tracking to have a consistent snapshot as
opposed to inheriting dirties from the past.
It's okay since it serves a functional purpose. Can you please add some
comments around the code to explain the rationale.
I added this comment below:
+ /*
+ * IOMMUFD core calls into a dirty tracking disabled domain without an
+ * IOVA bitmap set in order to clean dirty bits in all PTEs that might
+ * have occured when we stopped dirty tracking. This ensures that we
+ * never inherit dirtied bits from a previous cycle.
+ */
Yes. It's clear. Thank you!
Also fixed an issue where I could theoretically clear the bit with
IOMMU_NO_CLEAR. Essentially passed the read_and_clear_dirty flags and let
dma_sl_pte_test_and_clear_dirty() to test and test-and-clear, similar to AMD:
@@ -781,6 +788,16 @@ static inline bool dma_pte_present(struct dma_pte *pte)
return (pte->val & 3) != 0;
}
+static inline bool dma_sl_pte_test_and_clear_dirty(struct dma_pte *pte,
+ unsigned long flags)
+{
+ if (flags & IOMMU_DIRTY_NO_CLEAR)
+ return (pte->val & DMA_SL_PTE_DIRTY) != 0;
+
+ return test_and_clear_bit(DMA_SL_PTE_DIRTY_BIT,
+ (unsigned long *)&pte->val);
+}
+
Yes. Sure.
Anyhow, see below the full diff compared to this patch. Some things are in tree
that is different to submitted from this patch.
[...]
@@ -4113,7 +4123,7 @@ static void intel_iommu_domain_free(struct iommu_domain
*domain)
}
static int prepare_domain_attach_device(struct iommu_domain *domain,
- struct device *dev)
+ struct device *dev, ioasid_t pasid)
How about blocking pasid attaching in intel_iommu_set_dev_pasid().
{
struct dmar_domain *dmar_domain = to_dmar_domain(domain);
struct intel_iommu *iommu;
@@ -4126,7 +4136,8 @@ static int prepare_domain_attach_device(struct
iommu_domain *domain,
if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap))
return -EINVAL;
- if (domain->dirty_ops && !intel_iommu_slads_supported(iommu))
+ if (domain->dirty_ops &&
+ (!slads_supported(iommu) || pasid != IOMMU_NO_PASID))
return -EINVAL;
/* check if this iommu agaw is sufficient for max mapped address */
[...]
@@ -4886,14 +4897,16 @@ static int intel_iommu_read_and_clear_dirty(struct
iommu_domain *domain,
unsigned long pgsize;
bool ad_enabled;
- spin_lock(&dmar_domain->lock);
+ /*
+ * IOMMUFD core calls into a dirty tracking disabled domain without an
+ * IOVA bitmap set in order to clean dirty bits in all PTEs that might
+ * have occured when we stopped dirty tracking. This ensures that we
+ * never inherit dirtied bits from a previous cycle.
+ */
ad_enabled = dmar_domain->dirty_tracking;
- spin_unlock(&dmar_domain->lock);
-
if (!ad_enabled && dirty->bitmap)
How about
if (!dmar_domain->dirty_tracking && dirty->bitmap)
return -EINVAL;
?
return -EINVAL;
- rcu_read_lock();
do {
struct dma_pte *pte;
int lvl = 0;
@@ -4906,14 +4919,10 @@ static int intel_iommu_read_and_clear_dirty(struct
iommu_domain *domain,
continue;
}
- /* It is writable, set the bitmap */
- if (((flags & IOMMU_DIRTY_NO_CLEAR) &&
- dma_sl_pte_dirty(pte)) ||
- dma_sl_pte_test_and_clear_dirty(pte))
+ if (dma_sl_pte_test_and_clear_dirty(pte, flags))
iommu_dirty_bitmap_record(dirty, iova, pgsize);
iova += pgsize;
} while (iova < end);
- rcu_read_unlock();
return 0;
}
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index bccd44db3316..0b390d9e669b 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -542,6 +542,9 @@ enum {
#define sm_supported(iommu) (intel_iommu_sm && ecap_smts((iommu)->ecap))
#define pasid_supported(iommu) (sm_supported(iommu) && \
ecap_pasid((iommu)->ecap))
+#define slads_supported(iommu) (sm_supported(iommu) && \
+ ecap_slads((iommu)->ecap))
+
struct pasid_entry;
struct pasid_state_entry;
@@ -785,13 +788,12 @@ static inline bool dma_pte_present(struct dma_pte *pte)
return (pte->val & 3) != 0;
}
-static inline bool dma_sl_pte_dirty(struct dma_pte *pte)
+static inline bool dma_sl_pte_test_and_clear_dirty(struct dma_pte *pte,
+ unsigned long flags)
{
- return (pte->val & DMA_SL_PTE_DIRTY) != 0;
-}
+ if (flags & IOMMU_DIRTY_NO_CLEAR)
+ return (pte->val & DMA_SL_PTE_DIRTY) != 0;
-static inline bool dma_sl_pte_test_and_clear_dirty(struct dma_pte *pte)
-{
return test_and_clear_bit(DMA_SL_PTE_DIRTY_BIT,
(unsigned long *)&pte->val);
}
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 03814942d59c..785384a59d55 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -686,15 +686,29 @@ int intel_pasid_setup_dirty_tracking(struct intel_iommu
*iommu,
spin_lock(&iommu->lock);
- did = domain_id_iommu(domain, iommu);
pte = intel_pasid_get_entry(dev, pasid);
if (!pte) {
spin_unlock(&iommu->lock);
- dev_err(dev, "Failed to get pasid entry of PASID %d\n", pasid);
+ dev_err_ratelimited(dev,
+ "Failed to get pasid entry of PASID %d\n",
+ pasid);
return -ENODEV;
}
+ did = domain_id_iommu(domain, iommu);
pgtt = pasid_pte_get_pgtt(pte);
+ if (pgtt != PASID_ENTRY_PGTT_SL_ONLY && pgtt != PASID_ENTRY_PGTT_NESTED) {
+ spin_unlock(&iommu->lock);
+ dev_err_ratelimited(dev,
+ "Dirty tracking not supported on translation type %d\n",
+ pgtt);
+ return -EOPNOTSUPP;
+ }
+
+ if (pasid_get_ssade(pte) == enabled) {
+ spin_unlock(&iommu->lock);
+ return 0;
+ }
if (enabled)
pasid_set_ssade(pte);
@@ -702,6 +716,9 @@ int intel_pasid_setup_dirty_tracking(struct intel_iommu *iommu,
pasid_clear_ssade(pte);
spin_unlock(&iommu->lock);
+ if (!ecap_coherent(iommu->ecap))
+ clflush_cache_range(pte, sizeof(*pte));
+
/*
* From VT-d spec table 25 "Guidance to Software for Invalidations":
*
@@ -720,8 +737,6 @@ int intel_pasid_setup_dirty_tracking(struct intel_iommu *iommu,
if (pgtt == PASID_ENTRY_PGTT_SL_ONLY || pgtt == PASID_ENTRY_PGTT_NESTED)
iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
- else
- qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
/* Device IOTLB doesn't need to be flushed in caching mode. */
if (!cap_caching_mode(iommu->cap))
Others look good to me. Thanks a lot.
Best regards,
baolu
