All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/2] tests: acpi: improve tests reproducability
@ 2019-07-08  9:24 Igor Mammedov
  2019-07-12 15:36   ` [Qemu-devel] [PULL 4/8] " Michael S. Tsirkin
  2019-07-12 15:36   ` [Qemu-devel] [PULL 5/8] " Michael S. Tsirkin
  0 siblings, 2 replies; 33+ messages in thread
From: Igor Mammedov @ 2019-07-08  9:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: mst

ChangeLog since v1:
  * fixup commit message in 2/2
  * pickup Reviewed-bys and repost as v3

series should help to make tests more reproducable and not depend
on IASL being installed. IASL will be required only is user needs
to get textual diff of mismatching files.


Igor Mammedov (2):
  tests: acpi: do not require IASL for dumping AML blobs
  tests: acpi: do not skip tests when IASL is not installed

 tests/bios-tables-test.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

-- 
2.18.1



^ permalink raw reply	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PATCH v3 1/2] tests: acpi: do not require IASL for dumping AML blobs
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Igor Mammedov @ 2019-07-08  9:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: mst

IASL isn't needed when dumping ACPI tables from guest for
rebuild purposes. So move this part out from IASL branch.

Makes rebuild-expected-aml.sh work without IASL installed
on host.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 tests/bios-tables-test.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d863233fe9..13bd166b81 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -597,12 +597,10 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_rxsdt_table(data);
     test_acpi_fadt_table(data);
 
-    if (iasl) {
-        if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
-            dump_aml_files(data, true);
-        } else {
-            test_acpi_asl(data);
-        }
+    if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
+        dump_aml_files(data, true);
+    } else if (iasl) {
+        test_acpi_asl(data);
     }
 
     /*
-- 
2.18.1



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PATCH v3 2/2] tests: acpi: do not skip tests when IASL is not installed
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Igor Mammedov @ 2019-07-08  9:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: mst

tests do binary comparision so we can check tables without
IASL. Move IASL condition right before decompilation step
and skip it if IASL is not installed.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2:
 - fix typo in commit message
     Eric Auger <eric.auger@redhat.com>


 tests/bios-tables-test.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 13bd166b81..a356ac3489 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -389,6 +389,14 @@ static void test_acpi_asl(test_data *data)
         all_tables_match = all_tables_match &&
             test_acpi_find_diff_allowed(exp_sdt);
 
+        /*
+         *  don't try to decompile if IASL isn't present, in this case user
+         * will just 'get binary file mismatch' warnings and test failure
+         */
+        if (!iasl) {
+            continue;
+        }
+
         err = load_asl(data->tables, sdt);
         asl = normalize_asl(sdt->asl);
 
@@ -431,6 +439,11 @@ static void test_acpi_asl(test_data *data)
         g_string_free(asl, true);
         g_string_free(exp_asl, true);
     }
+    if (!iasl && !all_tables_match) {
+        fprintf(stderr, "to see ASL diff between mismatched files install IASL,"
+                " rebuild QEMU from scratch and re-run tests with V=1"
+                " environment variable set");
+    }
     g_assert(all_tables_match);
 
     free_test_data(&exp_data);
@@ -599,7 +612,7 @@ static void test_acpi_one(const char *params, test_data *data)
 
     if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
         dump_aml_files(data, true);
-    } else if (iasl) {
+    } else {
         test_acpi_asl(data);
     }
 
-- 
2.18.1



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PATCH] virtio-balloon: fix QEMU 4.0 config size migration incompatibility
@ 2019-07-12 15:36 ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Stefan Hajnoczi @ 2019-07-10 14:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Wolfgang Bumiller, Eduardo Habkost, Michael S. Tsirkin,
	Dr. David Alan Gilbert, Stefan Hajnoczi

The virtio-balloon config size changed in QEMU 4.0 even for existing
machine types.  Migration from QEMU 3.1 to 4.0 can fail in some
circumstances with the following error:

  qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: a1 device: 1 cmask: ff wmask: c0 w1cmask:0

This happens because the virtio-balloon config size affects the VIRTIO
Legacy I/O Memory PCI BAR size.

Introduce a qdev property called "qemu-4-0-config-size" and enable it
only for the QEMU 4.0 machine types.  This way <4.0 machine types use
the old size, 4.0 uses the larger size, and >4.0 machine types use the
appropriate size depending on enabled virtio-balloon features.

Live migration to and from old QEMUs to QEMU 4.1 works again as long as
a versioned machine type is specified (do not use just "pc"!).

Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/virtio/virtio-balloon.h |  2 ++
 hw/core/machine.c                  |  2 ++
 hw/virtio/virtio-balloon.c         | 28 +++++++++++++++++++++++++---
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 1afafb12f6..5a99293a45 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -71,6 +71,8 @@ typedef struct VirtIOBalloon {
     int64_t stats_poll_interval;
     uint32_t host_features;
     PartiallyBalloonedPage *pbp;
+
+    bool qemu_4_0_config_size;
 } VirtIOBalloon;
 
 #endif
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2be19ec0cd..c4ead16010 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,6 +34,7 @@ GlobalProperty hw_compat_4_0[] = {
     { "virtio-vga",     "edid", "false" },
     { "virtio-gpu-pci", "edid", "false" },
     { "virtio-device", "use-started", "false" },
+    { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
 };
 const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
 
@@ -49,6 +50,7 @@ GlobalProperty hw_compat_3_1[] = {
     { "usb-tablet", "serial", "42" },
     { "virtio-blk-device", "discard", "false" },
     { "virtio-blk-device", "write-zeroes", "false" },
+    { "virtio-balloon-device", "qemu-4-0-config-size", "false" },
 };
 const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
 
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 11fad86d64..e85d1c0d5c 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -615,6 +615,22 @@ virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
     return 0;
 }
 
+static size_t virtio_balloon_config_size(VirtIOBalloon *s)
+{
+    uint64_t features = s->host_features;
+
+    if (s->qemu_4_0_config_size) {
+        return sizeof(struct virtio_balloon_config);
+    }
+    if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON)) {
+        return sizeof(struct virtio_balloon_config);
+    }
+    if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+        return offsetof(struct virtio_balloon_config, poison_val);
+    }
+    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
+}
+
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
@@ -635,7 +651,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
     }
 
     trace_virtio_balloon_get_config(config.num_pages, config.actual);
-    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
+    memcpy(config_data, &config, virtio_balloon_config_size(dev));
 }
 
 static int build_dimm_list(Object *obj, void *opaque)
@@ -679,7 +695,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
     uint32_t oldactual = dev->actual;
     ram_addr_t vm_ram_size = get_current_ram_size();
 
-    memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
+    memcpy(&config, config_data, virtio_balloon_config_size(dev));
     dev->actual = le32_to_cpu(config.actual);
     if (dev->actual != oldactual) {
         qapi_event_send_balloon_change(vm_ram_size -
@@ -766,7 +782,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     int ret;
 
     virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
-                sizeof(struct virtio_balloon_config));
+                virtio_balloon_config_size(s));
 
     ret = qemu_add_balloon_handler(virtio_balloon_to_target,
                                    virtio_balloon_stat, s);
@@ -897,6 +913,12 @@ static Property virtio_balloon_properties[] = {
                     VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
     DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
                     VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
+    /* QEMU 4.0 accidentally changed the config size even when free-page-hint
+     * is disabled, resulting in QEMU 3.1 migration incompatibility.  This
+     * property retains this quirk for QEMU 4.1 machine types.
+     */
+    DEFINE_PROP_BOOL("qemu-4-0-config-size", VirtIOBalloon,
+                     qemu_4_0_config_size, false),
     DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
                      IOThread *),
     DEFINE_PROP_END_OF_LIST(),
