Re: [PATCH v6 0/7] iommufd: Enable noiommu mode for cdev

[Alex Williamson <alex@shazbot.org>]

On Tue, 26 May 2026 08:32:37 -0700
Jacob Pan <jacob.pan@linux.microsoft.com> wrote:

> Hi Kevin,
> 
> On Mon, 25 May 2026 08:30:12 +0000
> "Tian, Kevin" <kevin.tian@intel.com> wrote:
> 
> > Could you address the findings from Sashiko?
> > 
> > https://sashiko.dev/#/patchset/20260521221155.1375144-1-jacob.pan%40linux.microsoft.com
> >   
> I have go over my Sashiko review setup, but there are lots of
> false positives, like this one below we already discussed in earlier
> version. Is there a specific concern?
> 
> e.g.
> > +static bool iommufd_device_is_noiommu(struct iommufd_device *idev)
> > +{
> > +	return IS_ENABLED(CONFIG_IOMMUFD_NOIOMMU) &&
> > !idev->dev->iommu; +}  
> Does dynamically evaluating dev->iommu here allow the noiommu state to
> flip during the device's lifetime?

Yes, that one is at best a theoretical issue, but the next two NULL
pointer dereference if user passes noiommu device fd through an
unexpected iommufd interface appear quite real.

We're also still struggling with the Kconfig in patch 5, this Sashiko
comment is valid:

>> @@ -62,7 +61,10 @@ endif
>>  
>>  config VFIO_NOIOMMU
>>  	bool "VFIO No-IOMMU support"
>> -	depends on VFIO_GROUP
>> +	depends on VFIO_GROUP || VFIO_DEVICE_CDEV
>> +	depends on !VFIO_GROUP || VFIO_CONTAINER || IOMMUFD_VFIO_CONTAINER
>> +	depends on !VFIO_DEVICE_CDEV || !GENERIC_ATOMIC64
>
> Does this disable VFIO_NOIOMMU completely for legacy group users on
> architectures using generic atomic64 implementations?
>
> On architectures like 32-bit ARM or x86, !GENERIC_ATOMIC64 evaluates
> to false.  If a distribution enables VFIO_DEVICE_CDEV, this dependency
> resolves to false, silently breaking backwards compatibility and
> depriving legacy group-based users of noiommu support.

That's true, the question is do we care (I'd prefer to) and if so,
should we block the relevant interfaces from working though iommufd
rather than disallowing the Kconfig option.

The next issue regarding classifying emulated IOMMU devices as no-iommu
also appears valid, mdev devices like kvmgt for example.

The next issue raises a valid concern whether the dev_warn() should be
a dev_warn_once().

The sysfs naming comment is invalid, we intentionally name noiommu
devices uniquely to force userspace opt-in.

In patch 6, adding dead code is a valid comment, the unchecked asprintf
does seem to be an outlier in selftest code, the unmap comment may be a
false positive, as is the hugepage size, but the masked return comments
could arguably be skips or asserts.  There are potentially a couple
remaining nits and style issues noted.

Overall, more signal than noise afaict.  Thanks,

Alex