[PATCH 0/1][PULL REQUEST] iommu/vt-d: Fixes for intel iommu

[PATCH 0/1][PULL REQUEST] iommu/vt-d: Fixes for intel iommu

[Lu Baolu]

Hi Joerg,

The following fix has been queued for the intel driver:

 - Decouple igfx_off from graphic identity mapping

This change is not critical for v6.9.  I'm fine for it to be included in
either the fix or the next branch, whichever works best for you. With
this in mind, please pull it.

Best regards,
baolu

Lu Baolu (1):
  iommu/vt-d: Decouple igfx_off from graphic identity mapping

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

-- 
2.34.1

[PATCH 1/1] iommu/vt-d: Decouple igfx_off from graphic identity mapping

[Lu Baolu]

A kernel command called igfx_off was introduced in commit <ba39592764ed>
("Intel IOMMU: Intel IOMMU driver"). This command allows the user to
disable the IOMMU dedicated to SOC-integrated graphic devices.

Commit <9452618e7462> ("iommu/intel: disable DMAR for g4x integrated gfx")
used this mechanism to disable the graphic-dedicated IOMMU for some
problematic devices. Later, more problematic graphic devices were added
to the list by commit <1f76249cc3beb> ("iommu/vt-d: Declare Broadwell igfx
dmar support snafu").

On the other hand, commit <19943b0e30b05> ("intel-iommu: Unify hardware
and software passthrough support") uses the identity domain for graphic
devices if CONFIG_DMAR_BROKEN_GFX_WA is selected.

+       if (iommu_pass_through)
+               iommu_identity_mapping = 1;
+#ifdef CONFIG_DMAR_BROKEN_GFX_WA
+       else
+               iommu_identity_mapping = 2;
+#endif
...

static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
{
+        if (iommu_identity_mapping == 2)
+                return IS_GFX_DEVICE(pdev);
...

In the following driver evolution, CONFIG_DMAR_BROKEN_GFX_WA and
quirk_iommu_igfx() are mixed together, causing confusion in the driver's
device_def_domain_type callback. On one hand, dmar_map_gfx is used to turn
off the graphic-dedicated IOMMU as a workaround for some buggy hardware;
on the other hand, for those graphic devices, IDENTITY mapping is required
for the IOMMU core.

Commit <4b8d18c0c986> "iommu/vt-d: Remove INTEL_IOMMU_BROKEN_GFX_WA" has
removed the CONFIG_DMAR_BROKEN_GFX_WA option, so the IDENTITY_DOMAIN
requirement for graphic devices is no longer needed. Therefore, this
requirement can be removed from device_def_domain_type() and igfx_off can
be made independent.

Fixes: 4b8d18c0c986 ("iommu/vt-d: Remove INTEL_IOMMU_BROKEN_GFX_WA")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Link: https://lore.kernel.org/r/20240428032020.214616-1-baolu.lu@linux.intel.com
---
 drivers/iommu/intel/iommu.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index fbbf8fda22f3..2e9811bf2a4e 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -217,12 +217,11 @@ int intel_iommu_sm = IS_ENABLED(CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON);
 int intel_iommu_enabled = 0;
 EXPORT_SYMBOL_GPL(intel_iommu_enabled);
 
-static int dmar_map_gfx = 1;
 static int intel_iommu_superpage = 1;
 static int iommu_identity_mapping;
 static int iommu_skip_te_disable;
+static int disable_igfx_iommu;
 
-#define IDENTMAP_GFX		2
 #define IDENTMAP_AZALIA		4
 
 const struct iommu_ops intel_iommu_ops;
@@ -261,7 +260,7 @@ static int __init intel_iommu_setup(char *str)
 			no_platform_optin = 1;
 			pr_info("IOMMU disabled\n");
 		} else if (!strncmp(str, "igfx_off", 8)) {
-			dmar_map_gfx = 0;
+			disable_igfx_iommu = 1;
 			pr_info("Disable GFX device mapping\n");
 		} else if (!strncmp(str, "forcedac", 8)) {
 			pr_warn("intel_iommu=forcedac deprecated; use iommu.forcedac instead\n");
@@ -2196,9 +2195,6 @@ static int device_def_domain_type(struct device *dev)
 
 		if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
 			return IOMMU_DOMAIN_IDENTITY;
-
-		if ((iommu_identity_mapping & IDENTMAP_GFX) && IS_GFX_DEVICE(pdev))
-			return IOMMU_DOMAIN_IDENTITY;
 	}
 
 	return 0;
@@ -2499,9 +2495,6 @@ static int __init init_dmars(void)
 		iommu_set_root_entry(iommu);
 	}
 
-	if (!dmar_map_gfx)
-		iommu_identity_mapping |= IDENTMAP_GFX;
-
 	check_tylersburg_isoch();
 
 	ret = si_domain_init(hw_pass_through);
@@ -2592,7 +2585,7 @@ static void __init init_no_remapping_devices(void)
 		/* This IOMMU has *only* gfx devices. Either bypass it or
 		   set the gfx_mapped flag, as appropriate */
 		drhd->gfx_dedicated = 1;
-		if (!dmar_map_gfx)
+		if (disable_igfx_iommu)
 			drhd->ignored = 1;
 	}
 }
