* [Qemu-devel] [PATCH 0/3] Convert VFIO-PCI to realize @ 2016-09-06 7:11 Eric Auger 2016-09-06 7:11 ` [Qemu-devel] [PATCH 1/3] vfio/pci: conversion " Eric Auger ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Eric Auger @ 2016-09-06 7:11 UTC (permalink / raw) To: eric.auger, eric.auger.pro, qemu-devel, alex.williamson, armbru This series converts VFIO-PCI to realize. It also aims at improving the error reporting in case of QMP hot-plug. Before the series, a device_add failure would have reported: {"error": {"class": "GenericError", "desc": "Device initialization failed"}}. Now the actual error cause is reported. Eric Auger (3): vfio/pci: conversion to realize vfio/pci: pass an error object to vfio_populate_device vfio/pci: pass an error object to vfio_msix_early_setup hw/vfio/pci.c | 150 ++++++++++++++++++++++++++------------------------- hw/vfio/trace-events | 2 +- 2 files changed, 78 insertions(+), 74 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-06 7:11 [Qemu-devel] [PATCH 0/3] Convert VFIO-PCI to realize Eric Auger @ 2016-09-06 7:11 ` Eric Auger 2016-09-12 12:45 ` Markus Armbruster 2016-09-06 7:11 ` [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device Eric Auger 2016-09-06 7:11 ` [Qemu-devel] [PATCH 3/3] vfio/pci: pass an error object to vfio_msix_early_setup Eric Auger 2 siblings, 1 reply; 19+ messages in thread From: Eric Auger @ 2016-09-06 7:11 UTC (permalink / raw) To: eric.auger, eric.auger.pro, qemu-devel, alex.williamson, armbru 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" Subsequent patches will pass the error objects to - vfio_populate_device, - vfio_msix_early_setup. At this point if those functions fail let's just report an error at realize level. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- hw/vfio/pci.c | 81 ++++++++++++++++++++++++++++------------------------ hw/vfio/trace-events | 2 +- 2 files changed, 44 insertions(+), 39 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 7bfa17c..ae1967c 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2485,7 +2485,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) vdev->req_enabled = false; } -static int vfio_initfn(PCIDevice *pdev) +static void vfio_realize(PCIDevice *pdev, Error **errp) { VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); VFIODevice *vbasedev_iter; @@ -2504,9 +2504,9 @@ static int vfio_initfn(PCIDevice *pdev) } if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { - error_report("vfio: error: no such host device: %s", - vdev->vbasedev.sysfsdev); - return -errno; + error_setg_errno(errp, -errno, "vfio: error: no such host device: %s", + vdev->vbasedev.sysfsdev); + return; } vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev)); @@ -2518,45 +2518,46 @@ static int vfio_initfn(PCIDevice *pdev) g_free(tmp); if (len <= 0 || len >= sizeof(group_path)) { - error_report("vfio: error no iommu_group for device"); - return len < 0 ? -errno : -ENAMETOOLONG; + error_setg_errno(errp, len < 0 ? -errno : -ENAMETOOLONG, + "no iommu_group found"); + goto error; } group_path[len] = 0; group_name = basename(group_path); if (sscanf(group_name, "%d", &groupid) != 1) { - error_report("vfio: error reading %s: %m", group_path); - return -errno; + error_setg_errno(errp, -errno, "failed to read %s", group_path); + goto error; } - trace_vfio_initfn(vdev->vbasedev.name, groupid); + trace_vfio_realize(vdev->vbasedev.name, groupid); group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev)); if (!group) { - error_report("vfio: failed to get group %d", groupid); - return -ENOENT; + error_setg_errno(errp, -ENOENT, "failed to get group %d", groupid); + goto error; } QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) { - error_report("vfio: error: device %s is already attached", - vdev->vbasedev.name); + error_setg_errno(errp, -EBUSY, "device is already attached"); vfio_put_group(group); - return -EBUSY; + goto error; } } ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev); if (ret) { - error_report("vfio: failed to get device %s", vdev->vbasedev.name); + error_setg_errno(errp, ret, "failed to get the device"); vfio_put_group(group); - return ret; + goto error; } ret = vfio_populate_device(vdev); if (ret) { - return ret; + error_setg_errno(errp, ret, "failed to populate the device"); + goto error; } /* Get a copy of config space */ @@ -2565,8 +2566,8 @@ static int vfio_initfn(PCIDevice *pdev) vdev->config_offset); if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) { ret = ret < 0 ? -errno : -EFAULT; - error_report("vfio: Failed to read device config space"); - return ret; + error_setg_errno(errp, ret, "failed to read device config space"); + goto error; } /* vfio emulates a lot for us, but some bits need extra love */ @@ -2582,8 +2583,8 @@ static int vfio_initfn(PCIDevice *pdev) */ if (vdev->vendor_id != PCI_ANY_ID) { if (vdev->vendor_id >= 0xffff) { - error_report("vfio: Invalid PCI vendor ID provided"); - return -EINVAL; + error_setg_errno(errp, -EINVAL, "invalid PCI vendor ID provided"); + goto error; } vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0); trace_vfio_pci_emulated_vendor_id(vdev->vbasedev.name, vdev->vendor_id); @@ -2593,8 +2594,8 @@ static int vfio_initfn(PCIDevice *pdev) if (vdev->device_id != PCI_ANY_ID) { if (vdev->device_id > 0xffff) { - error_report("vfio: Invalid PCI device ID provided"); - return -EINVAL; + error_setg_errno(errp, -EINVAL, "invalid PCI device ID provided"); + goto error; } vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0); trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id); @@ -2604,8 +2605,9 @@ static int vfio_initfn(PCIDevice *pdev) if (vdev->sub_vendor_id != PCI_ANY_ID) { if (vdev->sub_vendor_id > 0xffff) { - error_report("vfio: Invalid PCI subsystem vendor ID provided"); - return -EINVAL; + error_setg_errno(errp, -EINVAL, + "invalid PCI subsystem vendor ID provided"); + goto error; } vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID, vdev->sub_vendor_id, ~0); @@ -2615,8 +2617,9 @@ static int vfio_initfn(PCIDevice *pdev) if (vdev->sub_device_id != PCI_ANY_ID) { if (vdev->sub_device_id > 0xffff) { - error_report("vfio: Invalid PCI subsystem device ID provided"); - return -EINVAL; + error_setg_errno(errp, -EINVAL, + "invalid PCI subsystem device ID provided"); + goto error; } vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0); trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name, @@ -2646,7 +2649,8 @@ static int vfio_initfn(PCIDevice *pdev) ret = vfio_msix_early_setup(vdev); if (ret) { - return ret; + error_setg_errno(errp, ret, "msix early setup failure"); + goto error; } vfio_bars_setup(vdev); @@ -2669,9 +2673,9 @@ static int vfio_initfn(PCIDevice *pdev) struct vfio_region_info *opregion; if (vdev->pdev.qdev.hotplugged) { - error_report("Cannot support IGD OpRegion feature on hotplugged " - "device %s", vdev->vbasedev.name); - ret = -EINVAL; + error_setg_errno(errp, -EINVAL, + "cannot support IGD OpRegion feature on hotplugged " + "device"); goto out_teardown; } @@ -2679,16 +2683,15 @@ static int vfio_initfn(PCIDevice *pdev) VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL, VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion); if (ret) { - error_report("Device %s does not support requested IGD OpRegion " - "feature", vdev->vbasedev.name); + error_setg_errno(errp, ret, + "does not support requested IGD OpRegion feature"); goto out_teardown; } ret = vfio_pci_igd_opregion_init(vdev, opregion); g_free(opregion); if (ret) { - error_report("Device %s IGD OpRegion initialization failed", - vdev->vbasedev.name); + error_setg_errno(errp, ret, "IGD OpRegion initialization failed"); goto out_teardown; } } @@ -2710,6 +2713,7 @@ static int vfio_initfn(PCIDevice *pdev) pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update); ret = vfio_intx_enable(vdev); if (ret) { + error_setg_errno(errp, ret, "failed to enable intx"); goto out_teardown; } } @@ -2718,13 +2722,14 @@ static int vfio_initfn(PCIDevice *pdev) vfio_register_req_notifier(vdev); vfio_setup_resetfn_quirk(vdev); - return 0; + return; out_teardown: pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); vfio_teardown_msi(vdev); vfio_bars_exit(vdev); - return ret; +error: + error_prepend(errp, "vfio: %s: ", vdev->vbasedev.name); } static void vfio_instance_finalize(Object *obj) @@ -2853,7 +2858,7 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) dc->vmsd = &vfio_pci_vmstate; dc->desc = "VFIO-based PCI device assignment"; set_bit(DEVICE_CATEGORY_MISC, dc->categories); - pdc->init = vfio_initfn; + pdc->realize = vfio_realize; pdc->exit = vfio_exitfn; pdc->config_read = vfio_pci_read_config; pdc->config_write = vfio_pci_write_config; diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index 4bb7690..14e3fc4 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -36,7 +36,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s" vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n size: 0x%lx, offset: 0x%lx, flags: 0x%lx" vfio_populate_device_get_irq_info_failure(void) "VFIO_DEVICE_GET_IRQ_INFO failure: %m" -vfio_initfn(const char *name, int group_id) " (%s) group %d" +vfio_realize(const char *name, int group_id) " (%s) group %d" vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s %x@%x" vfio_pci_reset(const char *name) " (%s)" vfio_pci_reset_flr(const char *name) "%s FLR/VFIO_DEVICE_RESET" -- 1.9.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-06 7:11 ` [Qemu-devel] [PATCH 1/3] vfio/pci: conversion " Eric Auger @ 2016-09-12 12:45 ` Markus Armbruster 2016-09-12 14:00 ` Auger Eric 0 siblings, 1 reply; 19+ messages in thread From: Markus Armbruster @ 2016-09-12 12:45 UTC (permalink / raw) To: Eric Auger; +Cc: eric.auger.pro, qemu-devel, alex.williamson Eric Auger <eric.auger@redhat.com> 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. * 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. Always, always, always test your error messages :) > Subsequent patches will pass the error objects to > - vfio_populate_device, > - vfio_msix_early_setup. > > At this point if those functions fail let's just report an error > at realize level. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > hw/vfio/pci.c | 81 ++++++++++++++++++++++++++++------------------------ > hw/vfio/trace-events | 2 +- > 2 files changed, 44 insertions(+), 39 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 7bfa17c..ae1967c 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2485,7 +2485,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) > vdev->req_enabled = false; > } > > -static int vfio_initfn(PCIDevice *pdev) > +static void vfio_realize(PCIDevice *pdev, Error **errp) > { > VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > VFIODevice *vbasedev_iter; > @@ -2504,9 +2504,9 @@ static int vfio_initfn(PCIDevice *pdev) > } > > if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { > - error_report("vfio: error: no such host device: %s", > - vdev->vbasedev.sysfsdev); > - return -errno; > + error_setg_errno(errp, -errno, "vfio: error: no such host device: %s", error_setg_errno() takes a *positive* errno. Same elsewhere. > + vdev->vbasedev.sysfsdev); > + return; > } > > vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev)); > @@ -2518,45 +2518,46 @@ static int vfio_initfn(PCIDevice *pdev) > g_free(tmp); > > if (len <= 0 || len >= sizeof(group_path)) { > - error_report("vfio: error no iommu_group for device"); > - return len < 0 ? -errno : -ENAMETOOLONG; > + error_setg_errno(errp, len < 0 ? -errno : -ENAMETOOLONG, > + "no iommu_group found"); > + goto error; > } > > group_path[len] = 0; > > group_name = basename(group_path); > if (sscanf(group_name, "%d", &groupid) != 1) { > - error_report("vfio: error reading %s: %m", group_path); > - return -errno; > + error_setg_errno(errp, -errno, "failed to read %s", group_path); > + goto error; > } > > - trace_vfio_initfn(vdev->vbasedev.name, groupid); > + trace_vfio_realize(vdev->vbasedev.name, groupid); > > group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev)); > if (!group) { > - error_report("vfio: failed to get group %d", groupid); > - return -ENOENT; > + error_setg_errno(errp, -ENOENT, "failed to get group %d", groupid); > + goto error; I understand this is a mechanical conversion, but are you sure the ": No such file or directory" suffix we get from passing ENOENT buys us anything? More of the same below. > } > > QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { > if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) { > - error_report("vfio: error: device %s is already attached", > - vdev->vbasedev.name); > + error_setg_errno(errp, -EBUSY, "device is already attached"); > vfio_put_group(group); > - return -EBUSY; > + goto error; > } > } > > ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev); > if (ret) { > - error_report("vfio: failed to get device %s", vdev->vbasedev.name); > + error_setg_errno(errp, ret, "failed to get the device"); > vfio_put_group(group); > - return ret; > + goto error; > } > > ret = vfio_populate_device(vdev); > if (ret) { > - return ret; > + error_setg_errno(errp, ret, "failed to populate the device"); > + goto error; > } vfio_populate_device() reports errors with error_report(). We get a specific error via error_report(), and a generic error here. PATCH 2 converts it to Error, getting rid of the generic error again. Works for me, but I usually order conversion patches the other way: first convert vfio_populate_device(), and report its Error like this: ret = vfio_populate_device(vdev, &err); if (err) { error_report_err(err); return ret; } PRO: no intermediate state with two errors. CON: have to touch vfio_populate_device() again to drop the return value. Matter of taste, thus your choice. Same for vfio_msix_early_setup() below. > > /* Get a copy of config space */ > @@ -2565,8 +2566,8 @@ static int vfio_initfn(PCIDevice *pdev) > vdev->config_offset); > if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) { > ret = ret < 0 ? -errno : -EFAULT; > - error_report("vfio: Failed to read device config space"); > - return ret; > + error_setg_errno(errp, ret, "failed to read device config space"); > + goto error; > } > > /* vfio emulates a lot for us, but some bits need extra love */ > @@ -2582,8 +2583,8 @@ static int vfio_initfn(PCIDevice *pdev) > */ > if (vdev->vendor_id != PCI_ANY_ID) { > if (vdev->vendor_id >= 0xffff) { > - error_report("vfio: Invalid PCI vendor ID provided"); > - return -EINVAL; > + error_setg_errno(errp, -EINVAL, "invalid PCI vendor ID provided"); > + goto error; > } > vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0); > trace_vfio_pci_emulated_vendor_id(vdev->vbasedev.name, vdev->vendor_id); > @@ -2593,8 +2594,8 @@ static int vfio_initfn(PCIDevice *pdev) > > if (vdev->device_id != PCI_ANY_ID) { > if (vdev->device_id > 0xffff) { > - error_report("vfio: Invalid PCI device ID provided"); > - return -EINVAL; > + error_setg_errno(errp, -EINVAL, "invalid PCI device ID provided"); > + goto error; > } > vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0); > trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id); > @@ -2604,8 +2605,9 @@ static int vfio_initfn(PCIDevice *pdev) > > if (vdev->sub_vendor_id != PCI_ANY_ID) { > if (vdev->sub_vendor_id > 0xffff) { > - error_report("vfio: Invalid PCI subsystem vendor ID provided"); > - return -EINVAL; > + error_setg_errno(errp, -EINVAL, > + "invalid PCI subsystem vendor ID provided"); > + goto error; > } > vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID, > vdev->sub_vendor_id, ~0); > @@ -2615,8 +2617,9 @@ static int vfio_initfn(PCIDevice *pdev) > > if (vdev->sub_device_id != PCI_ANY_ID) { > if (vdev->sub_device_id > 0xffff) { > - error_report("vfio: Invalid PCI subsystem device ID provided"); > - return -EINVAL; > + error_setg_errno(errp, -EINVAL, > + "invalid PCI subsystem device ID provided"); > + goto error; > } > vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0); > trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name, > @@ -2646,7 +2649,8 @@ static int vfio_initfn(PCIDevice *pdev) > > ret = vfio_msix_early_setup(vdev); > if (ret) { > - return ret; > + error_setg_errno(errp, ret, "msix early setup failure"); > + goto error; > } > > vfio_bars_setup(vdev); > @@ -2669,9 +2673,9 @@ static int vfio_initfn(PCIDevice *pdev) > struct vfio_region_info *opregion; > > if (vdev->pdev.qdev.hotplugged) { > - error_report("Cannot support IGD OpRegion feature on hotplugged " > - "device %s", vdev->vbasedev.name); > - ret = -EINVAL; > + error_setg_errno(errp, -EINVAL, > + "cannot support IGD OpRegion feature on hotplugged " > + "device"); > goto out_teardown; > } > > @@ -2679,16 +2683,15 @@ static int vfio_initfn(PCIDevice *pdev) > VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL, > VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion); > if (ret) { > - error_report("Device %s does not support requested IGD OpRegion " > - "feature", vdev->vbasedev.name); > + error_setg_errno(errp, ret, > + "does not support requested IGD OpRegion feature"); > goto out_teardown; > } > > ret = vfio_pci_igd_opregion_init(vdev, opregion); > g_free(opregion); > if (ret) { > - error_report("Device %s IGD OpRegion initialization failed", > - vdev->vbasedev.name); > + error_setg_errno(errp, ret, "IGD OpRegion initialization failed"); > goto out_teardown; > } > } > @@ -2710,6 +2713,7 @@ static int vfio_initfn(PCIDevice *pdev) > pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update); > ret = vfio_intx_enable(vdev); > if (ret) { > + error_setg_errno(errp, ret, "failed to enable intx"); > goto out_teardown; > } > } > @@ -2718,13 +2722,14 @@ static int vfio_initfn(PCIDevice *pdev) > vfio_register_req_notifier(vdev); > vfio_setup_resetfn_quirk(vdev); > > - return 0; > + return; > > out_teardown: > pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); > vfio_teardown_msi(vdev); > vfio_bars_exit(vdev); > - return ret; > +error: > + error_prepend(errp, "vfio: %s: ", vdev->vbasedev.name); > } > > static void vfio_instance_finalize(Object *obj) > @@ -2853,7 +2858,7 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) > dc->vmsd = &vfio_pci_vmstate; > dc->desc = "VFIO-based PCI device assignment"; > set_bit(DEVICE_CATEGORY_MISC, dc->categories); > - pdc->init = vfio_initfn; > + pdc->realize = vfio_realize; > pdc->exit = vfio_exitfn; > pdc->config_read = vfio_pci_read_config; > pdc->config_write = vfio_pci_write_config; > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events > index 4bb7690..14e3fc4 100644 > --- a/hw/vfio/trace-events > +++ b/hw/vfio/trace-events > @@ -36,7 +36,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int > vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s" > vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n size: 0x%lx, offset: 0x%lx, flags: 0x%lx" > vfio_populate_device_get_irq_info_failure(void) "VFIO_DEVICE_GET_IRQ_INFO failure: %m" > -vfio_initfn(const char *name, int group_id) " (%s) group %d" > +vfio_realize(const char *name, int group_id) " (%s) group %d" > vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s %x@%x" > vfio_pci_reset(const char *name) " (%s)" > vfio_pci_reset_flr(const char *name) "%s FLR/VFIO_DEVICE_RESET" ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-12 12:45 ` Markus Armbruster @ 2016-09-12 14:00 ` Auger Eric 2016-09-12 16:17 ` Alex Williamson 0 siblings, 1 reply; 19+ messages in thread From: Auger Eric @ 2016-09-12 14:00 UTC (permalink / raw) To: Markus Armbruster; +Cc: alex.williamson, qemu-devel, eric.auger.pro Hi Markus, On 12/09/2016 14:45, Markus Armbruster wrote: > Eric Auger <eric.auger@redhat.com> 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? > > Always, always, always test your error messages :) > >> Subsequent patches will pass the error objects to >> - vfio_populate_device, >> - vfio_msix_early_setup. >> >> At this point if those functions fail let's just report an error >> at realize level. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> hw/vfio/pci.c | 81 ++++++++++++++++++++++++++++------------------------ >> hw/vfio/trace-events | 2 +- >> 2 files changed, 44 insertions(+), 39 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 7bfa17c..ae1967c 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2485,7 +2485,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) >> vdev->req_enabled = false; >> } >> >> -static int vfio_initfn(PCIDevice *pdev) >> +static void vfio_realize(PCIDevice *pdev, Error **errp) >> { >> VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); >> VFIODevice *vbasedev_iter; >> @@ -2504,9 +2504,9 @@ static int vfio_initfn(PCIDevice *pdev) >> } >> >> if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { >> - error_report("vfio: error: no such host device: %s", >> - vdev->vbasedev.sysfsdev); >> - return -errno; >> + error_setg_errno(errp, -errno, "vfio: error: no such host device: %s", > > error_setg_errno() takes a *positive* errno. Same elsewhere. OK thanks! > >> + vdev->vbasedev.sysfsdev); >> + return; >> } >> >> vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev)); >> @@ -2518,45 +2518,46 @@ static int vfio_initfn(PCIDevice *pdev) >> g_free(tmp); >> >> if (len <= 0 || len >= sizeof(group_path)) { >> - error_report("vfio: error no iommu_group for device"); >> - return len < 0 ? -errno : -ENAMETOOLONG; >> + error_setg_errno(errp, len < 0 ? -errno : -ENAMETOOLONG, >> + "no iommu_group found"); >> + goto error; >> } >> >> group_path[len] = 0; >> >> group_name = basename(group_path); >> if (sscanf(group_name, "%d", &groupid) != 1) { >> - error_report("vfio: error reading %s: %m", group_path); >> - return -errno; >> + error_setg_errno(errp, -errno, "failed to read %s", group_path); >> + goto error; >> } >> >> - trace_vfio_initfn(vdev->vbasedev.name, groupid); >> + trace_vfio_realize(vdev->vbasedev.name, groupid); >> >> group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev)); >> if (!group) { >> - error_report("vfio: failed to get group %d", groupid); >> - return -ENOENT; >> + error_setg_errno(errp, -ENOENT, "failed to get group %d", groupid); >> + goto error; > > I understand this is a mechanical conversion, but are you sure the ": No > such file or directory" suffix we get from passing ENOENT buys us > anything? More of the same below. At that stage I prefered to stick to the original messages since Alex & VFIO users may have their habits. > >> } >> >> QLIST_FOREACH(vbasedev_iter, &group->device_list, next) { >> if (strcmp(vbasedev_iter->name, vdev->vbasedev.name) == 0) { >> - error_report("vfio: error: device %s is already attached", >> - vdev->vbasedev.name); >> + error_setg_errno(errp, -EBUSY, "device is already attached"); >> vfio_put_group(group); >> - return -EBUSY; >> + goto error; >> } >> } >> >> ret = vfio_get_device(group, vdev->vbasedev.name, &vdev->vbasedev); >> if (ret) { >> - error_report("vfio: failed to get device %s", vdev->vbasedev.name); >> + error_setg_errno(errp, ret, "failed to get the device"); >> vfio_put_group(group); >> - return ret; >> + goto error; >> } >> >> ret = vfio_populate_device(vdev); >> if (ret) { >> - return ret; >> + error_setg_errno(errp, ret, "failed to populate the device"); >> + goto error; >> } > > vfio_populate_device() reports errors with error_report(). We get a > specific error via error_report(), and a generic error here. PATCH 2 > converts it to Error, getting rid of the generic error again. Works for > me, but I usually order conversion patches the other way: first convert > vfio_populate_device(), and report its Error like this: > > ret = vfio_populate_device(vdev, &err); > if (err) { > error_report_err(err); > return ret; > } > > PRO: no intermediate state with two errors. CON: have to touch > vfio_populate_device() again to drop the return value. Matter of taste, > thus your choice. > > Same for vfio_msix_early_setup() below. Yes it makes sense. I will respin adopting the above strategy. > >> >> /* Get a copy of config space */ >> @@ -2565,8 +2566,8 @@ static int vfio_initfn(PCIDevice *pdev) >> vdev->config_offset); >> if (ret < (int)MIN(pci_config_size(&vdev->pdev), vdev->config_size)) { >> ret = ret < 0 ? -errno : -EFAULT; >> - error_report("vfio: Failed to read device config space"); >> - return ret; >> + error_setg_errno(errp, ret, "failed to read device config space"); >> + goto error; >> } >> >> /* vfio emulates a lot for us, but some bits need extra love */ >> @@ -2582,8 +2583,8 @@ static int vfio_initfn(PCIDevice *pdev) >> */ >> if (vdev->vendor_id != PCI_ANY_ID) { >> if (vdev->vendor_id >= 0xffff) { >> - error_report("vfio: Invalid PCI vendor ID provided"); >> - return -EINVAL; >> + error_setg_errno(errp, -EINVAL, "invalid PCI vendor ID provided"); >> + goto error; >> } >> vfio_add_emulated_word(vdev, PCI_VENDOR_ID, vdev->vendor_id, ~0); >> trace_vfio_pci_emulated_vendor_id(vdev->vbasedev.name, vdev->vendor_id); >> @@ -2593,8 +2594,8 @@ static int vfio_initfn(PCIDevice *pdev) >> >> if (vdev->device_id != PCI_ANY_ID) { >> if (vdev->device_id > 0xffff) { >> - error_report("vfio: Invalid PCI device ID provided"); >> - return -EINVAL; >> + error_setg_errno(errp, -EINVAL, "invalid PCI device ID provided"); >> + goto error; >> } >> vfio_add_emulated_word(vdev, PCI_DEVICE_ID, vdev->device_id, ~0); >> trace_vfio_pci_emulated_device_id(vdev->vbasedev.name, vdev->device_id); >> @@ -2604,8 +2605,9 @@ static int vfio_initfn(PCIDevice *pdev) >> >> if (vdev->sub_vendor_id != PCI_ANY_ID) { >> if (vdev->sub_vendor_id > 0xffff) { >> - error_report("vfio: Invalid PCI subsystem vendor ID provided"); >> - return -EINVAL; >> + error_setg_errno(errp, -EINVAL, >> + "invalid PCI subsystem vendor ID provided"); >> + goto error; >> } >> vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_VENDOR_ID, >> vdev->sub_vendor_id, ~0); >> @@ -2615,8 +2617,9 @@ static int vfio_initfn(PCIDevice *pdev) >> >> if (vdev->sub_device_id != PCI_ANY_ID) { >> if (vdev->sub_device_id > 0xffff) { >> - error_report("vfio: Invalid PCI subsystem device ID provided"); >> - return -EINVAL; >> + error_setg_errno(errp, -EINVAL, >> + "invalid PCI subsystem device ID provided"); >> + goto error; >> } >> vfio_add_emulated_word(vdev, PCI_SUBSYSTEM_ID, vdev->sub_device_id, ~0); >> trace_vfio_pci_emulated_sub_device_id(vdev->vbasedev.name, >> @@ -2646,7 +2649,8 @@ static int vfio_initfn(PCIDevice *pdev) >> >> ret = vfio_msix_early_setup(vdev); >> if (ret) { >> - return ret; >> + error_setg_errno(errp, ret, "msix early setup failure"); >> + goto error; >> } >> >> vfio_bars_setup(vdev); >> @@ -2669,9 +2673,9 @@ static int vfio_initfn(PCIDevice *pdev) >> struct vfio_region_info *opregion; >> >> if (vdev->pdev.qdev.hotplugged) { >> - error_report("Cannot support IGD OpRegion feature on hotplugged " >> - "device %s", vdev->vbasedev.name); >> - ret = -EINVAL; >> + error_setg_errno(errp, -EINVAL, >> + "cannot support IGD OpRegion feature on hotplugged " >> + "device"); >> goto out_teardown; >> } >> >> @@ -2679,16 +2683,15 @@ static int vfio_initfn(PCIDevice *pdev) >> VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL, >> VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion); >> if (ret) { >> - error_report("Device %s does not support requested IGD OpRegion " >> - "feature", vdev->vbasedev.name); >> + error_setg_errno(errp, ret, >> + "does not support requested IGD OpRegion feature"); >> goto out_teardown; >> } >> >> ret = vfio_pci_igd_opregion_init(vdev, opregion); >> g_free(opregion); >> if (ret) { >> - error_report("Device %s IGD OpRegion initialization failed", >> - vdev->vbasedev.name); >> + error_setg_errno(errp, ret, "IGD OpRegion initialization failed"); >> goto out_teardown; >> } >> } >> @@ -2710,6 +2713,7 @@ static int vfio_initfn(PCIDevice *pdev) >> pci_device_set_intx_routing_notifier(&vdev->pdev, vfio_intx_update); >> ret = vfio_intx_enable(vdev); >> if (ret) { >> + error_setg_errno(errp, ret, "failed to enable intx"); >> goto out_teardown; >> } >> } >> @@ -2718,13 +2722,14 @@ static int vfio_initfn(PCIDevice *pdev) >> vfio_register_req_notifier(vdev); >> vfio_setup_resetfn_quirk(vdev); >> >> - return 0; >> + return; >> >> out_teardown: >> pci_device_set_intx_routing_notifier(&vdev->pdev, NULL); >> vfio_teardown_msi(vdev); >> vfio_bars_exit(vdev); >> - return ret; >> +error: >> + error_prepend(errp, "vfio: %s: ", vdev->vbasedev.name); >> } >> >> static void vfio_instance_finalize(Object *obj) >> @@ -2853,7 +2858,7 @@ static void vfio_pci_dev_class_init(ObjectClass *klass, void *data) >> dc->vmsd = &vfio_pci_vmstate; >> dc->desc = "VFIO-based PCI device assignment"; >> set_bit(DEVICE_CATEGORY_MISC, dc->categories); >> - pdc->init = vfio_initfn; >> + pdc->realize = vfio_realize; >> pdc->exit = vfio_exitfn; >> pdc->config_read = vfio_pci_read_config; >> pdc->config_write = vfio_pci_write_config; >> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events >> index 4bb7690..14e3fc4 100644 >> --- a/hw/vfio/trace-events >> +++ b/hw/vfio/trace-events >> @@ -36,7 +36,7 @@ vfio_pci_hot_reset_dep_devices(int domain, int bus, int slot, int function, int >> vfio_pci_hot_reset_result(const char *name, const char *result) "%s hot reset: %s" >> vfio_populate_device_config(const char *name, unsigned long size, unsigned long offset, unsigned long flags) "Device %s config:\n size: 0x%lx, offset: 0x%lx, flags: 0x%lx" >> vfio_populate_device_get_irq_info_failure(void) "VFIO_DEVICE_GET_IRQ_INFO failure: %m" >> -vfio_initfn(const char *name, int group_id) " (%s) group %d" >> +vfio_realize(const char *name, int group_id) " (%s) group %d" >> vfio_add_ext_cap_dropped(const char *name, uint16_t cap, uint16_t offset) "%s %x@%x" >> vfio_pci_reset(const char *name) " (%s)" >> vfio_pci_reset_flr(const char *name) "%s FLR/VFIO_DEVICE_RESET" > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-12 14:00 ` Auger Eric @ 2016-09-12 16:17 ` Alex Williamson 2016-09-12 19:39 ` Auger Eric 2016-09-13 6:25 ` Markus Armbruster 0 siblings, 2 replies; 19+ messages in thread From: Alex Williamson @ 2016-09-12 16:17 UTC (permalink / raw) To: Auger Eric; +Cc: Markus Armbruster, qemu-devel, eric.auger.pro On Mon, 12 Sep 2016 16:00:18 +0200 Auger Eric <eric.auger@redhat.com> wrote: > Hi Markus, > > On 12/09/2016 14:45, Markus Armbruster wrote: > > Eric Auger <eric.auger@redhat.com> 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. > > > > Always, always, always test your error messages :) > > > >> Subsequent patches will pass the error objects to > >> - vfio_populate_device, > >> - vfio_msix_early_setup. > >> > >> At this point if those functions fail let's just report an error > >> at realize level. > >> > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >> --- > >> hw/vfio/pci.c | 81 ++++++++++++++++++++++++++++------------------------ > >> hw/vfio/trace-events | 2 +- > >> 2 files changed, 44 insertions(+), 39 deletions(-) > >> > >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > >> index 7bfa17c..ae1967c 100644 > >> --- a/hw/vfio/pci.c > >> +++ b/hw/vfio/pci.c > >> @@ -2485,7 +2485,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) > >> vdev->req_enabled = false; > >> } > >> > >> -static int vfio_initfn(PCIDevice *pdev) > >> +static void vfio_realize(PCIDevice *pdev, Error **errp) > >> { > >> VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); > >> VFIODevice *vbasedev_iter; > >> @@ -2504,9 +2504,9 @@ static int vfio_initfn(PCIDevice *pdev) > >> } > >> > >> if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { > >> - error_report("vfio: error: no such host device: %s", > >> - vdev->vbasedev.sysfsdev); > >> - return -errno; > >> + error_setg_errno(errp, -errno, "vfio: error: no such host device: %s", > > > > error_setg_errno() takes a *positive* errno. Same elsewhere. > OK thanks! > > > >> + vdev->vbasedev.sysfsdev); > >> + return; > >> } > >> > >> vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev)); > >> @@ -2518,45 +2518,46 @@ static int vfio_initfn(PCIDevice *pdev) > >> g_free(tmp); > >> > >> if (len <= 0 || len >= sizeof(group_path)) { > >> - error_report("vfio: error no iommu_group for device"); > >> - return len < 0 ? -errno : -ENAMETOOLONG; > >> + error_setg_errno(errp, len < 0 ? -errno : -ENAMETOOLONG, > >> + "no iommu_group found"); > >> + goto error; > >> } > >> > >> group_path[len] = 0; > >> > >> group_name = basename(group_path); > >> if (sscanf(group_name, "%d", &groupid) != 1) { > >> - error_report("vfio: error reading %s: %m", group_path); > >> - return -errno; > >> + error_setg_errno(errp, -errno, "failed to read %s", group_path); > >> + goto error; > >> } > >> > >> - trace_vfio_initfn(vdev->vbasedev.name, groupid); > >> + trace_vfio_realize(vdev->vbasedev.name, groupid); > >> > >> group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev)); > >> if (!group) { > >> - error_report("vfio: failed to get group %d", groupid); > >> - return -ENOENT; > >> + error_setg_errno(errp, -ENOENT, "failed to get group %d", groupid); > >> + goto error; > > > > I understand this is a mechanical conversion, but are you sure the ": No > > such file or directory" suffix we get from passing ENOENT buys us > > anything? More of the same below. > At that stage I prefered to stick to the original messages since Alex & > VFIO users may have their habits. ENOENT may be a relic from previous conversions. In general I have no attachment to any of our error messages. I might pay more attention to tracing since I definitely don't want to lose functionality there, but for errors, so long as it's unique and descriptive, please update as you see fit. You can always use google to see how common a particular error is and whether significantly rewording it could further confuse or help users. Thanks, Alex ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-12 16:17 ` Alex Williamson @ 2016-09-12 19:39 ` Auger Eric 2016-09-12 20:05 ` Alex Williamson 2016-09-13 6:25 ` Markus Armbruster 1 sibling, 1 reply; 19+ messages in thread From: Auger Eric @ 2016-09-12 19:39 UTC (permalink / raw) To: Alex Williamson; +Cc: Markus Armbruster, qemu-devel, eric.auger.pro Hi, On 12/09/2016 18:17, Alex Williamson wrote: > On Mon, 12 Sep 2016 16:00:18 +0200 > Auger Eric <eric.auger@redhat.com> wrote: > >> Hi Markus, >> >> On 12/09/2016 14:45, Markus Armbruster wrote: >>> Eric Auger <eric.auger@redhat.com> 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. On my side I would be inclined to keep the "vfio: BDF" prefix. Maybe the PCI domain may be omitted? > >>> >>> Always, always, always test your error messages :) >>> >>>> Subsequent patches will pass the error objects to >>>> - vfio_populate_device, >>>> - vfio_msix_early_setup. >>>> >>>> At this point if those functions fail let's just report an error >>>> at realize level. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> --- >>>> hw/vfio/pci.c | 81 ++++++++++++++++++++++++++++------------------------ >>>> hw/vfio/trace-events | 2 +- >>>> 2 files changed, 44 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>> index 7bfa17c..ae1967c 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -2485,7 +2485,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev) >>>> vdev->req_enabled = false; >>>> } >>>> >>>> -static int vfio_initfn(PCIDevice *pdev) >>>> +static void vfio_realize(PCIDevice *pdev, Error **errp) >>>> { >>>> VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev); >>>> VFIODevice *vbasedev_iter; >>>> @@ -2504,9 +2504,9 @@ static int vfio_initfn(PCIDevice *pdev) >>>> } >>>> >>>> if (stat(vdev->vbasedev.sysfsdev, &st) < 0) { >>>> - error_report("vfio: error: no such host device: %s", >>>> - vdev->vbasedev.sysfsdev); >>>> - return -errno; >>>> + error_setg_errno(errp, -errno, "vfio: error: no such host device: %s", >>> >>> error_setg_errno() takes a *positive* errno. Same elsewhere. >> OK thanks! >>> >>>> + vdev->vbasedev.sysfsdev); >>>> + return; >>>> } >>>> >>>> vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev)); >>>> @@ -2518,45 +2518,46 @@ static int vfio_initfn(PCIDevice *pdev) >>>> g_free(tmp); >>>> >>>> if (len <= 0 || len >= sizeof(group_path)) { >>>> - error_report("vfio: error no iommu_group for device"); >>>> - return len < 0 ? -errno : -ENAMETOOLONG; >>>> + error_setg_errno(errp, len < 0 ? -errno : -ENAMETOOLONG, >>>> + "no iommu_group found"); >>>> + goto error; >>>> } >>>> >>>> group_path[len] = 0; >>>> >>>> group_name = basename(group_path); >>>> if (sscanf(group_name, "%d", &groupid) != 1) { >>>> - error_report("vfio: error reading %s: %m", group_path); >>>> - return -errno; >>>> + error_setg_errno(errp, -errno, "failed to read %s", group_path); >>>> + goto error; >>>> } >>>> >>>> - trace_vfio_initfn(vdev->vbasedev.name, groupid); >>>> + trace_vfio_realize(vdev->vbasedev.name, groupid); >>>> >>>> group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev)); >>>> if (!group) { >>>> - error_report("vfio: failed to get group %d", groupid); >>>> - return -ENOENT; >>>> + error_setg_errno(errp, -ENOENT, "failed to get group %d", groupid); >>>> + goto error; >>> >>> I understand this is a mechanical conversion, but are you sure the ": No >>> such file or directory" suffix we get from passing ENOENT buys us >>> anything? More of the same below. >> At that stage I prefered to stick to the original messages since Alex & >> VFIO users may have their habits. > > ENOENT may be a relic from previous conversions. In general I have no > attachment to any of our error messages. I might pay more attention to > tracing since I definitely don't want to lose functionality there, but > for errors, so long as it's unique and descriptive, please update as > you see fit. You can always use google to see how common a particular > error is and whether significantly rewording it could further confuse > or help users. Thanks, OK in that case I will try to improve whenever it makes sense. Thanks Eric > > Alex > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-12 19:39 ` Auger Eric @ 2016-09-12 20:05 ` Alex Williamson 2016-09-12 20:51 ` Auger Eric 0 siblings, 1 reply; 19+ messages in thread From: Alex Williamson @ 2016-09-12 20:05 UTC (permalink / raw) To: Auger Eric; +Cc: Markus Armbruster, qemu-devel, eric.auger.pro On Mon, 12 Sep 2016 21:39:22 +0200 Auger Eric <eric.auger@redhat.com> wrote: > Hi, > On 12/09/2016 18:17, Alex Williamson wrote: > > On Mon, 12 Sep 2016 16:00:18 +0200 > > Auger Eric <eric.auger@redhat.com> wrote: > > > >> Hi Markus, > >> > >> On 12/09/2016 14:45, Markus Armbruster wrote: > >>> Eric Auger <eric.auger@redhat.com> 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. > On my side I would be inclined to keep the "vfio: BDF" prefix. Maybe the > PCI domain may be omitted? I don't really see the point of making the device name smaller. If a user happens to have multiple domains, they're going to care about that component of the address. Is QEMU going to probe the host system to see if multiple domains are available and update if a new PCI chassis handled as a separate domain is hot attached? Even an approach like only printing the domain if it's non-zero devolves into needing logic to know that basename is a PCI path and not a random sysfs device path. And then if we only print the domain when non-zero, what about the bus number or slot number. It's a lot of logic for a problem that I'm not even convinced is a problem. Thanks, Alex ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-12 20:05 ` Alex Williamson @ 2016-09-12 20:51 ` Auger Eric 0 siblings, 0 replies; 19+ messages in thread From: Auger Eric @ 2016-09-12 20:51 UTC (permalink / raw) To: Alex Williamson; +Cc: eric.auger.pro, Markus Armbruster, qemu-devel Hi, On 12/09/2016 22:05, Alex Williamson wrote: > On Mon, 12 Sep 2016 21:39:22 +0200 > Auger Eric <eric.auger@redhat.com> wrote: > >> Hi, >> On 12/09/2016 18:17, Alex Williamson wrote: >>> On Mon, 12 Sep 2016 16:00:18 +0200 >>> Auger Eric <eric.auger@redhat.com> wrote: >>> >>>> Hi Markus, >>>> >>>> On 12/09/2016 14:45, Markus Armbruster wrote: >>>>> Eric Auger <eric.auger@redhat.com> 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. >> On my side I would be inclined to keep the "vfio: BDF" prefix. Maybe the >> PCI domain may be omitted? > > I don't really see the point of making the device name smaller. If a > user happens to have multiple domains, they're going to care about that > component of the address. Is QEMU going to probe the host system to > see if multiple domains are available and update if a new PCI chassis > handled as a separate domain is hot attached? Even an approach like > only printing the domain if it's non-zero devolves into needing logic > to know that basename is a PCI path and not a random sysfs device > path. And then if we only print the domain when non-zero, what about > the bus number or slot number. It's a lot of logic for a problem that > I'm not even convinced is a problem. Thanks, I tend to agree. So I will keep the prefix as is and take into account Markus' other comments. Thanks! Eric > > Alex > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-12 16:17 ` Alex Williamson 2016-09-12 19:39 ` Auger Eric @ 2016-09-13 6:25 ` Markus Armbruster 2016-09-13 7:21 ` Auger Eric 1 sibling, 1 reply; 19+ messages in thread From: Markus Armbruster @ 2016-09-13 6:25 UTC (permalink / raw) To: Alex Williamson; +Cc: Auger Eric, eric.auger.pro, qemu-devel Alex Williamson <alex.williamson@redhat.com> writes: > On Mon, 12 Sep 2016 16:00:18 +0200 > Auger Eric <eric.auger@redhat.com> wrote: > >> Hi Markus, >> >> On 12/09/2016 14:45, Markus Armbruster wrote: >> > Eric Auger <eric.auger@redhat.com> 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. 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. >> > 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. [...] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-13 6:25 ` Markus Armbruster @ 2016-09-13 7:21 ` Auger Eric 2016-09-13 15:03 ` Alex Williamson 0 siblings, 1 reply; 19+ messages in thread From: Auger Eric @ 2016-09-13 7:21 UTC (permalink / raw) To: Markus Armbruster, Alex Williamson; +Cc: eric.auger.pro, qemu-devel Hi Markus, On 13/09/2016 08:25, Markus Armbruster wrote: > Alex Williamson <alex.williamson@redhat.com> writes: > >> On Mon, 12 Sep 2016 16:00:18 +0200 >> Auger Eric <eric.auger@redhat.com> wrote: >> >>> Hi Markus, >>> >>> On 12/09/2016 14:45, Markus Armbruster wrote: >>>> Eric Auger <eric.auger@redhat.com> 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. > > 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. 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 > > [...] > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] vfio/pci: conversion to realize 2016-09-13 7:21 ` Auger Eric @ 2016-09-13 15:03 ` Alex Williamson 0 siblings, 0 replies; 19+ messages in thread From: Alex Williamson @ 2016-09-13 15:03 UTC (permalink / raw) To: Auger Eric; +Cc: Markus Armbruster, eric.auger.pro, qemu-devel On Tue, 13 Sep 2016 09:21:17 +0200 Auger Eric <eric.auger@redhat.com> wrote: > Hi Markus, > > On 13/09/2016 08:25, Markus Armbruster wrote: > > Alex Williamson <alex.williamson@redhat.com> writes: > > > >> On Mon, 12 Sep 2016 16:00:18 +0200 > >> Auger Eric <eric.auger@redhat.com> wrote: > >> > >>> Hi Markus, > >>> > >>> On 12/09/2016 14:45, Markus Armbruster wrote: > >>>> Eric Auger <eric.auger@redhat.com> 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 > > > > [...] > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device 2016-09-06 7:11 [Qemu-devel] [PATCH 0/3] Convert VFIO-PCI to realize Eric Auger 2016-09-06 7:11 ` [Qemu-devel] [PATCH 1/3] vfio/pci: conversion " Eric Auger @ 2016-09-06 7:11 ` Eric Auger 2016-09-12 12:51 ` Markus Armbruster 2016-09-06 7:11 ` [Qemu-devel] [PATCH 3/3] vfio/pci: pass an error object to vfio_msix_early_setup Eric Auger 2 siblings, 1 reply; 19+ messages in thread From: Eric Auger @ 2016-09-06 7:11 UTC (permalink / raw) To: eric.auger, eric.auger.pro, qemu-devel, alex.williamson, armbru Let's expand the usage of QEMU Error objects to vfio_populate_device. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- hw/vfio/pci.c | 45 ++++++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index ae1967c..f7768e9 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2197,7 +2197,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev) return 0; } -static int vfio_populate_device(VFIOPCIDevice *vdev) +static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) { VFIODevice *vbasedev = &vdev->vbasedev; struct vfio_region_info *reg_info; @@ -2206,19 +2206,19 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) /* Sanity check device */ if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) { - error_report("vfio: Um, this isn't a PCI device"); - goto error; + error_setg(errp, "this isn't a PCI device"); + return; } if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) { - error_report("vfio: unexpected number of io regions %u", - vbasedev->num_regions); - goto error; + error_setg(errp, "unexpected number of io regions %u", + vbasedev->num_regions); + return; } if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) { - error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs); - goto error; + error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs); + return; } for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) { @@ -2229,8 +2229,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) g_free(name); if (ret) { - error_report("vfio: Error getting region %d info: %m", i); - goto error; + error_setg_errno(errp, ret, "failed to get region %d info", i); + return; } QLIST_INIT(&vdev->bars[i].quirks); @@ -2239,8 +2239,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) ret = vfio_get_region_info(vbasedev, VFIO_PCI_CONFIG_REGION_INDEX, ®_info); if (ret) { - error_report("vfio: Error getting config info: %m"); - goto error; + error_setg_errno(errp, ret, "failed to get config info"); + return; } trace_vfio_populate_device_config(vdev->vbasedev.name, @@ -2259,9 +2259,9 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) if (vdev->features & VFIO_FEATURE_ENABLE_VGA) { ret = vfio_populate_vga(vdev); if (ret) { - error_report( - "vfio: Device does not support requested feature x-vga"); - goto error; + error_setg_errno(errp, ret, + "device does not support requested feature x-vga"); + return; } } @@ -2271,17 +2271,11 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) if (ret) { /* This can fail for an old kernel or legacy PCI dev */ trace_vfio_populate_device_get_irq_info_failure(); - ret = 0; } else if (irq_info.count == 1) { vdev->pci_aer = true; } else { - error_report("vfio: %s " - "Could not enable error recovery for the device", - vbasedev->name); + error_setg_errno(errp, ret, "could not enable error recovery"); } - -error: - return ret; } static void vfio_put_device(VFIOPCIDevice *vdev) @@ -2491,6 +2485,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) VFIODevice *vbasedev_iter; VFIOGroup *group; char *tmp, group_path[PATH_MAX], *group_name; + Error *err = NULL; ssize_t len; struct stat st; int groupid; @@ -2554,9 +2549,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) goto error; } - ret = vfio_populate_device(vdev); - if (ret) { - error_setg_errno(errp, ret, "failed to populate the device"); + vfio_populate_device(vdev, &err); + if (err) { + error_propagate(errp, err); goto error; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device 2016-09-06 7:11 ` [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device Eric Auger @ 2016-09-12 12:51 ` Markus Armbruster 2016-09-12 15:15 ` Auger Eric 0 siblings, 1 reply; 19+ messages in thread From: Markus Armbruster @ 2016-09-12 12:51 UTC (permalink / raw) To: Eric Auger; +Cc: eric.auger.pro, qemu-devel, alex.williamson Eric Auger <eric.auger@redhat.com> writes: > Let's expand the usage of QEMU Error objects to vfio_populate_device. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > hw/vfio/pci.c | 45 ++++++++++++++++++++------------------------- > 1 file changed, 20 insertions(+), 25 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index ae1967c..f7768e9 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2197,7 +2197,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev) > return 0; > } > > -static int vfio_populate_device(VFIOPCIDevice *vdev) > +static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) > { > VFIODevice *vbasedev = &vdev->vbasedev; > struct vfio_region_info *reg_info; struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) }; int i, ret = -1; > @@ -2206,19 +2206,19 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) > > /* Sanity check device */ > if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) { > - error_report("vfio: Um, this isn't a PCI device"); > - goto error; > + error_setg(errp, "this isn't a PCI device"); > + return; This is actually a bug fix :) Before your series, vfio_populate_device() returns negative errno on some errors, and -1 on others. Its caller expects the former. Please mention the fix in the commit message. Fixing it in a separate commit would also be fine, and possibly clearer. > } > > if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) { > - error_report("vfio: unexpected number of io regions %u", > - vbasedev->num_regions); > - goto error; > + error_setg(errp, "unexpected number of io regions %u", > + vbasedev->num_regions); > + return; > } > > if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) { > - error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs); > - goto error; > + error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs); > + return; > } > > for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) { > @@ -2229,8 +2229,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) > g_free(name); > > if (ret) { > - error_report("vfio: Error getting region %d info: %m", i); > - goto error; > + error_setg_errno(errp, ret, "failed to get region %d info", i); > + return; > } > > QLIST_INIT(&vdev->bars[i].quirks); > @@ -2239,8 +2239,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) > ret = vfio_get_region_info(vbasedev, > VFIO_PCI_CONFIG_REGION_INDEX, ®_info); > if (ret) { > - error_report("vfio: Error getting config info: %m"); > - goto error; > + error_setg_errno(errp, ret, "failed to get config info"); > + return; > } > > trace_vfio_populate_device_config(vdev->vbasedev.name, > @@ -2259,9 +2259,9 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) > if (vdev->features & VFIO_FEATURE_ENABLE_VGA) { > ret = vfio_populate_vga(vdev); > if (ret) { > - error_report( > - "vfio: Device does not support requested feature x-vga"); > - goto error; > + error_setg_errno(errp, ret, > + "device does not support requested feature x-vga"); > + return; > } > } > > @@ -2271,17 +2271,11 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) > if (ret) { > /* This can fail for an old kernel or legacy PCI dev */ > trace_vfio_populate_device_get_irq_info_failure(); > - ret = 0; > } else if (irq_info.count == 1) { > vdev->pci_aer = true; > } else { > - error_report("vfio: %s " > - "Could not enable error recovery for the device", > - vbasedev->name); > + error_setg_errno(errp, ret, "could not enable error recovery"); This isn't an error before this patch (ret stays zero). Your patch turns it into one. Intentional? > } > - > -error: > - return ret; > } > > static void vfio_put_device(VFIOPCIDevice *vdev) > @@ -2491,6 +2485,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > VFIODevice *vbasedev_iter; > VFIOGroup *group; > char *tmp, group_path[PATH_MAX], *group_name; > + Error *err = NULL; > ssize_t len; > struct stat st; > int groupid; > @@ -2554,9 +2549,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > goto error; > } > > - ret = vfio_populate_device(vdev); > - if (ret) { > - error_setg_errno(errp, ret, "failed to populate the device"); > + vfio_populate_device(vdev, &err); > + if (err) { > + error_propagate(errp, err); > goto error; > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device 2016-09-12 12:51 ` Markus Armbruster @ 2016-09-12 15:15 ` Auger Eric 2016-09-12 15:50 ` Markus Armbruster 0 siblings, 1 reply; 19+ messages in thread From: Auger Eric @ 2016-09-12 15:15 UTC (permalink / raw) To: Markus Armbruster; +Cc: alex.williamson, qemu-devel, eric.auger.pro Hi Markus, On 12/09/2016 14:51, Markus Armbruster wrote: > Eric Auger <eric.auger@redhat.com> writes: > >> Let's expand the usage of QEMU Error objects to vfio_populate_device. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> hw/vfio/pci.c | 45 ++++++++++++++++++++------------------------- >> 1 file changed, 20 insertions(+), 25 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index ae1967c..f7768e9 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -2197,7 +2197,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev) >> return 0; >> } >> >> -static int vfio_populate_device(VFIOPCIDevice *vdev) >> +static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) >> { >> VFIODevice *vbasedev = &vdev->vbasedev; >> struct vfio_region_info *reg_info; > struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) }; > int i, ret = -1; >> @@ -2206,19 +2206,19 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) >> >> /* Sanity check device */ >> if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) { >> - error_report("vfio: Um, this isn't a PCI device"); >> - goto error; >> + error_setg(errp, "this isn't a PCI device"); >> + return; > > This is actually a bug fix :) > > Before your series, vfio_populate_device() returns negative errno on > some errors, and -1 on others. Its caller expects the former. Sorry but I don't get your comment. Who is the caller you refer to? > > Please mention the fix in the commit message. Fixing it in a separate > commit would also be fine, and possibly clearer. > >> } >> >> if (vbasedev->num_regions < VFIO_PCI_CONFIG_REGION_INDEX + 1) { >> - error_report("vfio: unexpected number of io regions %u", >> - vbasedev->num_regions); >> - goto error; >> + error_setg(errp, "unexpected number of io regions %u", >> + vbasedev->num_regions); >> + return; >> } >> >> if (vbasedev->num_irqs < VFIO_PCI_MSIX_IRQ_INDEX + 1) { >> - error_report("vfio: unexpected number of irqs %u", vbasedev->num_irqs); >> - goto error; >> + error_setg(errp, "unexpected number of irqs %u", vbasedev->num_irqs); >> + return; >> } >> >> for (i = VFIO_PCI_BAR0_REGION_INDEX; i < VFIO_PCI_ROM_REGION_INDEX; i++) { >> @@ -2229,8 +2229,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) >> g_free(name); >> >> if (ret) { >> - error_report("vfio: Error getting region %d info: %m", i); >> - goto error; >> + error_setg_errno(errp, ret, "failed to get region %d info", i); >> + return; >> } >> >> QLIST_INIT(&vdev->bars[i].quirks); >> @@ -2239,8 +2239,8 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) >> ret = vfio_get_region_info(vbasedev, >> VFIO_PCI_CONFIG_REGION_INDEX, ®_info); >> if (ret) { >> - error_report("vfio: Error getting config info: %m"); >> - goto error; >> + error_setg_errno(errp, ret, "failed to get config info"); >> + return; >> } >> >> trace_vfio_populate_device_config(vdev->vbasedev.name, >> @@ -2259,9 +2259,9 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) >> if (vdev->features & VFIO_FEATURE_ENABLE_VGA) { >> ret = vfio_populate_vga(vdev); >> if (ret) { >> - error_report( >> - "vfio: Device does not support requested feature x-vga"); >> - goto error; >> + error_setg_errno(errp, ret, >> + "device does not support requested feature x-vga"); >> + return; >> } >> } >> >> @@ -2271,17 +2271,11 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) >> if (ret) { >> /* This can fail for an old kernel or legacy PCI dev */ >> trace_vfio_populate_device_get_irq_info_failure(); >> - ret = 0; >> } else if (irq_info.count == 1) { >> vdev->pci_aer = true; >> } else { >> - error_report("vfio: %s " >> - "Could not enable error recovery for the device", >> - vbasedev->name); >> + error_setg_errno(errp, ret, "could not enable error recovery"); > > This isn't an error before this patch (ret stays zero). Your patch > turns it into one. Intentional? Hum no thank you for spotting this bug! Thanks Eric > >> } >> - >> -error: >> - return ret; >> } >> >> static void vfio_put_device(VFIOPCIDevice *vdev) >> @@ -2491,6 +2485,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> VFIODevice *vbasedev_iter; >> VFIOGroup *group; >> char *tmp, group_path[PATH_MAX], *group_name; >> + Error *err = NULL; >> ssize_t len; >> struct stat st; >> int groupid; >> @@ -2554,9 +2549,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> goto error; >> } >> >> - ret = vfio_populate_device(vdev); >> - if (ret) { >> - error_setg_errno(errp, ret, "failed to populate the device"); >> + vfio_populate_device(vdev, &err); >> + if (err) { >> + error_propagate(errp, err); >> goto error; >> } > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device 2016-09-12 15:15 ` Auger Eric @ 2016-09-12 15:50 ` Markus Armbruster 2016-09-12 15:52 ` Auger Eric 0 siblings, 1 reply; 19+ messages in thread From: Markus Armbruster @ 2016-09-12 15:50 UTC (permalink / raw) To: Auger Eric; +Cc: alex.williamson, qemu-devel, eric.auger.pro Auger Eric <eric.auger@redhat.com> writes: > Hi Markus, > > On 12/09/2016 14:51, Markus Armbruster wrote: >> Eric Auger <eric.auger@redhat.com> writes: >> >>> Let's expand the usage of QEMU Error objects to vfio_populate_device. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> --- >>> hw/vfio/pci.c | 45 ++++++++++++++++++++------------------------- >>> 1 file changed, 20 insertions(+), 25 deletions(-) >>> >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index ae1967c..f7768e9 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -2197,7 +2197,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev) >>> return 0; >>> } >>> >>> -static int vfio_populate_device(VFIOPCIDevice *vdev) >>> +static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) >>> { >>> VFIODevice *vbasedev = &vdev->vbasedev; >>> struct vfio_region_info *reg_info; >> struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) }; >> int i, ret = -1; >>> @@ -2206,19 +2206,19 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) >>> >>> /* Sanity check device */ >>> if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) { >>> - error_report("vfio: Um, this isn't a PCI device"); >>> - goto error; >>> + error_setg(errp, "this isn't a PCI device"); >>> + return; >> >> This is actually a bug fix :) >> >> Before your series, vfio_populate_device() returns negative errno on >> some errors, and -1 on others. Its caller expects the former. > > Sorry but I don't get your comment. Who is the caller you refer to? Correction: its caller vfio_initfn() doesn't actually expect -errno. Regardless, mixing -errno and -1 like vfio_populate_device() does in master is in bad taste. So this isn't a bug fix, just a cleanup. >> Please mention the fix in the commit message. Fixing it in a separate >> commit would also be fine, and possibly clearer. Mentioning the cleanup wouldn't hurt. [...] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device 2016-09-12 15:50 ` Markus Armbruster @ 2016-09-12 15:52 ` Auger Eric 0 siblings, 0 replies; 19+ messages in thread From: Auger Eric @ 2016-09-12 15:52 UTC (permalink / raw) To: Markus Armbruster; +Cc: alex.williamson, qemu-devel, eric.auger.pro Hi Markus, On 12/09/2016 17:50, Markus Armbruster wrote: > Auger Eric <eric.auger@redhat.com> writes: > >> Hi Markus, >> >> On 12/09/2016 14:51, Markus Armbruster wrote: >>> Eric Auger <eric.auger@redhat.com> writes: >>> >>>> Let's expand the usage of QEMU Error objects to vfio_populate_device. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> --- >>>> hw/vfio/pci.c | 45 ++++++++++++++++++++------------------------- >>>> 1 file changed, 20 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>> index ae1967c..f7768e9 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -2197,7 +2197,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev) >>>> return 0; >>>> } >>>> >>>> -static int vfio_populate_device(VFIOPCIDevice *vdev) >>>> +static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp) >>>> { >>>> VFIODevice *vbasedev = &vdev->vbasedev; >>>> struct vfio_region_info *reg_info; >>> struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) }; >>> int i, ret = -1; >>>> @@ -2206,19 +2206,19 @@ static int vfio_populate_device(VFIOPCIDevice *vdev) >>>> >>>> /* Sanity check device */ >>>> if (!(vbasedev->flags & VFIO_DEVICE_FLAGS_PCI)) { >>>> - error_report("vfio: Um, this isn't a PCI device"); >>>> - goto error; >>>> + error_setg(errp, "this isn't a PCI device"); >>>> + return; >>> >>> This is actually a bug fix :) >>> >>> Before your series, vfio_populate_device() returns negative errno on >>> some errors, and -1 on others. Its caller expects the former. >> >> Sorry but I don't get your comment. Who is the caller you refer to? > > Correction: its caller vfio_initfn() doesn't actually expect -errno. > Regardless, mixing -errno and -1 like vfio_populate_device() does in > master is in bad taste. So this isn't a bug fix, just a cleanup. > >>> Please mention the fix in the commit message. Fixing it in a separate >>> commit would also be fine, and possibly clearer. > > Mentioning the cleanup wouldn't hurt. OK Thanks! Eric > > [...] > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH 3/3] vfio/pci: pass an error object to vfio_msix_early_setup 2016-09-06 7:11 [Qemu-devel] [PATCH 0/3] Convert VFIO-PCI to realize Eric Auger 2016-09-06 7:11 ` [Qemu-devel] [PATCH 1/3] vfio/pci: conversion " Eric Auger 2016-09-06 7:11 ` [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device Eric Auger @ 2016-09-06 7:11 ` Eric Auger 2016-09-12 12:58 ` Markus Armbruster 2 siblings, 1 reply; 19+ messages in thread From: Eric Auger @ 2016-09-06 7:11 UTC (permalink / raw) To: eric.auger, eric.auger.pro, qemu-devel, alex.williamson, armbru Let's expand the usage of QEMU Error objects to vfio_msix_early_setup. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- hw/vfio/pci.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index f7768e9..b132bf1 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1275,7 +1275,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev) * need to first look for where the MSI-X table lives. So we * unfortunately split MSI-X setup across two functions. */ -static int vfio_msix_early_setup(VFIOPCIDevice *vdev) +static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) { uint8_t pos; uint16_t ctrl; @@ -1285,22 +1285,25 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX); if (!pos) { - return 0; + return; } if (pread(fd, &ctrl, sizeof(ctrl), vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) { - return -errno; + error_setg_errno(errp, -errno, "failed to read PCI MSIX FLAGS"); + return; } if (pread(fd, &table, sizeof(table), vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) { - return -errno; + error_setg_errno(errp, -errno, "failed to read PCI MSIX TABLE"); + return; } if (pread(fd, &pba, sizeof(pba), vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) { - return -errno; + error_setg_errno(errp, -errno, "failed to read PCI MSIX PBA"); + return; } ctrl = le16_to_cpu(ctrl); @@ -1330,10 +1333,11 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) (vdev->device_id & 0xff00) == 0x5800) { msix->pba_offset = 0x1000; } else { - error_report("vfio: Hardware reports invalid configuration, " - "MSIX PBA outside of specified BAR"); + error_setg_errno(errp, -EINVAL, + "hardware reports invalid configuration, " + "MSIX PBA outside of specified BAR"); g_free(msix); - return -EINVAL; + return; } } @@ -1343,7 +1347,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) vfio_pci_fixup_msix_region(vdev); - return 0; + return; } static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) @@ -2642,9 +2646,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) vfio_pci_size_rom(vdev); - ret = vfio_msix_early_setup(vdev); - if (ret) { - error_setg_errno(errp, ret, "msix early setup failure"); + vfio_msix_early_setup(vdev, &err); + if (err) { + error_propagate(errp, err); goto error; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] vfio/pci: pass an error object to vfio_msix_early_setup 2016-09-06 7:11 ` [Qemu-devel] [PATCH 3/3] vfio/pci: pass an error object to vfio_msix_early_setup Eric Auger @ 2016-09-12 12:58 ` Markus Armbruster 2016-09-12 15:15 ` Auger Eric 0 siblings, 1 reply; 19+ messages in thread From: Markus Armbruster @ 2016-09-12 12:58 UTC (permalink / raw) To: Eric Auger; +Cc: eric.auger.pro, qemu-devel, alex.williamson Eric Auger <eric.auger@redhat.com> writes: > Let's expand the usage of QEMU Error objects to vfio_msix_early_setup. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > hw/vfio/pci.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index f7768e9..b132bf1 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1275,7 +1275,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev) > * need to first look for where the MSI-X table lives. So we > * unfortunately split MSI-X setup across two functions. > */ > -static int vfio_msix_early_setup(VFIOPCIDevice *vdev) > +static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) > { > uint8_t pos; > uint16_t ctrl; > @@ -1285,22 +1285,25 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) > > pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX); > if (!pos) { > - return 0; > + return; > } > > if (pread(fd, &ctrl, sizeof(ctrl), > vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) { > - return -errno; > + error_setg_errno(errp, -errno, "failed to read PCI MSIX FLAGS"); > + return; Looks like a bug fix to me: before your series, this is a silent failure. PATCH 1 makes it non-silent, with a generic error message, and this patch replaces it with a specific error message. > } > > if (pread(fd, &table, sizeof(table), > vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) { > - return -errno; > + error_setg_errno(errp, -errno, "failed to read PCI MSIX TABLE"); > + return; Likewise. > } > > if (pread(fd, &pba, sizeof(pba), > vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) { > - return -errno; > + error_setg_errno(errp, -errno, "failed to read PCI MSIX PBA"); > + return; Likewise. > } > > ctrl = le16_to_cpu(ctrl); > @@ -1330,10 +1333,11 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) > (vdev->device_id & 0xff00) == 0x5800) { > msix->pba_offset = 0x1000; > } else { > - error_report("vfio: Hardware reports invalid configuration, " > - "MSIX PBA outside of specified BAR"); > + error_setg_errno(errp, -EINVAL, > + "hardware reports invalid configuration, " > + "MSIX PBA outside of specified BAR"); > g_free(msix); > - return -EINVAL; > + return; > } > } > > @@ -1343,7 +1347,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) > > vfio_pci_fixup_msix_region(vdev); > > - return 0; > + return; > } > > static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) > @@ -2642,9 +2646,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > vfio_pci_size_rom(vdev); > > - ret = vfio_msix_early_setup(vdev); > - if (ret) { > - error_setg_errno(errp, ret, "msix early setup failure"); > + vfio_msix_early_setup(vdev, &err); > + if (err) { > + error_propagate(errp, err); > goto error; > } vfio_realize() still calls a few functions that report errors with error_report(), including vfio_get_group(), vfio_get_device(), vfio_pci_igd_opregion_init(), vfio_intx_enable(). If you prefer not to complete the job right now, please mark such calls FIXME. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] vfio/pci: pass an error object to vfio_msix_early_setup 2016-09-12 12:58 ` Markus Armbruster @ 2016-09-12 15:15 ` Auger Eric 0 siblings, 0 replies; 19+ messages in thread From: Auger Eric @ 2016-09-12 15:15 UTC (permalink / raw) To: Markus Armbruster; +Cc: alex.williamson, qemu-devel, eric.auger.pro Hi Markus, On 12/09/2016 14:58, Markus Armbruster wrote: > Eric Auger <eric.auger@redhat.com> writes: > >> Let's expand the usage of QEMU Error objects to vfio_msix_early_setup. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> hw/vfio/pci.c | 28 ++++++++++++++++------------ >> 1 file changed, 16 insertions(+), 12 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index f7768e9..b132bf1 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1275,7 +1275,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev) >> * need to first look for where the MSI-X table lives. So we >> * unfortunately split MSI-X setup across two functions. >> */ >> -static int vfio_msix_early_setup(VFIOPCIDevice *vdev) >> +static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) >> { >> uint8_t pos; >> uint16_t ctrl; >> @@ -1285,22 +1285,25 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) >> >> pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX); >> if (!pos) { >> - return 0; >> + return; >> } >> >> if (pread(fd, &ctrl, sizeof(ctrl), >> vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) { >> - return -errno; >> + error_setg_errno(errp, -errno, "failed to read PCI MSIX FLAGS"); >> + return; > > Looks like a bug fix to me: before your series, this is a silent > failure. PATCH 1 makes it non-silent, with a generic error message, and > this patch replaces it with a specific error message. Actually the init process was stopped without reporting the actual issue. But that's the goal of the series. I will improve the commit message. > >> } >> >> if (pread(fd, &table, sizeof(table), >> vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) { >> - return -errno; >> + error_setg_errno(errp, -errno, "failed to read PCI MSIX TABLE"); >> + return; > > Likewise. > >> } >> >> if (pread(fd, &pba, sizeof(pba), >> vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) { >> - return -errno; >> + error_setg_errno(errp, -errno, "failed to read PCI MSIX PBA"); >> + return; > > Likewise. > >> } >> >> ctrl = le16_to_cpu(ctrl); >> @@ -1330,10 +1333,11 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) >> (vdev->device_id & 0xff00) == 0x5800) { >> msix->pba_offset = 0x1000; >> } else { >> - error_report("vfio: Hardware reports invalid configuration, " >> - "MSIX PBA outside of specified BAR"); >> + error_setg_errno(errp, -EINVAL, >> + "hardware reports invalid configuration, " >> + "MSIX PBA outside of specified BAR"); >> g_free(msix); >> - return -EINVAL; >> + return; >> } >> } >> >> @@ -1343,7 +1347,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) >> >> vfio_pci_fixup_msix_region(vdev); >> >> - return 0; >> + return; >> } >> >> static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) >> @@ -2642,9 +2646,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> >> vfio_pci_size_rom(vdev); >> >> - ret = vfio_msix_early_setup(vdev); >> - if (ret) { >> - error_setg_errno(errp, ret, "msix early setup failure"); >> + vfio_msix_early_setup(vdev, &err); >> + if (err) { >> + error_propagate(errp, err); >> goto error; >> } > > vfio_realize() still calls a few functions that report errors with > error_report(), including vfio_get_group(), vfio_get_device(), > vfio_pci_igd_opregion_init(), vfio_intx_enable(). If you prefer not to > complete the job right now, please mark such calls FIXME. I will complete the job but I just wanted to get some first feedbacks. Hopefully you already pointed out some issues! Thank you for your time. Best Regards Eric > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-09-13 15:04 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-06 7:11 [Qemu-devel] [PATCH 0/3] Convert VFIO-PCI to realize Eric Auger 2016-09-06 7:11 ` [Qemu-devel] [PATCH 1/3] vfio/pci: conversion " Eric Auger 2016-09-12 12:45 ` Markus Armbruster 2016-09-12 14:00 ` Auger Eric 2016-09-12 16:17 ` Alex Williamson 2016-09-12 19:39 ` Auger Eric 2016-09-12 20:05 ` Alex Williamson 2016-09-12 20:51 ` Auger Eric 2016-09-13 6:25 ` Markus Armbruster 2016-09-13 7:21 ` Auger Eric 2016-09-13 15:03 ` Alex Williamson 2016-09-06 7:11 ` [Qemu-devel] [PATCH 2/3] vfio/pci: pass an error object to vfio_populate_device Eric Auger 2016-09-12 12:51 ` Markus Armbruster 2016-09-12 15:15 ` Auger Eric 2016-09-12 15:50 ` Markus Armbruster 2016-09-12 15:52 ` Auger Eric 2016-09-06 7:11 ` [Qemu-devel] [PATCH 3/3] vfio/pci: pass an error object to vfio_msix_early_setup Eric Auger 2016-09-12 12:58 ` Markus Armbruster 2016-09-12 15:15 ` Auger Eric
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.