Hi Jean, On 7/17/20 11:09 AM, Jean-Philippe Brucker wrote: > 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. OK I now understand what you meant. Yes this actual change is brought by the next patch, ie. [PATCH v5 04/15] vfio/type1: Report iommu nesting info to userspace Thanks Eric > > I believe it's a good fix and completely harmless, but wanted to make sure > no one objects because it's an ABI change. > > 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 >>> >> >
