[Intel-gfx] [PATCH v2 10/10] vfio/pci: Add VFIO_DEVICE_GET_PCI_HOT_RESET_GROUP_INFO

[Liu, Yi L]

> From: Alex Williamson <alex.williamson at redhat.com>
> Sent: Wednesday, March 29, 2023 12:00 AM
> 
[...]
> > > > A check. If the device that the _INFIO is invoked is opened via cdev,
> but
> > > there
> > > > are devices in the dev_set that are got via
> VFIO_GROUP_GET_DEVICE_FD,
> > > should
> > > > I fail it or allow it?
> > >
> > > It's a niche case, but I think it needs to be allowed.
> >
> > I'm also wondering if it is common in the future. Actually, a user should be
> > preferred to either use the group or cdev, but not both. Otherwise, it
> looks
> > to be bad programming.:-)
> >
> > Also, as an earlier remark from Jason. If there are affected devices that
> are
> > opened by other users, the new _INFO should fail with -EPERM. I know
> this
> > remark was for the new _INFO ioctl. But now, we are going to reuse the
> > existing _INFO, so I'd also want to check if we still need this policy? If yes,
> > then it is a problem to check the owner of the devices that are opened by
> > the group path.
> >
> > https://lore.kernel.org/kvm/ZBsF950laMs2ldFc@nvidia.com/
> 
> Personally I don't like the suggestion to fail with -EPERM if the user
> doesn't own all the affected devices.  This isn't a "probe if I can do
> a reset" ioctl, it's a "provide information about the devices affected
> by a reset to know how to call the hot-reset ioctl".  We're returning
> the bdf to the cdev version of this ioctl for exactly this debugging
> purpose when the devices are not owned, that becomes useless if we give
> up an return -EPERM if ownership doesn't align.

Jason's suggestion makes sense for returning the case of returning dev_id
as dev_id is local to iommufd. If there are devices in the same dev_set are
opened by multiple users, multiple iommufd would be used. Then the
dev_id would have overlap. e.g. a dev_set has three devices. Device A and
B are opened by the current user as cdev, dev_id #1 and #2 are generated.
While device C opened by another user as cdev, dev_id #n is generated for
it. If dev_id #n happens to be #1, then user gets two info entries that have
the same dev_id.

I know the existing _INFO does not have such policy since the group/bdf
info are unique in the system. But for the dev_id case, seems really necessary
to fail if the dev_set is not only opened by one user. From this angle, will it be
good to have two ioctls. Sorry for the back-forth here. ☹

> > > We'd still
> > > report the bdf for those devices, but make use of the invalid/null
> > > dev-id.  I think this empowers userspace that they could make the same
> > > call on a group opened fd if necessary.
> >
> > For the devices opened via group path, it should have an entry that
> > includes invalid_dev_id+bdf. So user can map it to the device. But
> > there is no group_id, this may be fine since group is just a shortcut
> > to find the device. Is it?
> 
> Yes, it could be argued that the group-id itself is superfluous, the
> user can determine the group via the bdf, but it also aligns with the
> hot-reset ioctl, which currently requires the group fd.  Thanks,

I see. For the existing _INFO and HOT_RESET ioctl, I have no doubt.
Both of them use the group as a short-cut.

Regards,
Yi Liu