* [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
* [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
* [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 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 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 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 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 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 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
* 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
* 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
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.