-- 
2.21.0



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH] virtio-balloon: fix QEMU 4.0 config size migration incompatibility
  2019-07-12 15:36 ` [Qemu-devel] [PULL 3/8] " Michael S. Tsirkin
  (?)
@ 2019-07-10 15:24 ` Dr. David Alan Gilbert
  -1 siblings, 0 replies; 33+ messages in thread
From: Dr. David Alan Gilbert @ 2019-07-10 15:24 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Wolfgang Bumiller, Michael S. Tsirkin, qemu-devel, Eduardo Habkost

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> The virtio-balloon config size changed in QEMU 4.0 even for existing
> machine types.  Migration from QEMU 3.1 to 4.0 can fail in some
> circumstances with the following error:
> 
>   qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: a1 device: 1 cmask: ff wmask: c0 w1cmask:0
> 
> This happens because the virtio-balloon config size affects the VIRTIO
> Legacy I/O Memory PCI BAR size.
> 
> Introduce a qdev property called "qemu-4-0-config-size" and enable it
> only for the QEMU 4.0 machine types.  This way <4.0 machine types use
> the old size, 4.0 uses the larger size, and >4.0 machine types use the
> appropriate size depending on enabled virtio-balloon features.
> 
> Live migration to and from old QEMUs to QEMU 4.1 works again as long as
> a versioned machine type is specified (do not use just "pc"!).
> 
> Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
   Tried 3.1->head and back
   Tried head<->head (default options)
     in both cases I tried ballooning the guest up and down

Dave


> ---
>  include/hw/virtio/virtio-balloon.h |  2 ++
>  hw/core/machine.c                  |  2 ++
>  hw/virtio/virtio-balloon.c         | 28 +++++++++++++++++++++++++---
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 1afafb12f6..5a99293a45 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -71,6 +71,8 @@ typedef struct VirtIOBalloon {
>      int64_t stats_poll_interval;
>      uint32_t host_features;
>      PartiallyBalloonedPage *pbp;
> +
> +    bool qemu_4_0_config_size;
>  } VirtIOBalloon;
>  
>  #endif
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2be19ec0cd..c4ead16010 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -34,6 +34,7 @@ GlobalProperty hw_compat_4_0[] = {
>      { "virtio-vga",     "edid", "false" },
>      { "virtio-gpu-pci", "edid", "false" },
>      { "virtio-device", "use-started", "false" },
> +    { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
>  };
>  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
>  
> @@ -49,6 +50,7 @@ GlobalProperty hw_compat_3_1[] = {
>      { "usb-tablet", "serial", "42" },
>      { "virtio-blk-device", "discard", "false" },
>      { "virtio-blk-device", "write-zeroes", "false" },
> +    { "virtio-balloon-device", "qemu-4-0-config-size", "false" },
>  };
>  const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
>  
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 11fad86d64..e85d1c0d5c 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -615,6 +615,22 @@ virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
>      return 0;
>  }
>  
> +static size_t virtio_balloon_config_size(VirtIOBalloon *s)
> +{
> +    uint64_t features = s->host_features;
> +
> +    if (s->qemu_4_0_config_size) {
> +        return sizeof(struct virtio_balloon_config);
> +    }
> +    if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON)) {
> +        return sizeof(struct virtio_balloon_config);
> +    }
> +    if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> +        return offsetof(struct virtio_balloon_config, poison_val);
> +    }
> +    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
> +}
> +
>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  {
>      VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> @@ -635,7 +651,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>      }
>  
>      trace_virtio_balloon_get_config(config.num_pages, config.actual);
> -    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
> +    memcpy(config_data, &config, virtio_balloon_config_size(dev));
>  }
>  
>  static int build_dimm_list(Object *obj, void *opaque)
> @@ -679,7 +695,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>      uint32_t oldactual = dev->actual;
>      ram_addr_t vm_ram_size = get_current_ram_size();
>  
> -    memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
> +    memcpy(&config, config_data, virtio_balloon_config_size(dev));
>      dev->actual = le32_to_cpu(config.actual);
>      if (dev->actual != oldactual) {
>          qapi_event_send_balloon_change(vm_ram_size -
> @@ -766,7 +782,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      int ret;
>  
>      virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
> -                sizeof(struct virtio_balloon_config));
> +                virtio_balloon_config_size(s));
>  
>      ret = qemu_add_balloon_handler(virtio_balloon_to_target,
>                                     virtio_balloon_stat, s);
> @@ -897,6 +913,12 @@ static Property virtio_balloon_properties[] = {
>                      VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
>      DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
>                      VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
> +    /* QEMU 4.0 accidentally changed the config size even when free-page-hint
> +     * is disabled, resulting in QEMU 3.1 migration incompatibility.  This
> +     * property retains this quirk for QEMU 4.1 machine types.
> +     */
> +    DEFINE_PROP_BOOL("qemu-4-0-config-size", VirtIOBalloon,
> +                     qemu_4_0_config_size, false),
>      DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
>                       IOThread *),
>      DEFINE_PROP_END_OF_LIST(),
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH] virtio-balloon: fix QEMU 4.0 config size migration incompatibility
  2019-07-12 15:36 ` [Qemu-devel] [PULL 3/8] " Michael S. Tsirkin
  (?)
  (?)
@ 2019-07-11  7:18 ` Wolfgang Bumiller
  2019-07-11  8:30   ` Dr. David Alan Gilbert
  -1 siblings, 1 reply; 33+ messages in thread
From: Wolfgang Bumiller @ 2019-07-11  7:18 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Dr. David Alan Gilbert, Michael S. Tsirkin, qemu-devel, Eduardo Habkost

On Wed, Jul 10, 2019 at 04:14:40PM +0200, Stefan Hajnoczi wrote:
> The virtio-balloon config size changed in QEMU 4.0 even for existing
> machine types.  Migration from QEMU 3.1 to 4.0 can fail in some
> circumstances with the following error:
> 
>   qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: a1 device: 1 cmask: ff wmask: c0 w1cmask:0
> 
> This happens because the virtio-balloon config size affects the VIRTIO
> Legacy I/O Memory PCI BAR size.
> 
> Introduce a qdev property called "qemu-4-0-config-size" and enable it
> only for the QEMU 4.0 machine types.  This way <4.0 machine types use
> the old size, 4.0 uses the larger size, and >4.0 machine types use the
> appropriate size depending on enabled virtio-balloon features.
> 
> Live migration to and from old QEMUs to QEMU 4.1 works again as long as
> a versioned machine type is specified (do not use just "pc"!).
> 
> Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Tested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

Works with my otherwise failing VM from 3.0.1 -> patched-4.0.
Somehow I missed the `DEFINE_PROP_*` macros, sorry, and thanks for
fixing this up.

> ---
>  include/hw/virtio/virtio-balloon.h |  2 ++
>  hw/core/machine.c                  |  2 ++
>  hw/virtio/virtio-balloon.c         | 28 +++++++++++++++++++++++++---
>  3 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 1afafb12f6..5a99293a45 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -71,6 +71,8 @@ typedef struct VirtIOBalloon {
>      int64_t stats_poll_interval;
>      uint32_t host_features;
>      PartiallyBalloonedPage *pbp;
> +
> +    bool qemu_4_0_config_size;
>  } VirtIOBalloon;
>  
>  #endif
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2be19ec0cd..c4ead16010 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -34,6 +34,7 @@ GlobalProperty hw_compat_4_0[] = {
>      { "virtio-vga",     "edid", "false" },
>      { "virtio-gpu-pci", "edid", "false" },
>      { "virtio-device", "use-started", "false" },
> +    { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
>  };
>  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
>  
> @@ -49,6 +50,7 @@ GlobalProperty hw_compat_3_1[] = {
>      { "usb-tablet", "serial", "42" },
>      { "virtio-blk-device", "discard", "false" },
>      { "virtio-blk-device", "write-zeroes", "false" },
> +    { "virtio-balloon-device", "qemu-4-0-config-size", "false" },
>  };
>  const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
>  
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 11fad86d64..e85d1c0d5c 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -615,6 +615,22 @@ virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
>      return 0;
>  }
>  
> +static size_t virtio_balloon_config_size(VirtIOBalloon *s)
> +{
> +    uint64_t features = s->host_features;
> +
> +    if (s->qemu_4_0_config_size) {
> +        return sizeof(struct virtio_balloon_config);
> +    }
> +    if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON)) {
> +        return sizeof(struct virtio_balloon_config);
> +    }
> +    if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> +        return offsetof(struct virtio_balloon_config, poison_val);
> +    }
> +    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
> +}
> +
>  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>  {
>      VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> @@ -635,7 +651,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>      }
>  
>      trace_virtio_balloon_get_config(config.num_pages, config.actual);
> -    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
> +    memcpy(config_data, &config, virtio_balloon_config_size(dev));
>  }
>  
>  static int build_dimm_list(Object *obj, void *opaque)
> @@ -679,7 +695,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>      uint32_t oldactual = dev->actual;
>      ram_addr_t vm_ram_size = get_current_ram_size();
>  
> -    memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
> +    memcpy(&config, config_data, virtio_balloon_config_size(dev));
>      dev->actual = le32_to_cpu(config.actual);
>      if (dev->actual != oldactual) {
>          qapi_event_send_balloon_change(vm_ram_size -
> @@ -766,7 +782,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      int ret;
>  
>      virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
> -                sizeof(struct virtio_balloon_config));
> +                virtio_balloon_config_size(s));
>  
>      ret = qemu_add_balloon_handler(virtio_balloon_to_target,
>                                     virtio_balloon_stat, s);
> @@ -897,6 +913,12 @@ static Property virtio_balloon_properties[] = {
>                      VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
>      DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
>                      VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
> +    /* QEMU 4.0 accidentally changed the config size even when free-page-hint
> +     * is disabled, resulting in QEMU 3.1 migration incompatibility.  This
> +     * property retains this quirk for QEMU 4.1 machine types.
> +     */
> +    DEFINE_PROP_BOOL("qemu-4-0-config-size", VirtIOBalloon,
> +                     qemu_4_0_config_size, false),
>      DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
>                       IOThread *),
>      DEFINE_PROP_END_OF_LIST(),
> -- 
> 2.21.0
> 
> 



^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH] virtio-balloon: fix QEMU 4.0 config size migration incompatibility
  2019-07-11  7:18 ` Wolfgang Bumiller
