Re: [PATCH v2 4/6] iommu/vt-d: Remove 1:1 mappings from identity domain

[Baolu Lu]

On 12/13/23 11:04 AM, Tian, Kevin wrote:
> > From: Baolu Lu<baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, December 13, 2023 10:44 AM
> >
> > On 12/13/23 10:20 AM, Tian, Kevin wrote:
> > > > > 	if (!dmar_map_gfx)
> > > > > 		iommu_identity_mapping |= IDENTMAP_GFX;
> > > > So with above cleaned up, we have no need to worry about drivers that
> > > > are not capable of handling remapped dma address any more.
> > > >
> > > > Did I miss anything?
> > > I prefer to removing IDENTMAP_GFX in this series and put a comment
> > > explaining why Azalia device has no problem.
> > >
> > > Then send a separate patch to remove the GFX workaround option.
> > > If there is any valid usage still relying on that, it's easy to revert.
> > Agreed. We should be more cautious. Perhaps I will postpone this series
> > to a time when we are sure that graphic drivers are okay with this
> > change. As a first step, perhaps we can make a change to remove the
> > workaround for graphic drivers, so that any hidden bugs in the graphic
> > driver could be reported.
> >
> > The patch looks like,
> >
> > iommu/vt-d: Remove INTEL_IOMMU_BROKEN_GFX_WA
> >
> > Commit 62edf5dc4a524 ("intel-iommu: Restore DMAR_BROKEN_GFX_WA
> > option for
> > broken graphics drivers") was introduced 24 years ago as a temporary
> > workaround for graphics drivers that used physical addresses for DMA and
> > avoided DMA APIs. This workaround was disabled by default.
> >
> > As 24 years have passed, it is expected that graphics driver developers
> > have migrated their drivers to use kernel DMA APIs. Therefore, this
> > workaround is no longer required and could been removed.
> and igfx_off option is still available just in case.

Yeah!

> > Suggested-by: Kevin Tian<kevin.tian@xxxxxxxxx>
> > Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >    drivers/iommu/intel/iommu.c | 10 ----------
> >    drivers/iommu/intel/Kconfig | 11 -----------
> >    2 files changed, 21 deletions(-)
> >
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 84b78e42a470..27b8638291f2 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -2357,9 +2357,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;
> > @@ -2660,13 +2657,6 @@ static int __init init_dmars(void)
> >    		iommu_set_root_entry(iommu);
> >    	}
> >
> > -#ifdef CONFIG_INTEL_IOMMU_BROKEN_GFX_WA
> > -	dmar_map_gfx = 0;
> > -#endif
> > -
> > -	if (!dmar_map_gfx)
> > -		iommu_identity_mapping |= IDENTMAP_GFX;
> > -
> let's remove IDENTMAP_GFX (and all its references) in a separate patch.
>
> this patch is for removing the workaround option.
>
> another patch removes IDENTMAP_GFX as it's meaningless when
> dmar_map_gfx simply implies disabling the gfx iommu.

Okay, it's fine to me.

Best regards,
baolu