@@ -4621,7 +4614,7 @@ static void quirk_iommu_igfx(struct pci_dev *dev)
 		return;
 
 	pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
-	dmar_map_gfx = 0;
+	disable_igfx_iommu = 1;
 }
 
 /* G4x/GM45 integrated gfx dmar support is totally busted. */
@@ -4702,8 +4695,8 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
 
 	if (!(ggc & GGC_MEMORY_VT_ENABLED)) {
 		pci_info(dev, "BIOS has allocated no shadow GTT; disabling IOMMU for graphics\n");
-		dmar_map_gfx = 0;
-	} else if (dmar_map_gfx) {
+		disable_igfx_iommu = 1;
+	} else if (!disable_igfx_iommu) {
 		/* we have to ensure the gfx device is idle before we flush */
 		pci_info(dev, "Disabling batched IOTLB flush on Ironlake\n");
 		iommu_set_dma_strict();
-- 
2.34.1

Re: [PATCH 0/1][PULL REQUEST] iommu/vt-d: Fixes for intel iommu

[Joerg Roedel]

On Fri, May 03, 2024 at 09:36:01PM +0800, Lu Baolu wrote:
> Lu Baolu (1):
>   iommu/vt-d: Decouple igfx_off from graphic identity mapping

Applied, thanks.

[PATCH 1/1] iommu/vt-d: Decouple igfx_off from graphic identity mapping

[Lu Baolu]

A kernel command called igfx_off was introduced in commit <ba39592764ed>
("Intel IOMMU: Intel IOMMU driver"). This command allows the user to
disable the IOMMU dedicated to SOC-integrated graphic devices.

Commit <9452618e7462> ("iommu/intel: disable DMAR for g4x integrated gfx")
used this mechanism to disable the graphic-dedicated IOMMU for some
problematic devices. Later, more problematic graphic devices were added
to the list by commit <1f76249cc3beb> ("iommu/vt-d: Declare Broadwell igfx
dmar support snafu").

On the other hand, commit <19943b0e30b05> ("intel-iommu: Unify hardware
and software passthrough support") uses the identity domain for graphic
devices if CONFIG_DMAR_BROKEN_GFX_WA is selected.

+       if (iommu_pass_through)
+               iommu_identity_mapping = 1;
+#ifdef CONFIG_DMAR_BROKEN_GFX_WA
+       else
+               iommu_identity_mapping = 2;
+#endif
...

static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
{
+        if (iommu_identity_mapping == 2)
+                return IS_GFX_DEVICE(pdev);
...

In the following driver evolution, CONFIG_DMAR_BROKEN_GFX_WA and
quirk_iommu_igfx() are mixed together, causing confusion in the driver's
device_def_domain_type callback. On one hand, dmar_map_gfx is used to turn
off the graphic-dedicated IOMMU as a workaround for some buggy hardware;
on the other hand, for those graphic devices, IDENTITY mapping is required
for the IOMMU core.

Commit <4b8d18c0c986> "iommu/vt-d: Remove INTEL_IOMMU_BROKEN_GFX_WA" has
removed the CONFIG_DMAR_BROKEN_GFX_WA option, so the IDENTITY_DOMAIN
requirement for graphic devices is no longer needed. Therefore, this
requirement can be removed from device_def_domain_type() and igfx_off can
be made independent.

Fixes: 4b8d18c0c986 ("iommu/vt-d: Remove INTEL_IOMMU_BROKEN_GFX_WA")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index fbbf8fda22f3..57a986524502 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -217,12 +217,11 @@ int intel_iommu_sm = IS_ENABLED(CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON);
 int intel_iommu_enabled = 0;
 EXPORT_SYMBOL_GPL(intel_iommu_enabled);
 
-static int dmar_map_gfx = 1;
 static int intel_iommu_superpage = 1;
 static int iommu_identity_mapping;
 static int iommu_skip_te_disable;
+static int disable_igfx_dedicated_iommu;
 
-#define IDENTMAP_GFX		2
 #define IDENTMAP_AZALIA		4
 
 const struct iommu_ops intel_iommu_ops;
@@ -261,7 +260,7 @@ static int __init intel_iommu_setup(char *str)
 			no_platform_optin = 1;
 			pr_info("IOMMU disabled\n");
 		} else if (!strncmp(str, "igfx_off", 8)) {
-			dmar_map_gfx = 0;
+			disable_igfx_dedicated_iommu = 1;
 			pr_info("Disable GFX device mapping\n");
 		} else if (!strncmp(str, "forcedac", 8)) {
 			pr_warn("intel_iommu=forcedac deprecated; use iommu.forcedac instead\n");
@@ -2196,9 +2195,6 @@ static int device_def_domain_type(struct device *dev)
 
 		if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
 			return IOMMU_DOMAIN_IDENTITY;
-
-		if ((iommu_identity_mapping & IDENTMAP_GFX) && IS_GFX_DEVICE(pdev))
-			return IOMMU_DOMAIN_IDENTITY;
 	}
 
 	return 0;
@@ -2499,9 +2495,6 @@ static int __init init_dmars(void)
 		iommu_set_root_entry(iommu);
 	}
 
-	if (!dmar_map_gfx)
-		iommu_identity_mapping |= IDENTMAP_GFX;
-
 	check_tylersburg_isoch();
 
 	ret = si_domain_init(hw_pass_through);
@@ -2592,7 +2585,7 @@ static void __init init_no_remapping_devices(void)
 		/* This IOMMU has *only* gfx devices. Either bypass it or
 		   set the gfx_mapped flag, as appropriate */
 		drhd->gfx_dedicated = 1;
-		if (!dmar_map_gfx)
+		if (disable_igfx_dedicated_iommu)
 			drhd->ignored = 1;
 	}
 }
