[Qemu-devel] [PATCH] pc-dimm: fix crash when invalid slot number is used

[Qemu-devel] [PATCH] pc-dimm: fix crash when invalid slot number is used

[Igor Mammedov]

QEMU will crash with:
  Segmentation fault (core dumped)
when negative slot number is used, ex:
  qemu-system-x86_64 -m 1G,maxmem=20G,slots=256 \
      -object memory-backend-ram,id=mem1,size=1G \
      -device pc-dimm,id=dimm1,memdev=mem1,slot=-2

fix it by checking that slot number is within valid range.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/mem/pc-dimm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index b1239fd0d3..29c785799c 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -38,6 +38,13 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
 
     slot = object_property_get_int(OBJECT(dimm), PC_DIMM_SLOT_PROP,
                                    &error_abort);
+    if ((slot < 0 || slot >= machine->ram_slots) &&
+         slot != PC_DIMM_UNASSIGNED_SLOT) {
+        error_setg(&local_err, "invalid slot number, valid range is [0-%"
+                   PRIu64 "]", machine->ram_slots - 1);
+        goto out;
+    }
+
     slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
                                  machine->ram_slots, &local_err);
     if (local_err) {
-- 
2.18.1

Re: [Qemu-devel] [PATCH] pc-dimm: fix crash when invalid slot number is used

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

On Tue, Jul 23, 2019 at 12:08:59PM -0400, Igor Mammedov wrote:
> QEMU will crash with:
>   Segmentation fault (core dumped)
> when negative slot number is used, ex:
>   qemu-system-x86_64 -m 1G,maxmem=20G,slots=256 \
>       -object memory-backend-ram,id=mem1,size=1G \
>       -device pc-dimm,id=dimm1,memdev=mem1,slot=-2
> 
> fix it by checking that slot number is within valid range.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/mem/pc-dimm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index b1239fd0d3..29c785799c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -38,6 +38,13 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
>  
>      slot = object_property_get_int(OBJECT(dimm), PC_DIMM_SLOT_PROP,
>                                     &error_abort);
> +    if ((slot < 0 || slot >= machine->ram_slots) &&
> +         slot != PC_DIMM_UNASSIGNED_SLOT) {
> +        error_setg(&local_err, "invalid slot number, valid range is [0-%"
> +                   PRIu64 "]", machine->ram_slots - 1);
> +        goto out;
> +    }
> +
>      slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
>                                   machine->ram_slots, &local_err);
>      if (local_err) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] pc-dimm: fix crash when invalid slot number is used

[Li Qiang]

Igor Mammedov <imammedo@redhat.com> 于2019年7月24日周三 上午12:09写道：

> QEMU will crash with:
>   Segmentation fault (core dumped)
> when negative slot number is used, ex:
>   qemu-system-x86_64 -m 1G,maxmem=20G,slots=256 \
>       -object memory-backend-ram,id=mem1,size=1G \
>       -device pc-dimm,id=dimm1,memdev=mem1,slot=-2
>
> fix it by checking that slot number is within valid range.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>


