Skip to contentSkip to conversation

iommu/vt-d: Add MTL to quirk list to skip TE disabling

PATCH In review Lu Baolu 2 participants 4 messages Nov 16, 2023

Subsystems (3)
Stable branch IOMMU Intel IOMMU (VT-d)
lore
Share & tools
lore

Lu Baolu

author maintainer
#0 lore raw

PATCH 1/1 iommu/vt-d: Add MTL to quirk list to skip TE disabling

From: "Abdul Halim, Mohd Syazwan" <[email protected]>

The VT-d spec requires (10.4.4 Global Command Register, TE field) that:

Hardware implementations supporting DMA draining must drain any in-flight DMA read/write requests queued within the Root-Complex before completing the translation enable command and reflecting the status of the command through the TES field in the Global Status register.

Unfortunately, some integrated graphic devices fail to do so after some kind of power state transition. As the result, the system might stuck in iommu_disable_translation(), waiting for the completion of TE transition.

Add MTL to the quirk list for those devices and skips TE disabling if the qurik hits.

Fixes b1012ca8dc4f ("iommu/vt-d: Skip TE disabling on quirky gfx dedicated iommu")
Cc [email protected]
Signed-off-by Abdul Halim, Mohd Syazwan <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/iommu/intel/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/intel/iommu.c +1 -1
index 061df1b68ff7..22cadcefadaa 100644
@@ -5080,7 +5080,7 @@ static void quirk_igfx_skip_te_disable(struct pci_dev *dev)
 	ver = (dev->device >> 8) & 0xff;
 	if (ver != 0x45 && ver != 0x46 && ver != 0x4c &&
 	    ver != 0x4e && ver != 0x8a && ver != 0x98 &&
-	    ver != 0x9a && ver != 0xa7)
+	    ver != 0x9a && ver != 0xa7 && ver != 0x7d)
 		return;
 
 	if (risky_device(dev))
Signature · 1 line
2.34.1
#1 lore raw
Quoted 6 lines

From: Lu Baolu <[email protected]>
Sent: Thursday, November 16, 2023 10:23 AM

From: "Abdul Halim, Mohd Syazwan"
<[email protected]>

Quoted 7 lines

The VT-d spec requires (10.4.4 Global Command Register, TE field) that:

Hardware implementations supporting DMA draining must drain any in-flight DMA read/write requests queued within the Root-Complex before completing
the translation enable command and reflecting the status of the command through the TES field in the Global Status register.

this talks about 'enable'...

Quoted 4 lines

Unfortunately, some integrated graphic devices fail to do so after some kind of power state transition. As the result, the system might stuck in iommu_disable_translation(), waiting for the completion of TE transition.

...while this fixes 'disable'. wrong citation?

drivers/iommu/intel/iommu.c static void quirk_igfx_skip_te_disable(struct · line 5086

Quoted 9 lines
drivers/iommu/intel/iommu.c +1 -1
@@ -5080,7 +5080,7 @@ static void quirk_igfx_skip_te_disable(struct
 pci_dev *dev)
 	ver = (dev->device >> 8) & 0xff;
 	if (ver != 0x45 && ver != 0x46 && ver != 0x4c &&
 	    ver != 0x4e && ver != 0x8a && ver != 0x98 &&
-	    ver != 0x9a && ver != 0xa7)
+	    ver != 0x9a && ver != 0xa7 && ver != 0x7d)
 		return;

this fix alone is fine, but I found this quirk overall is not cleanly handled.

Basically it sets iommu_skip_te_disable as true, leading to early return in iommu_disable_translation():

	if (iommu_skip_te_disable && iommu->drhd->gfx_dedicated &&
	   (cap_read_drain(iommu->cap) || cap_write_drain(iommu->cap)))
		return;

However the caller of iommu_disable_translation() is not aware of this quirk and continues as if the iommu is disabled. IMHO this is problematic w/o meeting the caller's assumption.

e.g. kdump and suspend. We may want to abort those paths early in case of such quirk...

author maintainer
#2 lore raw
Quoted 14 lines · Kevin
Quoted 6 lines

From: Lu Baolu <[email protected]>
Sent: Thursday, November 16, 2023 10:23 AM

From: "Abdul Halim, Mohd Syazwan"
<[email protected]>

