rom_bar is tristate but was defined as uint32_t so convert it into OnOffAuto to clarify that. For compatibility, a uint32 value set via QOM will be converted into OnOffAuto. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- Akihiko Odaki (4): qapi: Add visit_type_str_preserving() qapi: Do not consume a value when visit_type_enum() fails hw/pci: Convert rom_bar into OnOffAuto hw/qdev: Remove opts member docs/igd-assign.txt | 2 +- include/hw/pci/pci_device.h | 2 +- include/hw/qdev-core.h | 4 --- include/qapi/visitor-impl.h | 3 ++- include/qapi/visitor.h | 25 +++++++++++++---- hw/core/qdev.c | 1 - hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++-- hw/vfio/pci-quirks.c | 2 +- hw/vfio/pci.c | 11 ++++---- hw/xen/xen_pt_load_rom.c | 4 +-- qapi/opts-visitor.c | 12 ++++----- qapi/qapi-clone-visitor.c | 2 +- qapi/qapi-dealloc-visitor.c | 4 +-- qapi/qapi-forward-visitor.c | 4 ++- qapi/qapi-visit-core.c | 21 ++++++++++++--- qapi/qobject-input-visitor.c | 23 ++++++++-------- qapi/qobject-output-visitor.c | 2 +- qapi/string-input-visitor.c | 2 +- qapi/string-output-visitor.c | 2 +- system/qdev-monitor.c | 12 +++++---- tests/qtest/virtio-net-failover.c | 32 +++++++++++----------- 21 files changed, 154 insertions(+), 73 deletions(-) --- base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6 change-id: 20240704-rombar-1a4ba2890d6c Best regards, -- Akihiko Odaki <akihiko.odaki@daynix.com> visit_type_str_preserving() is mostly indentical with visit_type_str() but leaves the value intact. This is useful when the caller may interpret the value with a different type. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/qapi/visitor-impl.h | 3 ++- include/qapi/visitor.h | 20 ++++++++++++++++++++ qapi/opts-visitor.c | 7 +++++-- qapi/qapi-clone-visitor.c | 2 +- qapi/qapi-dealloc-visitor.c | 4 ++-- qapi/qapi-forward-visitor.c | 4 +++- qapi/qapi-visit-core.c | 17 +++++++++++++++-- qapi/qobject-input-visitor.c | 23 ++++++++++++----------- qapi/qobject-output-visitor.c | 2 +- qapi/string-input-visitor.c | 2 +- qapi/string-output-visitor.c | 2 +- 11 files changed, 63 insertions(+), 23 deletions(-) diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index XXXXXXX..XXXXXXX 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -XXX,XX +XXX,XX @@ struct Visitor bool (*type_bool)(Visitor *v, const char *name, bool *obj, Error **errp); /* Must be set */ - bool (*type_str)(Visitor *v, const char *name, char **obj, Error **errp); + bool (*type_str)(Visitor *v, const char *name, char **obj, bool consume, + Error **errp); /* Must be set to visit numbers */ bool (*type_number)(Visitor *v, const char *name, double *obj, diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index XXXXXXX..XXXXXXX 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -XXX,XX +XXX,XX @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp); */ bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp); +/* + * Visit a string value but do not consume it. + * + * @name expresses the relationship of this string to its parent + * container; see the general description of @name above. + * + * @obj must be non-NULL. Input and clone visitors set *@obj to the + * value (always using "" rather than NULL for an empty string). + * Other visitors leave *@obj unchanged, and commonly treat NULL like + * "". + * + * This function must be called only with an input visitor. + * + * This is mostly identical with visit_type_str() but leaves the value intact. + * This is useful when the caller may interpret the value with a different + * type. + */ +bool visit_type_str_preserving(Visitor *v, const char *name, char **obj, + Error **errp); + /* * Visit a number (i.e. double) value. * diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -XXX,XX +XXX,XX @@ processed(OptsVisitor *ov, const char *name) static bool -opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) +opts_type_str(Visitor *v, const char *name, char **obj, bool consume, + Error **errp) { OptsVisitor *ov = to_ov(v); const QemuOpt *opt; @@ -XXX,XX +XXX,XX @@ opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) * valid enum value; this is harmless because tracking what gets * consumed only matters to visit_end_struct() as the final error * check if there were no other failures during the visit. */ - processed(ov, name); + if (consume) { + processed(ov, name); + } return true; } diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-clone-visitor.c +++ b/qapi/qapi-clone-visitor.c @@ -XXX,XX +XXX,XX @@ static bool qapi_clone_type_bool(Visitor *v, const char *name, bool *obj, } static bool qapi_clone_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { QapiCloneVisitor *qcv = to_qcv(v); diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -XXX,XX +XXX,XX @@ static void qapi_dealloc_end_list(Visitor *v, void **obj) } static bool qapi_dealloc_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { - if (obj) { + if (obj && consume) { g_free(*obj); } return true; diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-forward-visitor.c +++ b/qapi/qapi-forward-visitor.c @@ -XXX,XX +XXX,XX @@ static bool forward_field_type_bool(Visitor *v, const char *name, bool *obj, } static bool forward_field_type_str(Visitor *v, const char *name, char **obj, + bool consume, Error **errp) { ForwardFieldVisitor *ffv = to_ffv(v); @@ -XXX,XX +XXX,XX @@ static bool forward_field_type_str(Visitor *v, const char *name, char **obj, if (!forward_field_translate_name(ffv, &name, errp)) { return false; } - return visit_type_str(ffv->target, name, obj, errp); + return (consume ? visit_type_str : visit_type_str_preserving)( + ffv->target, name, obj, errp); } static bool forward_field_type_size(Visitor *v, const char *name, uint64_t *obj, diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -XXX,XX +XXX,XX @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) return v->type_bool(v, name, obj, errp); } -bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) +static bool visit_type_str_common(Visitor *v, const char *name, char **obj, + bool consume, Error **errp) { bool ok; @@ -XXX,XX +XXX,XX @@ bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) assert(!(v->type & VISITOR_OUTPUT) || *obj); */ trace_visit_type_str(v, name, obj); - ok = v->type_str(v, name, obj, errp); + ok = v->type_str(v, name, obj, consume, errp); if (v->type & VISITOR_INPUT) { assert(ok != !*obj); } return ok; } +bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) +{ + return visit_type_str_common(v, name, obj, true, errp); +} + +bool visit_type_str_preserving(Visitor *v, const char *name, char **obj, + Error **errp) +{ + assert(v->type & VISITOR_INPUT); + return visit_type_str_common(v, name, obj, false, errp); +} + bool visit_type_number(Visitor *v, const char *name, double *obj, Error **errp) { diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -XXX,XX +XXX,XX @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv, } static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv, - const char *name, + const char *name, bool consume, Error **errp) { QObject *qobj; QString *qstr; - qobj = qobject_input_get_object(qiv, name, true, errp); + qobj = qobject_input_get_object(qiv, name, consume, errp); if (!qobj) { return NULL; } @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_int64_keyval(Visitor *v, const char *name, int64_t *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); if (!str) { return false; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_uint64_keyval(Visitor *v, const char *name, uint64_t *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); if (!str) { return false; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_bool_keyval(Visitor *v, const char *name, bool *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); if (!str) { return false; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_bool_keyval(Visitor *v, const char *name, } static bool qobject_input_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - QObject *qobj = qobject_input_get_object(qiv, name, true, errp); + QObject *qobj = qobject_input_get_object(qiv, name, consume, errp); QString *qstr; *obj = NULL; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_str(Visitor *v, const char *name, char **obj, } static bool qobject_input_type_str_keyval(Visitor *v, const char *name, - char **obj, Error **errp) + char **obj, bool consume, + Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, consume, errp); *obj = g_strdup(str); return !!str; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_number_keyval(Visitor *v, const char *name, double *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); double val; if (!str) { @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_size_keyval(Visitor *v, const char *name, uint64_t *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); if (!str) { return false; diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qobject-output-visitor.c +++ b/qapi/qobject-output-visitor.c @@ -XXX,XX +XXX,XX @@ static bool qobject_output_type_bool(Visitor *v, const char *name, bool *obj, } static bool qobject_output_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { QObjectOutputVisitor *qov = to_qov(v); if (*obj) { diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -XXX,XX +XXX,XX @@ static bool parse_type_bool(Visitor *v, const char *name, bool *obj, } static bool parse_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { StringInputVisitor *siv = to_siv(v); diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -XXX,XX +XXX,XX @@ static bool print_type_bool(Visitor *v, const char *name, bool *obj, } static bool print_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { StringOutputVisitor *sov = to_sov(v); char *out; -- 2.45.2 Consuming a value when visit_type_enum() fails makes it impossible to reinterpret the value with a different type. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/qapi/visitor.h | 5 ----- qapi/opts-visitor.c | 5 ----- qapi/qapi-visit-core.c | 4 +++- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index XXXXXXX..XXXXXXX 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -XXX,XX +XXX,XX @@ void visit_set_policy(Visitor *v, CompatPolicy *policy); * is an input visitor. * * Return true on success, false on failure. - * - * May call visit_type_str() under the hood, and the enum visit may - * fail even if the corresponding string visit succeeded; this implies - * that an input visitor's visit_type_str() must have no unwelcome - * side effects. */ bool visit_type_enum(Visitor *v, const char *name, int *obj, const QEnumLookup *lookup, Error **errp); diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -XXX,XX +XXX,XX @@ opts_type_str(Visitor *v, const char *name, char **obj, bool consume, return false; } *obj = g_strdup(opt->str ? opt->str : ""); - /* Note that we consume a string even if this is called as part of - * an enum visit that later fails because the string is not a - * valid enum value; this is harmless because tracking what gets - * consumed only matters to visit_end_struct() as the final error - * check if there were no other failures during the visit. */ if (consume) { processed(ov, name); } diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -XXX,XX +XXX,XX @@ static bool input_type_enum(Visitor *v, const char *name, int *obj, int64_t value; g_autofree char *enum_str = NULL; - if (!visit_type_str(v, name, &enum_str, errp)) { + if (!visit_type_str_preserving(v, name, &enum_str, errp)) { return false; } @@ -XXX,XX +XXX,XX @@ static bool input_type_enum(Visitor *v, const char *name, int *obj, return false; } + enum_str = NULL; + visit_type_str(v, name, &enum_str, &error_abort); *obj = value; return true; } -- 2.45.2 rom_bar is tristate but was defined as uint32_t so convert it into OnOffAuto to clarify that. For compatibility, a uint32 value set via QOM will be converted into OnOffAuto. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- docs/igd-assign.txt | 2 +- include/hw/pci/pci_device.h | 2 +- hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++-- hw/vfio/pci-quirks.c | 2 +- hw/vfio/pci.c | 11 ++++---- hw/xen/xen_pt_load_rom.c | 4 +-- tests/qtest/virtio-net-failover.c | 32 +++++++++++----------- 7 files changed, 81 insertions(+), 29 deletions(-) diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt index XXXXXXX..XXXXXXX 100644 --- a/docs/igd-assign.txt +++ b/docs/igd-assign.txt @@ -XXX,XX +XXX,XX @@ IGD has two different modes for assignment using vfio-pci: ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at PCI address 1f.0. * The IGD device must have a VGA ROM, either provided via the romfile - option or loaded automatically through vfio (standard). rombar=0 + option or loaded automatically through vfio (standard). rombar=off will disable legacy mode support. * Hotplug of the IGD device is not supported. * The IGD device must be a SandyBridge or newer model device. diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/pci/pci_device.h +++ b/include/hw/pci/pci_device.h @@ -XXX,XX +XXX,XX @@ struct PCIDevice { uint32_t romsize; bool has_rom; MemoryRegion rom; - uint32_t rom_bar; + OnOffAuto rom_bar; /* INTx routing notifier */ PCIINTxRoutingNotifier intx_routing_notifier; diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ static char *pcibus_get_fw_dev_path(DeviceState *dev); static void pcibus_reset_hold(Object *obj, ResetType type); static bool pcie_has_upstream_port(PCIDevice *dev); +static void rom_bar_get(Object *obj, Visitor *v, const char *name, void *opaque, + Error **errp) +{ + Property *prop = opaque; + int *ptr = object_field_prop_ptr(obj, prop); + + visit_type_enum(v, name, ptr, prop->info->enum_table, errp); +} + +static void rom_bar_set(Object *obj, Visitor *v, const char *name, void *opaque, + Error **errp) +{ + Property *prop = opaque; + Error *local_err = NULL; + int *ptr = object_field_prop_ptr(obj, prop); + uint32_t value; + + visit_type_enum(v, name, ptr, prop->info->enum_table, &local_err); + if (!local_err) { + return; + } + + if (visit_type_uint32(v, name, &value, NULL)) { + if (value) { + *ptr = ON_OFF_AUTO_ON; + warn_report("Specifying a number for rombar is deprecated; replace a non-zero value with 'on'"); + } else { + *ptr = ON_OFF_AUTO_OFF; + warn_report("Specifying a number for rombar is deprecated; replace 0 with 'off'"); + } + + return; + } + + error_propagate(errp, local_err); +} + +static void rom_bar_set_default_value(ObjectProperty *op, const Property *prop) +{ + object_property_set_default_str(op, + qapi_enum_lookup(prop->info->enum_table, prop->defval.i)); +} + +static const PropertyInfo qdev_prop_rom_bar = { + .name = "OnOffAuto", + .description = "on/off/auto", + .enum_table = &OnOffAuto_lookup, + .get = rom_bar_get, + .set = rom_bar_set, + .set_default_value = rom_bar_set_default_value, +}; + static Property pci_props[] = { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), DEFINE_PROP_STRING("romfile", PCIDevice, romfile), DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, UINT32_MAX), - DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1), + DEFINE_PROP_SIGNED("rombar", PCIDevice, rom_bar, ON_OFF_AUTO_AUTO, + qdev_prop_rom_bar, OnOffAuto), DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), DEFINE_PROP_BIT("x-pcie-lnksta-dllla", PCIDevice, cap_present, @@ -XXX,XX +XXX,XX @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, return; } - if (!pdev->rom_bar) { + if (pdev->rom_bar == ON_OFF_AUTO_OFF) { /* * Load rom via fw_cfg instead of creating a rom bar, * for 0.11 compatibility. diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci-quirks.c +++ b/hw/vfio/pci-quirks.c @@ -XXX,XX +XXX,XX @@ * execution as noticed with the BCM 57810 card for lack of a * more better way to handle such issues. * The user can still override by specifying a romfile or - * rombar=1. + * rombar=on. * Please see https://bugs.launchpad.net/qemu/+bug/1284874 * for an analysis of the 57810 card hang. When adding * a new vendor id/device id combination below, please also add diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -XXX,XX +XXX,XX @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) error_report("vfio-pci: Cannot read device rom at " "%s", vdev->vbasedev.name); error_printf("Device option ROM contents are probably invalid " - "(check dmesg).\nSkip option ROM probe with rombar=0, " + "(check dmesg).\nSkip option ROM probe with rombar=off, " "or load from file with romfile=\n"); return; } @@ -XXX,XX +XXX,XX @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev) { uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK); off_t offset = vdev->config_offset + PCI_ROM_ADDRESS; - DeviceState *dev = DEVICE(vdev); char *name; int fd = vdev->vbasedev.fd; - if (vdev->pdev.romfile || !vdev->pdev.rom_bar) { + if (vdev->pdev.romfile || vdev->pdev.rom_bar == ON_OFF_AUTO_OFF) { /* Since pci handles romfile, just print a message and return */ if (vfio_opt_rom_in_denylist(vdev) && vdev->pdev.romfile) { warn_report("Device at %s is known to cause system instability" @@ -XXX,XX +XXX,XX @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev) } if (vfio_opt_rom_in_denylist(vdev)) { - if (dev->opts && qdict_haskey(dev->opts, "rombar")) { + if (vdev->pdev.rom_bar == ON_OFF_AUTO_ON) { warn_report("Device at %s is known to cause system instability" " issues during option rom execution", vdev->vbasedev.name); error_printf("Proceeding anyway since user specified" - " non zero value for rombar\n"); + " on for rombar\n"); } else { warn_report("Rom loading for device at %s has been disabled" " due to system instability issues", vdev->vbasedev.name); - error_printf("Specify rombar=1 or romfile to force\n"); + error_printf("Specify rombar=on or romfile to force\n"); return; } } diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c index XXXXXXX..XXXXXXX 100644 --- a/hw/xen/xen_pt_load_rom.c +++ b/hw/xen/xen_pt_load_rom.c @@ -XXX,XX +XXX,XX @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev, Object *owner = OBJECT(dev); /* If loading ROM from file, pci handles it */ - if (dev->romfile || !dev->rom_bar) { + if (dev->romfile || dev->rom_bar == ON_OFF_AUTO_OFF) { return NULL; } @@ -XXX,XX +XXX,XX @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev, if (!fread(ptr, 1, st.st_size, fp)) { error_report("pci-assign: Cannot read from host %s", rom_file); error_printf("Device option ROM contents are probably invalid " - "(check dmesg).\nSkip option ROM probe with rombar=0, " + "(check dmesg).\nSkip option ROM probe with rombar=off, " "or load from file with romfile=\n"); goto close_rom; } diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/virtio-net-failover.c +++ b/tests/qtest/virtio-net-failover.c @@ -XXX,XX +XXX,XX @@ static void test_hotplug_2_reverse(void) "{'bus': 'root0'," "'failover': true," "'netdev': 'hs0'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_STANDBY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_out(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_in(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_off_migrate_out(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_off_migrate_in(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_guest_off_migrate_out(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_guest_off_migrate_in(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_guest_off_abort(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_abort_wait_unplug(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_abort_active(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_off_abort(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_abort_timeout(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_multi_out(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_multi_out(gconstpointer opaque) "{'bus': 'root3'," "'failover_pair_id': 'standby1'," "'netdev': 'hs3'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY1"'}"); @@ -XXX,XX +XXX,XX @@ static void test_multi_in(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_multi_in(gconstpointer opaque) "{'bus': 'root3'," "'failover_pair_id': 'standby1'," "'netdev': 'hs3'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY1"'}"); -- 2.45.2 It is no longer used. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Markus Armbruster <armbru@redhat.com> --- include/hw/qdev-core.h | 4 ---- hw/core/qdev.c | 1 - system/qdev-monitor.c | 12 +++++++----- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -XXX,XX +XXX,XX @@ struct DeviceState { * @pending_deleted_expires_ms: optional timeout for deletion events */ int64_t pending_deleted_expires_ms; - /** - * @opts: QDict of options for the device - */ - QDict *opts; /** * @hotplugged: was device added after PHASE_MACHINE_READY? */ diff --git a/hw/core/qdev.c b/hw/core/qdev.c index XXXXXXX..XXXXXXX 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -XXX,XX +XXX,XX @@ static void device_finalize(Object *obj) dev->canonical_path = NULL; } - qobject_unref(dev->opts); g_free(dev->id); } diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index XXXXXXX..XXXXXXX 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -XXX,XX +XXX,XX @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, char *id; DeviceState *dev = NULL; BusState *bus = NULL; + QDict *properties; driver = qdict_get_try_str(opts, "driver"); if (!driver) { @@ -XXX,XX +XXX,XX @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, } /* set properties */ - dev->opts = qdict_clone_shallow(opts); - qdict_del(dev->opts, "driver"); - qdict_del(dev->opts, "bus"); - qdict_del(dev->opts, "id"); + properties = qdict_clone_shallow(opts); + qdict_del(properties, "driver"); + qdict_del(properties, "bus"); + qdict_del(properties, "id"); - object_set_properties_from_keyval(&dev->parent_obj, dev->opts, from_json, + object_set_properties_from_keyval(&dev->parent_obj, properties, from_json, errp); + qobject_unref(properties); if (*errp) { goto err_del_dev; } -- 2.45.2 rom_bar is tristate but was defined as uint32_t so convert it into OnOffAuto to clarify that. For compatibility, a uint32 value set via QOM will be converted into OnOffAuto. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- Changes in v2: - Documented in docs/about/deprecated.rst (Daniel P. Berrangé) - Link to v1: https://lore.kernel.org/r/20240706-rombar-v1-0-802daef2aec1@daynix.com --- Akihiko Odaki (4): qapi: Add visit_type_str_preserving() qapi: Do not consume a value when visit_type_enum() fails hw/pci: Convert rom_bar into OnOffAuto hw/qdev: Remove opts member docs/about/deprecated.rst | 7 +++++ docs/igd-assign.txt | 2 +- include/hw/pci/pci_device.h | 2 +- include/hw/qdev-core.h | 4 --- include/qapi/visitor-impl.h | 3 ++- include/qapi/visitor.h | 25 +++++++++++++---- hw/core/qdev.c | 1 - hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++-- hw/vfio/pci-quirks.c | 2 +- hw/vfio/pci.c | 11 ++++---- hw/xen/xen_pt_load_rom.c | 4 +-- qapi/opts-visitor.c | 12 ++++----- qapi/qapi-clone-visitor.c | 2 +- qapi/qapi-dealloc-visitor.c | 4 +-- qapi/qapi-forward-visitor.c | 4 ++- qapi/qapi-visit-core.c | 21 ++++++++++++--- qapi/qobject-input-visitor.c | 23 ++++++++-------- qapi/qobject-output-visitor.c | 2 +- qapi/string-input-visitor.c | 2 +- qapi/string-output-visitor.c | 2 +- system/qdev-monitor.c | 12 +++++---- tests/qtest/virtio-net-failover.c | 32 +++++++++++----------- 22 files changed, 161 insertions(+), 73 deletions(-) --- base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6 change-id: 20240704-rombar-1a4ba2890d6c Best regards, -- Akihiko Odaki <akihiko.odaki@daynix.com> visit_type_str_preserving() is mostly indentical with visit_type_str() but leaves the value intact. This is useful when the caller may interpret the value with a different type. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/qapi/visitor-impl.h | 3 ++- include/qapi/visitor.h | 20 ++++++++++++++++++++ qapi/opts-visitor.c | 7 +++++-- qapi/qapi-clone-visitor.c | 2 +- qapi/qapi-dealloc-visitor.c | 4 ++-- qapi/qapi-forward-visitor.c | 4 +++- qapi/qapi-visit-core.c | 17 +++++++++++++++-- qapi/qobject-input-visitor.c | 23 ++++++++++++----------- qapi/qobject-output-visitor.c | 2 +- qapi/string-input-visitor.c | 2 +- qapi/string-output-visitor.c | 2 +- 11 files changed, 63 insertions(+), 23 deletions(-) diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index XXXXXXX..XXXXXXX 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -XXX,XX +XXX,XX @@ struct Visitor bool (*type_bool)(Visitor *v, const char *name, bool *obj, Error **errp); /* Must be set */ - bool (*type_str)(Visitor *v, const char *name, char **obj, Error **errp); + bool (*type_str)(Visitor *v, const char *name, char **obj, bool consume, + Error **errp); /* Must be set to visit numbers */ bool (*type_number)(Visitor *v, const char *name, double *obj, diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index XXXXXXX..XXXXXXX 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -XXX,XX +XXX,XX @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp); */ bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp); +/* + * Visit a string value but do not consume it. + * + * @name expresses the relationship of this string to its parent + * container; see the general description of @name above. + * + * @obj must be non-NULL. Input and clone visitors set *@obj to the + * value (always using "" rather than NULL for an empty string). + * Other visitors leave *@obj unchanged, and commonly treat NULL like + * "". + * + * This function must be called only with an input visitor. + * + * This is mostly identical with visit_type_str() but leaves the value intact. + * This is useful when the caller may interpret the value with a different + * type. + */ +bool visit_type_str_preserving(Visitor *v, const char *name, char **obj, + Error **errp); + /* * Visit a number (i.e. double) value. * diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -XXX,XX +XXX,XX @@ processed(OptsVisitor *ov, const char *name) static bool -opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) +opts_type_str(Visitor *v, const char *name, char **obj, bool consume, + Error **errp) { OptsVisitor *ov = to_ov(v); const QemuOpt *opt; @@ -XXX,XX +XXX,XX @@ opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) * valid enum value; this is harmless because tracking what gets * consumed only matters to visit_end_struct() as the final error * check if there were no other failures during the visit. */ - processed(ov, name); + if (consume) { + processed(ov, name); + } return true; } diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-clone-visitor.c +++ b/qapi/qapi-clone-visitor.c @@ -XXX,XX +XXX,XX @@ static bool qapi_clone_type_bool(Visitor *v, const char *name, bool *obj, } static bool qapi_clone_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { QapiCloneVisitor *qcv = to_qcv(v); diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -XXX,XX +XXX,XX @@ static void qapi_dealloc_end_list(Visitor *v, void **obj) } static bool qapi_dealloc_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { - if (obj) { + if (obj && consume) { g_free(*obj); } return true; diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-forward-visitor.c +++ b/qapi/qapi-forward-visitor.c @@ -XXX,XX +XXX,XX @@ static bool forward_field_type_bool(Visitor *v, const char *name, bool *obj, } static bool forward_field_type_str(Visitor *v, const char *name, char **obj, + bool consume, Error **errp) { ForwardFieldVisitor *ffv = to_ffv(v); @@ -XXX,XX +XXX,XX @@ static bool forward_field_type_str(Visitor *v, const char *name, char **obj, if (!forward_field_translate_name(ffv, &name, errp)) { return false; } - return visit_type_str(ffv->target, name, obj, errp); + return (consume ? visit_type_str : visit_type_str_preserving)( + ffv->target, name, obj, errp); } static bool forward_field_type_size(Visitor *v, const char *name, uint64_t *obj, diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -XXX,XX +XXX,XX @@ bool visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) return v->type_bool(v, name, obj, errp); } -bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) +static bool visit_type_str_common(Visitor *v, const char *name, char **obj, + bool consume, Error **errp) { bool ok; @@ -XXX,XX +XXX,XX @@ bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) assert(!(v->type & VISITOR_OUTPUT) || *obj); */ trace_visit_type_str(v, name, obj); - ok = v->type_str(v, name, obj, errp); + ok = v->type_str(v, name, obj, consume, errp); if (v->type & VISITOR_INPUT) { assert(ok != !*obj); } return ok; } +bool visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) +{ + return visit_type_str_common(v, name, obj, true, errp); +} + +bool visit_type_str_preserving(Visitor *v, const char *name, char **obj, + Error **errp) +{ + assert(v->type & VISITOR_INPUT); + return visit_type_str_common(v, name, obj, false, errp); +} + bool visit_type_number(Visitor *v, const char *name, double *obj, Error **errp) { diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -XXX,XX +XXX,XX @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv, } static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv, - const char *name, + const char *name, bool consume, Error **errp) { QObject *qobj; QString *qstr; - qobj = qobject_input_get_object(qiv, name, true, errp); + qobj = qobject_input_get_object(qiv, name, consume, errp); if (!qobj) { return NULL; } @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_int64_keyval(Visitor *v, const char *name, int64_t *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); if (!str) { return false; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_uint64_keyval(Visitor *v, const char *name, uint64_t *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); if (!str) { return false; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_bool_keyval(Visitor *v, const char *name, bool *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); if (!str) { return false; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_bool_keyval(Visitor *v, const char *name, } static bool qobject_input_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - QObject *qobj = qobject_input_get_object(qiv, name, true, errp); + QObject *qobj = qobject_input_get_object(qiv, name, consume, errp); QString *qstr; *obj = NULL; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_str(Visitor *v, const char *name, char **obj, } static bool qobject_input_type_str_keyval(Visitor *v, const char *name, - char **obj, Error **errp) + char **obj, bool consume, + Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, consume, errp); *obj = g_strdup(str); return !!str; @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_number_keyval(Visitor *v, const char *name, double *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); double val; if (!str) { @@ -XXX,XX +XXX,XX @@ static bool qobject_input_type_size_keyval(Visitor *v, const char *name, uint64_t *obj, Error **errp) { QObjectInputVisitor *qiv = to_qiv(v); - const char *str = qobject_input_get_keyval(qiv, name, errp); + const char *str = qobject_input_get_keyval(qiv, name, true, errp); if (!str) { return false; diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qobject-output-visitor.c +++ b/qapi/qobject-output-visitor.c @@ -XXX,XX +XXX,XX @@ static bool qobject_output_type_bool(Visitor *v, const char *name, bool *obj, } static bool qobject_output_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { QObjectOutputVisitor *qov = to_qov(v); if (*obj) { diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -XXX,XX +XXX,XX @@ static bool parse_type_bool(Visitor *v, const char *name, bool *obj, } static bool parse_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { StringInputVisitor *siv = to_siv(v); diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -XXX,XX +XXX,XX @@ static bool print_type_bool(Visitor *v, const char *name, bool *obj, } static bool print_type_str(Visitor *v, const char *name, char **obj, - Error **errp) + bool consume, Error **errp) { StringOutputVisitor *sov = to_sov(v); char *out; -- 2.45.2 Consuming a value when visit_type_enum() fails makes it impossible to reinterpret the value with a different type. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/qapi/visitor.h | 5 ----- qapi/opts-visitor.c | 5 ----- qapi/qapi-visit-core.c | 4 +++- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index XXXXXXX..XXXXXXX 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -XXX,XX +XXX,XX @@ void visit_set_policy(Visitor *v, CompatPolicy *policy); * is an input visitor. * * Return true on success, false on failure. - * - * May call visit_type_str() under the hood, and the enum visit may - * fail even if the corresponding string visit succeeded; this implies - * that an input visitor's visit_type_str() must have no unwelcome - * side effects. */ bool visit_type_enum(Visitor *v, const char *name, int *obj, const QEnumLookup *lookup, Error **errp); diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -XXX,XX +XXX,XX @@ opts_type_str(Visitor *v, const char *name, char **obj, bool consume, return false; } *obj = g_strdup(opt->str ? opt->str : ""); - /* Note that we consume a string even if this is called as part of - * an enum visit that later fails because the string is not a - * valid enum value; this is harmless because tracking what gets - * consumed only matters to visit_end_struct() as the final error - * check if there were no other failures during the visit. */ if (consume) { processed(ov, name); } diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index XXXXXXX..XXXXXXX 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -XXX,XX +XXX,XX @@ static bool input_type_enum(Visitor *v, const char *name, int *obj, int64_t value; g_autofree char *enum_str = NULL; - if (!visit_type_str(v, name, &enum_str, errp)) { + if (!visit_type_str_preserving(v, name, &enum_str, errp)) { return false; } @@ -XXX,XX +XXX,XX @@ static bool input_type_enum(Visitor *v, const char *name, int *obj, return false; } + enum_str = NULL; + visit_type_str(v, name, &enum_str, &error_abort); *obj = value; return true; } -- 2.45.2 rom_bar is tristate but was defined as uint32_t so convert it into OnOffAuto to clarify that. For compatibility, a uint32 value set via QOM will be converted into OnOffAuto. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- docs/about/deprecated.rst | 7 +++++ docs/igd-assign.txt | 2 +- include/hw/pci/pci_device.h | 2 +- hw/pci/pci.c | 57 +++++++++++++++++++++++++++++++++++++-- hw/vfio/pci-quirks.c | 2 +- hw/vfio/pci.c | 11 ++++---- hw/xen/xen_pt_load_rom.c | 4 +-- tests/qtest/virtio-net-failover.c | 32 +++++++++++----------- 8 files changed, 88 insertions(+), 29 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index XXXXXXX..XXXXXXX 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -XXX,XX +XXX,XX @@ recommending to switch to their stable counterparts: SD physical layer specification v2.00 supersedes the v1.10 one. v2.00 is the default since QEMU 3.0.0. +Integer specification of PCI device's ``rombar`` property (since 9.1) +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Specifying an integer for the ``rombar`` property of a PCI device is being +deprecated starting in 9.1 due to obscurity of such an specification. Replace +zero with ``off`` and a non-zero value with ``on``, respectively. + Block device options '''''''''''''''''''' diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt index XXXXXXX..XXXXXXX 100644 --- a/docs/igd-assign.txt +++ b/docs/igd-assign.txt @@ -XXX,XX +XXX,XX @@ IGD has two different modes for assignment using vfio-pci: ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at PCI address 1f.0. * The IGD device must have a VGA ROM, either provided via the romfile - option or loaded automatically through vfio (standard). rombar=0 + option or loaded automatically through vfio (standard). rombar=off will disable legacy mode support. * Hotplug of the IGD device is not supported. * The IGD device must be a SandyBridge or newer model device. diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/pci/pci_device.h +++ b/include/hw/pci/pci_device.h @@ -XXX,XX +XXX,XX @@ struct PCIDevice { uint32_t romsize; bool has_rom; MemoryRegion rom; - uint32_t rom_bar; + OnOffAuto rom_bar; /* INTx routing notifier */ PCIINTxRoutingNotifier intx_routing_notifier; diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ static char *pcibus_get_fw_dev_path(DeviceState *dev); static void pcibus_reset_hold(Object *obj, ResetType type); static bool pcie_has_upstream_port(PCIDevice *dev); +static void rom_bar_get(Object *obj, Visitor *v, const char *name, void *opaque, + Error **errp) +{ + Property *prop = opaque; + int *ptr = object_field_prop_ptr(obj, prop); + + visit_type_enum(v, name, ptr, prop->info->enum_table, errp); +} + +static void rom_bar_set(Object *obj, Visitor *v, const char *name, void *opaque, + Error **errp) +{ + Property *prop = opaque; + Error *local_err = NULL; + int *ptr = object_field_prop_ptr(obj, prop); + uint32_t value; + + visit_type_enum(v, name, ptr, prop->info->enum_table, &local_err); + if (!local_err) { + return; + } + + if (visit_type_uint32(v, name, &value, NULL)) { + if (value) { + *ptr = ON_OFF_AUTO_ON; + warn_report("Specifying a number for rombar is deprecated; replace a non-zero value with 'on'"); + } else { + *ptr = ON_OFF_AUTO_OFF; + warn_report("Specifying a number for rombar is deprecated; replace 0 with 'off'"); + } + + return; + } + + error_propagate(errp, local_err); +} + +static void rom_bar_set_default_value(ObjectProperty *op, const Property *prop) +{ + object_property_set_default_str(op, + qapi_enum_lookup(prop->info->enum_table, prop->defval.i)); +} + +static const PropertyInfo qdev_prop_rom_bar = { + .name = "OnOffAuto", + .description = "on/off/auto", + .enum_table = &OnOffAuto_lookup, + .get = rom_bar_get, + .set = rom_bar_set, + .set_default_value = rom_bar_set_default_value, +}; + static Property pci_props[] = { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), DEFINE_PROP_STRING("romfile", PCIDevice, romfile), DEFINE_PROP_UINT32("romsize", PCIDevice, romsize, UINT32_MAX), - DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1), + DEFINE_PROP_SIGNED("rombar", PCIDevice, rom_bar, ON_OFF_AUTO_AUTO, + qdev_prop_rom_bar, OnOffAuto), DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), DEFINE_PROP_BIT("x-pcie-lnksta-dllla", PCIDevice, cap_present, @@ -XXX,XX +XXX,XX @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, return; } - if (!pdev->rom_bar) { + if (pdev->rom_bar == ON_OFF_AUTO_OFF) { /* * Load rom via fw_cfg instead of creating a rom bar, * for 0.11 compatibility. diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci-quirks.c +++ b/hw/vfio/pci-quirks.c @@ -XXX,XX +XXX,XX @@ * execution as noticed with the BCM 57810 card for lack of a * more better way to handle such issues. * The user can still override by specifying a romfile or - * rombar=1. + * rombar=on. * Please see https://bugs.launchpad.net/qemu/+bug/1284874 * for an analysis of the 57810 card hang. When adding * a new vendor id/device id combination below, please also add diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -XXX,XX +XXX,XX @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) error_report("vfio-pci: Cannot read device rom at " "%s", vdev->vbasedev.name); error_printf("Device option ROM contents are probably invalid " - "(check dmesg).\nSkip option ROM probe with rombar=0, " + "(check dmesg).\nSkip option ROM probe with rombar=off, " "or load from file with romfile=\n"); return; } @@ -XXX,XX +XXX,XX @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev) { uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK); off_t offset = vdev->config_offset + PCI_ROM_ADDRESS; - DeviceState *dev = DEVICE(vdev); char *name; int fd = vdev->vbasedev.fd; - if (vdev->pdev.romfile || !vdev->pdev.rom_bar) { + if (vdev->pdev.romfile || vdev->pdev.rom_bar == ON_OFF_AUTO_OFF) { /* Since pci handles romfile, just print a message and return */ if (vfio_opt_rom_in_denylist(vdev) && vdev->pdev.romfile) { warn_report("Device at %s is known to cause system instability" @@ -XXX,XX +XXX,XX @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev) } if (vfio_opt_rom_in_denylist(vdev)) { - if (dev->opts && qdict_haskey(dev->opts, "rombar")) { + if (vdev->pdev.rom_bar == ON_OFF_AUTO_ON) { warn_report("Device at %s is known to cause system instability" " issues during option rom execution", vdev->vbasedev.name); error_printf("Proceeding anyway since user specified" - " non zero value for rombar\n"); + " on for rombar\n"); } else { warn_report("Rom loading for device at %s has been disabled" " due to system instability issues", vdev->vbasedev.name); - error_printf("Specify rombar=1 or romfile to force\n"); + error_printf("Specify rombar=on or romfile to force\n"); return; } } diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c index XXXXXXX..XXXXXXX 100644 --- a/hw/xen/xen_pt_load_rom.c +++ b/hw/xen/xen_pt_load_rom.c @@ -XXX,XX +XXX,XX @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev, Object *owner = OBJECT(dev); /* If loading ROM from file, pci handles it */ - if (dev->romfile || !dev->rom_bar) { + if (dev->romfile || dev->rom_bar == ON_OFF_AUTO_OFF) { return NULL; } @@ -XXX,XX +XXX,XX @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev, if (!fread(ptr, 1, st.st_size, fp)) { error_report("pci-assign: Cannot read from host %s", rom_file); error_printf("Device option ROM contents are probably invalid " - "(check dmesg).\nSkip option ROM probe with rombar=0, " + "(check dmesg).\nSkip option ROM probe with rombar=off, " "or load from file with romfile=\n"); goto close_rom; } diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c index XXXXXXX..XXXXXXX 100644 --- a/tests/qtest/virtio-net-failover.c +++ b/tests/qtest/virtio-net-failover.c @@ -XXX,XX +XXX,XX @@ static void test_hotplug_2_reverse(void) "{'bus': 'root0'," "'failover': true," "'netdev': 'hs0'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_STANDBY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_out(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_in(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_off_migrate_out(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_off_migrate_in(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_guest_off_migrate_out(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_guest_off_migrate_in(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_guest_off_abort(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_abort_wait_unplug(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_abort_active(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_off_abort(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_migrate_abort_timeout(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_multi_out(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_multi_out(gconstpointer opaque) "{'bus': 'root3'," "'failover_pair_id': 'standby1'," "'netdev': 'hs3'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY1"'}"); @@ -XXX,XX +XXX,XX @@ static void test_multi_in(gconstpointer opaque) "{'bus': 'root1'," "'failover_pair_id': 'standby0'," "'netdev': 'hs1'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY0"'}"); @@ -XXX,XX +XXX,XX @@ static void test_multi_in(gconstpointer opaque) "{'bus': 'root3'," "'failover_pair_id': 'standby1'," "'netdev': 'hs3'," - "'rombar': 0," + "'rombar': 'off'," "'romfile': ''," "'mac': '"MAC_PRIMARY1"'}"); -- 2.45.2 It is no longer used. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Markus Armbruster <armbru@redhat.com> --- include/hw/qdev-core.h | 4 ---- hw/core/qdev.c | 1 - system/qdev-monitor.c | 12 +++++++----- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -XXX,XX +XXX,XX @@ struct DeviceState { * @pending_deleted_expires_ms: optional timeout for deletion events */ int64_t pending_deleted_expires_ms; - /** - * @opts: QDict of options for the device - */ - QDict *opts; /** * @hotplugged: was device added after PHASE_MACHINE_READY? */ diff --git a/hw/core/qdev.c b/hw/core/qdev.c index XXXXXXX..XXXXXXX 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -XXX,XX +XXX,XX @@ static void device_finalize(Object *obj) dev->canonical_path = NULL; } - qobject_unref(dev->opts); g_free(dev->id); } diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index XXXXXXX..XXXXXXX 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -XXX,XX +XXX,XX @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, char *id; DeviceState *dev = NULL; BusState *bus = NULL; + QDict *properties; driver = qdict_get_try_str(opts, "driver"); if (!driver) { @@ -XXX,XX +XXX,XX @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts, } /* set properties */ - dev->opts = qdict_clone_shallow(opts); - qdict_del(dev->opts, "driver"); - qdict_del(dev->opts, "bus"); - qdict_del(dev->opts, "id"); + properties = qdict_clone_shallow(opts); + qdict_del(properties, "driver"); + qdict_del(properties, "bus"); + qdict_del(properties, "id"); - object_set_properties_from_keyval(&dev->parent_obj, dev->opts, from_json, + object_set_properties_from_keyval(&dev->parent_obj, properties, from_json, errp); + qobject_unref(properties); if (*errp) { goto err_del_dev; } -- 2.45.2