@@ -4621,7 +4614,7 @@ static void quirk_iommu_igfx(struct pci_dev *dev)
 		return;
 
 	pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
-	dmar_map_gfx = 0;
+	disable_igfx_dedicated_iommu = 1;
 }
 
 /* G4x/GM45 integrated gfx dmar support is totally busted. */
@@ -4702,8 +4695,8 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
 
 	if (!(ggc & GGC_MEMORY_VT_ENABLED)) {
 		pci_info(dev, "BIOS has allocated no shadow GTT; disabling IOMMU for graphics\n");
-		dmar_map_gfx = 0;
-	} else if (dmar_map_gfx) {
+		disable_igfx_dedicated_iommu = 1;
+	} else if (!disable_igfx_dedicated_iommu) {
 		/* we have to ensure the gfx device is idle before we flush */
 		pci_info(dev, "Disabling batched IOTLB flush on Ironlake\n");
 		iommu_set_dma_strict();
-- 
2.34.1

RE: [PATCH 1/1] iommu/vt-d: Decouple igfx_off from graphic identity mapping

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Sunday, April 28, 2024 11:20 AM
> 
> A kernel command called igfx_off was introduced in commit <ba39592764ed>
> ("Intel IOMMU: Intel IOMMU driver"). This command allows the user to
> disable the IOMMU dedicated to SOC-integrated graphic devices.
> 
> Commit <9452618e7462> ("iommu/intel: disable DMAR for g4x integrated
> gfx")
> used this mechanism to disable the graphic-dedicated IOMMU for some
> problematic devices. Later, more problematic graphic devices were added
> to the list by commit <1f76249cc3beb> ("iommu/vt-d: Declare Broadwell igfx
> dmar support snafu").
> 
> On the other hand, commit <19943b0e30b05> ("intel-iommu: Unify
> hardware
> and software passthrough support") uses the identity domain for graphic
> devices if CONFIG_DMAR_BROKEN_GFX_WA is selected.
> 
> +       if (iommu_pass_through)
> +               iommu_identity_mapping = 1;
> +#ifdef CONFIG_DMAR_BROKEN_GFX_WA
> +       else
> +               iommu_identity_mapping = 2;
> +#endif
> ...
> 
> static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
> {
> +        if (iommu_identity_mapping == 2)
> +                return IS_GFX_DEVICE(pdev);
> ...
> 
> In the following driver evolution, CONFIG_DMAR_BROKEN_GFX_WA and
> quirk_iommu_igfx() are mixed together, causing confusion in the driver's
> device_def_domain_type callback. On one hand, dmar_map_gfx is used to
> turn
> off the graphic-dedicated IOMMU as a workaround for some buggy
> hardware;
> on the other hand, for those graphic devices, IDENTITY mapping is required
> for the IOMMU core.
> 
> Commit <4b8d18c0c986> "iommu/vt-d: Remove
> INTEL_IOMMU_BROKEN_GFX_WA" has
> removed the CONFIG_DMAR_BROKEN_GFX_WA option, so the
> IDENTITY_DOMAIN
> requirement for graphic devices is no longer needed. Therefore, this
> requirement can be removed from device_def_domain_type() and igfx_off
> can
> be made independent.
> 
> Fixes: 4b8d18c0c986 ("iommu/vt-d: Remove
> INTEL_IOMMU_BROKEN_GFX_WA")
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index fbbf8fda22f3..57a986524502 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -217,12 +217,11 @@ int intel_iommu_sm =
> IS_ENABLED(CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON);
>  int intel_iommu_enabled = 0;
>  EXPORT_SYMBOL_GPL(intel_iommu_enabled);
> 
> -static int dmar_map_gfx = 1;
>  static int intel_iommu_superpage = 1;
>  static int iommu_identity_mapping;
>  static int iommu_skip_te_disable;
> +static int disable_igfx_dedicated_iommu;
> 

what about 'no_igfx_iommu"? dedicated is implied for igfx
so a shorter name might be slightly better.

otherwise it looks good:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 1/1] iommu/vt-d: Decouple igfx_off from graphic identity mapping

[Baolu Lu]

On 2024/4/28 14:45, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Sunday, April 28, 2024 11:20 AM
>>
>> A kernel command called igfx_off was introduced in commit <ba39592764ed>
>> ("Intel IOMMU: Intel IOMMU driver"). This command allows the user to
>> disable the IOMMU dedicated to SOC-integrated graphic devices.
>>
>> Commit <9452618e7462> ("iommu/intel: disable DMAR for g4x integrated
>> gfx")
>> used this mechanism to disable the graphic-dedicated IOMMU for some
>> problematic devices. Later, more problematic graphic devices were added
>> to the list by commit <1f76249cc3beb> ("iommu/vt-d: Declare Broadwell igfx
>> dmar support snafu").
>>
>> On the other hand, commit <19943b0e30b05> ("intel-iommu: Unify
>> hardware
>> and software passthrough support") uses the identity domain for graphic
>> devices if CONFIG_DMAR_BROKEN_GFX_WA is selected.
>>
>> +       if (iommu_pass_through)
>> +               iommu_identity_mapping = 1;
>> +#ifdef CONFIG_DMAR_BROKEN_GFX_WA
>> +       else
>> +               iommu_identity_mapping = 2;
>> +#endif
>> ...
>>
>> static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
>> {
>> +        if (iommu_identity_mapping == 2)
>> +                return IS_GFX_DEVICE(pdev);
>> ...
>>
>> In the following driver evolution, CONFIG_DMAR_BROKEN_GFX_WA and
>> quirk_iommu_igfx() are mixed together, causing confusion in the driver's
>> device_def_domain_type callback. On one hand, dmar_map_gfx is used to
>> turn
>> off the graphic-dedicated IOMMU as a workaround for some buggy
>> hardware;
>> on the other hand, for those graphic devices, IDENTITY mapping is required
>> for the IOMMU core.
>>
>> Commit <4b8d18c0c986> "iommu/vt-d: Remove
>> INTEL_IOMMU_BROKEN_GFX_WA" has
>> removed the CONFIG_DMAR_BROKEN_GFX_WA option, so the
>> IDENTITY_DOMAIN
>> requirement for graphic devices is no longer needed. Therefore, this
>> requirement can be removed from device_def_domain_type() and igfx_off
>> can
>> be made independent.
>>
>> Fixes: 4b8d18c0c986 ("iommu/vt-d: Remove
>> INTEL_IOMMU_BROKEN_GFX_WA")
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 19 ++++++-------------
>>   1 file changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index fbbf8fda22f3..57a986524502 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -217,12 +217,11 @@ int intel_iommu_sm =
>> IS_ENABLED(CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON);
>>   int intel_iommu_enabled = 0;
>>   EXPORT_SYMBOL_GPL(intel_iommu_enabled);
>>
>> -static int dmar_map_gfx = 1;
>>   static int intel_iommu_superpage = 1;
>>   static int iommu_identity_mapping;
>>   static int iommu_skip_te_disable;
>> +static int disable_igfx_dedicated_iommu;
>>
> what about 'no_igfx_iommu"? dedicated is implied for igfx
> so a shorter name might be slightly better.

I like disable_igfx_iommu more. :-)

Best regards,
baolu