> ---
>  hw/mem/pc-dimm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index b1239fd0d3..29c785799c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -38,6 +38,13 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState
> *machine,
>
>      slot = object_property_get_int(OBJECT(dimm), PC_DIMM_SLOT_PROP,
>                                     &error_abort);
> +    if ((slot < 0 || slot >= machine->ram_slots) &&
> +         slot != PC_DIMM_UNASSIGNED_SLOT) {
> +        error_setg(&local_err, "invalid slot number, valid range is [0-%"
> +                   PRIu64 "]", machine->ram_slots - 1);
> +        goto out;
> +    }
> +
>      slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL :
> &slot,
>                                   machine->ram_slots, &local_err);
>      if (local_err) {
> --
> 2.18.1
>
>
>

Re: [Qemu-devel] [PATCH] pc-dimm: fix crash when invalid slot number is used

[Pankaj Gupta]

> QEMU will crash with:
>   Segmentation fault (core dumped)
> when negative slot number is used, ex:
>   qemu-system-x86_64 -m 1G,maxmem=20G,slots=256 \
>       -object memory-backend-ram,id=mem1,size=1G \
>       -device pc-dimm,id=dimm1,memdev=mem1,slot=-2
> 
> fix it by checking that slot number is within valid range.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/mem/pc-dimm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index b1239fd0d3..29c785799c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -38,6 +38,13 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState
> *machine,
>  
>      slot = object_property_get_int(OBJECT(dimm), PC_DIMM_SLOT_PROP,
>                                     &error_abort);
> +    if ((slot < 0 || slot >= machine->ram_slots) &&
> +         slot != PC_DIMM_UNASSIGNED_SLOT) {
> +        error_setg(&local_err, "invalid slot number, valid range is [0-%"
> +                   PRIu64 "]", machine->ram_slots - 1);
> +        goto out;
> +    }
> +
>      slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL :
>      &slot,
>                                   machine->ram_slots, &local_err);
>      if (local_err) {
> --

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

> 2.18.1
> 
> 
> 

[Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Revert a couple of patches that break PCIe capabilities in virtio
devices. The 'optional' revert is just reverted to make the main
reversion trivial.

Symptom:
  Loss of PCIe capabilities in virtio devices hung off PCIe bridges

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


Dr. David Alan Gilbert (2):
  Revert "Revert "globals: Allow global properties to be optional""
  Revert "hw: report invalid disable-legacy|modern usage for
    virtio-1-only devs"

 hw/core/machine.c             | 23 +++--------------------
 hw/display/virtio-gpu-pci.c   |  4 +---
 hw/display/virtio-vga.c       |  4 +---
 hw/virtio/virtio-crypto-pci.c |  4 +---
 hw/virtio/virtio-input-pci.c  |  4 +---
 hw/virtio/virtio-pci.c        | 26 ++++++++++----------------
 hw/virtio/virtio-pci.h        | 31 ++++++-------------------------
 include/hw/qdev-core.h        |  3 +++
 qom/object.c                  |  3 +++
 9 files changed, 29 insertions(+), 73 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/2] Revert "Revert "globals: Allow global properties to be optional""

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This reverts commit 8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0.

Because we're about to revert it's neighbour and thus uses an optional
again.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/hw/qdev-core.h | 3 +++
 qom/object.c           | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e157fc4acd..136df7774c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -252,6 +252,8 @@ struct PropertyInfo {
 /**
  * GlobalProperty:
  * @used: Set to true if property was used when initializing a device.
+ * @optional: If set to true, GlobalProperty will be skipped without errors
+ *            if the property doesn't exist.
  *
  * An error is fatal for non-hotplugged devices, when the global is applied.
  */
@@ -260,6 +262,7 @@ typedef struct GlobalProperty {
     const char *property;
     const char *value;
     bool used;
+    bool optional;
 } GlobalProperty;
 
 static inline void
diff --git a/qom/object.c b/qom/object.c
index 3966a3d461..1555547727 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -386,6 +386,9 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
         if (object_dynamic_cast(obj, p->driver) == NULL) {
             continue;
         }
+        if (p->optional && !object_property_find(obj, p->property, NULL)) {
+            continue;
+        }
         p->used = true;
         object_property_parse(obj, p->value, p->property, &err);
         if (err != NULL) {
-- 
2.21.0

[Qemu-devel] [PATCH v2 2/2] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
since that accidentally removes the PCIe capabilities from virtio
devices because virtio_pci_dc_realize is called before the new 'mode'
flag is set.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/core/machine.c             | 23 +++--------------------
 hw/display/virtio-gpu-pci.c   |  4 +---
 hw/display/virtio-vga.c       |  4 +---
 hw/virtio/virtio-crypto-pci.c |  4 +---
 hw/virtio/virtio-input-pci.c  |  4 +---
 hw/virtio/virtio-pci.c        | 26 ++++++++++----------------
 hw/virtio/virtio-pci.h        | 31 ++++++-------------------------
 7 files changed, 23 insertions(+), 73 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c58a8e594e..c4a2ab2282 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -115,26 +115,9 @@ const size_t hw_compat_2_7_len = G_N_ELEMENTS(hw_compat_2_7);
 
 GlobalProperty hw_compat_2_6[] = {
     { "virtio-mmio", "format_transport_address", "off" },
-    /*
-     * don't include devices which are modern-only
-     * ie keyboard, mouse, tablet, gpu, vga & crypto
-     */
-    { "virtio-9p-pci", "disable-modern", "on" },
-    { "virtio-9p-pci", "disable-legacy", "off" },
-    { "virtio-balloon-pci", "disable-modern", "on" },
-    { "virtio-balloon-pci", "disable-legacy", "off" },
-    { "virtio-blk-pci", "disable-modern", "on" },
-    { "virtio-blk-pci", "disable-legacy", "off" },
-    { "virtio-input-host-pci", "disable-modern", "on" },
-    { "virtio-input-host-pci", "disable-legacy", "off" },
-    { "virtio-net-pci", "disable-modern", "on" },
-    { "virtio-net-pci", "disable-legacy", "off" },
-    { "virtio-rng-pci", "disable-modern", "on" },
-    { "virtio-rng-pci", "disable-legacy", "off" },
-    { "virtio-scsi-pci", "disable-modern", "on" },
-    { "virtio-scsi-pci", "disable-legacy", "off" },
-    { "virtio-serial-pci", "disable-modern", "on" },
-    { "virtio-serial-pci", "disable-legacy", "off" },
+    /* Optional because not all virtio-pci devices support legacy mode */
+    { "virtio-pci", "disable-modern", "on",  .optional = true },
+    { "virtio-pci", "disable-legacy", "off", .optional = true },
 };
 const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6);
 
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index d6f01b4a98..e4c7eb6193 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -33,9 +33,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     Error *local_error = NULL;
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
-    if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-        return;
-    }
+    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(vdev), true, "realized", &local_error);
 
     if (local_error) {
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 416e7fec87..79a145e284 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -137,9 +137,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 
     /* init virtio bits */
     qdev_set_parent_bus(DEVICE(g), BUS(&vpci_dev->bus));
-    if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-        return;
-    }
+    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(g), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
index c8a2317a10..91d4446080 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -53,9 +53,7 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     }
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
-    if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-        return;
-    }
+    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(vdev), true, "realized", errp);
     object_property_set_link(OBJECT(vcrypto),
                  OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev",
diff --git a/hw/virtio/virtio-input-pci.c b/hw/virtio/virtio-input-pci.c
index 1c40292abc..ad7774e93e 100644
--- a/hw/virtio/virtio-input-pci.c
+++ b/hw/virtio/virtio-input-pci.c
@@ -49,9 +49,7 @@ static void virtio_input_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     DeviceState *vdev = DEVICE(&vinput->vdev);
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
-    if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-        return;
-    }
+    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce928f2429..f6d2223e78 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1723,22 +1723,16 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
                        /* PCI BAR regions must be powers of 2 */
                        pow2ceil(proxy->notify.offset + proxy->notify.size));
 
