From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50515) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjpFa-0002uA-Qb for qemu-devel@nongnu.org; Tue, 13 Sep 2016 11:04:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjpFZ-0004KO-JE for qemu-devel@nongnu.org; Tue, 13 Sep 2016 11:03:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29020) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjpFZ-0004Jb-Ba for qemu-devel@nongnu.org; Tue, 13 Sep 2016 11:03:53 -0400 Date: Tue, 13 Sep 2016 09:03:52 -0600 From: Alex Williamson Message-ID: <20160913090352.05918144@t450s.home> In-Reply-To: <9c97a5ae-da7b-d025-4348-c821f90476df@redhat.com> References: <1473145893-17088-1-git-send-email-eric.auger@redhat.com> <1473145893-17088-2-git-send-email-eric.auger@redhat.com> <87y42xxp5m.fsf@dusky.pond.sub.org> <20160912101722.149f3a8a@t450s.home> <878tuwiadn.fsf@dusky.pond.sub.org> <9c97a5ae-da7b-d025-4348-c821f90476df@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Auger Eric Cc: Markus Armbruster , eric.auger.pro@gmail.com, qemu-devel@nongnu.org On Tue, 13 Sep 2016 09:21:17 +0200 Auger Eric wrote: > Hi Markus, > > On 13/09/2016 08:25, Markus Armbruster wrote: > > Alex Williamson writes: > > > >> On Mon, 12 Sep 2016 16:00:18 +0200 > >> Auger Eric wrote: > >> > >>> Hi Markus, > >>> > >>> On 12/09/2016 14:45, Markus Armbruster wrote: > >>>> Eric Auger 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 > > > > [...] > >