RE: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Tuesday, September 13, 2022 5:25 PM
> 
> On 2022/9/13 16:01, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >> Sent: Monday, September 12, 2022 10:48 AM
> >>
> >> @@ -1401,7 +1403,6 @@ static void iommu_enable_dev_iotlb(struct
> >> device_domain_info *info)
> >
> > This is not the right name now as dev_iotlb is only related to ATS.
> 
> Yes. This name is confusing. Perhaps we can split it into some specific
> helpers,
> 
> - intel_iommu_enable_pci_ats()
> - intel_iommu_enabel_pci_pri()
> - intel_iommu_enable_pci_pasid()
> ?

Probably intel_iommu_enable_pci_caps()

> 
> >
> >>   		info->pfsid = pci_dev_id(pf_pdev);
> >>   	}
> >>
> >> -#ifdef CONFIG_INTEL_IOMMU_SVM
> >>   	/* The PCIe spec, in its wisdom, declares that the behaviour of
> >>   	   the device if you enable PASID support after ATS support is
> >>   	   undefined. So always enable PASID support on devices which
> >> @@ -1414,7 +1415,7 @@ static void iommu_enable_dev_iotlb(struct
> >> device_domain_info *info)
> >>   	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)
> >> &&
> >>   	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
> >>   		info->pri_enabled = 1;
> >> -#endif
> >> +
> >>   	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
> >>   	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
> >>   		info->ats_enabled = 1;
> >
> > iommu_enable_dev_iotlb() is currently called both when the device is
> probed
> > and when sva is enabled (which is actually useless). From this angle the
> commit
> > msg is inaccurate.
> 
> The logic is a bit tricky. iommu_support_dev_iotlb() only returns a
> devinfo pointer when ATS is supported on the device. So, you are right
> if device supports both ATS and PASID; otherwise PASID will not be
> enabled.

Yes, that is what the first part of this patch fixes.

But my point is about the message that previously PASID was enabled
only when FEAT_SVA is enabled and now the patch moves it to the
probe time. 

My point is that even in old way iommu_enable_dev_iotlb() was called
twice: one at probe time and the other at FEAT_SVA. If ATS exists
then PASID is enabled at probe time already. If ATS doesn't exist then
PASID is always disabled.

So this patch is really to decouple PASID enabling from ATS and remove
the unnecessary/duplicated call of iommu_enable_dev_iotlb() in
intel_iommu_enable_sva().

Thanks
Kevin