-    if ((proxy->disable_legacy == ON_OFF_AUTO_ON) ||
-        ((proxy->disable_legacy == ON_OFF_AUTO_AUTO) && pcie_port)) {
-        if (proxy->disable_modern) {
-            error_setg(errp, "device cannot work as neither modern nor "
-                       "legacy mode is enabled");
-            error_append_hint(errp, "Set either disable-modern or "
-                              "disable-legacy to off\n");
-            return;
-        }
-        proxy->mode = VIRTIO_PCI_MODE_MODERN;
-    } else {
-        if (proxy->disable_modern) {
-            proxy->mode = VIRTIO_PCI_MODE_LEGACY;
-        } else {
-            proxy->mode = VIRTIO_PCI_MODE_TRANSITIONAL;
-        }
+    if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
+        proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+    }
+
+    if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
+        error_setg(errp, "device cannot work as neither modern nor legacy mode"
+                   " is enabled");
+        error_append_hint(errp, "Set either disable-modern or disable-legacy"
+                          " to off\n");
+        return;
     }
 
     if (pcie_port && pci_is_express(pci_dev)) {
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 619d9098c1..292275acb1 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -15,7 +15,6 @@
 #ifndef QEMU_VIRTIO_PCI_H
 #define QEMU_VIRTIO_PCI_H
 
-#include "qapi/error.h"
 #include "hw/pci/msi.h"
 #include "hw/virtio/virtio-bus.h"
 
@@ -119,12 +118,6 @@ typedef struct VirtIOPCIQueue {
   uint32_t used[2];
 } VirtIOPCIQueue;
 
-typedef enum {
-    VIRTIO_PCI_MODE_LEGACY,
-    VIRTIO_PCI_MODE_TRANSITIONAL,
-    VIRTIO_PCI_MODE_MODERN,
-} VirtIOPCIMode;
-
 struct VirtIOPCIProxy {
     PCIDevice pci_dev;
     MemoryRegion bar;
@@ -149,7 +142,6 @@ struct VirtIOPCIProxy {
     bool disable_modern;
     bool ignore_backend_features;
     OnOffAuto disable_legacy;
-    VirtIOPCIMode mode;
     uint32_t class_code;
     uint32_t nvectors;
     uint32_t dfselect;
@@ -164,34 +156,23 @@ struct VirtIOPCIProxy {
 
 static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
 {
-    return proxy->mode != VIRTIO_PCI_MODE_LEGACY;
+    return !proxy->disable_modern;
 }
 
 static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy)
 {
-    return proxy->mode != VIRTIO_PCI_MODE_MODERN;
+    return proxy->disable_legacy == ON_OFF_AUTO_OFF;
 }
 
-static inline bool virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy,
-                                             Error **errp)
+static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
 {
-    if (proxy->disable_legacy == ON_OFF_AUTO_OFF) {
-        error_setg(errp, "Unable to set disable-legacy=off on a virtio-1.0 "
-                   "only device");
-        return false;
-    }
-    if (proxy->disable_modern == true) {
-        error_setg(errp, "Unable to set disable-modern=on on a virtio-1.0 "
-                   "only device");
-        return false;
-    }
-    proxy->mode = VIRTIO_PCI_MODE_MODERN;
-    return true;
+    proxy->disable_modern = false;
+    proxy->disable_legacy = ON_OFF_AUTO_ON;
 }
 
 static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
 {
-    proxy->mode = VIRTIO_PCI_MODE_LEGACY;
+    proxy->disable_modern = true;
 }
 
 /*
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 1/2] Revert "Revert "globals: Allow global properties to be optional""

[Daniel P. Berrangé]

On Mon, Jul 29, 2019 at 05:29:02PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This reverts commit 8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0.
> 
> Because we're about to revert it's neighbour and thus uses an optional
> again.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/hw/qdev-core.h | 3 +++
>  qom/object.c           | 3 +++
>  2 files changed, 6 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 2/2] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Daniel P. Berrangé]

On Mon, Jul 29, 2019 at 05:29:03PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
> since that accidentally removes the PCIe capabilities from virtio
> devices because virtio_pci_dc_realize is called before the new 'mode'
> flag is set.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/core/machine.c             | 23 +++--------------------
>  hw/display/virtio-gpu-pci.c   |  4 +---
>  hw/display/virtio-vga.c       |  4 +---
>  hw/virtio/virtio-crypto-pci.c |  4 +---
>  hw/virtio/virtio-input-pci.c  |  4 +---
>  hw/virtio/virtio-pci.c        | 26 ++++++++++----------------
>  hw/virtio/virtio-pci.h        | 31 ++++++-------------------------
>  7 files changed, 23 insertions(+), 73 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Cornelia Huck]

On Mon, 29 Jul 2019 17:29:01 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Revert a couple of patches that break PCIe capabilities in virtio
> devices. The 'optional' revert is just reverted to make the main
> reversion trivial.

Don't want to spoil the party here; but wasn't the optional stuff
removed because it was deemed to be a bad idea?

> 
> Symptom:
>   Loss of PCIe capabilities in virtio devices hung off PCIe bridges
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> 
> Dr. David Alan Gilbert (2):
>   Revert "Revert "globals: Allow global properties to be optional""
>   Revert "hw: report invalid disable-legacy|modern usage for
>     virtio-1-only devs"
> 
>  hw/core/machine.c             | 23 +++--------------------
>  hw/display/virtio-gpu-pci.c   |  4 +---
>  hw/display/virtio-vga.c       |  4 +---
>  hw/virtio/virtio-crypto-pci.c |  4 +---
>  hw/virtio/virtio-input-pci.c  |  4 +---
>  hw/virtio/virtio-pci.c        | 26 ++++++++++----------------
>  hw/virtio/virtio-pci.h        | 31 ++++++-------------------------
>  include/hw/qdev-core.h        |  3 +++
>  qom/object.c                  |  3 +++
>  9 files changed, 29 insertions(+), 73 deletions(-)
> 

Re: [Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Dr. David Alan Gilbert]

* Cornelia Huck (cohuck@redhat.com) wrote:
> On Mon, 29 Jul 2019 17:29:01 +0100
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Revert a couple of patches that break PCIe capabilities in virtio
> > devices. The 'optional' revert is just reverted to make the main
> > reversion trivial.
> 
> Don't want to spoil the party here; but wasn't the optional stuff
> removed because it was deemed to be a bad idea?

I'm perfectly happy to go either way with this; it maybe a bad idea
but it's harmless I think.

Dave

> > 
> > Symptom:
> >   Loss of PCIe capabilities in virtio devices hung off PCIe bridges
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > 
> > Dr. David Alan Gilbert (2):
> >   Revert "Revert "globals: Allow global properties to be optional""
> >   Revert "hw: report invalid disable-legacy|modern usage for
> >     virtio-1-only devs"
> > 
> >  hw/core/machine.c             | 23 +++--------------------
> >  hw/display/virtio-gpu-pci.c   |  4 +---
> >  hw/display/virtio-vga.c       |  4 +---
> >  hw/virtio/virtio-crypto-pci.c |  4 +---
> >  hw/virtio/virtio-input-pci.c  |  4 +---
> >  hw/virtio/virtio-pci.c        | 26 ++++++++++----------------
> >  hw/virtio/virtio-pci.h        | 31 ++++++-------------------------
> >  include/hw/qdev-core.h        |  3 +++
> >  qom/object.c                  |  3 +++
> >  9 files changed, 29 insertions(+), 73 deletions(-)
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Peter Maydell]

On Mon, 29 Jul 2019 at 17:36, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Cornelia Huck (cohuck@redhat.com) wrote:
> > On Mon, 29 Jul 2019 17:29:01 +0100
> > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> >
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > >
> > > Revert a couple of patches that break PCIe capabilities in virtio
> > > devices. The 'optional' revert is just reverted to make the main
> > > reversion trivial.
> >
> > Don't want to spoil the party here; but wasn't the optional stuff
> > removed because it was deemed to be a bad idea?
>
> I'm perfectly happy to go either way with this; it maybe a bad idea
> but it's harmless I think.

It seems like the original commits were:
 * patch that does something
 * patch that removes no-longer used functionality (optional globals)

so it makes sense to me that if we want to revert the 'patch that
does something' we should first revert the patch that cleaned
up unused-functionality (because now we need it again). Is
that right?

If optional-globals are a bad idea then we should take another
run at this for 4.2, but as a "revert stuff for 4.1" strategy
it seems fine to me.

thanks
-- PMM

Re: [Qemu-devel] [For 4.1 PATCH v2 0/2] Reversions to fix PCIe in virtio

[Daniel P. Berrangé]

On Mon, Jul 29, 2019 at 05:43:17PM +0100, Peter Maydell wrote:
> On Mon, 29 Jul 2019 at 17:36, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Cornelia Huck (cohuck@redhat.com) wrote:
> > > On Mon, 29 Jul 2019 17:29:01 +0100
> > > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > >
> > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > >
> > > > Revert a couple of patches that break PCIe capabilities in virtio
> > > > devices. The 'optional' revert is just reverted to make the main
> > > > reversion trivial.
> > >
> > > Don't want to spoil the party here; but wasn't the optional stuff
> > > removed because it was deemed to be a bad idea?
> >
> > I'm perfectly happy to go either way with this; it maybe a bad idea
> > but it's harmless I think.
> 
> It seems like the original commits were:
>  * patch that does something
>  * patch that removes no-longer used functionality (optional globals)
> 
> so it makes sense to me that if we want to revert the 'patch that
> does something' we should first revert the patch that cleaned
> up unused-functionality (because now we need it again). Is
> that right?
> 
> If optional-globals are a bad idea then we should take another
> run at this for 4.2, but as a "revert stuff for 4.1" strategy
> it seems fine to me.

Functionally both approaches are supposed to be identical, but given
that we already found one last minute problem with the 2nd patch, the
full revert of both feels ever so slightly safer.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 1/2] Revert "Revert "globals: Allow global properties to be optional""

[Cornelia Huck]

On Mon, 29 Jul 2019 17:29:02 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This reverts commit 8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0.
> 
> Because we're about to revert it's neighbour and thus uses an optional
> again.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/hw/qdev-core.h | 3 +++
>  qom/object.c           | 3 +++
>  2 files changed, 6 insertions(+)

As discussed on IRC, the 'bugs' mentioned in that commit were related
to the other patch that is being reverted; so I don't really mind it
for 4.1 (IOW, either of the two revert approaches should be fine.)

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

Re: [Qemu-devel] [PATCH v2 2/2] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Cornelia Huck]

On Mon, 29 Jul 2019 17:29:03 +0100
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
> since that accidentally removes the PCIe capabilities from virtio
> devices because virtio_pci_dc_realize is called before the new 'mode'
> flag is set.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/core/machine.c             | 23 +++--------------------
>  hw/display/virtio-gpu-pci.c   |  4 +---
>  hw/display/virtio-vga.c       |  4 +---
>  hw/virtio/virtio-crypto-pci.c |  4 +---
>  hw/virtio/virtio-input-pci.c  |  4 +---
>  hw/virtio/virtio-pci.c        | 26 ++++++++++----------------
>  hw/virtio/virtio-pci.h        | 31 ++++++-------------------------
>  7 files changed, 23 insertions(+), 73 deletions(-)

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

[Qemu-devel] [PULL 0/3] virtio, pc: fixes

[Michael S. Tsirkin]

I'm sending this out now as these patches are ready,
but it seems likely we'll need another patch for pci,
and as it deals with migration compat it might be a blocker.
Will know more tomorrow :(


The following changes since commit fff3159900d2b95613a9cb75fc3703e67a674729:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190726' into staging (2019-07-26 16:23:07 +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 22235bb609c18547cf6b215bad1f9d2ec56ad371:

  pc-dimm: fix crash when invalid slot number is used (2019-07-29 16:57:27 -0400)

----------------------------------------------------------------
virtio, pc: fixes

A couple of last minute bugfixes.

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

----------------------------------------------------------------
Dr. David Alan Gilbert (2):
      Revert "Revert "globals: Allow global properties to be optional""
      Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

Igor Mammedov (1):
      pc-dimm: fix crash when invalid slot number is used

 hw/virtio/virtio-pci.h        | 31 ++++++-------------------------
 include/hw/qdev-core.h        |  3 +++
 hw/core/machine.c             | 23 +++--------------------
 hw/display/virtio-gpu-pci.c   |  4 +---
 hw/display/virtio-vga.c       |  4 +---
 hw/mem/pc-dimm.c              |  7 +++++++
 hw/virtio/virtio-crypto-pci.c |  4 +---
 hw/virtio/virtio-input-pci.c  |  4 +---
 hw/virtio/virtio-pci.c        | 26 ++++++++++----------------
 qom/object.c                  |  3 +++
 10 files changed, 36 insertions(+), 73 deletions(-)

[Qemu-devel] [PULL 1/3] Revert "Revert "globals: Allow global properties to be optional""

[Michael S. Tsirkin]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This reverts commit 8fa70dbd8bb478d9483c1da3e9976a2d86b3f9a0.

Because we're about to revert it's neighbour and thus uses an optional
again.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20190729162903.4489-2-dgilbert@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>
---
 include/hw/qdev-core.h | 3 +++
 qom/object.c           | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e157fc4acd..136df7774c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -252,6 +252,8 @@ struct PropertyInfo {
 /**
  * GlobalProperty:
  * @used: Set to true if property was used when initializing a device.
+ * @optional: If set to true, GlobalProperty will be skipped without errors
+ *            if the property doesn't exist.
  *
  * An error is fatal for non-hotplugged devices, when the global is applied.
  */
@@ -260,6 +262,7 @@ typedef struct GlobalProperty {
     const char *property;
     const char *value;
     bool used;
+    bool optional;
 } GlobalProperty;
 
 static inline void
diff --git a/qom/object.c b/qom/object.c
index 3966a3d461..1555547727 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -386,6 +386,9 @@ void object_apply_global_props(Object *obj, const GPtrArray *props, Error **errp
         if (object_dynamic_cast(obj, p->driver) == NULL) {
             continue;
         }
+        if (p->optional && !object_property_find(obj, p->property, NULL)) {
+            continue;
+        }
         p->used = true;
         object_property_parse(obj, p->value, p->property, &err);
         if (err != NULL) {
-- 
MST

[Qemu-devel] [PULL 2/3] Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"

[Michael S. Tsirkin]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This reverts commit f2784eed306449c3d04a71a05ed6463b8289aedf
since that accidentally removes the PCIe capabilities from virtio
devices because virtio_pci_dc_realize is called before the new 'mode'
flag is set.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20190729162903.4489-3-dgilbert@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/core/machine.c             | 23 +++--------------------
 hw/display/virtio-gpu-pci.c   |  4 +---
 hw/display/virtio-vga.c       |  4 +---
 hw/virtio/virtio-crypto-pci.c |  4 +---
 hw/virtio/virtio-input-pci.c  |  4 +---
 hw/virtio/virtio-pci.c        | 26 ++++++++++----------------
 hw/virtio/virtio-pci.h        | 31 ++++++-------------------------
 7 files changed, 23 insertions(+), 73 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c58a8e594e..c4a2ab2282 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -115,26 +115,9 @@ const size_t hw_compat_2_7_len = G_N_ELEMENTS(hw_compat_2_7);
 
 GlobalProperty hw_compat_2_6[] = {
     { "virtio-mmio", "format_transport_address", "off" },
-    /*
-     * don't include devices which are modern-only
-     * ie keyboard, mouse, tablet, gpu, vga & crypto
-     */
-    { "virtio-9p-pci", "disable-modern", "on" },
-    { "virtio-9p-pci", "disable-legacy", "off" },
-    { "virtio-balloon-pci", "disable-modern", "on" },
-    { "virtio-balloon-pci", "disable-legacy", "off" },
-    { "virtio-blk-pci", "disable-modern", "on" },
-    { "virtio-blk-pci", "disable-legacy", "off" },
-    { "virtio-input-host-pci", "disable-modern", "on" },
-    { "virtio-input-host-pci", "disable-legacy", "off" },
-    { "virtio-net-pci", "disable-modern", "on" },
-    { "virtio-net-pci", "disable-legacy", "off" },
-    { "virtio-rng-pci", "disable-modern", "on" },
-    { "virtio-rng-pci", "disable-legacy", "off" },
-    { "virtio-scsi-pci", "disable-modern", "on" },
-    { "virtio-scsi-pci", "disable-legacy", "off" },
-    { "virtio-serial-pci", "disable-modern", "on" },
-    { "virtio-serial-pci", "disable-legacy", "off" },
+    /* Optional because not all virtio-pci devices support legacy mode */
+    { "virtio-pci", "disable-modern", "on",  .optional = true },
+    { "virtio-pci", "disable-legacy", "off", .optional = true },
 };
 const size_t hw_compat_2_6_len = G_N_ELEMENTS(hw_compat_2_6);
 
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index d6f01b4a98..e4c7eb6193 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -33,9 +33,7 @@ static void virtio_gpu_pci_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     Error *local_error = NULL;
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
-    if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-        return;
-    }
+    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(vdev), true, "realized", &local_error);
 
     if (local_error) {
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index 416e7fec87..79a145e284 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -137,9 +137,7 @@ static void virtio_vga_base_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
 
     /* init virtio bits */
     qdev_set_parent_bus(DEVICE(g), BUS(&vpci_dev->bus));
-    if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-        return;
-    }
+    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(g), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
diff --git a/hw/virtio/virtio-crypto-pci.c b/hw/virtio/virtio-crypto-pci.c
index c8a2317a10..91d4446080 100644
--- a/hw/virtio/virtio-crypto-pci.c
+++ b/hw/virtio/virtio-crypto-pci.c
@@ -53,9 +53,7 @@ static void virtio_crypto_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     }
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
-    if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-        return;
-    }
+    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(vdev), true, "realized", errp);
     object_property_set_link(OBJECT(vcrypto),
                  OBJECT(vcrypto->vdev.conf.cryptodev), "cryptodev",
diff --git a/hw/virtio/virtio-input-pci.c b/hw/virtio/virtio-input-pci.c
index 1c40292abc..ad7774e93e 100644
--- a/hw/virtio/virtio-input-pci.c
+++ b/hw/virtio/virtio-input-pci.c
@@ -49,9 +49,7 @@ static void virtio_input_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     DeviceState *vdev = DEVICE(&vinput->vdev);
 
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
-    if (!virtio_pci_force_virtio_1(vpci_dev, errp)) {
-        return;
-    }
+    virtio_pci_force_virtio_1(vpci_dev);
     object_property_set_bool(OBJECT(vdev), true, "realized", errp);
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce928f2429..f6d2223e78 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1723,22 +1723,16 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
                        /* PCI BAR regions must be powers of 2 */
                        pow2ceil(proxy->notify.offset + proxy->notify.size));
 
