linux-kernel - RE: [PATCH] iommu/vt-d: Refactor PCI PRI enabling/disabling callbacks

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 for Android: free password hash cracker in your pocket

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

Message-ID: <BN9PR11MB52763CC7D59269696F0C22998CD72@BN9PR11MB5276.namprd11.prod.outlook.com>
Date: Thu, 27 Jun 2024 00:00:56 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: Baolu Lu <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>
CC: "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] iommu/vt-d: Refactor PCI PRI enabling/disabling callbacks

> From: Baolu Lu <baolu.lu@...ux.intel.com>
> Sent: Wednesday, June 26, 2024 9:05 PM
> 
> On 2024/6/26 14:53, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@...ux.intel.com>
> >> Sent: Thursday, June 6, 2024 11:40 AM
> >>
> >> +/*
> >> + * Invalidate the caches for a present-to-present change in a context
> >> + * table entry according to the Spec 6.5.3.3 (Guidance to Software for
> >> + * Invalidations).
> >> + *
> >> + * Since context entry is not encoded by domain-id when operating in
> >> + * scalable-mode (refer Section 6.2.1), this performs coarser
> >> + * invalidation than the domain-selective granularity requested.
> >> + */
> >> +static void invalidate_present_context_change(struct
> device_domain_info
> >> *info)
> >> +{
> >> +	struct intel_iommu *iommu = info->iommu;
> >> +
> >> +	iommu->flush.flush_context(iommu, 0, 0, 0,
> >> DMA_CCMD_GLOBAL_INVL);
> >> +	if (sm_supported(iommu))
> >> +		qi_flush_pasid_cache(iommu, 0, QI_PC_GLOBAL, 0);
> >> +	iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH);
> >> +	__iommu_flush_dev_iotlb(info, 0, MAX_AGAW_PFN_WIDTH);
> >> +}
> >> +
> >
> > this invalidates the entire cache/iotlb for all devices behind this
> > iommu just due to a PRI enable/disable operation on a single
> > device.
> >
> > No that's way too much. If there is a burden to identify all active
> > DIDs used by this device then pay it and penalize only that device.
> 
> You are right. We should not simplify the flow like this.
> 
> >
> > btw in concept PRI will not be enabled/disabled when there are
> > PASIDs of this device being actively attached. So at this point
> > there should only be RID with attached domain then we only
> > need to find that DID out and use it to invalidate related caches.
> 
> The assumption of "PRI will not be enabled/disabled when there are
> PASIDs of this device being actively attached" is not always correct.
> Both the pasid domain attachment and PRI are controlled by the device
> driver and there is no order rules for the drivers.

Yeah. Not sure how I got it wrong in previous reply. 😊

> 
> For example, the idxd driver attaches the default domain to a PASID and
> use it for kernel ENQCMD and use other PASIDs for SVA usage.
> 
> I am considering working out a generic helper to handle caches after
> change to a context entry what was present. How do you like below code
> (compiled but not tested)?
> 
> /*
>   * Cache invalidations after change in a context table entry that was
> present
>   * according to the Spec 6.5.3.3 (Guidance to Software for
> Invalidations). If
>   * IOMMU is in scalable mode and all PASID table entries of the device were
>   * non-present, set affect_domains to true. Otherwise, false.
>   */
> void intel_context_flush_present(struct device_domain_info *info,
> 				 struct context_entry *context,
> 				 bool affect_domains)
> {
> 	struct intel_iommu *iommu = info->iommu;
> 	u16 did = context_domain_id(context);
> 	struct pasid_entry *pte;
> 	int i;
> 
> 	assert_spin_locked(&iommu->lock);
> 
> 	/*
> 	 * Device-selective context-cache invalidation. The Domain-ID field
> 	 * of the Context-cache Invalidate Descriptor is ignored by hardware
> 	 * when operating in scalable mode. Therefore the @did value
> doesn't
> 	 * matter in scalable mode.
> 	 */
> 	iommu->flush.flush_context(iommu, did, PCI_DEVID(info->bus, info-
> >devfn),
> 				   DMA_CCMD_MASK_NOBIT,
> DMA_CCMD_DEVICE_INVL);
> 
> 	/*
> 	 * For legacy mode:
> 	 * - Domain-selective IOTLB invalidation
> 	 * - Global Device-TLB invalidation to all affected functions
> 	 */
> 	if (!sm_supported(iommu)) {
> 		iommu->flush.flush_iotlb(iommu, did, 0, 0,
> DMA_TLB_DSI_FLUSH);
> 		__iommu_flush_dev_iotlb(info, 0, MAX_AGAW_PFN_WIDTH);
> 
> 		return;
> 	}
> 
> 	/*
> 	 * For scalable mode:
> 	 * - Domain-selective PASID-cache invalidation to affected domains
> 	 * - Domain-selective IOTLB invalidation to affected domains
> 	 * - Global Device-TLB invalidation to affected functions
> 	 */
> 	if (affect_domains) {
> 		for (i = 0; i < info->pasid_table->max_pasid; i++) {
> 			pte = intel_pasid_get_entry(info->dev, i);
> 			if (!pte || !pasid_pte_is_present(pte))
> 				continue;
> 
> 			did = pasid_get_domain_id(pte);
> 			qi_flush_pasid_cache(iommu, did,
> QI_PC_ALL_PASIDS, 0);
> 			iommu->flush.flush_iotlb(iommu, did, 0, 0,
> DMA_TLB_DSI_FLUSH);
> 		}
> 	}
> 
> 	__iommu_flush_dev_iotlb(info, 0, MAX_AGAW_PFN_WIDTH);
> }
> 

looks good to me.