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

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

[Lu Baolu]

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))
-- 
2.34.1

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

[Tian, Kevin]

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

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

[Baolu Lu]

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

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

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

[Tian, Kevin]

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