RE: [PATCH v7 03/11] iommu/vt-d: Add custom allocator for IOASID

[Tian, Kevin]

> From: Lu Baolu [mailto:baolu.lu@xxxxxxxxxxxxxxx]
> Sent: Friday, October 25, 2019 10:39 PM
> 
> Hi,
> 
> On 10/25/19 2:40 PM, Tian, Kevin wrote:
> >>>> ioasid_register_allocator(&iommu->pasid_allocator);
> >>>> +			if (ret) {
> >>>> +				pr_warn("Custom PASID allocator
> >>>> registeration failed\n");
> >>>> +				/*
> >>>> +				 * Disable scalable mode on this
> >>>> IOMMU if there
> >>>> +				 * is no custom allocator. Mixing
> >>>> SM capable vIOMMU
> >>>> +				 * and non-SM vIOMMU are not
> >>>> supported.
> >>>> +				 */
> >>>> +				intel_iommu_sm = 0;
> >>> It's insufficient to disable scalable mode by only clearing
> >>> intel_iommu_sm. The DMA_RTADDR_SMT bit in root entry has already
> >> been
> >>> set. Probably, you need to
> >>>
> >>> for each iommu
> >>> 	clear DMA_RTADDR_SMT in root entry
> >>>
> >>> Alternatively, since vSVA is the only customer of this custom PASID
> >>> allocator, is it possible to only disable SVA here?
> >>>
> >> Yeah, I think disable SVA is better. We can still do gIOVA in SM. I
> >> guess we need to introduce a flag for sva_enabled.
> > I'm not sure whether tying above logic to SVA is the right approach.
> > If vcmd interface doesn't work, the whole SM mode doesn't make
> > sense which is based on PASID-granular protection (SVA is only one
> > usage atop). If the only remaining usage of SM is to map gIOVA using
> > reserved PASID#0, then why not disabling SM and just fallback to
> > legacy mode?
> >
> > Based on that I prefer to disabling the SM mode completely (better
> > through an interface), and move the logic out of CONFIG_INTEL_
> > IOMMU_SVM
> >
> 
> Unfortunately, it is dangerous to disable SM after boot. SM uses
> different root/device contexts and pasid table formats. Disabling SM
> after boot requires changing above from SM format into legacy format.

You are correct.

> 
> Since ioasid registration failure is a rare case. How about moving this
> part of code up to the early stage of intel_iommu_init() and returning
> error if hardware present vcmd capability but software fails to register
> a custom ioasid allocator?
> 

It makes sense to me.

Thanks
Kevin