Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize

[Alex Williamson]

On Tue, 13 Sep 2016 09:21:17 +0200
Auger Eric <address@hidden> wrote:

> Hi Markus,
> 
> On 13/09/2016 08:25, Markus Armbruster wrote:
> > Alex Williamson <address@hidden> writes:
> >   
> >> On Mon, 12 Sep 2016 16:00:18 +0200
> >> Auger Eric <address@hidden> wrote:
> >>  
> >>> Hi Markus,
> >>>
> >>> On 12/09/2016 14:45, Markus Armbruster wrote:  
> >>>> Eric Auger <address@hidden> writes:
> >>>>     
> >>>>> This patch converts VFIO PCI to realize function.
> >>>>>
> >>>>> Also original initfn errors now are propagated using QEMU
> >>>>> error objects. All errors are formatted with the same pattern:
> >>>>> "vfio: %s: the error description"    
> >>>>
> >>>> Example:
> >>>>
> >>>> $ upstream-qemu -device vfio-pci
> >>>> upstream-qemu: -device vfio-pci: vfio: 0000:00:00.0: no iommu_group 
> >>>> found: Unknown error -2
> >>>>
> >>>> Two remarks:
> >>>>
> >>>> * "Unknown error -2" should be "No such file or directory".  See below.  
> >>>>   
> >>> Hum. I noticed that but I didn't have the presence of mind to get it was
> >>> due to -errno!  
> >>>>
> >>>> * Five colons, not counting the ones in the PCI address.  Do we need the
> >>>>   "vfio: 0000:00:00.0:" part?  If yes, could we find a nicer way to
> >>>>   print it?  Up to you.    
> >>> Well we have quite a lot of traces with such format, hence my choice.
> >>> Alex do you want to change this?  
> >>
> >> Well, we need to identify the component with the error, it's not
> >> uncommon to have multiple assigned devices.  The PCI address is just
> >> the basename in vfio code, it might also be the name of a device node
> >> in sysfs, such as a uuid of an mdev devices.  AFAIK we cannot count on
> >> a id and even if we could libvirt assigns them based on order in the
> >> xml, making them a bit magic.  Maybe I'm not understanding the
> >> question.  Regarding trace vs error message, I expect that it's going
> >> to be a more advanced user/developer enabling tracing, error reports
> >> should try a little harder to be comprehensible to an average user.  
> > 
> > Yes, the error message needs to identify the part that's wrong.
> > However, we need to consider the *complete* error message to judge it.
> > Consider:
> > 
> >     $ upstream-qemu -device vfio-pci
> >     upstream-qemu: -device vfio-pci: vfio: 0000:00:00.0: no iommu_group 
> > found: No such file or directory
> > 
> > Which parts are actually useful for the user?  "-device vfio-pci"
> > identifies the part that's wrong.  "vfio: 0000:00:00.0" is gobbledygook
> > unless you're somewhat familiar with vfio, and then it's redundant.

I think you're identifying a bug in our ability to detect whether
DEFINE_PROP_PCI_HOST_DEVADDR has been set or not.  If a user had
instead run:

-device vfio-pci,host=0000:00:00.0 -device vfio-pci,host=0000:00:00.1

Then yes, the distinction between zeros and .1 is useful, but without
specifying a host or sysfsdev, we need a new error path.  The "vfio:"
may certainly be redundant when "-device vfio-pci" is already
pre-pended to the error message.

> > 
> > The "vfio: 0000:00:00.0" *is* useful in messages outside realize()
> > context, because then the system can't prefix the "-device vfio-pci"
> > part.  
> 
> Here the end-user omitted the host device and effectively the error
> message isn't very useful in that case. I will improve that. Maybe I can
> use error_append_hint.

Right, it seems like this needs to be detected and a new error path
added.  We require either a host= or sysfsdev= option and should never
try to use an unset "host" value.  Thanks,

Alex

> In some other parts of the realize(), vfio_populate_device, msix_setup,
> understanding which device caused the error is meaningful I think.
> 
> Typically when several devices are passthrough'ed, for instance:
> upstream-qemu -device vfio-pci,host=0000:01:10.0 -device
> vfio-pci,host=0000:01:10.4
> 
> >   
> >>>> Always, always, always test your error messages :)  
> > 
> > Because what you think you see in the code may differ from what the user
> > will see.
> > 
> > Anyway, your choice, I'm just giving feedback on the error messages I
> > observe in testing.  
> Yes that's really useful!
> 
> Thanks
> 
> Eric
> > 
> > [...]
> >