From: "Abdul Halim, Mohd Syazwan" mohd.syazwan.abdul.halim@intel.com
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: stable@vger.kernel.org Signed-off-by: Abdul Halim, Mohd Syazwan mohd.syazwan.abdul.halim@intel.com Signed-off-by: Lu Baolu baolu.lu@linux.intel.com --- drivers/iommu/intel/iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 061df1b68ff7..22cadcefadaa 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -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))
From: Lu Baolu baolu.lu@linux.intel.com Sent: Thursday, November 16, 2023 10:23 AM
From: "Abdul Halim, Mohd Syazwan" mohd.syazwan.abdul.halim@intel.com
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'...
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?
@@ -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)
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...
On 2023/11/16 11:27, Tian, Kevin wrote:
From: Lu Baolu baolu.lu@linux.intel.com Sent: Thursday, November 16, 2023 10:23 AM
From: "Abdul Halim, Mohd Syazwan" mohd.syazwan.abdul.halim@intel.com
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'...
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. "
@@ -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)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...
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
From: Baolu Lu baolu.lu@linux.intel.com Sent: Thursday, November 16, 2023 3:25 PM
On 2023/11/16 11:27, Tian, Kevin wrote:
From: Lu Baolu baolu.lu@linux.intel.com Sent: Thursday, November 16, 2023 10:23 AM
@@ -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)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...
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
linux-stable-mirror@lists.linaro.org
-
Baolu Lu -
Lu Baolu
-
Tian, Kevin