RE: [PATCH v5 03/15] iommu/smmu: Report empty domain nesting info

[Liu, Yi L]

Hi Jean,

> From: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> Sent: Friday, July 17, 2020 5:09 PM
> 
> On Thu, Jul 16, 2020 at 10:38:17PM +0200, Auger Eric wrote:
> > Hi Jean,
> >
> > On 7/16/20 5:39 PM, Jean-Philippe Brucker wrote:
> > > On Tue, Jul 14, 2020 at 10:12:49AM +0000, Liu, Yi L wrote:
> > >>> Have you verified that this doesn't break the existing usage of
> > >>> DOMAIN_ATTR_NESTING in drivers/vfio/vfio_iommu_type1.c?
> > >>
> > >> I didn't have ARM machine on my hand. But I contacted with Jean
> > >> Philippe, he confirmed no compiling issue. I didn't see any code
> > >> getting DOMAIN_ATTR_NESTING attr in current
> drivers/vfio/vfio_iommu_type1.c.
> > >> What I'm adding is to call iommu_domai_get_attr(, DOMAIN_ATTR_NESTIN)
> > >> and won't fail if the iommu_domai_get_attr() returns 0. This patch
> > >> returns an empty nesting info for DOMAIN_ATTR_NESTIN and return
> > >> value is 0 if no error. So I guess it won't fail nesting for ARM.
> > >
> > > I confirm that this series doesn't break the current support for
> > > VFIO_IOMMU_TYPE1_NESTING with an SMMUv3. That said...
> > >
> > > If the SMMU does not support stage-2 then there is a change in behavior
> > > (untested): after the domain is silently switched to stage-1 by the SMMU
> > > driver, VFIO will now query nesting info and obtain -ENODEV. Instead of
> > > succeding as before, the VFIO ioctl will now fail. I believe that's a fix
> > > rather than a regression, it should have been like this since the
> > > beginning. No known userspace has been using VFIO_IOMMU_TYPE1_NESTING
> so
> > > far, so I don't think it should be a concern.
> > But as Yi mentioned ealier, in the current vfio code there is no
> > DOMAIN_ATTR_NESTING query yet.
> 
> That's why something that would have succeeded before will now fail:
> Before this series, if user asked for a VFIO_IOMMU_TYPE1_NESTING, it would
> have succeeded even if the SMMU didn't support stage-2, as the driver
> would have silently fallen back on stage-1 mappings (which work exactly
> the same as stage-2-only since there was no nesting supported). After the
> series, we do check for DOMAIN_ATTR_NESTING so if user asks for
> VFIO_IOMMU_TYPE1_NESTING and the SMMU doesn't support stage-2, the ioctl
> fails.

I think this depends on iommu driver. I noticed current SMMU driver
doesn't check physical IOMMU about nesting capability. So I guess
the SET_IOMMU would still work for SMMU. but it will fail as you
mentioned in prior email, userspace will check the nesting info and
would fail as it gets an empty struct from host.

20200716153959.GA447208@myrica/">https://lore.kernel.org/kvm/20200716153959.GA447208@myrica/

> 
> I believe it's a good fix and completely harmless, but wanted to make sure
> no one objects because it's an ABI change.

yes.

Regards,
Yi Liu

> Thanks,
> Jean
> 
> > In my SMMUV3 nested stage series, I added
> > such a query in vfio-pci.c to detect if I need to expose a fault region
> > but I already test both the returned value and the output arg. So to me
> > there is no issue with that change.
> > >
> > > And if userspace queries the nesting properties using the new ABI
> > > introduced in this patchset, it will obtain an empty struct. I think
> > > that's acceptable, but it may be better to avoid adding the nesting cap if
> > > @format is 0?
> > agreed
> >
> > Thanks
> >
> > Eric
> > >
> > > Thanks,
> > > Jean
> > >
> > >>
> > >> @Eric, how about your opinion? your dual-stage vSMMU support may
> > >> also share the vfio_iommu_type1.c code.
> > >>
> > >> Regards,
> > >> Yi Liu
> > >>
> > >>> Will
> > >
> >