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
