| Message ID | 1473145893-17088-2-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
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"
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" >
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
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 >
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
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 >
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. [...]
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 > > [...] >
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 > > > > [...] > >
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"
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(-)