Skip to contentSkip to conversation

iommu/vt-d: Handle volatile descriptor status read

PATCH In review Jacob Pan 3 participants 4 messages Jun 7, 2024 – Jun 18, 2024

Subsystems (2)
IOMMU Intel IOMMU (VT-d)
lore
Share & tools
lore

Jacob Pan

author
#0 lore raw

PATCH 1/1 iommu/vt-d: Handle volatile descriptor status read

Queued invalidation wait descriptor status is volatile in that IOMMU hardware writes the data upon completion.

Use READ_ONCE() to prevent compiler optimizations which ensures memory reads every time. As a side effect, READ_ONCE() also enforces strict types and may add an extra instruction. But it should not have negative performance impact since we use cpu_relax anyway and the extra time(by adding an instruction) may allow IOMMU HW request cacheline ownership easier.

e.g. gcc 12.3
BEFORE:

	81 38 ad de 00 00       cmpl   $0x2,(%rax)

AFTER (with READ_ONCE())

    772f:       8b 00                   mov    (%rax),%eax
    7731:       3d ad de 00 00          cmp    $0x2,%eax //status data is 32 bit
Signed-off-by Jacob Pan <[email protected]>
---
 drivers/iommu/intel/dmar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/intel/dmar.c +1 -1
index 304e84949ca7..1c8d3141cb55 100644
@@ -1446,7 +1446,7 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
 	 */
 	writel(qi->free_head << shift, iommu->reg + DMAR_IQT_REG);
 
-	while (qi->desc_status[wait_index] != QI_DONE) {
+	while (READ_ONCE(qi->desc_status[wait_index]) != QI_DONE) {
 		/*
 		 * We will leave the interrupts disabled, to prevent interrupt
 		 * context to queue another cmd while a cmd is already submitted
Signature · 1 line
2.25.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 7 lines

From: Jacob Pan <[email protected]>
Sent: Saturday, June 8, 2024 1:38 AM

Queued invalidation wait descriptor status is volatile in that IOMMU hardware
writes the data upon completion.

Quoted 7 lines

Use READ_ONCE() to prevent compiler optimizations which ensures memory reads every time. As a side effect, READ_ONCE() also enforces strict types and
may add an extra instruction. But it should not have negative performance impact since we use cpu_relax anyway and the extra time(by adding an instruction) may allow IOMMU HW request cacheline ownership easier.

I didn't get the meaning of the last sentence.

Quoted 10 lines

e.g. gcc 12.3
BEFORE:

	81 38 ad de 00 00       cmpl   $0x2,(%rax)

AFTER (with READ_ONCE())

    772f:       8b 00                   mov    (%rax),%eax
    7731:       3d ad de 00 00          cmp    $0x2,%eax //status data is 32 bit
Signed-off-by Jacob Pan <[email protected]>

Do we need a fix tag here?

otherwise looks good to me:

Reviewed-by Kevin Tian <[email protected]>
author
#2 lore raw
Quoted 15 lines · Kevin
Quoted 7 lines

From: Jacob Pan <[email protected]>
Sent: Saturday, June 8, 2024 1:38 AM

Queued invalidation wait descriptor status is volatile in that IOMMU hardware
writes the data upon completion.

Quoted 7 lines

Use READ_ONCE() to prevent compiler optimizations which ensures memory reads every time. As a side effect, READ_ONCE() also enforces strict types and
may add an extra instruction. But it should not have negative performance impact since we use cpu_relax anyway and the extra time(by adding an instruction) may allow IOMMU HW request cacheline ownership easier.

Quoted 1 line

I didn't get the meaning of the last sentence.

The wait descriptor is polled by the CPU and written by the IOMMU concurrently. The IOMMU needs to have the cacheline ownership before writing the status data to signal completion of the wait descriptor.

If the CPU polling loop is very tight, it might make IOMMU request for ownership contentious/difficult. Since we already use pause (cpu_relax()) to ease the contention, adding an additional instruction

mov (%rax),%eax

Will make the cacheline even less contentious since it is just register mov, no memory access.

Quoted 5 lines
Quoted 5 lines

e.g. gcc 12.3
BEFORE:

	81 38 ad de 00 00       cmpl   $0x2,(%rax)
Quoted 8 lines
Quoted 6 lines

AFTER (with READ_ONCE())

    772f:       8b 00                   mov    (%rax),%eax
    7731:       3d ad de 00 00          cmp    $0x2,%eax //status data

is 32 bit

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

Do we need a fix tag here?

I cannot find the exact commit, this is really old code.

Quoted 3 lines

otherwise looks good to me:

Reviewed-by Kevin Tian <[email protected]>

Thanks,

Jacob

Reviewed-by on v1 1/1
#3 lore raw
In reply to Jacob Pan
Quoted 27 lines · Jacob Pan

Queued invalidation wait descriptor status is volatile in that IOMMU hardware writes the data upon completion.

Use READ_ONCE() to prevent compiler optimizations which ensures memory reads every time. As a side effect, READ_ONCE() also enforces strict types and may add an extra instruction. But it should not have negative performance impact since we use cpu_relax anyway and the extra time(by adding an instruction) may allow IOMMU HW request cacheline ownership easier.

e.g. gcc 12.3
BEFORE:

	81 38 ad de 00 00       cmpl   $0x2,(%rax)

AFTER (with READ_ONCE())

     772f:       8b 00                   mov    (%rax),%eax
     7731:       3d ad de 00 00          cmp    $0x2,%eax //status data is 32 bit
Signed-off-by Jacob Pan <[email protected]>
---
  drivers/iommu/intel/dmar.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/intel/dmar.c +0 -0
index 304e84949ca7..1c8d3141cb55 100644

drivers/iommu/intel/dmar.c int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc, · line 1452

Quoted 8 lines
drivers/iommu/intel/dmar.c +1 -1
@@ -1446,7 +1446,7 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
  	 */
  	writel(qi->free_head << shift, iommu->reg + DMAR_IQT_REG);
 
-	while (qi->desc_status[wait_index] != QI_DONE) {
+	while (READ_ONCE(qi->desc_status[wait_index]) != QI_DONE) {
  		/*
  		 * We will leave the interrupts disabled, to prevent interrupt
  		 * context to queue another cmd while a cmd is already submitted
Reviewed-by Yi Liu <[email protected]>
Signature · 2 lines
Regards,
Yi Liu