From 556068eed04b7f11187aabd89b981552d8d0c30e Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Wed, 6 Aug 2014 15:18:21 -0300 Subject: [PATCH 01/15] hw/machine: Free old values of string properties Reviewed-by: Markus Armbruster Reviewed-by: Marcel Apfelbaum Signed-off-by: Eduardo Habkost Acked-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Amos Kong Cc: qemu-stable@nongnu.org --- hw/core/machine.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/core/machine.c b/hw/core/machine.c index f0046d6de3..7f3418c5af 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -24,6 +24,7 @@ static void machine_set_accel(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->accel); ms->accel = g_strdup(value); } @@ -79,6 +80,7 @@ static void machine_set_kernel(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->kernel_filename); ms->kernel_filename = g_strdup(value); } @@ -93,6 +95,7 @@ static void machine_set_initrd(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->initrd_filename); ms->initrd_filename = g_strdup(value); } @@ -107,6 +110,7 @@ static void machine_set_append(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->kernel_cmdline); ms->kernel_cmdline = g_strdup(value); } @@ -121,6 +125,7 @@ static void machine_set_dtb(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->dtb); ms->dtb = g_strdup(value); } @@ -135,6 +140,7 @@ static void machine_set_dumpdtb(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->dumpdtb); ms->dumpdtb = g_strdup(value); } @@ -176,6 +182,7 @@ static void machine_set_dt_compatible(Object *obj, const char *value, Error **er { MachineState *ms = MACHINE(obj); + g_free(ms->dt_compatible); ms->dt_compatible = g_strdup(value); } @@ -232,6 +239,7 @@ static void machine_set_firmware(Object *obj, const char *value, Error **errp) { MachineState *ms = MACHINE(obj); + g_free(ms->firmware); ms->firmware = g_strdup(value); } From 70e98b230fa58d7da22bd6c45b206fc58e9908c2 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Fri, 8 Aug 2014 16:03:26 -0300 Subject: [PATCH 02/15] test-qdev-global-props: Trivial comment fix Signed-off-by: Eduardo Habkost Acked-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/test-qdev-global-props.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c index 2bef04c76f..e1a1317dd9 100644 --- a/tests/test-qdev-global-props.c +++ b/tests/test-qdev-global-props.c @@ -143,7 +143,7 @@ static const TypeInfo dynamic_prop_type = { .class_init = dynamic_class_init, }; -/* Test setting of static property using global properties */ +/* Test setting of dynamic properties using global properties */ static void test_dynamic_globalprop(void) { MyType *mt; From 9d41401b90fa10b335d2e739149d36437cfbf622 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 18 Sep 2014 20:46:45 +0300 Subject: [PATCH 03/15] tests: disable global props test for old glib MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit follow-up patch moves global property tests to subprocesses. Unfortunately with old glib this causes: tests/test-qdev-global-props.c: In function ‘test_static_prop’: tests/test-qdev-global-props.c:80:5: error: implicit declaration of function ‘g_test_trap_subprocess’ [-Werror=implicit-function-declaration] tests/test-qdev-global-props.c:80:5: error: nested extern declaration of ‘g_test_trap_subprocess’ [-Werror=nested-externs] This function was only added in glib 2.38, and our minimum version is 2.12. To fix, disable the test for glib < 2.38. Apply before that patch to avoid breaking bisect. Reported-by: Peter Maydell Signed-off-by: Michael S. Tsirkin --- configure | 9 +++++++++ tests/Makefile | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 961bf6fd43..6f1284aa79 100755 --- a/configure +++ b/configure @@ -2716,6 +2716,12 @@ for i in $glib_modules; do fi done +# g_test_trap_subprocess added in 2.38. Used by some tests. +glib_subprocess=yes +if ! $pkg_config --atleast-version=2.38 glib-2.0; then + glib_subprocess=no +fi + ########################################## # SHA command probe for modules if test "$modules" = yes; then @@ -4585,6 +4591,9 @@ if test "$bluez" = "yes" ; then echo "CONFIG_BLUEZ=y" >> $config_host_mak echo "BLUEZ_CFLAGS=$bluez_cflags" >> $config_host_mak fi +if test "glib_subprocess" = "yes" ; then + echo "CONFIG_HAS_GLIB_SUBPROCESS_TESTS=y" >> $config_host_mak +fi echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak if test "$gtk" = "yes" ; then echo "CONFIG_GTK=y" >> $config_host_mak diff --git a/tests/Makefile b/tests/Makefile index d5db97ba63..a5e3d0c369 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -58,7 +58,7 @@ check-unit-y += tests/test-int128$(EXESUF) # all code tested by test-int128 is inside int128.h gcov-files-test-int128-y = check-unit-y += tests/test-bitops$(EXESUF) -check-unit-y += tests/test-qdev-global-props$(EXESUF) +check-unit-$(CONFIG_HAS_GLIB_SUBPROCESS_TESTS) += tests/test-qdev-global-props$(EXESUF) check-unit-y += tests/check-qom-interface$(EXESUF) gcov-files-check-qom-interface-y = qom/object.c check-unit-$(CONFIG_POSIX) += tests/test-vmstate$(EXESUF) From 2177801a4899bf29108b3d471417a5b4d701ec29 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Fri, 8 Aug 2014 16:03:27 -0300 Subject: [PATCH 04/15] test-qdev-global-props: Run tests on subprocess There are multiple reasons for running the global property tests on a subprocess: * We need the global_props lists to be empty for each test case, so global properties from the previous test won't affect the next one; * We don't want the qdev_prop_check_global() warnings to pollute test output; * With a subprocess, we can ensure qdev_prop_check_global() is printing the warning messages it should. Signed-off-by: Eduardo Habkost Acked-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/test-qdev-global-props.c | 49 +++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c index e1a1317dd9..34223a7101 100644 --- a/tests/test-qdev-global-props.c +++ b/tests/test-qdev-global-props.c @@ -65,7 +65,7 @@ static const TypeInfo static_prop_type = { }; /* Test simple static property setting to default value */ -static void test_static_prop(void) +static void test_static_prop_subprocess(void) { MyType *mt; @@ -75,8 +75,16 @@ static void test_static_prop(void) g_assert_cmpuint(mt->prop1, ==, PROP_DEFAULT); } +static void test_static_prop(void) +{ + g_test_trap_subprocess("/qdev/properties/static/default/subprocess", 0, 0); + g_test_trap_assert_passed(); + g_test_trap_assert_stderr(""); + g_test_trap_assert_stdout(""); +} + /* Test setting of static property using global properties */ -static void test_static_globalprop(void) +static void test_static_globalprop_subprocess(void) { MyType *mt; static GlobalProperty props[] = { @@ -93,6 +101,14 @@ static void test_static_globalprop(void) g_assert_cmpuint(mt->prop2, ==, PROP_DEFAULT); } +static void test_static_globalprop(void) +{ + g_test_trap_subprocess("/qdev/properties/static/global/subprocess", 0, 0); + g_test_trap_assert_passed(); + g_test_trap_assert_stderr(""); + g_test_trap_assert_stdout(""); +} + #define TYPE_DYNAMIC_PROPS "dynamic-prop-type" #define DYNAMIC_TYPE(obj) \ OBJECT_CHECK(MyType, (obj), TYPE_DYNAMIC_PROPS) @@ -144,7 +160,7 @@ static const TypeInfo dynamic_prop_type = { }; /* Test setting of dynamic properties using global properties */ -static void test_dynamic_globalprop(void) +static void test_dynamic_globalprop_subprocess(void) { MyType *mt; static GlobalProperty props[] = { @@ -166,6 +182,16 @@ static void test_dynamic_globalprop(void) g_assert_cmpuint(all_used, ==, 1); } +static void test_dynamic_globalprop(void) +{ + g_test_trap_subprocess("/qdev/properties/dynamic/global/subprocess", 0, 0); + g_test_trap_assert_passed(); + g_test_trap_assert_stderr_unmatched("*prop1*"); + g_test_trap_assert_stderr_unmatched("*prop2*"); + g_test_trap_assert_stderr("*Warning: \"-global dynamic-prop-type-bad.prop3=103\" not used\n*"); + g_test_trap_assert_stdout(""); +} + int main(int argc, char **argv) { g_test_init(&argc, &argv, NULL); @@ -174,9 +200,20 @@ int main(int argc, char **argv) type_register_static(&static_prop_type); type_register_static(&dynamic_prop_type); - g_test_add_func("/qdev/properties/static/default", test_static_prop); - g_test_add_func("/qdev/properties/static/global", test_static_globalprop); - g_test_add_func("/qdev/properties/dynamic/global", test_dynamic_globalprop); + g_test_add_func("/qdev/properties/static/default/subprocess", + test_static_prop_subprocess); + g_test_add_func("/qdev/properties/static/default", + test_static_prop); + + g_test_add_func("/qdev/properties/static/global/subprocess", + test_static_globalprop_subprocess); + g_test_add_func("/qdev/properties/static/global", + test_static_globalprop); + + g_test_add_func("/qdev/properties/dynamic/global/subprocess", + test_dynamic_globalprop_subprocess); + g_test_add_func("/qdev/properties/dynamic/global", + test_dynamic_globalprop); g_test_run(); From 45de81735b01b59fbc45803b2c1c8ca37cb55ead Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Fri, 8 Aug 2014 16:03:28 -0300 Subject: [PATCH 05/15] test-qdev-global-props: Initialize not_used=true for all props This will ensure we are actually testing the code which sets not_used=false when the property is used. Signed-off-by: Eduardo Habkost Acked-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/test-qdev-global-props.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c index 34223a7101..5488937dde 100644 --- a/tests/test-qdev-global-props.c +++ b/tests/test-qdev-global-props.c @@ -164,8 +164,8 @@ static void test_dynamic_globalprop_subprocess(void) { MyType *mt; static GlobalProperty props[] = { - { TYPE_DYNAMIC_PROPS, "prop1", "101" }, - { TYPE_DYNAMIC_PROPS, "prop2", "102" }, + { TYPE_DYNAMIC_PROPS, "prop1", "101", true }, + { TYPE_DYNAMIC_PROPS, "prop2", "102", true }, { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true }, {} }; @@ -180,6 +180,9 @@ static void test_dynamic_globalprop_subprocess(void) g_assert_cmpuint(mt->prop2, ==, 102); all_used = qdev_prop_check_global(); g_assert_cmpuint(all_used, ==, 1); + g_assert(!props[0].not_used); + g_assert(!props[1].not_used); + g_assert(props[2].not_used); } static void test_dynamic_globalprop(void) From 08ac80cd614ca4c81cc8799749977d5ff02d4ea5 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Fri, 8 Aug 2014 16:03:29 -0300 Subject: [PATCH 06/15] test-qdev-global-props: Test handling of hotpluggable and non-device types Ensure no warning will be printed for hotpluggable types, and warnings will be printed for non-device types. Signed-off-by: Eduardo Habkost Acked-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- tests/test-qdev-global-props.c | 55 ++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c index 5488937dde..26d8cb2da7 100644 --- a/tests/test-qdev-global-props.c +++ b/tests/test-qdev-global-props.c @@ -113,6 +113,9 @@ static void test_static_globalprop(void) #define DYNAMIC_TYPE(obj) \ OBJECT_CHECK(MyType, (obj), TYPE_DYNAMIC_PROPS) +#define TYPE_UNUSED_HOTPLUG "hotplug-type" +#define TYPE_UNUSED_NOHOTPLUG "nohotplug-type" + static void prop1_accessor(Object *obj, Visitor *v, void *opaque, @@ -159,6 +162,45 @@ static const TypeInfo dynamic_prop_type = { .class_init = dynamic_class_init, }; +static void hotplug_class_init(ObjectClass *oc, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(oc); + + dc->realize = NULL; + dc->hotpluggable = true; +} + +static const TypeInfo hotplug_type = { + .name = TYPE_UNUSED_HOTPLUG, + .parent = TYPE_DEVICE, + .instance_size = sizeof(MyType), + .instance_init = dynamic_instance_init, + .class_init = hotplug_class_init, +}; + +static void nohotplug_class_init(ObjectClass *oc, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(oc); + + dc->realize = NULL; + dc->hotpluggable = false; +} + +static const TypeInfo nohotplug_type = { + .name = TYPE_UNUSED_NOHOTPLUG, + .parent = TYPE_DEVICE, + .instance_size = sizeof(MyType), + .instance_init = dynamic_instance_init, + .class_init = nohotplug_class_init, +}; + +#define TYPE_NONDEVICE "nondevice-type" + +static const TypeInfo nondevice_type = { + .name = TYPE_NONDEVICE, + .parent = TYPE_OBJECT, +}; + /* Test setting of dynamic properties using global properties */ static void test_dynamic_globalprop_subprocess(void) { @@ -167,6 +209,10 @@ static void test_dynamic_globalprop_subprocess(void) { TYPE_DYNAMIC_PROPS, "prop1", "101", true }, { TYPE_DYNAMIC_PROPS, "prop2", "102", true }, { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true }, + /* .not_used=false to emulate what qdev_add_one_global() does: */ + { TYPE_UNUSED_HOTPLUG, "prop4", "104", false }, + { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true }, + { TYPE_NONDEVICE, "prop6", "106", true }, {} }; int all_used; @@ -183,6 +229,9 @@ static void test_dynamic_globalprop_subprocess(void) g_assert(!props[0].not_used); g_assert(!props[1].not_used); g_assert(props[2].not_used); + g_assert(!props[3].not_used); + g_assert(props[4].not_used); + g_assert(props[5].not_used); } static void test_dynamic_globalprop(void) @@ -192,6 +241,9 @@ static void test_dynamic_globalprop(void) g_test_trap_assert_stderr_unmatched("*prop1*"); g_test_trap_assert_stderr_unmatched("*prop2*"); g_test_trap_assert_stderr("*Warning: \"-global dynamic-prop-type-bad.prop3=103\" not used\n*"); + g_test_trap_assert_stderr_unmatched("*prop4*"); + g_test_trap_assert_stderr("*Warning: \"-global nohotplug-type.prop5=105\" not used\n*"); + g_test_trap_assert_stderr("*Warning: \"-global nondevice-type.prop6=106\" not used\n*"); g_test_trap_assert_stdout(""); } @@ -202,6 +254,9 @@ int main(int argc, char **argv) module_call_init(MODULE_INIT_QOM); type_register_static(&static_prop_type); type_register_static(&dynamic_prop_type); + type_register_static(&hotplug_type); + type_register_static(&nohotplug_type); + type_register_static(&nondevice_type); g_test_add_func("/qdev/properties/static/default/subprocess", test_static_prop_subprocess); From d828c430eb7dd481d6399f8b56e9641e47a40cea Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Fri, 8 Aug 2014 16:03:30 -0300 Subject: [PATCH 07/15] qdev: Rename qdev_prop_check_global() to qdev_prop_check_globals() Signed-off-by: Eduardo Habkost Acked-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/core/qdev-properties.c | 2 +- include/hw/qdev-properties.h | 2 +- tests/test-qdev-global-props.c | 2 +- vl.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 3d12560f43..9075453f2d 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -955,7 +955,7 @@ void qdev_prop_register_global_list(GlobalProperty *props) } } -int qdev_prop_check_global(void) +int qdev_prop_check_globals(void) { GlobalProperty *prop; int ret = 0; diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h index 77fe3a12b7..ae56ee59a4 100644 --- a/include/hw/qdev-properties.h +++ b/include/hw/qdev-properties.h @@ -177,7 +177,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value); void qdev_prop_register_global(GlobalProperty *prop); void qdev_prop_register_global_list(GlobalProperty *props); -int qdev_prop_check_global(void); +int qdev_prop_check_globals(void); void qdev_prop_set_globals(DeviceState *dev, Error **errp); void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, Error **errp); diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c index 26d8cb2da7..e1b008af11 100644 --- a/tests/test-qdev-global-props.c +++ b/tests/test-qdev-global-props.c @@ -224,7 +224,7 @@ static void test_dynamic_globalprop_subprocess(void) g_assert_cmpuint(mt->prop1, ==, 101); g_assert_cmpuint(mt->prop2, ==, 102); - all_used = qdev_prop_check_global(); + all_used = qdev_prop_check_globals(); g_assert_cmpuint(all_used, ==, 1); g_assert(!props[0].not_used); g_assert(!props[1].not_used); diff --git a/vl.c b/vl.c index 9c9acf5e65..2aaa558893 100644 --- a/vl.c +++ b/vl.c @@ -4541,7 +4541,7 @@ int main(int argc, char **argv, char **envp) } } - qdev_prop_check_global(); + qdev_prop_check_globals(); if (vmstate_dump_file) { /* dump and exit */ dump_vmstate_json_to_file(vmstate_dump_file); From b3ce84fea466f3bca2ff85d158744f00c0f429bd Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Fri, 8 Aug 2014 16:03:31 -0300 Subject: [PATCH 08/15] qdev: Move global validation to a single function Currently GlobalProperty.not_used=false has multiple meanings: * It may be a property for a hotpluggable device, which may or may not have been used by a device; * It may be a machine-type-provided property, which may or may not have been used by a device. * It may be a user-provided property that was actually not used by any device. Simplify the logic by having two separate fields: 'user_provided' and 'used'. This allows the entire global property validation logic to be contained in a single function, and allows more specific error messages. Signed-off-by: Eduardo Habkost Acked-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/core/qdev-properties-system.c | 18 +-------- hw/core/qdev-properties.c | 28 +++++++++++--- include/hw/qdev-core.h | 10 ++--- tests/test-qdev-global-props.c | 66 ++++++++++++++++++++++++++------ 4 files changed, 83 insertions(+), 39 deletions(-) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index ae0900f651..84caa1d694 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -388,28 +388,12 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd) static int qdev_add_one_global(QemuOpts *opts, void *opaque) { GlobalProperty *g; - ObjectClass *oc; g = g_malloc0(sizeof(*g)); g->driver = qemu_opt_get(opts, "driver"); g->property = qemu_opt_get(opts, "property"); g->value = qemu_opt_get(opts, "value"); - oc = object_class_dynamic_cast(object_class_by_name(g->driver), - TYPE_DEVICE); - if (oc) { - DeviceClass *dc = DEVICE_CLASS(oc); - - if (dc->hotpluggable) { - /* If hotpluggable then skip not_used checking. */ - g->not_used = false; - } else { - /* Maybe a typo. */ - g->not_used = true; - } - } else { - /* Maybe a typo. */ - g->not_used = true; - } + g->user_provided = true; qdev_prop_register_global(g); return 0; } diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 9075453f2d..66556d3bf9 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -961,13 +961,29 @@ int qdev_prop_check_globals(void) int ret = 0; QTAILQ_FOREACH(prop, &global_props, next) { - if (!prop->not_used) { + ObjectClass *oc; + DeviceClass *dc; + if (prop->used) { + continue; + } + if (!prop->user_provided) { + continue; + } + oc = object_class_by_name(prop->driver); + oc = object_class_dynamic_cast(oc, TYPE_DEVICE); + if (!oc) { + error_report("Warning: global %s.%s has invalid class name", + prop->driver, prop->property); + ret = 1; + continue; + } + dc = DEVICE_CLASS(oc); + if (!dc->hotpluggable && !prop->used) { + error_report("Warning: global %s.%s=%s not used", + prop->driver, prop->property, prop->value); + ret = 1; continue; } - ret = 1; - error_report("Warning: \"-global %s.%s=%s\" not used", - prop->driver, prop->property, prop->value); - } return ret; } @@ -983,7 +999,7 @@ void qdev_prop_set_globals_for_type(DeviceState *dev, const char *typename, if (strcmp(typename, prop->driver) != 0) { continue; } - prop->not_used = false; + prop->used = true; object_property_parse(OBJECT(dev), prop->value, prop->property, &err); if (err != NULL) { error_propagate(errp, err); diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 0799ff29b0..178fee2ef6 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -242,16 +242,16 @@ struct PropertyInfo { /** * GlobalProperty: - * @not_used: Track use of a global property. Defaults to false in all C99 - * struct initializations. - * - * This prevents reports of .compat_props when they are not used. + * @user_provided: Set to true if property comes from user-provided config + * (command-line or config file). + * @used: Set to true if property was used when initializing a device. */ typedef struct GlobalProperty { const char *driver; const char *property; const char *value; - bool not_used; + bool user_provided; + bool used; QTAILQ_ENTRY(GlobalProperty) next; } GlobalProperty; diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c index e1b008af11..0be98355c0 100644 --- a/tests/test-qdev-global-props.c +++ b/tests/test-qdev-global-props.c @@ -209,8 +209,7 @@ static void test_dynamic_globalprop_subprocess(void) { TYPE_DYNAMIC_PROPS, "prop1", "101", true }, { TYPE_DYNAMIC_PROPS, "prop2", "102", true }, { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true }, - /* .not_used=false to emulate what qdev_add_one_global() does: */ - { TYPE_UNUSED_HOTPLUG, "prop4", "104", false }, + { TYPE_UNUSED_HOTPLUG, "prop4", "104", true }, { TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true }, { TYPE_NONDEVICE, "prop6", "106", true }, {} @@ -226,12 +225,12 @@ static void test_dynamic_globalprop_subprocess(void) g_assert_cmpuint(mt->prop2, ==, 102); all_used = qdev_prop_check_globals(); g_assert_cmpuint(all_used, ==, 1); - g_assert(!props[0].not_used); - g_assert(!props[1].not_used); - g_assert(props[2].not_used); - g_assert(!props[3].not_used); - g_assert(props[4].not_used); - g_assert(props[5].not_used); + g_assert(props[0].used); + g_assert(props[1].used); + g_assert(!props[2].used); + g_assert(!props[3].used); + g_assert(!props[4].used); + g_assert(!props[5].used); } static void test_dynamic_globalprop(void) @@ -240,10 +239,50 @@ static void test_dynamic_globalprop(void) g_test_trap_assert_passed(); g_test_trap_assert_stderr_unmatched("*prop1*"); g_test_trap_assert_stderr_unmatched("*prop2*"); - g_test_trap_assert_stderr("*Warning: \"-global dynamic-prop-type-bad.prop3=103\" not used\n*"); + g_test_trap_assert_stderr("*Warning: global dynamic-prop-type-bad.prop3 has invalid class name\n*"); g_test_trap_assert_stderr_unmatched("*prop4*"); - g_test_trap_assert_stderr("*Warning: \"-global nohotplug-type.prop5=105\" not used\n*"); - g_test_trap_assert_stderr("*Warning: \"-global nondevice-type.prop6=106\" not used\n*"); + g_test_trap_assert_stderr("*Warning: global nohotplug-type.prop5=105 not used\n*"); + g_test_trap_assert_stderr("*Warning: global nondevice-type.prop6 has invalid class name\n*"); + g_test_trap_assert_stdout(""); +} + +/* Test setting of dynamic properties using user_provided=false properties */ +static void test_dynamic_globalprop_nouser_subprocess(void) +{ + MyType *mt; + static GlobalProperty props[] = { + { TYPE_DYNAMIC_PROPS, "prop1", "101" }, + { TYPE_DYNAMIC_PROPS, "prop2", "102" }, + { TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" }, + { TYPE_UNUSED_HOTPLUG, "prop4", "104" }, + { TYPE_UNUSED_NOHOTPLUG, "prop5", "105" }, + { TYPE_NONDEVICE, "prop6", "106" }, + {} + }; + int all_used; + + qdev_prop_register_global_list(props); + + mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS)); + qdev_init_nofail(DEVICE(mt)); + + g_assert_cmpuint(mt->prop1, ==, 101); + g_assert_cmpuint(mt->prop2, ==, 102); + all_used = qdev_prop_check_globals(); + g_assert_cmpuint(all_used, ==, 0); + g_assert(props[0].used); + g_assert(props[1].used); + g_assert(!props[2].used); + g_assert(!props[3].used); + g_assert(!props[4].used); + g_assert(!props[5].used); +} + +static void test_dynamic_globalprop_nouser(void) +{ + g_test_trap_subprocess("/qdev/properties/dynamic/global/nouser/subprocess", 0, 0); + g_test_trap_assert_passed(); + g_test_trap_assert_stderr(""); g_test_trap_assert_stdout(""); } @@ -273,6 +312,11 @@ int main(int argc, char **argv) g_test_add_func("/qdev/properties/dynamic/global", test_dynamic_globalprop); + g_test_add_func("/qdev/properties/dynamic/global/nouser/subprocess", + test_dynamic_globalprop_nouser_subprocess); + g_test_add_func("/qdev/properties/dynamic/global/nouser", + test_dynamic_globalprop_nouser); + g_test_run(); return 0; From abb4d5f2e2830b7a6dc4ddcc612dfab15e3a320d Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Thu, 4 Sep 2014 19:10:47 +0300 Subject: [PATCH 09/15] Revert "rng-egd: remove redundant free" This reverts commit 5e490b6a504912225dff0e520e1c6af68295d238. Cc: qemu-stable@nongnu.org Reviewed-by: Markus Armbruster Signed-off-by: Eduardo Habkost Signed-off-by: Michael S. Tsirkin --- backends/rng-egd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/backends/rng-egd.c b/backends/rng-egd.c index 25bb3b453b..2962795a8f 100644 --- a/backends/rng-egd.c +++ b/backends/rng-egd.c @@ -169,6 +169,7 @@ static void rng_egd_set_chardev(Object *obj, const char *value, Error **errp) if (b->opened) { error_set(errp, QERR_PERMISSION_DENIED); } else { + g_free(s->chr_name); s->chr_name = g_strdup(value); } } From 131c5221fe25a9547c4a388a3d26ff7fd14843e5 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 11 Sep 2014 18:32:51 +0300 Subject: [PATCH 10/15] virtio-net: drop assert on vm stop On vm stop, vm_running state set to stopped before device is notified, so callbacks can get envoked with vm_running = false; and this is not an error. Cc: qemu-stable@nongnu.org Acked-by: Jason Wang Signed-off-by: Michael S. Tsirkin --- hw/net/virtio-net.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 826a2a5fca..2040eac9a1 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1125,8 +1125,6 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) return num_packets; } - assert(vdev->vm_running); - if (q->async_tx.elem.out_num) { virtio_queue_set_notification(q->tx_vq, 0); return num_packets; From 9e8e8c48653471fa5fed447e388fdef57d4f6998 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 11 Sep 2014 18:42:02 +0300 Subject: [PATCH 11/15] Revert "virtio: don't call device on !vm_running" This reverts commit a1bc7b827e422e1ff065640d8ec5347c4aadfcd8. virtio: don't call device on !vm_running It turns out that virtio net assumes that vm_running is updated before device status callback in many places, so this change leads to asserts. Previous commit fixes the root issue that motivated a1bc7b827e422e1ff065640d8ec5347c4aadfcd8 differently, so there's no longer a need for this change. In the future, we might be able to drop checking vm_running completely, and check vm state directly. Reported-by: Dietmar Maurer Cc: qemu-stable@nongnu.org Acked-by: Jason Wang Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index ac222385d6..5c981801f3 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1108,10 +1108,7 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state) BusState *qbus = qdev_get_parent_bus(DEVICE(vdev)); VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); bool backend_run = running && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK); - - if (running) { - vdev->vm_running = running; - } + vdev->vm_running = running; if (backend_run) { virtio_set_status(vdev, vdev->status); @@ -1124,10 +1121,6 @@ static void virtio_vmstate_change(void *opaque, int running, RunState state) if (!backend_run) { virtio_set_status(vdev, vdev->status); } - - if (!running) { - vdev->vm_running = running; - } } void virtio_init(VirtIODevice *vdev, const char *name, From e43c0b2ea5574efb0bedebf6a7d05916eefeba52 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 11 Sep 2014 18:45:33 +0200 Subject: [PATCH 12/15] virtio-pci: enable bus master for old guests commit cc943c36faa192cd4b32af8fe5edb31894017d35 pci: Use bus master address space for delivering MSI/MSI-X messages breaks virtio-net for rhel6.[56] x86 guests because they don't enable bus mastering for virtio PCI devices. For the same reason, rhel6.[56] ppc64 guests cannot boot on a virtio-blk disk anymore. Old guests forgot to enable bus mastering, enable it automatically on DRIVER (guests use some devices before DRIVER_OK). Reported-by: Greg Kurz Reviewed-by: Greg Kurz Tested-by: Greg Kurz Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio-pci.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index ddb5da181c..a827cd41bf 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -314,6 +314,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) msix_unuse_all_vectors(&proxy->pci_dev); } + /* Linux before 2.6.34 drives the device without enabling + the PCI device bus master bit. Enable it automatically + for the guest. This is a PCI spec violation but so is + initiating DMA with bus master bit clear. */ + if (val == (VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER)) { + pci_default_write_config(&proxy->pci_dev, PCI_COMMAND, + proxy->pci_dev.config[PCI_COMMAND] | + PCI_COMMAND_MASTER, 1); + } + /* Linux before 2.6.34 sets the device as OK without enabling the PCI device bus master bit. In this case we need to disable some safety checks. */ From d8e80ae37a7acfea416ad9abbe76b453a73d9cc0 Mon Sep 17 00:00:00 2001 From: Damjan Marion Date: Thu, 11 Sep 2014 14:55:48 -0700 Subject: [PATCH 13/15] vhost-user: fix VIRTIO_NET_F_MRG_RXBUF negotiation Header length check should happen only if backend is kernel. For user backend there is no reason to reset this bit. vhost-user code does not define .has_vnet_hdr_len so VIRTIO_NET_F_MRG_RXBUF cannot be negotiated even if both sides support it. Signed-off-by: Damjan Marion Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin --- hw/net/vhost_net.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index b21e7a434f..77bb93e4d7 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -163,11 +163,11 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) if (r < 0) { goto fail; } - if (!qemu_has_vnet_hdr_len(options->net_backend, - sizeof(struct virtio_net_hdr_mrg_rxbuf))) { - net->dev.features &= ~(1 << VIRTIO_NET_F_MRG_RXBUF); - } if (backend_kernel) { + if (!qemu_has_vnet_hdr_len(options->net_backend, + sizeof(struct virtio_net_hdr_mrg_rxbuf))) { + net->dev.features &= ~(1 << VIRTIO_NET_F_MRG_RXBUF); + } if (~net->dev.features & net->dev.backend_features) { fprintf(stderr, "vhost lacks feature mask %" PRIu64 " for backend\n", From 4d43d3f3c8147ade184df9a1e9e82826edd39e19 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 11 Sep 2014 18:34:29 +0300 Subject: [PATCH 14/15] virtio-pci: fix migration for pci bus master Current support for bus master (clearing OK bit) together with the need to support guests which do not enable PCI bus mastering, leads to extra state in VIRTIO_PCI_FLAG_BUS_MASTER_BUG bit, which isn't robust in case of cross-version migration for the case when guests use the device before setting DRIVER_OK. Rip out VIRTIO_PCI_FLAG_BUS_MASTER_BUG and implement a simpler work-around: treat clearing of PCI_COMMAND as a virtio reset. Old guests never touch this bit so they will work. As reset clears device status, DRIVER and MASTER bits are now in sync, so we can fix up cross-version migration simply by synchronising them, without need to detect a buggy guest explicitly. Drop tracking VIRTIO_PCI_FLAG_BUS_MASTER_BUG completely. As reset makes the device quiescent, in the future we'll be able to drop checking OK bit in a bunch of places. Cc: Jason Wang Cc: Greg Kurz Signed-off-by: Michael S. Tsirkin --- hw/virtio/virtio-pci.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index a827cd41bf..f5608140f9 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -86,9 +86,6 @@ * 12 is historical, and due to x86 page size. */ #define VIRTIO_PCI_QUEUE_ADDR_SHIFT 12 -/* Flags track per-device state like workarounds for quirks in older guests. */ -#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0) - static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size, VirtIOPCIProxy *dev); @@ -323,14 +320,6 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) proxy->pci_dev.config[PCI_COMMAND] | PCI_COMMAND_MASTER, 1); } - - /* Linux before 2.6.34 sets the device as OK without enabling - the PCI device bus master bit. In this case we need to disable - some safety checks. */ - if ((val & VIRTIO_CONFIG_S_DRIVER_OK) && - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; - } break; case VIRTIO_MSI_CONFIG_VECTOR: msix_vector_unuse(&proxy->pci_dev, vdev->config_vector); @@ -480,13 +469,18 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); + uint8_t cmd = proxy->pci_dev.config[PCI_COMMAND]; + pci_default_write_config(pci_dev, address, val, len); if (range_covers_byte(address, len, PCI_COMMAND) && !(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER) && - !(proxy->flags & VIRTIO_PCI_FLAG_BUS_MASTER_BUG)) { + (cmd & PCI_COMMAND_MASTER)) { + /* Bus driver disables bus mastering - make it act + * as a kind of reset to render the device quiescent. */ virtio_pci_stop_ioeventfd(proxy); - virtio_set_status(vdev, vdev->status & ~VIRTIO_CONFIG_S_DRIVER_OK); + virtio_reset(vdev); + msix_unuse_all_vectors(&proxy->pci_dev); } } @@ -895,11 +889,19 @@ static void virtio_pci_vmstate_change(DeviceState *d, bool running) VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); if (running) { - /* Try to find out if the guest has bus master disabled, but is - in ready state. Then we have a buggy guest OS. */ - if ((vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && - !(proxy->pci_dev.config[PCI_COMMAND] & PCI_COMMAND_MASTER)) { - proxy->flags |= VIRTIO_PCI_FLAG_BUS_MASTER_BUG; + /* Linux before 2.6.34 drives the device without enabling + the PCI device bus master bit. Enable it automatically + for the guest. This is a PCI spec violation but so is + initiating DMA with bus master bit clear. + Note: this only makes a difference when migrating + across QEMU versions from an old QEMU, as for new QEMU + bus master and driver bits are always in sync. + TODO: consider enabling conditionally for compat machine types. */ + if (vdev->status & (VIRTIO_CONFIG_S_ACKNOWLEDGE | + VIRTIO_CONFIG_S_DRIVER)) { + pci_default_write_config(&proxy->pci_dev, PCI_COMMAND, + proxy->pci_dev.config[PCI_COMMAND] | + PCI_COMMAND_MASTER, 1); } virtio_pci_start_ioeventfd(proxy); } else { @@ -1040,7 +1042,6 @@ static void virtio_pci_reset(DeviceState *qdev) virtio_pci_stop_ioeventfd(proxy); virtio_bus_reset(bus); msix_unuse_all_vectors(&proxy->pci_dev); - proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG; } static Property virtio_pci_properties[] = { From 438f92ee9f6a4f78f8adcc399809e252b6da72a2 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Thu, 18 Sep 2014 16:32:07 +0300 Subject: [PATCH 15/15] pc: leave more space for BIOS allocations Since QEMU 2.1, we are allocating more space for ACPI tables, so no space is left after initrd for the BIOS to allocate memory. Besides ACPI tables, there are a few other uses of high memory in SeaBIOS: SMBIOS tables and USB drivers use it in particular. These uses allocate a very small amount of memory. Malloc metadata also lives there. So we need _some_ extra padding there to avoid initrd breakage, but not much. John Snow found a case where RHEL5 was broken by the recent change to ACPI_TABLE_SIZE; in his case 4KB of extra padding are fine, but just to be safe I am adding 32KB, which is roughly the same amount of padding that was left by QEMU 2.0 and earlier. Move initrd to leave some space for the BIOS. Cc: qemu-stable@nongnu.org Signed-off-by: Paolo Bonzini Reported-by: John Snow Signed-off-by: Michael S. Tsirkin --- hw/i386/pc.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index b6c9b61801..9ac5bd208d 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -72,8 +72,10 @@ #define DPRINTF(fmt, ...) #endif -/* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables. */ -unsigned acpi_data_size = 0x20000; +/* Leave a chunk of memory at the top of RAM for the BIOS ACPI tables + * (128K) and other BIOS datastructures (less than 4K reported to be used at + * the moment, 32K should be enough for a while). */ +unsigned acpi_data_size = 0x20000 + 0x8000; void pc_set_legacy_acpi_data_size(void) { acpi_data_size = 0x10000;