Hi Baolu,

On Sat, 4 Feb 2023 11:43:01 +0800, Baolu Lu baolu.lu@linux.intel.com wrote:

On 2023/2/4 6:07, Jacob Pan wrote:

...

On platforms that do not support IOMMU Extended capability bit 0 Page-walk Coherency, CPU caches are not snooped when IOMMU is accessing any translation structures. IOMMU access goes only directly to memory. Intel IOMMU code was missing a flush for the PASID table directory that resulted in the unrecoverable fault as shown below.

Thanks for the fix.

...

This patch adds a clflush when activating a PASID table directory. There's no need to do clflush of the PASID directory pointer when we deactivate a context entry in that IOMMU hardware will not see the old PASID directory pointer after we clear the context entry.

[ 0.555386] DMAR: DRHD: handling fault status reg 3 [ 0.555805] DMAR: [DMA Read NO_PASID] Request device [00:0d.2] fault addr 0x1026a4000 [fault reason 0x51] SM: Present bit in Directory Entry is clear [ 0.556348] DMAR: Dump dmar1 table entries for IOVA 0x1026a4000 [ 0.556348] DMAR: scalable mode root entry: hi 0x0000000102448001, low 0x0000000101b3e001 [ 0.556348] DMAR: context entry: hi 0x0000000000000000, low 0x0000000101b4d401 [ 0.556348] DMAR: pasid dir entry: 0x0000000101b4e001 [ 0.556348] DMAR: pasid table entry[0]: 0x0000000000000109 [ 0.556348] DMAR: pasid table entry[1]: 0x0000000000000001 [ 0.556348] DMAR: pasid table entry[2]: 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[3]: 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[4]: 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[5]: 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[6]: 0x0000000000000000 [ 0.556348] DMAR: pasid table entry[7]: 0x0000000000000000 [ 0.556348] DMAR: PTE not present at level 4

Cc: stable@vger.kernel.org Reported-by: Sukumar Ghorai sukumar.ghorai@intel.com Signed-off-by: Ashok Raj ashok.raj@intel.com Signed-off-by: Jacob Pan jacob.jun.pan@linux.intel.com

Add a Fixes tag so that people know how far this fix should be back ported.

will do.

...

---

drivers/iommu/intel/iommu.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 59df7e42fd53..b4878c7ac008 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -1976,6 +1976,12 @@ static int domain_context_mapping_one(struct dmar_domain *domain, pds = context_get_sm_pds(table); context->lo = (u64)virt_to_phys(table->table) | context_pdts(pds);

• /*
  

•  * Scalable-mode PASID directory pointer is not
  

snooped if the

•  * coherent bit is not set.
  

•  */
  

• if (!ecap_coherent(iommu->ecap))
  

• 	clflush_cache_range(table->table, sizeof(void
  

*));

This isn't comprehensive. The clflush should be called whenever the pasid directory table is allocated or updated.

allocate a pasid table does not mean it gets used by iommu hw, not until it is programmed into context entry.

...

/* Setup the RID_PASID field: */ context_set_sm_rid2pasid(context, PASID_RID2PASID);

Best regards, baolu

Thanks,

Jacob

Hi Kevin,

On Tue, 7 Feb 2023 00:41:16 +0000, "Tian, Kevin" kevin.tian@intel.com wrote:

...

From: Jacob Pan jacob.jun.pan@linux.intel.com Sent: Tuesday, February 7, 2023 1:25 AM

...

...

@@ -1976,6 +1976,12 @@ static int

domain_context_mapping_one(struct

...

...

dmar_domain *domain, pds = context_get_sm_pds(table); context->lo = (u64)virt_to_phys(table->table) | context_pdts(pds);

• /*
  

•  * Scalable-mode PASID directory pointer is not
  

snooped if the

•  * coherent bit is not set.
  

•  */
  

• if (!ecap_coherent(iommu->ecap))
  

• 	clflush_cache_range(table->table,
  

sizeof(void *));

This isn't comprehensive. The clflush should be called whenever the pasid directory table is allocated or updated.

allocate a pasid table does not mean it gets used by iommu hw, not until it is programmed into context entry.

this is insufficient.

Even after this point the PASID directory entry could be changed when a new PASID table is allocated, e.g. in intel_pasid_get_entry().

you are right, will include updates in v2.

Thanks,

Jacob

Hi Baolu,

On Tue, 7 Feb 2023 15:10:48 +0800, Baolu Lu baolu.lu@linux.intel.com wrote:

On 2023/2/7 1:25, Jacob Pan wrote:

...

...

...

---

drivers/iommu/intel/iommu.c | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 59df7e42fd53..b4878c7ac008 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -1976,6 +1976,12 @@ static int domain_context_mapping_one(struct dmar_domain *domain, pds = context_get_sm_pds(table); context->lo = (u64)virt_to_phys(table->table) | context_pdts(pds);

• /*
  

•  * Scalable-mode PASID directory pointer is not
  

snooped if the

•  * coherent bit is not set.
  

•  */
  

• if (!ecap_coherent(iommu->ecap))
  

• 	clflush_cache_range(table->table, sizeof(void
  

*));

This isn't comprehensive. The clflush should be called whenever the pasid directory table is allocated or updated.

allocate a pasid table does not mean it gets used by iommu hw, not until it is programmed into context entry.

Hi Jacob,

This page is used by the device, and the device's access to this memory is not coherent. So after the page is allocated, any changes made by the CPU to this page must be written back to the real memory.

This patch only flushes the first 8 bytes of the table. That's not enough.

make sense, thanks for the explanation.

Be aware that page allocation also requires a clflush, because at least __GFP_ ZERO implies modification to page.

Yes, zeroing dir page does change it but I think if we intercept every update to the directory entry, it should be sufficient?

will send an updated version.

Thanks,

Jacob