[Qemu-devel] [PULL v3 00/15] pci, pc, virtio, misc bugfixes

[Qemu-devel] [PULL v3 00/15] pci, pc, virtio, misc bugfixes

[Michael S. Tsirkin]

The following changes since commit 4c24f4004089a308c5de8ed720cf6bd1746aedd8:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20140912' into staging (2014-09-12 15:12:26 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 438f92ee9f6a4f78f8adcc399809e252b6da72a2:

  pc: leave more space for BIOS allocations (2014-09-18 21:51:24 +0300)

----------------------------------------------------------------
pci, pc, virtio, misc bugfixes

A bunch of bugfixes - some of these will make sense for 2.1.2
I put Cc: qemu-stable included where appropriate.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Damjan Marion (1):
      vhost-user: fix VIRTIO_NET_F_MRG_RXBUF negotiation

Eduardo Habkost (8):
      hw/machine: Free old values of string properties
      test-qdev-global-props: Trivial comment fix
      test-qdev-global-props: Run tests on subprocess
      test-qdev-global-props: Initialize not_used=true for all props
      test-qdev-global-props: Test handling of hotpluggable and non-device types
      qdev: Rename qdev_prop_check_global() to qdev_prop_check_globals()
      qdev: Move global validation to a single function
      Revert "rng-egd: remove redundant free"

Michael S. Tsirkin (6):
      tests: disable global props test for old glib
      virtio-net: drop assert on vm stop
      Revert "virtio: don't call device on !vm_running"
      virtio-pci: enable bus master for old guests
      virtio-pci: fix migration for pci bus master
      pc: leave more space for BIOS allocations

 configure                        |   9 +++
 include/hw/qdev-core.h           |  10 +--
 include/hw/qdev-properties.h     |   2 +-
 backends/rng-egd.c               |   1 +
 hw/core/machine.c                |   8 ++
 hw/core/qdev-properties-system.c |  18 +----
 hw/core/qdev-properties.c        |  30 ++++++--
 hw/i386/pc.c                     |   6 +-
 hw/net/vhost_net.c               |   8 +-
 hw/net/virtio-net.c              |   2 -
 hw/virtio/virtio-pci.c           |  45 ++++++-----
 hw/virtio/virtio.c               |   9 +--
 tests/test-qdev-global-props.c   | 159 ++++++++++++++++++++++++++++++++++++---
 vl.c                             |   2 +-
 tests/Makefile                   |   2 +-
 15 files changed, 236 insertions(+), 75 deletions(-)

[Qemu-devel] [PULL v3 01/15] hw/machine: Free old values of string properties

[Michael S. Tsirkin]

From: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Amos Kong <akong@redhat.com>
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 f0046d6..7f3418c 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);
 }
 
-- 
MST

[Qemu-devel] [PULL v3 02/15] test-qdev-global-props: Trivial comment fix

[Michael S. Tsirkin]

From: Eduardo Habkost <ehabkost@redhat.com>

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 2bef04c..e1a1317 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;
-- 
MST

[Qemu-devel] [PULL v3 03/15] tests: disable global props test for old glib

[Michael S. Tsirkin]

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 <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 configure      | 9 +++++++++
 tests/Makefile | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 961bf6f..6f1284a 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 d5db97b..a5e3d0c 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)
-- 
MST

[Qemu-devel] [PULL v3 04/15] test-qdev-global-props: Run tests on subprocess

[Michael S. Tsirkin]

From: Eduardo Habkost <ehabkost@redhat.com>

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 <ehabkost@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 e1a1317..34223a7 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();
 
-- 
MST

[Qemu-devel] [PULL v3 05/15] test-qdev-global-props: Initialize not_used=true for all props

[Michael S. Tsirkin]

From: Eduardo Habkost <ehabkost@redhat.com>

This will ensure we are actually testing the code which sets
not_used=false when the property is used.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 34223a7..5488937 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)
-- 
MST

[Qemu-devel] [PULL v3 06/15] test-qdev-global-props: Test handling of hotpluggable and non-device types

[Michael S. Tsirkin]

From: Eduardo Habkost <ehabkost@redhat.com>

Ensure no warning will be printed for hotpluggable types, and warnings
will be printed for non-device types.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 5488937..26d8cb2 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);
-- 
MST

[Qemu-devel] [PULL v3 07/15] qdev: Rename qdev_prop_check_global() to qdev_prop_check_globals()

[Michael S. Tsirkin]

