Re: [PATCH v3 12/12] vfio/pci: Report dev_id in VFIO_DEVICE_GET_PCI_HOT_RESET_INFO

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 18 Apr 2023 05:02:44 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Tuesday, April 18, 2023 12:11 PM
> > 
> > On Tue, 18 Apr 2023 03:24:46 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >   
> > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > Sent: Tuesday, April 18, 2023 4:07 AM
> > > >
> > > > On Mon, 17 Apr 2023 16:31:56 -0300
> > > > Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > >  
> > > > > On Mon, Apr 17, 2023 at 01:01:40PM -0600, Alex Williamson wrote:  
> > > > > > Yes, it's not trivial, but Jason is now proposing that we consider
> > > > > > mixing groups, cdevs, and multiple iommufd_ctxs as invalid.  I think
> > > > > > this means that regardless of which device calls INFO, there's only one
> > > > > > answer (assuming same set of devices opened, all cdev, all within  
> > same  
> > > > > > iommufd_ctx).  Based on what I explained about my understanding of  
> > > > INFO2  
> > > > > > and Jason agreed to, I think the output would be:
> > > > > >
> > > > > > flags: NOT_RESETABLE | DEV_ID
> > > > > > {
> > > > > >   { valid devA-id,  devA-BDF },
> > > > > >   { valid devC-id,  devC-BDF },
> > > > > >   { valid devD-id,  devD-BDF },
> > > > > >   { invalid dev-id, devE-BDF },
> > > > > > }
> > > > > >
> > > > > > Here devB gets dropped because the kernel understands that devB is
> > > > > > unopened, affected, and owned.  It's therefore not a blocker for
> > > > > > hot-reset.  
> > > > >
> > > > > I don't think we want to drop anything because it makes the API
> > > > > ill suited for the debugging purpose.
> > > > >
> > > > > devb should be returned with an invalid dev_id if I understand your
> > > > > example. Maybe it should return with -1 as the dev_id instead of 0, to
> > > > > make the debugging a bit better.
> > > > >
> > > > > Userspace should look at only NOT_RESETTABLE to determine if it
> > > > > proceeds or not, and it should use the valid dev_id list to iterate
> > > > > over the devices it has open to do the config stuff.  
> > > >
> > > > If an affected device is owned, not opened, and not interfering with
> > > > the reset, what is it adding to the API to report it for debugging
> > > > purposes?  I'm afraid this leads into expanding "invalid dev-id" into an  
> > >
> > > consistent output before and after devB is opened.  
> > 
> > In the case where devB is not opened including it only provides
> > useless information.  In the case where devB is opened it's necessary
> > to be reported as an opened, affected device.
> >   
> > > > errno or bitmap of error conditions that the user needs to parse.
> > > >  
> > >
> > > Not exactly.
> > >
> > > If RESETABLE invalid dev_id doesn't matter. The user only use the
> > > valid dev_id list to iterate as Jason pointed out.  
> > 
> > Yes, but...
> >   
> > > If NOT_RESETTABLE due to devE not assigned to the VM one can
> > > easily figure out the fact by simply looking at the list of affected BDFs
> > > and the configuration of assigned devices of the VM. Then invalid
> > > dev_id also doesn't matter.  
> > 
> > Huh?
> > 
> > Given:
> > 
> > flags: NOT_RESETABLE | DEV_ID
> > {
> >   { valid devA-id,  devA-BDF },
> >   { invalid dev-id, devB-BDF },
> >   { valid devC-id,  devC-BDF },
> >   { valid devD-id,  devD-BDF },
> >   { invalid dev-id, devE-BDF },
> > }
> > 
> > How does the user determine that devE is to blame and not devB based on
> > BDF?  The user cannot rely on sysfs for help, they don't know the IOMMU
> > grouping, nor do they know the BDF except as inferred by matching valid
> > dev-ids in the above output.  
> 
> emmm aren't we talking about the 'person' who does diagnostic? This guy
> will look at the VM configuration file to know that devA/B/C/D have been
> assigned to the VM but not devE.

Actually the scenario is that devA/C/D are assigned, devB is implicitly
owned, and it's devE that blocks the reset.  If you've followed any of
the community forums for vfio over the years, it should be readily
apparent that placing the burden solely on the end user to perform such
a diagnosis is an unreasonable expectation.

> > > If NOT_RESETTABLE while devE is already assigned to the VM then it's
> > > indication of mixing groups, cdevs or multiple iommufd_ctxs. Then
> > > people should debug with other means/hints to dig out the exact
> > > culprit.  
> > 
> > I don't know what situation you're trying to explain here.  If devE
> > were opened within the same iommufd_ctx, this becomes:  
> 
> It's about a scenario where the mgmt.. stack has assigned all affected
> devices to Qemu but Qemu itself messed it up with mixed group/cdev
> or multiple iommufd_ctx so hitting the NON_RESETTABLE situation.

Is this a reasonable scenario?  I expect the QEMU support to favor cdev
access where available and fd passing methods will only use cdev, so
QEMU should never mess up to create such an environment.  There should
never be a case where a device is exclusively available via group
rather than cdev.

> > flags: RESETABLE | DEV_ID
> > {
> >   { valid devA-id,  devA-BDF },
> >   { invalid dev-id, devB-BDF },
> >   { valid devC-id,  devC-BDF },
> >   { valid devD-id,  devD-BDF },
> >   { valid devE-id,  devE-BDF },
> > }
> > 
> > Yes, the user should only be looking at the flag to determine the
> > availability of hot-reset, (here's the but) but how is it consistent to
> > indicate both that hot-reset is available and include an invalid
> > dev-id?  The consistency as I propose is that an invalid dev-id is only
> > presented with NOT_RESETTABLE for the device blocking hot-reset.  In
> > the previous case, devB is not blocking reset and reporting an invalid
> > dev-id only serves to obfuscate determining the blocking device.
> > 
> > For the cases of affected group-opened devices or separate
> > iommufd_ctxs, the user gets invalid dev-ids for anything outside of
> > the calling device's iommufd_ctx.
> > 
> > We haven't discussed how it fails when called on a group-opened device
> > in a mixed environment.  I'd propose that the INFO ioctl behaves
> > exactly as it does today, reporting group-id and BDF for each affected
> > device.  However, the hot-reset ioctl itself is not extended to accept
> > devicefd because there is no proof-of-ownership model for cdevs.
> > Therefore even if the user could map group-id to devicefd, they get
> > -EINVAL calling HOT_RESET with a devicefd when the ioctl is called from
> > a group-opened device.  Thanks,
> >   
> 
> Yes I chatted with Yi about it.
> 
> If the calling device of the INFO ioctl is opened by group then behave
> as it does today.
> 
> If the calling device is opened via cdev then use dev_id scheme as
> discussed above.
> 
> in hot_reset ioctl the fd array only accepts group fd's.
> 
> cdev can be reset only via null fd array.
> 
> It remains a small open that null fd array could potentially work for
> group-opened device too if vfio-compat is used. In that case devices
> are in same iommufd ctx with valid dev_id even though they are opened 
> via group. But probably it's not worthy blocking it?

Yes, let's not create new models for the compatibility interface, stick
with group-opened = group-id = proof-of-ownership.  Thanks,

Alex