@ 2019-07-11  8:30   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 33+ messages in thread
From: Dr. David Alan Gilbert @ 2019-07-11  8:30 UTC (permalink / raw)
  To: Wolfgang Bumiller
  Cc: Eduardo Habkost, Michael S. Tsirkin, qemu-devel, Stefan Hajnoczi

* Wolfgang Bumiller (w.bumiller@proxmox.com) wrote:
> On Wed, Jul 10, 2019 at 04:14:40PM +0200, Stefan Hajnoczi wrote:
> > The virtio-balloon config size changed in QEMU 4.0 even for existing
> > machine types.  Migration from QEMU 3.1 to 4.0 can fail in some
> > circumstances with the following error:
> > 
> >   qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: a1 device: 1 cmask: ff wmask: c0 w1cmask:0
> > 
> > This happens because the virtio-balloon config size affects the VIRTIO
> > Legacy I/O Memory PCI BAR size.
> > 
> > Introduce a qdev property called "qemu-4-0-config-size" and enable it
> > only for the QEMU 4.0 machine types.  This way <4.0 machine types use
> > the old size, 4.0 uses the larger size, and >4.0 machine types use the
> > appropriate size depending on enabled virtio-balloon features.
> > 
> > Live migration to and from old QEMUs to QEMU 4.1 works again as long as
> > a versioned machine type is specified (do not use just "pc"!).
> > 
> > Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Tested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> 
> Works with my otherwise failing VM from 3.0.1 -> patched-4.0.
> Somehow I missed the `DEFINE_PROP_*` macros, sorry, and thanks for
> fixing this up.

Thanks again for spotting it!

Dave

> > ---
> >  include/hw/virtio/virtio-balloon.h |  2 ++
> >  hw/core/machine.c                  |  2 ++
> >  hw/virtio/virtio-balloon.c         | 28 +++++++++++++++++++++++++---
> >  3 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> > index 1afafb12f6..5a99293a45 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -71,6 +71,8 @@ typedef struct VirtIOBalloon {
> >      int64_t stats_poll_interval;
> >      uint32_t host_features;
> >      PartiallyBalloonedPage *pbp;
> > +
> > +    bool qemu_4_0_config_size;
> >  } VirtIOBalloon;
> >  
> >  #endif
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 2be19ec0cd..c4ead16010 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -34,6 +34,7 @@ GlobalProperty hw_compat_4_0[] = {
> >      { "virtio-vga",     "edid", "false" },
> >      { "virtio-gpu-pci", "edid", "false" },
> >      { "virtio-device", "use-started", "false" },
> > +    { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
> >  };
> >  const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
> >  
> > @@ -49,6 +50,7 @@ GlobalProperty hw_compat_3_1[] = {
> >      { "usb-tablet", "serial", "42" },
> >      { "virtio-blk-device", "discard", "false" },
> >      { "virtio-blk-device", "write-zeroes", "false" },
> > +    { "virtio-balloon-device", "qemu-4-0-config-size", "false" },
> >  };
> >  const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
> >  
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 11fad86d64..e85d1c0d5c 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -615,6 +615,22 @@ virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
> >      return 0;
> >  }
> >  
> > +static size_t virtio_balloon_config_size(VirtIOBalloon *s)
> > +{
> > +    uint64_t features = s->host_features;
> > +
> > +    if (s->qemu_4_0_config_size) {
> > +        return sizeof(struct virtio_balloon_config);
> > +    }
> > +    if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON)) {
> > +        return sizeof(struct virtio_balloon_config);
> > +    }
> > +    if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> > +        return offsetof(struct virtio_balloon_config, poison_val);
> > +    }
> > +    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
> > +}
> > +
> >  static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >  {
> >      VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
> > @@ -635,7 +651,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
> >      }
> >  
> >      trace_virtio_balloon_get_config(config.num_pages, config.actual);
> > -    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
> > +    memcpy(config_data, &config, virtio_balloon_config_size(dev));
> >  }
> >  
> >  static int build_dimm_list(Object *obj, void *opaque)
> > @@ -679,7 +695,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> >      uint32_t oldactual = dev->actual;
> >      ram_addr_t vm_ram_size = get_current_ram_size();
> >  
> > -    memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
> > +    memcpy(&config, config_data, virtio_balloon_config_size(dev));
> >      dev->actual = le32_to_cpu(config.actual);
> >      if (dev->actual != oldactual) {
> >          qapi_event_send_balloon_change(vm_ram_size -
> > @@ -766,7 +782,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >      int ret;
> >  
> >      virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
> > -                sizeof(struct virtio_balloon_config));
> > +                virtio_balloon_config_size(s));
> >  
> >      ret = qemu_add_balloon_handler(virtio_balloon_to_target,
> >                                     virtio_balloon_stat, s);
> > @@ -897,6 +913,12 @@ static Property virtio_balloon_properties[] = {
> >                      VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
> >      DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
> >                      VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
> > +    /* QEMU 4.0 accidentally changed the config size even when free-page-hint
> > +     * is disabled, resulting in QEMU 3.1 migration incompatibility.  This
> > +     * property retains this quirk for QEMU 4.1 machine types.
> > +     */
> > +    DEFINE_PROP_BOOL("qemu-4-0-config-size", VirtIOBalloon,
> > +                     qemu_4_0_config_size, false),
> >      DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
> >                       IOThread *),
> >      DEFINE_PROP_END_OF_LIST(),
> > -- 
> > 2.21.0
> > 
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes
@ 2019-07-12  7:35 Pankaj Gupta
  2019-07-12 15:36   ` [Qemu-devel] [PULL 6/8] " Michael S. Tsirkin
                   ` (3 more replies)
  0 siblings, 4 replies; 33+ messages in thread
