Re: iommu_sva_bind_device question

[Fenghua Yu <fenghua.yu@intel.com>]

Hi, Jerry and Baolu,

On Fri, Jun 24, 2022 at 07:47:30AM -0700, Jerry Snitselaar wrote:
> > > > > > Hi Baolu & Dave,
> > > > fails.
> > > > 
> > > > You also will get the following warning if you don't have scalable
> > > > mode enabled (either not enabled by default, or if enabled by default
> > > > and passed intel_iommu=on,sm_off):
> > > 
> > > If scalable mode is disabled, iommu_dev_enable_feature(IOMMU_SVA) will
> > > return failure, hence driver should not call iommu_sva_bind_device().
> > > I guess below will disappear if above is fixed in the idxd driver.

Yes, Jerry's patch fixes the WARNING as well.

> > > 
> > > Best regards,
> > > baolu
> > >
> > 
> > It looks like there was a recent maintainer change, and Fenghua is now
> > the maintainer. Fenghua thoughts on this? With 42a1b73852c4
> > ("dmaengine: idxd: Separate user and kernel pasid enabling") the code
> > no longer depends on iommu_dev_feature_enable succeeding. Testing with
> > something like this works (ran dmatest without sm_on, and
> > dsa_user_test_runner.sh with sm_on, plus booting with various
> > intel_iommu= combinations):
> > 
> > diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> > index 355fb3ef4cbf..5b49fd5c1e25 100644
> > --- a/drivers/dma/idxd/init.c
> > +++ b/drivers/dma/idxd/init.c
> > @@ -514,13 +514,14 @@ static int idxd_probe(struct idxd_device *idxd)
> >         if (IS_ENABLED(CONFIG_INTEL_IDXD_SVM) && sva) {
> >                 if (iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA))
> >                         dev_warn(dev, "Unable to turn on user SVA feature.\n");
> > -               else
> > +               else {
> >                         set_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
> > 
> > -               if (idxd_enable_system_pasid(idxd))

Please add "{" after this if.

> > -                       dev_warn(dev, "No in-kernel DMA with PASID.\n");
> > -               else
then "}" before this else.

> > -                       set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
> > +                       if (idxd_enable_system_pasid(idxd))
> > +                               dev_warn(dev, "No in-kernel DMA with PASID.\n");
> > +                       else
> > +                               set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
> > +               }
> >         } else if (!sva) {
> >                 dev_warn(dev, "User forced SVA off via module param.\n");
> >         }

The patch was copied/pasted here. So the tabs are lost at beginning of each
line. So it cannot be applied. Please change the tabs back.

Could you please send this patch in a separate email so that it has a
right patch format and description and ready to be picked up?

> > 
> > The commit description is a bit confusing, because it talks about there
> > being no dependency, but ties user pasid to enabling/disabling the SVA
> > feature, which system pasid would depend on as well.
> > 
> > Regards,
> > Jerry
> 
> Things like that warning message "Unable to turn on user SVA feature" when
> iommu_dev_enable_feature fails though seems to be misleading with user
> inserted in there. I'll leave it to the idxd folks to figure out.

How about removing "user" from the warning message? So the message will
be "Unable to turn on SVA feature"?

Thanks.

-Fenghua