linux-kernel - RE: [PATCH 1/1] iommu/vt-d: Decouple igfx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BN9PR11MB5276AF4C9A57C3B8D3A25E8E8C142@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Sun, 28 Apr 2024 06:45:17 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>, "Will
 Deacon" <will@...nel.org>, Robin Murphy <robin.murphy@....com>, "Jason
 Gunthorpe" <jgg@...pe.ca>, "Wang, Zhenyu Z" <zhenyu.z.wang@...el.com>
CC: "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/1] iommu/vt-d: Decouple igfx_off from graphic identity
 mapping

> From: Lu Baolu <baolu.lu@...ux.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@...ux.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@...el.com>