More information is required to assist trouble-shooting when
QEMU fails to initialise the event notifications for devices
assigned with VFIO-PCI. Instead of supplying the user with a cryptic
error number only, print out a proper error message with strerror()
so that the user has a better way to figure out what the problem is.
Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com>
---
Cc: qemu-trivial@nongnu.org
Cc: mjt@tls.msk.ru
Cc: laurent@vivier.eu
Cc: alex.williamson@redhat.com
---
hw/vfio/pci.c | 35 ++++++++++++++++++++++++-----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 31e1edf..3bffb93 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -430,13 +430,16 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool msix)
static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector,
int vector_n, bool msix)
{
- int virq;
+ int virq, ret;
if ((msix && vdev->no_kvm_msix) || (!msix && vdev->no_kvm_msi)) {
return;
}
- if (event_notifier_init(&vector->kvm_interrupt, 0)) {
+ ret = event_notifier_init(&vector->kvm_interrupt, 0);
+ if (ret) {
+ error_report("vfio (%s): Error: unable to init event notifier: %s (%d)",
+ __func__, strerror(-ret), -ret);
return;
}
@@ -486,8 +489,11 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
if (!vector->use) {
vector->vdev = vdev;
vector->virq = -1;
- if (event_notifier_init(&vector->interrupt, 0)) {
- error_report("vfio: Error: event_notifier_init failed");
+ ret = event_notifier_init(&vector->interrupt, 0);
+ if (ret) {
+ error_report("vfio (%s): Error: "
+ "unable to init event notifier: %s (%d)",
+ __func__, strerror(-ret), -ret);
}
vector->use = true;
msix_vector_use(pdev, nr);
@@ -658,8 +664,11 @@ retry:
vector->virq = -1;
vector->use = true;
- if (event_notifier_init(&vector->interrupt, 0)) {
- error_report("vfio: Error: event_notifier_init failed");
+ ret = event_notifier_init(&vector->interrupt, 0);
+ if (ret) {
+ error_report("vfio (%s): Error: "
+ "unable to init event notifier: %s (%d)",
+ __func__, strerror(-ret), -ret);
}
qemu_set_fd_handler(event_notifier_get_fd(&vector->interrupt),
@@ -2471,8 +2480,10 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
return;
}
- if (event_notifier_init(&vdev->err_notifier, 0)) {
- error_report("vfio: Unable to init event notifier for error detection");
+ ret = event_notifier_init(&vdev->err_notifier, 0);
+ if (ret) {
+ error_report("vfio (%s): Error: unable to init event notifier: %s (%d)"
+ " for error detection", __func__, strerror(-ret), -ret);
vdev->pci_aer = false;
return;
}
@@ -2553,7 +2564,7 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
{
struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
.index = VFIO_PCI_REQ_IRQ_INDEX };
- int argsz;
+ int argsz, ret;
struct vfio_irq_set *irq_set;
int32_t *pfd;
@@ -2566,8 +2577,10 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
return;
}
- if (event_notifier_init(&vdev->req_notifier, 0)) {
- error_report("vfio: Unable to init event notifier for device request");
+ ret = event_notifier_init(&vdev->req_notifier, 0);
+ if (ret) {
+ error_report("vfio (%s): Error: unable to init event notifier: %s (%d)"
+ " for device request", __func__, strerror(-ret), -ret);
return;
}
--
1.8.3.1
10.10.2017 13:22, Jim Quigley wrote: > More information is required to assist trouble-shooting when > QEMU fails to initialise the event notifications for devices > assigned with VFIO-PCI. Instead of supplying the user with a cryptic > error number only, print out a proper error message with strerror() > so that the user has a better way to figure out what the problem is. > > Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com> > --- > Cc: qemu-trivial@nongnu.org > Cc: mjt@tls.msk.ru > Cc: laurent@vivier.eu > Cc: alex.williamson@redhat.com > --- > hw/vfio/pci.c | 35 ++++++++++++++++++++++++----------- > 1 file changed, 24 insertions(+), 11 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 31e1edf..3bffb93 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -430,13 +430,16 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool msix) > static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector, > int vector_n, bool msix) > { > - int virq; > + int virq, ret; > > if ((msix && vdev->no_kvm_msix) || (!msix && vdev->no_kvm_msi)) { > return; > } > > - if (event_notifier_init(&vector->kvm_interrupt, 0)) { > + ret = event_notifier_init(&vector->kvm_interrupt, 0); > + if (ret) { > + error_report("vfio (%s): Error: unable to init event notifier: %s (%d)", > + __func__, strerror(-ret), -ret); Since this pattern gets repeated again and again, maybe we can either use a common wrapper or move that eror reporting into event_notifier_init()? Note there are other users of this function, besides hw/vfio, and maybe these, too, can benefit from better error reporting? Thanks, /mjt
On 16/10/2017 19:07, Michael Tokarev wrote: > 10.10.2017 13:22, Jim Quigley wrote: >> More information is required to assist trouble-shooting when >> QEMU fails to initialise the event notifications for devices >> assigned with VFIO-PCI. Instead of supplying the user with a cryptic >> error number only, print out a proper error message with strerror() >> so that the user has a better way to figure out what the problem is. >> >> Reviewed-by: Liam Merwick <liam.merwick@oracle.com> >> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com> >> --- >> Cc: qemu-trivial@nongnu.org >> Cc: mjt@tls.msk.ru >> Cc: laurent@vivier.eu >> Cc: alex.williamson@redhat.com >> --- >> hw/vfio/pci.c | 35 ++++++++++++++++++++++++----------- >> 1 file changed, 24 insertions(+), 11 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 31e1edf..3bffb93 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -430,13 +430,16 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool msix) >> static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector, >> int vector_n, bool msix) >> { >> - int virq; >> + int virq, ret; >> >> if ((msix && vdev->no_kvm_msix) || (!msix && vdev->no_kvm_msi)) { >> return; >> } >> >> - if (event_notifier_init(&vector->kvm_interrupt, 0)) { >> + ret = event_notifier_init(&vector->kvm_interrupt, 0); >> + if (ret) { >> + error_report("vfio (%s): Error: unable to init event notifier: %s (%d)", >> + __func__, strerror(-ret), -ret); > Since this pattern gets repeated again and again, maybe we can either > use a common wrapper or move that eror reporting into event_notifier_init()? > Note there are other users of this function, besides hw/vfio, and maybe > these, too, can benefit from better error reporting? Ideally the strerror() would be included in the error_report() function, (as per the error_setg() function), which obviously would involve a more extensive change to the code base. Would that be an acceptable solution ? Or I can move the reporting into theevent_notifier_init() function if that is the preferred approach ? thanks regards Jim Q. > > Thanks, > > /mjt > >
Jim Quigley <jim.quigley@oracle.com> writes: > On 16/10/2017 19:07, Michael Tokarev wrote: >> 10.10.2017 13:22, Jim Quigley wrote: >>> More information is required to assist trouble-shooting when >>> QEMU fails to initialise the event notifications for devices >>> assigned with VFIO-PCI. Instead of supplying the user with a cryptic >>> error number only, print out a proper error message with strerror() >>> so that the user has a better way to figure out what the problem is. >>> >>> Reviewed-by: Liam Merwick <liam.merwick@oracle.com> >>> Signed-off-by: Jim Quigley <Jim.Quigley@oracle.com> >>> --- >>> Cc: qemu-trivial@nongnu.org >>> Cc: mjt@tls.msk.ru >>> Cc: laurent@vivier.eu >>> Cc: alex.williamson@redhat.com >>> --- >>> hw/vfio/pci.c | 35 ++++++++++++++++++++++++----------- >>> 1 file changed, 24 insertions(+), 11 deletions(-) >>> >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index 31e1edf..3bffb93 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -430,13 +430,16 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool msix) >>> static void vfio_add_kvm_msi_virq(VFIOPCIDevice *vdev, VFIOMSIVector *vector, >>> int vector_n, bool msix) >>> { >>> - int virq; >>> + int virq, ret; >>> if ((msix && vdev->no_kvm_msix) || (!msix && >>> vdev->no_kvm_msi)) { >>> return; >>> } >>> - if (event_notifier_init(&vector->kvm_interrupt, 0)) { >>> + ret = event_notifier_init(&vector->kvm_interrupt, 0); >>> + if (ret) { >>> + error_report("vfio (%s): Error: unable to init event notifier: %s (%d)", >>> + __func__, strerror(-ret), -ret); >> Since this pattern gets repeated again and again, maybe we can either Parts of it are anti-patterns: * Error messages are by definition meant for the user. If you feel you need to include __func__ so it makes sense, you're obviously failing to address the end user. * Likewise for the numeric encoding of errno. * Printing "Error: " or similar is error_report()'s job. That leaves printing strerror(). >> use a common wrapper or move that eror reporting into event_notifier_init()? >> Note there are other users of this function, besides hw/vfio, and maybe >> these, too, can benefit from better error reporting? > > Ideally the strerror() would be included in the error_report() > function, > (as per the error_setg() function), which obviously would involve > a more > extensive change to the code base. Would that be an acceptable > solution ? All existing uses error_report() that print strerror() would have to be converted to the new function. Same for error_vreport(), warn_report(), ... Whether that's worthwhile is not obvious until we see patches. > Or I can move the reporting into theevent_notifier_init() function > if that is > the preferred approach ? Moving error_report() into event_notifier_init() makes it unusable in contexts where error_report() is unwanted or wrong.
© 2016 - 2024 Red Hat, Inc.