On Thu, 30 Mar 2023 19:44:55 -0300 Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > On Thu, Mar 30, 2023 at 12:48:03PM +0000, Liu, Yi L wrote: > > + /* > > + * If dev_id is needed, fill in the dev_id field, otherwise > > + * fill in group_id. > > + */ > > + if (fill->require_devid) { > > + /* > > + * Report the devices that are opened as cdev and have > > + * the same iommufd with the fill->iommufd. Otherwise, > > + * just fill in an IOMMUFD_INVALID_ID. > > + */ > > + vdev = vfio_pci_find_device_in_devset(dev_set, pdev); > > + if (vdev && !vfio_device_cdev_opened(vdev) && > > + fill->iommufd == vfio_iommufd_physical_ictx(vdev)) > > + vfio_iommufd_physical_devid(vdev, &fill->devices[fill->cur].dev_id); > > + fill->devices[fill->cur].dev_id = IOMMUFD_INVALID_ID; > > This needs an else? > > I suggest to check for VFIO_PCI_HOT_RESET_FLAG_IOMMUFD_DEV_ID on input > as well. I know the old kernels don't enforce this but at least we > could start enforcing it going forward so that the group path would > reject it to catch userspace bugs. > > May as well fix it up to fully validate the flags Is this under the guise of "if nobody complains it's ok, otherwise revert" plan? We report dev-id based on the nature of the device, not the provided flags, so I'm not sure I follow how this protects the group path, unless we've failed to clear the output flags on that path with this change. Thanks, Alex