iommu/vt-d: Fix PASID directory pointer coherency

PATCH Superseded Jacob Pan 3 participants 7 messages Feb 3, 2023 – Feb 7, 2023

Subsystems (3)

Stable branch IOMMU Intel IOMMU (VT-d)

lore

Share & tools

lore

Jacob Pan

author

2023-02-03 22:03 UTC #0 lore raw

PATCH 1/1 iommu/vt-d: Fix PASID directory pointer coherency

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.

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 <[email protected]>

Reported-by Sukumar Ghorai <[email protected]>

Signed-off-by Ashok Raj <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

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

drivers/iommu/intel/iommu.c +6 -0

index 59df7e42fd53..b4878c7ac008 100644
@@ -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 *));
 
 		/* Setup the RID_PASID field: */
 		context_set_sm_rid2pasid(context, PASID_RID2PASID);

Signature · 1 line

2.25.1

Baolu Lu

maintainer

2023-02-04 03:43 UTC #1 lore raw

Quoted 5 lines · Jacob Pan

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.

Quoted 21 lines

[    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

Quoted 4 lines

Cc <[email protected]>
Reported-by Sukumar Ghorai <[email protected]>
Signed-off-by Ashok Raj <[email protected]>
Signed-off-by Jacob Pan <[email protected]>

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

Quoted 9 lines

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

drivers/iommu/intel/iommu.c +0 -0

index 59df7e42fd53..b4878c7ac008 100644

drivers/iommu/intel/iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, · line 1984

Quoted 9 lines

drivers/iommu/intel/iommu.c +6 -0

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

drivers/iommu/intel/iommu.c static int domain_context_mapping_one(struct dmar_domain *domain, · line 1987

Quoted 3 lines

drivers/iommu/intel/iommu.c +0 -0

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

Best regards,
baolu

Jacob Pan

author

2023-02-06 17:21 UTC #2 lore raw

Hi Baolu,

Quoted 31 lines · Baolu Lu

Quoted 5 lines · Jacob Pan
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.
Quoted 22 lines
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

Quoted 8 lines

Quoted 4 lines
Cc <[email protected]>
Reported-by Sukumar Ghorai <[email protected]>
Signed-off-by Ashok Raj <[email protected]>
Signed-off-by Jacob Pan <[email protected]>
Add a Fixes tag so that people know how far this fix should be back ported.

will do.

Quoted 15 lines

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

drivers/iommu/intel/iommu.c +2 -0

index 59df7e42fd53..b4878c7ac008 100644
@@ -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

Quoted 9 lines

Quoted 5 lines
drivers/iommu/intel/iommu.c +4 -0
+		 * 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.

Quoted 7 lines

Quoted 3 lines

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

Best regards,
baolu

Thanks,

Jacob

Tian, Kevin

2023-02-07 00:41 UTC #3 lore raw

Quoted 20 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, February 7, 2023 1:25 AM
Quoted 1 line
Review static int
Quoted 1 line
(quoted diff) +0 -0
@@ -1976,6 +1976,12 @@ static int
domain_context_mapping_one(struct
Quoted 6 lines
Quoted 6 lines
drivers/iommu/intel/iommu.c +2 -0
 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
Quoted 9 lines
Quoted 5 lines
drivers/iommu/intel/iommu.c +4 -0
+		 * 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.

Quoted 3 lines

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().

Jacob Pan

author

2023-02-07 18:06 UTC #4 lore raw

Hi Kevin,

Quoted 20 lines · Kevin

Quoted 20 lines
From: Jacob Pan <[email protected]>
Sent: Tuesday, February 7, 2023 1:25 AM
Quoted 1 line
Review static int
Quoted 1 line
(quoted diff) +0 -0
@@ -1976,6 +1976,12 @@ static int  
domain_context_mapping_one(struct
Quoted 15 lines
Quoted 11 lines
drivers/iommu/intel/iommu.c +2 -0
 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
drivers/iommu/intel/iommu.c +4 -0
+		 * 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.

Quoted 9 lines

Quoted 3 lines
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

Baolu Lu

maintainer

2023-02-07 07:10 UTC #5 lore raw

In reply to Jacob Pan

Quoted 23 lines · Jacob Pan

Quoted 14 lines

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

drivers/iommu/intel/iommu.c +2 -0

index 59df7e42fd53..b4878c7ac008 100644
@@ -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

Quoted 9 lines

Quoted 6 lines
snooped if the
drivers/iommu/intel/iommu.c +4 -0
+		 * 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.

Quoted 2 lines

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.

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

Best regards,
baolu

Jacob Pan

author

2023-02-07 18:05 UTC #6 lore raw

Hi Baolu,

Quoted 27 lines · Baolu Lu

Quoted 23 lines · Jacob Pan
Quoted 14 lines
Quoted 14 lines
---
   drivers/iommu/intel/iommu.c | 6 ++++++
   1 file changed, 6 insertions(+)
drivers/iommu/intel/iommu.c +2 -0
index 59df7e42fd53..b4878c7ac008 100644
@@ -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
Quoted 9 lines
Quoted 6 lines
snooped if the
drivers/iommu/intel/iommu.c +4 -0
+		 * 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.
Quoted 2 lines
allocate a pasid table does not mean it gets used by iommu hw, not until it is programmed into context entry.

Quoted 9 lines

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.

Quoted 3 lines

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