From: Pankaj Gupta @ 2019-07-12  7:35 UTC (permalink / raw)
  To: mst; +Cc: pagupta, peter.maydell, cohuck, qemu-devel, david

This patch series two fixes for coverity and a 
transactional info removal patch.

Pankaj Gupta (3):
  virtio pmem: fix wrong mem region condition
  virtio pmem: remove memdev null check
  virtio pmem: remove transational device info

 hw/virtio/virtio-pmem-pci.c | 4 +---
 hw/virtio/virtio-pmem.c     | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.14.5



^ permalink raw reply	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Pankaj Gupta @ 2019-07-12  7:35 UTC (permalink / raw)
  To: mst; +Cc: pagupta, peter.maydell, cohuck, qemu-devel, david

Coverity reported memory region returns zero
for non-null value. This is because of wrong
arguments to '?:' , fixing this.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 hw/virtio/virtio-pmem-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
index 8b2d0dbccc..0da6627469 100644
--- a/hw/virtio/virtio-pmem-pci.c
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -57,7 +57,7 @@ static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState *md,
     MemoryRegion *mr = vpc->get_memory_region(pmem, errp);
 
     /* the plugged size corresponds to the region size */
-    return mr ? 0 : memory_region_size(mr);
+    return mr ? memory_region_size(mr) : 0;
 }
 
 static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md,
-- 
2.14.5



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PATCH 2/3] virtio pmem: remove memdev null check
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Pankaj Gupta @ 2019-07-12  7:35 UTC (permalink / raw)
  To: mst; +Cc: pagupta, peter.maydell, cohuck, qemu-devel, david

Coverity reports that when we're assigning vi->size we handle the 
"pmem->memdev is NULL" case; but we then pass it into 
object_get_canonical_path(), which unconditionally dereferences it
and will crash if it is NULL. If this pointer can be NULL then we
need to do something else here.

We are removing 'pmem->memdev' null check here as memdev will never
be null in this function.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 hw/virtio/virtio-pmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index adbfb603ab..17c196d107 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -134,8 +134,8 @@ static void virtio_pmem_fill_device_info(const VirtIOPMEM *pmem,
                                          VirtioPMEMDeviceInfo *vi)
 {
     vi->memaddr = pmem->start;
-    vi->size = pmem->memdev ? memory_region_size(&pmem->memdev->mr) : 0;
-    vi->memdev = object_get_canonical_path(OBJECT(pmem->memdev));
+    vi->size    = memory_region_size(&pmem->memdev->mr);
+    vi->memdev  = object_get_canonical_path(OBJECT(pmem->memdev));
 }
 
 static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
-- 
2.14.5



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Pankaj Gupta @ 2019-07-12  7:35 UTC (permalink / raw)
  To: mst; +Cc: pagupta, peter.maydell, cohuck, qemu-devel, david

Remove transactional & non transactional device info
for virtio pmem. 

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
 hw/virtio/virtio-pmem-pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
index 0da6627469..fe2af00fa1 100644
--- a/hw/virtio/virtio-pmem-pci.c
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -113,8 +113,6 @@ static void virtio_pmem_pci_instance_init(Object *obj)
 static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = {
     .base_name             = TYPE_VIRTIO_PMEM_PCI,
     .generic_name          = "virtio-pmem-pci",
-    .transitional_name     = "virtio-pmem-pci-transitional",
-    .non_transitional_name = "virtio-pmem-pci-non-transitional",
     .instance_size = sizeof(VirtIOPMEMPCI),
     .instance_init = virtio_pmem_pci_instance_init,
     .class_init    = virtio_pmem_pci_class_init,
-- 
2.14.5



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition
  2019-07-12 15:36   ` [Qemu-devel] [PULL 6/8] " Michael S. Tsirkin
  (?)
@ 2019-07-12  8:50   ` Stefano Garzarella
  -1 siblings, 0 replies; 33+ messages in thread
From: Stefano Garzarella @ 2019-07-12  8:50 UTC (permalink / raw)
  To: Pankaj Gupta; +Cc: peter.maydell, cohuck, david, qemu-devel, mst

On Fri, Jul 12, 2019 at 01:05:52PM +0530, Pankaj Gupta wrote:
> Coverity reported memory region returns zero
> for non-null value. This is because of wrong
> arguments to '?:' , fixing this.
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio-pmem-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
> index 8b2d0dbccc..0da6627469 100644
> --- a/hw/virtio/virtio-pmem-pci.c
> +++ b/hw/virtio/virtio-pmem-pci.c
> @@ -57,7 +57,7 @@ static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState *md,
>      MemoryRegion *mr = vpc->get_memory_region(pmem, errp);
>  
>      /* the plugged size corresponds to the region size */
> -    return mr ? 0 : memory_region_size(mr);
> +    return mr ? memory_region_size(mr) : 0;
>  }
>  
>  static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md,
> -- 
> 2.14.5


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition
  2019-07-12 15:36   ` [Qemu-devel] [PULL 6/8] " Michael S. Tsirkin
  (?)
  (?)
@ 2019-07-12  9:56   ` Cornelia Huck
  -1 siblings, 0 replies; 33+ messages in thread
From: Cornelia Huck @ 2019-07-12  9:56 UTC (permalink / raw)
  To: Pankaj Gupta; +Cc: peter.maydell, david, qemu-devel, mst

On Fri, 12 Jul 2019 13:05:52 +0530
Pankaj Gupta <pagupta@redhat.com> wrote:

> Coverity reported memory region returns zero
> for non-null value. This is because of wrong
> arguments to '?:' , fixing this.
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio-pmem-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] virtio pmem: remove memdev null check
  2019-07-12 15:36   ` [Qemu-devel] [PULL 7/8] " Michael S. Tsirkin
  (?)
@ 2019-07-12 10:01   ` Cornelia Huck
  -1 siblings, 0 replies; 33+ messages in thread
From: Cornelia Huck @ 2019-07-12 10:01 UTC (permalink / raw)
  To: Pankaj Gupta; +Cc: peter.maydell, david, qemu-devel, mst

On Fri, 12 Jul 2019 13:05:53 +0530
Pankaj Gupta <pagupta@redhat.com> wrote:

> Coverity reports that when we're assigning vi->size we handle the 
> "pmem->memdev is NULL" case; but we then pass it into 
> object_get_canonical_path(), which unconditionally dereferences it
> and will crash if it is NULL. If this pointer can be NULL then we
> need to do something else here.
> 
> We are removing 'pmem->memdev' null check here as memdev will never
> be null in this function.

Indeed, we'll fail to realize the device if it is NULL.

> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio-pmem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info
  2019-07-12 15:36   ` [Qemu-devel] [PULL 8/8] virtio pmem: remove transitional names Michael S. Tsirkin
  (?)
@ 2019-07-12 10:03   ` Cornelia Huck
  2019-07-12 10:06     ` Pankaj Gupta
  -1 siblings, 1 reply; 33+ messages in thread
From: Cornelia Huck @ 2019-07-12 10:03 UTC (permalink / raw)
  To: Pankaj Gupta; +Cc: peter.maydell, david, qemu-devel, mst

On Fri, 12 Jul 2019 13:05:54 +0530
Pankaj Gupta <pagupta@redhat.com> wrote:

> Remove transactional & non transactional device info
> for virtio pmem. 

s/device info/names/ ?

> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio-pmem-pci.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes
  2019-07-12  7:35 [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes Pankaj Gupta
                   ` (2 preceding siblings ...)
  2019-07-12 15:36   ` [Qemu-devel] [PULL 8/8] virtio pmem: remove transitional names Michael S. Tsirkin
@ 2019-07-12 10:03 ` Cornelia Huck
  2019-07-12 10:05   ` Pankaj Gupta
  3 siblings, 1 reply; 33+ messages in thread
From: Cornelia Huck @ 2019-07-12 10:03 UTC (permalink / raw)
  To: Pankaj Gupta; +Cc: peter.maydell, david, qemu-devel, mst

On Fri, 12 Jul 2019 13:05:51 +0530
Pankaj Gupta <pagupta@redhat.com> wrote:

> This patch series two fixes for coverity and a 
> transactional info removal patch.
> 
> Pankaj Gupta (3):
>   virtio pmem: fix wrong mem region condition
>   virtio pmem: remove memdev null check
>   virtio pmem: remove transational device info
> 
>  hw/virtio/virtio-pmem-pci.c | 4 +---
>  hw/virtio/virtio-pmem.c     | 4 ++--
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 

I think all of this is 4.1 material.


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes
  2019-07-12 10:03 ` [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes Cornelia Huck
@ 2019-07-12 10:05   ` Pankaj Gupta
  0 siblings, 0 replies; 33+ messages in thread
From: Pankaj Gupta @ 2019-07-12 10:05 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: peter maydell, mst, qemu-devel, david


> 
> On Fri, 12 Jul 2019 13:05:51 +0530
> Pankaj Gupta <pagupta@redhat.com> wrote:
> 
> > This patch series two fixes for coverity and a
> > transactional info removal patch.
> > 
> > Pankaj Gupta (3):
> >   virtio pmem: fix wrong mem region condition
> >   virtio pmem: remove memdev null check
> >   virtio pmem: remove transational device info
> > 
> >  hw/virtio/virtio-pmem-pci.c | 4 +---
> >  hw/virtio/virtio-pmem.c     | 4 ++--
> >  2 files changed, 3 insertions(+), 5 deletions(-)
> > 
> 
> I think all of this is 4.1 material.

Yes, forgot to add in the subject.

Thank you for the review.

Best regards,
Pankaj

> 
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info
  2019-07-12 10:03   ` [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info Cornelia Huck
@ 2019-07-12 10:06     ` Pankaj Gupta
  0 siblings, 0 replies; 33+ messages in thread
From: Pankaj Gupta @ 2019-07-12 10:06 UTC (permalink / raw)
  To: Cornelia Huck; +Cc: peter maydell, david, qemu-devel, mst


> 
> On Fri, 12 Jul 2019 13:05:54 +0530
> Pankaj Gupta <pagupta@redhat.com> wrote:
> 
> > Remove transactional & non transactional device info
> > for virtio pmem.
> 
> s/device info/names/ ?

yes.

> 
> > 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  hw/virtio/virtio-pmem-pci.c | 2 --
> >  1 file changed, 2 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests
@ 2019-07-12 15:36 Michael S. Tsirkin
  2019-07-12 15:36 ` [Qemu-devel] [PULL 1/8] xio3130_downstream: typo fix Michael S. Tsirkin
                   ` (2 more replies)
  0 siblings, 3 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell

The following changes since commit a2a9d4adabe340617a24eb73a8b2a116d28a6b38:

  Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-4.1-20190712' into staging (2019-07-12 11:06:48 +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 207efa18ac9eb7085a44cad24489d0da54bc5f65:

  virtio pmem: remove transitional names (2019-07-12 10:57:27 -0400)

----------------------------------------------------------------
virtio, pc, pci: fixes, cleanups, tests

A bunch of fixes all over the place.
ACPI tests will now run on more systems: might
introduce new failure reports but that's for
the best, isn't it?

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

----------------------------------------------------------------
Igor Mammedov (2):
      tests: acpi: do not require IASL for dumping AML blobs
      tests: acpi: do not skip tests when IASL is not installed

Michael S. Tsirkin (2):
      xio3130_downstream: typo fix
      pcie: consistent names for function args

Pankaj Gupta (3):
      virtio pmem: fix wrong mem region condition
      virtio pmem: remove memdev null check
      virtio pmem: remove transitional names

Stefan Hajnoczi (1):
      virtio-balloon: fix QEMU 4.0 config size migration incompatibility

 include/hw/pci/pcie.h              |  4 ++--
 include/hw/virtio/virtio-balloon.h |  2 ++
 hw/core/machine.c                  |  2 ++
 hw/pci-bridge/xio3130_downstream.c |  2 +-
 hw/virtio/virtio-balloon.c         | 28 +++++++++++++++++++++++++---
 hw/virtio/virtio-pmem-pci.c        |  4 +---
 hw/virtio/virtio-pmem.c            |  4 ++--
 tests/bios-tables-test.c           | 23 +++++++++++++++++------
 8 files changed, 52 insertions(+), 17 deletions(-)



^ permalink raw reply	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 1/8] xio3130_downstream: typo fix
  2019-07-12 15:36 [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests Michael S. Tsirkin
@ 2019-07-12 15:36 ` Michael S. Tsirkin
  2019-07-12 15:36 ` [Qemu-devel] [PULL 2/8] pcie: consistent names for function args Michael S. Tsirkin
  2019-07-15  8:45 ` [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests Peter Maydell
  2 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell

slt ctl/status are passed in incorrect order.
Fix this up.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
 hw/pci-bridge/xio3130_downstream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 899b0fd6c9..182e164f74 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -43,7 +43,7 @@ static void xio3130_downstream_write_config(PCIDevice *d, uint32_t address,
 {
     uint16_t slt_ctl, slt_sta;
 
-    pcie_cap_slot_get(d, &slt_sta, &slt_ctl);
+    pcie_cap_slot_get(d, &slt_ctl, &slt_sta);
     pci_bridge_write_config(d, address, val, len);
     pcie_cap_flr_write_config(d, address, val, len);
     pcie_cap_slot_write_config(d, slt_ctl, slt_sta, address, val, len);
-- 
MST



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 2/8] pcie: consistent names for function args
  2019-07-12 15:36 [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests Michael S. Tsirkin
  2019-07-12 15:36 ` [Qemu-devel] [PULL 1/8] xio3130_downstream: typo fix Michael S. Tsirkin
@ 2019-07-12 15:36 ` Michael S. Tsirkin
  2019-07-15  8:45 ` [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests Peter Maydell
  2 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell

The function declarations for pci_cap_slot_get and
pci_cap_slot_write_config call the argument "slot_ctl", but the function
definitions and all the call sites drop the 'o' and call it "slt_ctl".
Let's be consistent.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
---
 include/hw/pci/pcie.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 34f277735c..8cf3361fc4 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -107,9 +107,9 @@ void pcie_cap_lnkctl_reset(PCIDevice *dev);
 
 void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot);
 void pcie_cap_slot_reset(PCIDevice *dev);
-void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slot_ctl, uint16_t *slt_sta);
+void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta);
 void pcie_cap_slot_write_config(PCIDevice *dev,
-                                uint16_t old_slot_ctl, uint16_t old_slt_sta,
+                                uint16_t old_slt_ctl, uint16_t old_slt_sta,
                                 uint32_t addr, uint32_t val, int len);
 int pcie_cap_slot_post_load(void *opaque, int version_id);
 void pcie_cap_slot_push_attention_button(PCIDevice *dev);
-- 
MST



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 3/8] virtio-balloon: fix QEMU 4.0 config size migration incompatibility
@ 2019-07-12 15:36 ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Eduardo Habkost, Wolfgang Bumiller,
	Stefan Hajnoczi, Dr . David Alan Gilbert

From: Stefan Hajnoczi <stefanha@redhat.com>

The virtio-balloon config size changed in QEMU 4.0 even for existing
machine types.  Migration from QEMU 3.1 to 4.0 can fail in some
circumstances with the following error:

  qemu-system-x86_64: get_pci_config_device: Bad config data: i=0x10 read: a1 device: 1 cmask: ff wmask: c0 w1cmask:0

This happens because the virtio-balloon config size affects the VIRTIO
Legacy I/O Memory PCI BAR size.

Introduce a qdev property called "qemu-4-0-config-size" and enable it
only for the QEMU 4.0 machine types.  This way <4.0 machine types use
the old size, 4.0 uses the larger size, and >4.0 machine types use the
appropriate size depending on enabled virtio-balloon features.

Live migration to and from old QEMUs to QEMU 4.1 works again as long as
a versioned machine type is specified (do not use just "pc"!).

Originally-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20190710141440.27635-1-stefanha@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Tested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/virtio/virtio-balloon.h |  2 ++
 hw/core/machine.c                  |  2 ++
 hw/virtio/virtio-balloon.c         | 28 +++++++++++++++++++++++++---
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 1afafb12f6..5a99293a45 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -71,6 +71,8 @@ typedef struct VirtIOBalloon {
     int64_t stats_poll_interval;
     uint32_t host_features;
     PartiallyBalloonedPage *pbp;
+
+    bool qemu_4_0_config_size;
 } VirtIOBalloon;
 
 #endif
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2be19ec0cd..c4ead16010 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,6 +34,7 @@ GlobalProperty hw_compat_4_0[] = {
     { "virtio-vga",     "edid", "false" },
     { "virtio-gpu-pci", "edid", "false" },
     { "virtio-device", "use-started", "false" },
+    { "virtio-balloon-device", "qemu-4-0-config-size", "true" },
 };
 const size_t hw_compat_4_0_len = G_N_ELEMENTS(hw_compat_4_0);
 
@@ -49,6 +50,7 @@ GlobalProperty hw_compat_3_1[] = {
     { "usb-tablet", "serial", "42" },
     { "virtio-blk-device", "discard", "false" },
     { "virtio-blk-device", "write-zeroes", "false" },
+    { "virtio-balloon-device", "qemu-4-0-config-size", "false" },
 };
 const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
 
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 11fad86d64..e85d1c0d5c 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -615,6 +615,22 @@ virtio_balloon_free_page_report_notify(NotifierWithReturn *n, void *data)
     return 0;
 }
 
+static size_t virtio_balloon_config_size(VirtIOBalloon *s)
+{
+    uint64_t features = s->host_features;
+
+    if (s->qemu_4_0_config_size) {
+        return sizeof(struct virtio_balloon_config);
+    }
+    if (virtio_has_feature(features, VIRTIO_BALLOON_F_PAGE_POISON)) {
+        return sizeof(struct virtio_balloon_config);
+    }
+    if (virtio_has_feature(features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+        return offsetof(struct virtio_balloon_config, poison_val);
+    }
+    return offsetof(struct virtio_balloon_config, free_page_report_cmd_id);
+}
+
 static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
 {
     VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
@@ -635,7 +651,7 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
     }
 
     trace_virtio_balloon_get_config(config.num_pages, config.actual);
-    memcpy(config_data, &config, sizeof(struct virtio_balloon_config));
+    memcpy(config_data, &config, virtio_balloon_config_size(dev));
 }
 
 static int build_dimm_list(Object *obj, void *opaque)
@@ -679,7 +695,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
     uint32_t oldactual = dev->actual;
     ram_addr_t vm_ram_size = get_current_ram_size();
 
-    memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
+    memcpy(&config, config_data, virtio_balloon_config_size(dev));
     dev->actual = le32_to_cpu(config.actual);
     if (dev->actual != oldactual) {
         qapi_event_send_balloon_change(vm_ram_size -
@@ -766,7 +782,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     int ret;
 
     virtio_init(vdev, "virtio-balloon", VIRTIO_ID_BALLOON,
-                sizeof(struct virtio_balloon_config));
+                virtio_balloon_config_size(s));
 
     ret = qemu_add_balloon_handler(virtio_balloon_to_target,
                                    virtio_balloon_stat, s);
@@ -897,6 +913,12 @@ static Property virtio_balloon_properties[] = {
                     VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
     DEFINE_PROP_BIT("free-page-hint", VirtIOBalloon, host_features,
                     VIRTIO_BALLOON_F_FREE_PAGE_HINT, false),
+    /* QEMU 4.0 accidentally changed the config size even when free-page-hint
+     * is disabled, resulting in QEMU 3.1 migration incompatibility.  This
+     * property retains this quirk for QEMU 4.1 machine types.
+     */
+    DEFINE_PROP_BOOL("qemu-4-0-config-size", VirtIOBalloon,
+                     qemu_4_0_config_size, false),
     DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
                      IOThread *),
     DEFINE_PROP_END_OF_LIST(),
-- 
MST



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 4/8] tests: acpi: do not require IASL for dumping AML blobs
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Peter Maydell, Thomas Huth, Eric Auger,
	Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé

From: Igor Mammedov <imammedo@redhat.com>

IASL isn't needed when dumping ACPI tables from guest for
rebuild purposes. So move this part out from IASL branch.

Makes rebuild-expected-aml.sh work without IASL installed
on host.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Message-Id: <20190708092410.11167-2-imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/bios-tables-test.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index d863233fe9..13bd166b81 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -597,12 +597,10 @@ static void test_acpi_one(const char *params, test_data *data)
     test_acpi_rxsdt_table(data);
     test_acpi_fadt_table(data);
 
-    if (iasl) {
-        if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
-            dump_aml_files(data, true);
-        } else {
-            test_acpi_asl(data);
-        }
+    if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
+        dump_aml_files(data, true);
+    } else if (iasl) {
+        test_acpi_asl(data);
     }
 
     /*
-- 
MST



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 5/8] tests: acpi: do not skip tests when IASL is not installed
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: Laurent Vivier, Peter Maydell, Thomas Huth, Eric Auger,
	Paolo Bonzini, Igor Mammedov, Philippe Mathieu-Daudé

From: Igor Mammedov <imammedo@redhat.com>

tests do binary comparision so we can check tables without
IASL. Move IASL condition right before decompilation step
and skip it if IASL is not installed.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190708092410.11167-3-imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/bios-tables-test.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 13bd166b81..a356ac3489 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -389,6 +389,14 @@ static void test_acpi_asl(test_data *data)
         all_tables_match = all_tables_match &&
             test_acpi_find_diff_allowed(exp_sdt);
 
+        /*
+         *  don't try to decompile if IASL isn't present, in this case user
+         * will just 'get binary file mismatch' warnings and test failure
+         */
+        if (!iasl) {
+            continue;
+        }
+
         err = load_asl(data->tables, sdt);
         asl = normalize_asl(sdt->asl);
 
@@ -431,6 +439,11 @@ static void test_acpi_asl(test_data *data)
         g_string_free(asl, true);
         g_string_free(exp_asl, true);
     }
+    if (!iasl && !all_tables_match) {
+        fprintf(stderr, "to see ASL diff between mismatched files install IASL,"
+                " rebuild QEMU from scratch and re-run tests with V=1"
+                " environment variable set");
+    }
     g_assert(all_tables_match);
 
     free_test_data(&exp_data);
@@ -599,7 +612,7 @@ static void test_acpi_one(const char *params, test_data *data)
 
     if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
         dump_aml_files(data, true);
-    } else if (iasl) {
+    } else {
         test_acpi_asl(data);
     }
 
-- 
MST



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 6/8] virtio pmem: fix wrong mem region condition
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: Cornelia Huck, Peter Maydell, Stefano Garzarella, Pankaj Gupta

From: Pankaj Gupta <pagupta@redhat.com>

Coverity reported memory region returns zero
for non-null value. This is because of wrong
arguments to '?:' , fixing this.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
Message-Id: <20190712073554.21918-2-pagupta@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/virtio/virtio-pmem-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
index 8b2d0dbccc..0da6627469 100644
--- a/hw/virtio/virtio-pmem-pci.c
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -57,7 +57,7 @@ static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState *md,
     MemoryRegion *mr = vpc->get_memory_region(pmem, errp);
 
     /* the plugged size corresponds to the region size */
-    return mr ? 0 : memory_region_size(mr);
+    return mr ? memory_region_size(mr) : 0;
 }
 
 static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md,
-- 
MST



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 7/8] virtio pmem: remove memdev null check
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, Cornelia Huck, Pankaj Gupta

From: Pankaj Gupta <pagupta@redhat.com>

Coverity reports that when we're assigning vi->size we handle the
"pmem->memdev is NULL" case; but we then pass it into
object_get_canonical_path(), which unconditionally dereferences it
and will crash if it is NULL. If this pointer can be NULL then we
need to do something else here.

We are removing 'pmem->memdev' null check here as memdev will never
be null in this function.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
Message-Id: <20190712073554.21918-3-pagupta@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/virtio/virtio-pmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pmem.c b/hw/virtio/virtio-pmem.c
index adbfb603ab..17c196d107 100644
--- a/hw/virtio/virtio-pmem.c
+++ b/hw/virtio/virtio-pmem.c
@@ -134,8 +134,8 @@ static void virtio_pmem_fill_device_info(const VirtIOPMEM *pmem,
                                          VirtioPMEMDeviceInfo *vi)
 {
     vi->memaddr = pmem->start;
-    vi->size = pmem->memdev ? memory_region_size(&pmem->memdev->mr) : 0;
-    vi->memdev = object_get_canonical_path(OBJECT(pmem->memdev));
+    vi->size    = memory_region_size(&pmem->memdev->mr);
+    vi->memdev  = object_get_canonical_path(OBJECT(pmem->memdev));
 }
 
 static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
-- 
MST



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* [Qemu-devel] [PULL 8/8] virtio pmem: remove transitional names
@ 2019-07-12 15:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2019-07-12 15:36 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, Cornelia Huck, Pankaj Gupta

From: Pankaj Gupta <pagupta@redhat.com>

Remove transitional & non transitional names for virtio pmem.
Only virtio 1.0 and up is supported.

Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
Message-Id: <20190712073554.21918-4-pagupta@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/virtio/virtio-pmem-pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
index 0da6627469..fe2af00fa1 100644
--- a/hw/virtio/virtio-pmem-pci.c
+++ b/hw/virtio/virtio-pmem-pci.c
@@ -113,8 +113,6 @@ static void virtio_pmem_pci_instance_init(Object *obj)
 static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = {
     .base_name             = TYPE_VIRTIO_PMEM_PCI,
     .generic_name          = "virtio-pmem-pci",
-    .transitional_name     = "virtio-pmem-pci-transitional",
-    .non_transitional_name = "virtio-pmem-pci-non-transitional",
     .instance_size = sizeof(VirtIOPMEMPCI),
     .instance_init = virtio_pmem_pci_instance_init,
     .class_init    = virtio_pmem_pci_class_init,
-- 
MST



^ permalink raw reply related	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition
  2019-07-12 15:36   ` [Qemu-devel] [PULL 6/8] " Michael S. Tsirkin
                     ` (2 preceding siblings ...)
  (?)
@ 2019-07-12 17:05   ` Philippe Mathieu-Daudé
  -1 siblings, 0 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-12 17:05 UTC (permalink / raw)
  To: Pankaj Gupta, mst; +Cc: peter.maydell, cohuck, qemu-devel, david

On 7/12/19 9:35 AM, Pankaj Gupta wrote:
> Coverity reported memory region returns zero
> for non-null value. This is because of wrong
> arguments to '?:' , fixing this.
> 

Please amend:

Fixes: Coverity (CID 1403009)

> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/virtio/virtio-pmem-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
> index 8b2d0dbccc..0da6627469 100644
> --- a/hw/virtio/virtio-pmem-pci.c
> +++ b/hw/virtio/virtio-pmem-pci.c
> @@ -57,7 +57,7 @@ static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState *md,
>      MemoryRegion *mr = vpc->get_memory_region(pmem, errp);
>  
>      /* the plugged size corresponds to the region size */
> -    return mr ? 0 : memory_region_size(mr);
> +    return mr ? memory_region_size(mr) : 0;
>  }
>  
>  static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md,
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition
  2019-07-12 15:36   ` [Qemu-devel] [PULL 6/8] " Michael S. Tsirkin
                     ` (3 preceding siblings ...)
  (?)
@ 2019-07-15  6:44   ` David Hildenbrand
  -1 siblings, 0 replies; 33+ messages in thread
From: David Hildenbrand @ 2019-07-15  6:44 UTC (permalink / raw)
  To: Pankaj Gupta, mst; +Cc: peter.maydell, cohuck, qemu-devel

On 12.07.19 09:35, Pankaj Gupta wrote:
> Coverity reported memory region returns zero
> for non-null value. This is because of wrong
> arguments to '?:' , fixing this.
> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio-pmem-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
> index 8b2d0dbccc..0da6627469 100644
> --- a/hw/virtio/virtio-pmem-pci.c
> +++ b/hw/virtio/virtio-pmem-pci.c
> @@ -57,7 +57,7 @@ static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState *md,
>      MemoryRegion *mr = vpc->get_memory_region(pmem, errp);
>  
>      /* the plugged size corresponds to the region size */
> -    return mr ? 0 : memory_region_size(mr);
> +    return mr ? memory_region_size(mr) : 0;
>  }
>  
>  static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md,
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info
  2019-07-12 15:36   ` [Qemu-devel] [PULL 8/8] virtio pmem: remove transitional names Michael S. Tsirkin
  (?)
  (?)
@ 2019-07-15  6:44   ` David Hildenbrand
  2019-07-15  7:22     ` Pankaj Gupta
  -1 siblings, 1 reply; 33+ messages in thread
From: David Hildenbrand @ 2019-07-15  6:44 UTC (permalink / raw)
  To: Pankaj Gupta, mst; +Cc: peter.maydell, cohuck, qemu-devel

On 12.07.19 09:35, Pankaj Gupta wrote:
> Remove transactional & non transactional device info
> for virtio pmem. 

Can you explain and add *why* ?

> 
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>  hw/virtio/virtio-pmem-pci.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
> index 0da6627469..fe2af00fa1 100644
> --- a/hw/virtio/virtio-pmem-pci.c
> +++ b/hw/virtio/virtio-pmem-pci.c
> @@ -113,8 +113,6 @@ static void virtio_pmem_pci_instance_init(Object *obj)
>  static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = {
>      .base_name             = TYPE_VIRTIO_PMEM_PCI,
>      .generic_name          = "virtio-pmem-pci",
> -    .transitional_name     = "virtio-pmem-pci-transitional",
> -    .non_transitional_name = "virtio-pmem-pci-non-transitional",
>      .instance_size = sizeof(VirtIOPMEMPCI),
>      .instance_init = virtio_pmem_pci_instance_init,
>      .class_init    = virtio_pmem_pci_class_init,
> 


-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info
  2019-07-15  6:44   ` David Hildenbrand
@ 2019-07-15  7:22     ` Pankaj Gupta
  2019-07-15  7:25       ` David Hildenbrand
  0 siblings, 1 reply; 33+ messages in thread
From: Pankaj Gupta @ 2019-07-15  7:22 UTC (permalink / raw)
  To: David Hildenbrand; +Cc: peter maydell, cohuck, qemu-devel, mst


> 
> On 12.07.19 09:35, Pankaj Gupta wrote:
> > Remove transactional & non transactional device info
> > for virtio pmem.
> 
> Can you explain and add *why* ?

As per upstream suggestion by Cornelia & MST, transactional devices are for
legacy purpose. So, does not make sense for virtio-pmem.

Thanks,
Pankaj 

> 
> > 
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> >  hw/virtio/virtio-pmem-pci.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
> > index 0da6627469..fe2af00fa1 100644
> > --- a/hw/virtio/virtio-pmem-pci.c
> > +++ b/hw/virtio/virtio-pmem-pci.c
> > @@ -113,8 +113,6 @@ static void virtio_pmem_pci_instance_init(Object *obj)
> >  static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = {
> >      .base_name             = TYPE_VIRTIO_PMEM_PCI,
> >      .generic_name          = "virtio-pmem-pci",
> > -    .transitional_name     = "virtio-pmem-pci-transitional",
> > -    .non_transitional_name = "virtio-pmem-pci-non-transitional",
> >      .instance_size = sizeof(VirtIOPMEMPCI),
> >      .instance_init = virtio_pmem_pci_instance_init,
> >      .class_init    = virtio_pmem_pci_class_init,
> > 
> 
> 
> --
> 
> Thanks,
> 
> David / dhildenb
> 


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info
  2019-07-15  7:22     ` Pankaj Gupta
@ 2019-07-15  7:25       ` David Hildenbrand
  0 siblings, 0 replies; 33+ messages in thread
From: David Hildenbrand @ 2019-07-15  7:25 UTC (permalink / raw)
  To: Pankaj Gupta; +Cc: peter maydell, cohuck, qemu-devel, mst

On 15.07.19 09:22, Pankaj Gupta wrote:
> 
>>
>> On 12.07.19 09:35, Pankaj Gupta wrote:
>>> Remove transactional & non transactional device info
>>> for virtio pmem.
>>
>> Can you explain and add *why* ?
> 
> As per upstream suggestion by Cornelia & MST, transactional devices are for
> legacy purpose. So, does not make sense for virtio-pmem.

With something like that added to the description

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> Thanks,
> Pankaj 
> 
>>
>>>
>>> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>>> ---
>>>  hw/virtio/virtio-pmem-pci.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c
>>> index 0da6627469..fe2af00fa1 100644
>>> --- a/hw/virtio/virtio-pmem-pci.c
>>> +++ b/hw/virtio/virtio-pmem-pci.c
>>> @@ -113,8 +113,6 @@ static void virtio_pmem_pci_instance_init(Object *obj)
>>>  static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = {
>>>      .base_name             = TYPE_VIRTIO_PMEM_PCI,
>>>      .generic_name          = "virtio-pmem-pci",
>>> -    .transitional_name     = "virtio-pmem-pci-transitional",
>>> -    .non_transitional_name = "virtio-pmem-pci-non-transitional",
>>>      .instance_size = sizeof(VirtIOPMEMPCI),
>>>      .instance_init = virtio_pmem_pci_instance_init,
>>>      .class_init    = virtio_pmem_pci_class_init,
>>>
>>
>>
>> --
>>
>> Thanks,
>>
>> David / dhildenb
>>


-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 33+ messages in thread

* Re: [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests
  2019-07-12 15:36 [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests Michael S. Tsirkin
  2019-07-12 15:36 ` [Qemu-devel] [PULL 1/8] xio3130_downstream: typo fix Michael S. Tsirkin
  2019-07-12 15:36 ` [Qemu-devel] [PULL 2/8] pcie: consistent names for function args Michael S. Tsirkin
@ 2019-07-15  8:45 ` Peter Maydell
  2 siblings, 0 replies; 33+ messages in thread
From: Peter Maydell @ 2019-07-15  8:45 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: QEMU Developers

On Fri, 12 Jul 2019 at 16:36, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit a2a9d4adabe340617a24eb73a8b2a116d28a6b38:
>
>   Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-4.1-20190712' into staging (2019-07-12 11:06:48 +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 207efa18ac9eb7085a44cad24489d0da54bc5f65:
>
>   virtio pmem: remove transitional names (2019-07-12 10:57:27 -0400)
>
> ----------------------------------------------------------------
> virtio, pc, pci: fixes, cleanups, tests
>
> A bunch of fixes all over the place.
> ACPI tests will now run on more systems: might
> introduce new failure reports but that's for
> the best, isn't it?
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1
for any user-visible changes.

-- PMM


^ permalink raw reply	[flat|nested] 33+ messages in thread

end of thread, other threads:[~2019-07-15  8:46 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-10 14:14 [Qemu-devel] [PATCH] virtio-balloon: fix QEMU 4.0 config size migration incompatibility Stefan Hajnoczi
2019-07-12 15:36 ` [Qemu-devel] [PULL 3/8] " Michael S. Tsirkin
2019-07-10 15:24 ` [Qemu-devel] [PATCH] " Dr. David Alan Gilbert
2019-07-11  7:18 ` Wolfgang Bumiller
2019-07-11  8:30   ` Dr. David Alan Gilbert
  -- strict thread matches above, loose matches on Subject: below --
2019-07-12 15:36 [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests Michael S. Tsirkin
2019-07-12 15:36 ` [Qemu-devel] [PULL 1/8] xio3130_downstream: typo fix Michael S. Tsirkin
2019-07-12 15:36 ` [Qemu-devel] [PULL 2/8] pcie: consistent names for function args Michael S. Tsirkin
2019-07-15  8:45 ` [Qemu-devel] [PULL 0/8] virtio, pc, pci: fixes, cleanups, tests Peter Maydell
2019-07-12  7:35 [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes Pankaj Gupta
2019-07-12  7:35 ` [Qemu-devel] [PATCH 1/3] virtio pmem: fix wrong mem region condition Pankaj Gupta
2019-07-12 15:36   ` [Qemu-devel] [PULL 6/8] " Michael S. Tsirkin
2019-07-12  8:50   ` [Qemu-devel] [PATCH 1/3] " Stefano Garzarella
2019-07-12  9:56   ` Cornelia Huck
2019-07-12 17:05   ` Philippe Mathieu-Daudé
2019-07-15  6:44   ` David Hildenbrand
2019-07-12  7:35 ` [Qemu-devel] [PATCH 2/3] virtio pmem: remove memdev null check Pankaj Gupta
2019-07-12 15:36   ` [Qemu-devel] [PULL 7/8] " Michael S. Tsirkin
2019-07-12 10:01   ` [Qemu-devel] [PATCH 2/3] " Cornelia Huck
2019-07-12  7:35 ` [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info Pankaj Gupta
2019-07-12 15:36   ` [Qemu-devel] [PULL 8/8] virtio pmem: remove transitional names Michael S. Tsirkin
2019-07-12 10:03   ` [Qemu-devel] [PATCH 3/3] virtio pmem: remove transational device info Cornelia Huck
2019-07-12 10:06     ` Pankaj Gupta
2019-07-15  6:44   ` David Hildenbrand
2019-07-15  7:22     ` Pankaj Gupta
2019-07-15  7:25       ` David Hildenbrand
2019-07-12 10:03 ` [Qemu-devel] [PATCH 0/3] virtio pmem: coverity fixes Cornelia Huck
2019-07-12 10:05   ` Pankaj Gupta
2019-07-08  9:24 [Qemu-devel] [PATCH v3 0/2] tests: acpi: improve tests reproducability Igor Mammedov
2019-07-08  9:24 ` [Qemu-devel] [PATCH v3 1/2] tests: acpi: do not require IASL for dumping AML blobs Igor Mammedov
2019-07-12 15:36   ` [Qemu-devel] [PULL 4/8] " Michael S. Tsirkin
2019-07-08  9:24 ` [Qemu-devel] [PATCH v3 2/2] tests: acpi: do not skip tests when IASL is not installed Igor Mammedov
2019-07-12 15:36   ` [Qemu-devel] [PULL 5/8] " Michael S. Tsirkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.