Re: [PATCH 1/2] vfio: Fix unbalanced vfio_df_close call in no-iommu mode

[Jacob Pan <jacob.pan@linux.microsoft.com>]

Hi Yi,

On Wed, 28 May 2025 15:16:57 +0800
Yi Liu <yi.l.liu@intel.com> wrote:

> On 2025/5/27 08:05, Jason Gunthorpe wrote:
> > On Fri, May 16, 2025 at 09:45:21AM -0700, Jacob Pan wrote:  
> >> For no-iommu enabled devices working under IOMMUFD VFIO compat
> >> mode, the group open path does not call vfio_df_open() and the
> >> open_count is 0. So calling vfio_df_close() in the group close
> >> path will trigger warning in vfio_assert_device_open(device);
> >>
> >> E.g. The following warning can be seen by running VFIO test.
> >> https://github.com/awilliam/tests/blob/master/vfio-noiommu-pci-device-open.c
> >> CONFIG_VFIO_CONTAINER = n
> >> [   29.094781] vfio-pci 0000:02:01.0: vfio-noiommu device opened
> >> by user (vfio-noiommu-pc:164) Failed to get device info
> >> [   29.096540] ------------[ cut here ]------------
> >> [   29.096616] WARNING: CPU: 1 PID: 164 at
> >> drivers/vfio/vfio_main.c:487 vfio_df_close+0xac/0xb4
> >>
> >> This patch adds checks for no-iommu mode and open_count to skip
> >> calling vfio_df_close.  
> 
> thanks for catching it. :)
> 
> >> Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
> >> ---
> >>   drivers/vfio/group.c | 7 ++++---
> >>   1 file changed, 4 insertions(+), 3 deletions(-)  
> > 
> > Sorry, this should have a fixes line:
> > 
> > I think it is probably
> > 
> > Fixes: 6086efe73498 ("vfio-iommufd: Move noiommu compat validation
> > out of vfio_iommufd_bind()")
> > 
> > By the look of it, since that is what started skipping the
> > vfio_df_open()
> > 
> > But after looking at that patch I'm now doubting that this is the
> > right fix.
> > 
> > Previously we'd still do vfio_df_device_first_open(), just the
> > vfio_df_iommufd_bind() was skipped.
> > 
> > Now we skip all of vfio_df_device_first_open() which also means we
> > skip:
> > 
> > 	if (!try_module_get(device->dev->driver->owner))
> > 		return -ENODEV;
> > 
> > and
> > 	if (device->ops->open_device) {
> > 		ret = device->ops->open_device(device);
> > 
> > Which seems wrong to me?? We only want to skip the bind, we should
> > still do open_device! At least that is how it was before 6086e
> > 
> > So.. This may not be the right fix.  
> 
> yes. this makes sense. If not opened, userspace is not able to use the
> device.
> 
Put this bug aside for now, I'm still unclear on why we do not allow
bind for no-IOMMU devices. Per my understanding, no-IOMMU only means no
translation. But since device still has been granted access, we should
be able to allow binding device in no-IOMMU mode with IOMMU-FD
context while simply disallowing IOAS attachment?

The reason I am asking is that I am working on enabling cdev with
noiommu mode based on Yi's patch
(https://lore.kernel.org/kvm/20230601082413.22a55ac4.alex.williamson@redhat.com/),
it seems having iommufd implicit ownership model is key to enable PCI
HOT RESET. Our goal is to leverage persistent iommufd context for kexec
handle off (KHO) usage, we currently have noiommu mode. This requires
binding of device with iommufd ctx (can be marked as persistent) AFAIK,
any suggestions?"


Thanks,

Jacob