Re: iommu_sva_bind_device question

[Jerry Snitselaar <jsnitsel@redhat.com>]

On Sat, Jun 25, 2022 at 12:52:27PM -0700, Fenghua Yu wrote:
> 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?
> 

Sure, if you feel this is the correct solution. Just to be clear you would
like the end result to be:

	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 {
			set_bit(IDXD_FLAG_USER_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 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"?
>

I think what was confusing to me is it seemed to tie the SVA iommu
feature to the user, and talked about independence of the kernel and
user pasids. I understand the pasids themselves being independent, but
both depend on the SVA feature being enabled. So idxd_enable_system_pasid
can only happen if iommu_dev_feature_enable(dev, IOMMU_DEV_FEAT_SVA)
has succeeded prior to it, and idxd_disable_system_pasid should be
called if needed before there is a call to
iommu_dev_feature_disable(dev, IOMMU_DEV_FEAT_SVA).

When I looked at the code that seemed to be the case outside of this
block in idxd_probe, but I wasn't sure if I was missing something else.

Regards,
Jerry

> Thanks.
> 
> -Fenghua