-    if ((proxy->disable_legacy == ON_OFF_AUTO_ON) ||
-        ((proxy->disable_legacy == ON_OFF_AUTO_AUTO) && pcie_port)) {
-        if (proxy->disable_modern) {
-            error_setg(errp, "device cannot work as neither modern nor "
-                       "legacy mode is enabled");
-            error_append_hint(errp, "Set either disable-modern or "
-                              "disable-legacy to off\n");
-            return;
-        }
-        proxy->mode = VIRTIO_PCI_MODE_MODERN;
-    } else {
-        if (proxy->disable_modern) {
-            proxy->mode = VIRTIO_PCI_MODE_LEGACY;
-        } else {
-            proxy->mode = VIRTIO_PCI_MODE_TRANSITIONAL;
-        }
+    if (proxy->disable_legacy == ON_OFF_AUTO_AUTO) {
+        proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+    }
+
+    if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
+        error_setg(errp, "device cannot work as neither modern nor legacy mode"
+                   " is enabled");
+        error_append_hint(errp, "Set either disable-modern or disable-legacy"
+                          " to off\n");
+        return;
     }
 
     if (pcie_port && pci_is_express(pci_dev)) {
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 619d9098c1..292275acb1 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -15,7 +15,6 @@
 #ifndef QEMU_VIRTIO_PCI_H
 #define QEMU_VIRTIO_PCI_H
 
-#include "qapi/error.h"
 #include "hw/pci/msi.h"
 #include "hw/virtio/virtio-bus.h"
 
@@ -119,12 +118,6 @@ typedef struct VirtIOPCIQueue {
   uint32_t used[2];
 } VirtIOPCIQueue;
 
-typedef enum {
-    VIRTIO_PCI_MODE_LEGACY,
-    VIRTIO_PCI_MODE_TRANSITIONAL,
-    VIRTIO_PCI_MODE_MODERN,
-} VirtIOPCIMode;
-
 struct VirtIOPCIProxy {
     PCIDevice pci_dev;
     MemoryRegion bar;
@@ -149,7 +142,6 @@ struct VirtIOPCIProxy {
     bool disable_modern;
     bool ignore_backend_features;
     OnOffAuto disable_legacy;
-    VirtIOPCIMode mode;
     uint32_t class_code;
     uint32_t nvectors;
     uint32_t dfselect;
@@ -164,34 +156,23 @@ struct VirtIOPCIProxy {
 
 static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
 {
-    return proxy->mode != VIRTIO_PCI_MODE_LEGACY;
+    return !proxy->disable_modern;
 }
 
 static inline bool virtio_pci_legacy(VirtIOPCIProxy *proxy)
 {
-    return proxy->mode != VIRTIO_PCI_MODE_MODERN;
+    return proxy->disable_legacy == ON_OFF_AUTO_OFF;
 }
 
-static inline bool virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy,
-                                             Error **errp)
+static inline void virtio_pci_force_virtio_1(VirtIOPCIProxy *proxy)
 {
-    if (proxy->disable_legacy == ON_OFF_AUTO_OFF) {
-        error_setg(errp, "Unable to set disable-legacy=off on a virtio-1.0 "
-                   "only device");
-        return false;
-    }
-    if (proxy->disable_modern == true) {
-        error_setg(errp, "Unable to set disable-modern=on on a virtio-1.0 "
-                   "only device");
-        return false;
-    }
-    proxy->mode = VIRTIO_PCI_MODE_MODERN;
-    return true;
+    proxy->disable_modern = false;
+    proxy->disable_legacy = ON_OFF_AUTO_ON;
 }
 
 static inline void virtio_pci_disable_modern(VirtIOPCIProxy *proxy)
 {
-    proxy->mode = VIRTIO_PCI_MODE_LEGACY;
+    proxy->disable_modern = true;
 }
 
 /*
-- 
MST

[Qemu-devel] [PULL 3/3] pc-dimm: fix crash when invalid slot number is used

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

QEMU will crash with:
  Segmentation fault (core dumped)
when negative slot number is used, ex:
  qemu-system-x86_64 -m 1G,maxmem=20G,slots=256 \
      -object memory-backend-ram,id=mem1,size=1G \
      -device pc-dimm,id=dimm1,memdev=mem1,slot=-2

fix it by checking that slot number is within valid range.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20190723160859.27250-1-imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
Signed-off-by: Igor Mammedov &lt;<a href="mailto:imammedo@redhat.com" target="_blank">imammedo@redhat.com</a>&gt;<br></blockquote><div><br></div><div>Reviewed-by: Li Qiang &lt;<a href="mailto:liq3ea@gmail.com">liq3ea@gmail.com</a>&gt;<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Reviewed-by: Pankaj Gupta <pagupta@redhat.com>
---
 hw/mem/pc-dimm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index b1239fd0d3..29c785799c 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -38,6 +38,13 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
 
     slot = object_property_get_int(OBJECT(dimm), PC_DIMM_SLOT_PROP,
                                    &error_abort);
+    if ((slot < 0 || slot >= machine->ram_slots) &&
+         slot != PC_DIMM_UNASSIGNED_SLOT) {
+        error_setg(&local_err, "invalid slot number, valid range is [0-%"
+                   PRIu64 "]", machine->ram_slots - 1);
+        goto out;
+    }
+
     slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
                                  machine->ram_slots, &local_err);
     if (local_err) {
-- 
MST

Re: [Qemu-devel] [PULL 0/3] virtio, pc: fixes

[Peter Maydell]

On Mon, 29 Jul 2019 at 22:16, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> I'm sending this out now as these patches are ready,
> but it seems likely we'll need another patch for pci,
> and as it deals with migration compat it might be a blocker.
> Will know more tomorrow :(
>
>
> The following changes since commit fff3159900d2b95613a9cb75fc3703e67a674729:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20190726' into staging (2019-07-26 16:23:07 +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 22235bb609c18547cf6b215bad1f9d2ec56ad371:
>
>   pc-dimm: fix crash when invalid slot number is used (2019-07-29 16:57:27 -0400)
>
> ----------------------------------------------------------------
> virtio, pc: fixes
>
> A couple of last minute bugfixes.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ----------------------------------------------------------------
> Dr. David Alan Gilbert (2):
>       Revert "Revert "globals: Allow global properties to be optional""
>       Revert "hw: report invalid disable-legacy|modern usage for virtio-1-only devs"
>
> Igor Mammedov (1):
>       pc-dimm: fix crash when invalid slot number is used
>


Applied, thanks.

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

-- PMM

Re: [Qemu-devel] [PULL 3/3] pc-dimm: fix crash when invalid slot number is used

[Igor Mammedov]

On Mon, 29 Jul 2019 17:16:14 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

Hi Michael,

it seems tooling used for pull req is a bit broken
 * minor issue is CC list contains bogus addresses like: &lt@redhat.com, Mammedov@redhat.com,
 * a bigger issie is that Message-Id is taken from original patch even though [PULL 3/3]
   is not 100% the same as original which confuses mail clients quite a bit.

Re: [Qemu-devel] [PULL 3/3] pc-dimm: fix crash when invalid slot number is used

[Michael S. Tsirkin]

On Tue, Jul 30, 2019 at 02:36:38PM +0200, Igor Mammedov wrote:
> On Mon, 29 Jul 2019 17:16:14 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> Hi Michael,
> 
> it seems tooling used for pull req is a bit broken
>  * minor issue is CC list contains bogus addresses like: &lt@redhat.com, Mammedov@redhat.com,

Ouch. Looks like the html version ended up there in git somehow.
I'll look into fixing that.

>  * a bigger issie is that Message-Id is taken from original patch even though [PULL 3/3]
>    is not 100% the same as original which confuses mail clients quite a bit.

That's somehow the fault of the mail sending script.
I checked and I don't see any logs of how it run though.
I think I somehow triggered an old version by mistake.
Sorry :(

-- 
MST