On Fri, 14 Apr 2023 09:11:30 +0000 "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote: > > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > > Sent: Friday, April 14, 2023 2:07 AM > > > > We had already iterated a proposal where the group-id is replaced with > > a dev-id in the existing ioctl and a flag indicates when the return > > value is a dev-id vs group-id. This had a gap that userspace cannot > > determine if a reset is available given this information since un-owned > > devices report an invalid dev-id and userspace can't know if it has > > implicit ownership. > > > > It seems cleaner to me though that we would could still re-use INFO in > > a similar way, simply defining a new flag bit which is valid only in > > the case of returning dev-ids and indicates if the reset is available. > > Therefore in one ioctl, userspace knows if hot-reset is available > > (based on a kernel determination) and can pull valid dev-ids from the > > So the kernel needs to compare the group id between devices with > valid dev-ids and devices with invalid dev-ids to decide the implicit > ownership. For noiommu device which has no group_id when > VFIO_GROUP is off then it's resettable only if having a valid dev_id. With no-iommu and VFIO_GROUP on, each no-iommu device gets it's own group and the user must have ownership of each affected group, so there's really no difference here. Every affected no-iommu device must be owned in either case. > The only corner case with this option is when a user mixes group > and cdev usages. iirc you mentioned it's a valid usage to be supported. > In that case the kernel doesn't have sufficient knowledge to judge > 'resettable' as it doesn't know which groups are opened by this user. So for example we might have a 2-function device, fn0 is opened via cdev and part of an iommufd ctx and fn1 is opened via the group interface and potentially bound to a type1 container context. In the INFO/INFO2 proposal, the INFO ioctl would return an array reporting the group and BDF for each function. The INFO ioctl is callable from either device (aiui). The INFO2 ioctl would fail on the group opened device because it doesn't have an iommufd_ctx. When called on the cdev opened device, INFO2 would fail because the dev-set is not represented within the iommufd_ctx. Is this right? In my proposal, the INFO ioctl can also be called on either device. When called on the cdev opened device, the return structure provides dev-ids with a flag indicating such in the return structure. The cdev device has a valid dev-id, the group device invalid. The reset-available flag is clear because the kernel cannot infer ownership of the group opened device. When called on the group opened device, the IOMMU group and BDF are returned for each device. So both approaches have similar issues here, but I think there's an advantage to the approach of extending INFO. In that case, the user still gets the dev-id of the affected cdev device and therefore could build a hot-reset ioctl call using a combination of groupfds and devicefds, even if the cdev opened device are passed by fd. Perhaps it's obvious that the hot-reset device is itself affected by the reset, but I think the example scenario could be extended to one where there are multiple cdev opened devices and one or more group opened devices. AIUI, the INFO2 proposal essentially only returns success if the null-array approach is supported, ie. the kernel can infer the full ownership of the dev-set. However, I think we could still support a proof-of-ownership based hot-reset with devicefds and groupfds provide by the user. I think what this means is that the flag we're exposing is not "hot-reset available", but really whether the kernel can infer ownership and the ownership conditions are satisfied. Therefore it essentially only flags the availability of the null-array interface while the proof-of-ownership approach is always available. > Not sure whether we can leave it in a ugly way so INFO may not tell > 'resettable' accurately in that weird scenario. Is it still ugly with the above design? Thanks, Alex
