RE: [PATCH 1/1] iommu/vt-d: Disable PCI ATS in legacy passthrough mode

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Friday, November 17, 2023 9:10 AM
> 
> On 11/16/23 4:24 PM, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> >> Sent: Thursday, November 16, 2023 3:35 PM
> >>
> >> On 2023/11/14 9:10, Lu Baolu wrote:
> >>> When IOMMU hardware operates in legacy mode, the TT field of the
> >> context
> >>> entry determines the translation type, with three supported types
> (Section
> >>> 9.3 Context Entry):
> >>>
> >>> - DMA translation without device TLB support
> >>> - DMA translation with device TLB support
> >>> - Passthrough mode with translated and translation requests blocked
> >>>
> >>> Device TLB support is absent when hardware is configured in
> passthrough
> >>> mode.
> >>>
> >>> Disable the PCI ATS feature when IOMMU is configured for passthrough
> >>> translation type in legacy (non-scalable) mode.
> >>>
> >>> Fixes: 0faa19a1515f ("iommu/vt-d: Decouple PASID & PRI enabling from
> >> SVA")
> >>> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >>> ---
> >>>    drivers/iommu/intel/iommu.c | 4 ++++
> >>>    1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/drivers/iommu/intel/iommu.c
> b/drivers/iommu/intel/iommu.c
> >>> index 11670cd812a3..c3ec09118ab1 100644
> >>> --- a/drivers/iommu/intel/iommu.c
> >>> +++ b/drivers/iommu/intel/iommu.c
> >>> @@ -1413,6 +1413,10 @@ static void iommu_enable_pci_caps(struct
> >> device_domain_info *info)
> >>>    	if (!dev_is_pci(info->dev))
> >>>    		return;
> >>>
> >>> +	if (!sm_supported(info->iommu) && info->domain &&
> >>> +	    domain_type_is_si(info->domain))
> >>> +		return;
> >>> +
> >>>    	pdev = to_pci_dev(info->dev);
> >>>
> >>>    	/* The PCIe spec, in its wisdom, declares that the behaviour of
> >>
> >> Perhaps we could move the check into the caller and make this helper
> >> transparent to the iommu mode and domain type?
> >>
> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> >> index 11670cd812a3..9bddd4fbbdf8 100644
> >> --- a/drivers/iommu/intel/iommu.c
> >> +++ b/drivers/iommu/intel/iommu.c
> >> @@ -2492,7 +2492,8 @@ static int dmar_domain_attach_device(struct
> >> dmar_domain *domain,
> >>                   return ret;
> >>           }
> >>
> >> -       iommu_enable_pci_caps(info);
> >> +       if (sm_supported(info->iommu) || !domain_type_is_si(info-
> >domain))
> >> +               iommu_enable_pci_caps(info);
> >>
> >
> > IMHO both old and this new version are confusing regarding to that
> > the commit msg talks only about ATS but the actual code disable all
> > pci caps. It's correct, being that only ATS is relevant in legacy mode,
> > but the readability is not good.
> 
> The function name is a bit misleading, but its actual purpose is to
> enable the ATS feature. PASID is enabled within this function due to a
> specification requirement that prevents modifications to PASID registers
> after ATS activation.
> 
> >
> > what about introducing a helper e.g. device_domain_ats_supported(info)
> > which includes above checks plus info->ats_supported and then use it
> > to replace info->ats_supported in iommu_enable_pci_caps()?
> >
> 
> I have a patch series under test which moves the ATS control out to the
> device drivers or users (through sysfs node). With that done, we could
> simply remove all these and give the control to device drivers or users.
> 
> How about considering above after I post that series? And for now, we
> can make the fix patch quick and simple.
> 

Then please add a comment for the rationale.