From: Eduardo Habkost <ehabkost@redhat.com>

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/qdev-properties.h   | 2 +-
 hw/core/qdev-properties.c      | 2 +-
 tests/test-qdev-global-props.c | 2 +-
 vl.c                           | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 77fe3a1..ae56ee5 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/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 3d12560..9075453 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/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index 26d8cb2..e1b008a 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 9c9acf5..2aaa558 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);
-- 
MST

[Qemu-devel] [PULL v3 08/15] qdev: Move global validation to a single function

[Michael S. Tsirkin]

From: Eduardo Habkost <ehabkost@redhat.com>

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 <ehabkost@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/qdev-core.h           | 10 +++---
 hw/core/qdev-properties-system.c | 18 +----------
 hw/core/qdev-properties.c        | 28 +++++++++++++----
 tests/test-qdev-global-props.c   | 66 +++++++++++++++++++++++++++++++++-------
 4 files changed, 83 insertions(+), 39 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 0799ff2..178fee2 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/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index ae0900f..84caa1d 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 9075453..66556d3 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/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index e1b008a..0be9835 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;
-- 
MST

[Qemu-devel] [PULL v3 09/15] Revert "rng-egd: remove redundant free"

[Michael S. Tsirkin]

From: Eduardo Habkost <ehabkost@redhat.com>

This reverts commit 5e490b6a504912225dff0e520e1c6af68295d238.

Cc: qemu-stable@nongnu.org
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 backends/rng-egd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 25bb3b4..2962795 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);
     }
 }
-- 
MST

[Qemu-devel] [PULL v3 10/15] virtio-net: drop assert on vm stop

[Michael S. Tsirkin]

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 <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 826a2a5..2040eac 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;
-- 
MST

[Qemu-devel] [PULL v3 11/15] Revert "virtio: don't call device on !vm_running"

[Michael S. Tsirkin]

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 <dietmar@proxmox.com>
Cc: qemu-stable@nongnu.org
Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 ac22238..5c98180 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,
-- 
MST

[Qemu-devel] [PULL v3 12/15] virtio-pci: enable bus master for old guests

[Michael S. Tsirkin]

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 <gkurz@linux.vnet.ibm.com>
Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 ddb5da1..a827cd4 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. */
-- 
MST

[Qemu-devel] [PULL v3 13/15] vhost-user: fix VIRTIO_NET_F_MRG_RXBUF negotiation

[Michael S. Tsirkin]

From: Damjan Marion <damarion@cisco.com>

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 <damarion@cisco.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 b21e7a4..77bb93e 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",
-- 
MST

[Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Michael S. Tsirkin]

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 <jasowang@redhat.com>
Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 a827cd4..f560814 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[] = {
-- 
MST

Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Greg Kurz]

On Thu, 18 Sep 2014 21:54:58 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <jasowang@redhat.com>
> Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---

Hi,

This commit prevents pseries to boot. SLOF complains with the following messages:

Trying to load:  from: /pci@800000020000000/scsi@0 ... virtioblk_read failed! status = 255
virtioblk_read failed! status = 255
virtioblk_read failed! status = 255
...

I'll try to debug some more.

Cheers.

--
Greg

>  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 a827cd4..f560814 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[] = {

Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Michael S. Tsirkin]

On Mon, Sep 22, 2014 at 07:28:57PM +0200, Greg Kurz wrote:
> On Thu, 18 Sep 2014 21:54:58 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <jasowang@redhat.com>
> > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> 
> Hi,
> 
> This commit prevents pseries to boot. SLOF complains with the following messages:
> 
> Trying to load:  from: /pci@800000020000000/scsi@0 ... virtioblk_read failed! status = 255
> virtioblk_read failed! status = 255
> virtioblk_read failed! status = 255
> ...
> 
> I'll try to debug some more.
> 
> Cheers.

A trace recording all reads and writes of pci status
and virtio status would help.
Thanks!

> --
> Greg
> 
> >  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 a827cd4..f560814 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[] = {

Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Greg Kurz]

On Tue, 23 Sep 2014 07:26:32 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Sep 22, 2014 at 07:28:57PM +0200, Greg Kurz wrote:
> > On Thu, 18 Sep 2014 21:54:58 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > 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 <jasowang@redhat.com>
> > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > 
> > Hi,
> > 
> > This commit prevents pseries to boot. SLOF complains with the following messages:
> > 
> > Trying to load:  from: /pci@800000020000000/scsi@0 ... virtioblk_read failed! status = 255
> > virtioblk_read failed! status = 255
> > virtioblk_read failed! status = 255
> > ...
> > 
> > I'll try to debug some more.
> > 
> > Cheers.
> 
> A trace recording all reads and writes of pci status
> and virtio status would help.
> Thanks!
> 
> > --
> > Greg
> > 

