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

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]

Date:   Thu, 15 Sep 2022 03:22:49 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Baolu Lu <baolu.lu@...ux.intel.com>,
        "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>
CC:     Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        "Robin Murphy" <robin.murphy@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

> From: Baolu Lu <baolu.lu@...ux.intel.com>
> Sent: Tuesday, September 13, 2022 5:25 PM
> 
> On 2022/9/13 16:01, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@...ux.intel.com>
> >> 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