Quoted 7 lines

The VT-d spec requires (10.4.4 Global Command Register, TE field) that:

Hardware implementations supporting DMA draining must drain any in-flight DMA read/write requests queued within the Root-Complex before completing
the translation enable command and reflecting the status of the command through the TES field in the Global Status register.

Quoted 8 lines

this talks about 'enable'...

Quoted 4 lines

Unfortunately, some integrated graphic devices fail to do so after some kind of power state transition. As the result, the system might stuck in iommu_disable_translation(), waiting for the completion of TE transition.

...while this fixes 'disable'. wrong citation?

Right. It's confusing. I will change it to below.

"
...before switching address translation on or off and reflecting the status of the command through the TES field in the Global Status register.
"

Quoted 20 lines

drivers/iommu/intel/iommu.c static void quirk_igfx_skip_te_disable(struct · line 5086

Quoted 10 lines
drivers/iommu/intel/iommu.c +1 -1
@@ -5080,7 +5080,7 @@ static void quirk_igfx_skip_te_disable(struct
 pci_dev *dev)
  	ver = (dev->device >> 8) & 0xff;
  	if (ver != 0x45 && ver != 0x46 && ver != 0x4c &&
  	    ver != 0x4e && ver != 0x8a && ver != 0x98 &&
-	    ver != 0x9a && ver != 0xa7)
+	    ver != 0x9a && ver != 0xa7 && ver != 0x7d)
  		return;

this fix alone is fine, but I found this quirk overall is not cleanly handled.

Basically it sets iommu_skip_te_disable as true, leading to early return in iommu_disable_translation():

	if (iommu_skip_te_disable && iommu->drhd->gfx_dedicated &&
	   (cap_read_drain(iommu->cap) || cap_write_drain(iommu->cap)))
		return;
Quoted 6 lines

However the caller of iommu_disable_translation() is not aware of this quirk and continues as if the iommu is disabled. IMHO this is problematic w/o meeting the caller's assumption.

e.g. kdump and suspend. We may want to abort those paths early in case of such quirk...

I can see your point.

This fix is just to add a new device model to the established quirk list. All devices (including the new one) in this quirk list have undergone thorough verification. Therefor, I'd like to keep it as-is.

We can refine the quirk implementation in a separated patch series with sufficient consideration and verification. Does this work for you?

Best regards,
baolu

#3 lore raw
Quoted 33 lines

From: Baolu Lu <[email protected]>
Sent: Thursday, November 16, 2023 3:25 PM

Quoted 22 lines · Kevin

drivers/iommu/intel/iommu.c static void quirk_igfx_skip_te_disable(struct · line 5086

Quoted 12 lines

From: Lu Baolu <[email protected]>
Sent: Thursday, November 16, 2023 10:23 AM

drivers/iommu/intel/iommu.c +1 -1
@@ -5080,7 +5080,7 @@ static void quirk_igfx_skip_te_disable(struct
 pci_dev *dev)
  	ver = (dev->device >> 8) & 0xff;
  	if (ver != 0x45 && ver != 0x46 && ver != 0x4c &&
  	    ver != 0x4e && ver != 0x8a && ver != 0x98 &&
-	    ver != 0x9a && ver != 0xa7)
+	    ver != 0x9a && ver != 0xa7 && ver != 0x7d)
  		return;

this fix alone is fine, but I found this quirk overall is not cleanly handled.

Basically it sets iommu_skip_te_disable as true, leading to early return in iommu_disable_translation():

	if (iommu_skip_te_disable && iommu->drhd->gfx_dedicated &&
	   (cap_read_drain(iommu->cap) || cap_write_drain(iommu->cap)))
		return;
Quoted 6 lines

However the caller of iommu_disable_translation() is not aware of this quirk and continues as if the iommu is disabled. IMHO this is problematic w/o meeting the caller's assumption.

e.g. kdump and suspend. We may want to abort those paths early in case of such quirk...

Quoted 8 lines

I can see your point.

This fix is just to add a new device model to the established quirk list. All devices (including the new one) in this quirk list have undergone thorough verification. Therefor, I'd like to keep it as-is.

We can refine the quirk implementation in a separated patch series with sufficient consideration and verification. Does this work for you?

sure