I've added the traces for reads/writes of PCI_COMMAND, PCI_STATUS and
VIRTIO_PCI_STATUS. Also an extra trace in your patch where virtio_reset
is called in case BM gets disabled.



SLOF **********************************************************************
QEMU Starting
 Build Date = Jul  3 2014 23:12:05
 FW Version = git-f284ab3f03ae69a2
 Press "s" to enter Open Firmware.

Populating /vdevice methods
Populating /vdevice/vty@71000000
Populating /vdevice/nvram@71000001
Populating /pci@800000020000000
 Adapters on 0800000020000000
       pci_default_read_config: addr = 0x4 val = 0x100000 len= 4
      pci_default_write_config: addr = 0x4 val = 0x100000 len= 4
       pci_default_read_config: addr = 0x4 val = 0x0 len= 2
      pci_default_write_config: addr = 0x4 val = 0x140 len= 2
                     00 0000 (D) : 1af4 1001    virtio [ block ]
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
      pci_default_write_config: addr = 0x4 val = 0x100104 len= 4
       pci_default_read_config: addr = 0x4 val = 0x104 len= 2
      pci_default_write_config: addr = 0x4 val = 0x106 len= 2
       pci_default_read_config: addr = 0x4 val = 0x106 len= 2
      pci_default_write_config: addr = 0x4 val = 0x107 len= 2

BM gets enabled

       pci_default_read_config: addr = 0x4 val = 0x100107 len= 4
      pci_default_write_config: addr = 0x4 val = 0x100100 len= 4
           virtio_write_config: RESET

device gets disabled => virtio_reset() gets called

       pci_default_read_config: addr = 0x4 val = 0x100000 len= 4
      pci_default_write_config: addr = 0x4 val = 0x100000 len= 4
       pci_default_read_config: addr = 0x4 val = 0x0 len= 2
      pci_default_write_config: addr = 0x4 val = 0x140 len= 2
                     00 0800 (D) : 1af4 1000    virtio [ net ]
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
       pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
       pci_default_read_config: addr = 0x4 val = 0x100 len= 2
      pci_default_write_config: addr = 0x4 val = 0x101 len= 2
       pci_default_read_config: addr = 0x4 val = 0x100101 len= 4
      pci_default_write_config: addr = 0x4 val = 0x100100 len= 4
No NVRAM common partition, re-initializing...
Scanning USB 
Using default console: /vdevice/vty@71000000
     
  Welcome to Open Firmware

  Copyright (c) 2004, 2011 IBM Corporation All rights reserved.
  This program and the accompanying materials are made available
  under the terms of the BSD License available at
  http://www.opensource.org/licenses/bsd-license.php


Trying to load:  from: /pci@800000020000000/scsi@0 ...        pci_default_read_config: addr = 0x4 val = 0x100100 len= 4
      pci_default_write_config: addr = 0x4 val = 0x100104 len= 4
       pci_default_read_config: addr = 0x4 val = 0x104 len= 2
      pci_default_write_config: addr = 0x4 val = 0x106 len= 2
       pci_default_read_config: addr = 0x4 val = 0x106 len= 2
      pci_default_write_config: addr = 0x4 val = 0x107 len= 2
           virtio_ioport_write: VIRTIO_PCI_STATUS = 0x1
           virtio_ioport_write: VIRTIO_PCI_STATUS = 0x3
      pci_default_write_config: addr = 0x4 val = 0x7 len= 1
           virtio_ioport_write: VIRTIO_PCI_STATUS = 0x7
virtioblk_read failed! status = 255
virtioblk_read failed! status = 255
virtioblk_read failed! status = 255
...




The guest boots well without your patch. Here's a diff of
the outputs for both runs:

[greg@alize ~]$ diff virtio-pci-ok.log virtio-pci-broken.log 
28a29
>            virtio_write_config: RESET
64,66c65,67
<        pci_default_read_config: addr = 0x4 val = 0x100107 len= 4
<       pci_default_write_config: addr = 0x4 val = 0x100100 len= 4
<   Successfully loaded
---
> virtioblk_read failed! status = 255
> virtioblk_read failed! status = 255
> virtioblk_read failed! status = 255

