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

Tian, Kevin
2019-10-25T15:52:44Z

> From: Lu Baolu [mailto:baolu.lu@linux.intel.com]
> 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