FWIW, if I comment the call to virtio_reset() when BM gets disabled, the guest
boots... I don't why SLOF doesn't like the device to be reset during the PCI
bus probing though... A suivre.

> > >  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 a827cd4..f560814 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[] = {
> 

Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Nikunj A Dadhania]

Hi Alex/Peter,

The below patch is already been picked in master and ppc-next and has
broken pseries booting from virtio-blk device

Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> On Tue, 23 Sep 2014 07:26:32 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> > 
>> > This commit prevents pseries to boot. SLOF complains with the following messages:
>> > 
>> > Trying to load:  from: /pci@800000020000000/scsi@0 ... virtioblk_read failed! status = 255
>> > virtioblk_read failed! status = 255
>> > virtioblk_read failed! status = 255
>> > ...
>> > 
>> > I'll try to debug some more.
>> > 

>> > > @@ -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);
>> > >      }
>> > >  }
>> > > 
>> 

Regards
Nikunj

Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Michael S. Tsirkin]

On Mon, Sep 22, 2014 at 07:28:57PM +0200, Greg Kurz wrote:
> On Thu, 18 Sep 2014 21:54:58 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <jasowang@redhat.com>
> > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> 
> Hi,
> 
> This commit prevents pseries to boot.

OK I'll revert for now.

> SLOF complains with the following messages:
> 
> Trying to load:  from: /pci@800000020000000/scsi@0 ... virtioblk_read failed! status = 255
> virtioblk_read failed! status = 255
> virtioblk_read failed! status = 255
> ...
> 
> I'll try to debug some more.
> 
> Cheers.
> 
> --
> Greg
> 
> >  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 a827cd4..f560814 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[] = {

Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Greg Kurz]

On Mon, 29 Sep 2014 19:15:05 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Sep 22, 2014 at 07:28:57PM +0200, Greg Kurz wrote:
> > On Thu, 18 Sep 2014 21:54:58 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > 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 <jasowang@redhat.com>
> > > Cc: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > 
> > Hi,
> > 
> > This commit prevents pseries to boot.
> 
> OK I'll revert for now.
> 

It may be possible to change the way SLOF configures the virtq so
that it wouldn't suffer from the device being reset. We have some
time left before 2.2. I have a tentative patch for SLOF that I'll
be glad to share with Nikunj ASAP.

> > SLOF complains with the following messages:
> > 
> > Trying to load:  from: /pci@800000020000000/scsi@0 ... virtioblk_read failed! status = 255
> > virtioblk_read failed! status = 255
> > virtioblk_read failed! status = 255
> > ...
> > 
> > I'll try to debug some more.
> > 
> > Cheers.
> > 
> > --
> > Greg
> > 
> > >  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 a827cd4..f560814 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[] = {
> 

Re: [Qemu-devel] [PULL v3 14/15] virtio-pci: fix migration for pci bus master

[Nikunj A Dadhania]

Greg Kurz <gkurz@linux.vnet.ibm.com> writes:
> On Mon, 29 Sep 2014 19:15:05 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> It may be possible to change the way SLOF configures the virtq so
> that it wouldn't suffer from the device being reset. We have some
> time left before 2.2. I have a tentative patch for SLOF that I'll
> be glad to share with Nikunj ASAP.

virtio-block is straight forward. I think we will have similar problem
with virtio-scsi as well. That driver will be tricky as it is
inter-linked with generic scsi code in SLOF.

Regards
Nikunj

[Qemu-devel] [PULL v3 15/15] pc: leave more space for BIOS allocations

[Michael S. Tsirkin]

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 <pbonzini@redhat.com>
Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 b6c9b61..9ac5bd2 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;
-- 
MST

Re: [Qemu-devel] [PULL v3 00/15] pci, pc, virtio, misc bugfixes

[Peter Maydell]

On 18 September 2014 11:54, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit 4c24f4004089a308c5de8ed720cf6bd1746aedd8:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20140912' into staging (2014-09-12 15:12:26 +0100)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 438f92ee9f6a4f78f8adcc399809e252b6da72a2:
>
>   pc: leave more space for BIOS allocations (2014-09-18 21:51:24 +0300)
>
> ----------------------------------------------------------------
> pci, pc, virtio, misc bugfixes
>
> A bunch of bugfixes - some of these will make sense for 2.1.2
> I put Cc: qemu-stable included where appropriate.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


Applied, thanks.

-- PMM