Message ID | 1417002601-20799-1-git-send-email-marcel.a@redhat.com |
---|---|
State | New |
Headers | show |
On Wed, 26 Nov 2014 13:50:01 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > The commits: > - 6a1fa9f5 (monitor: add del completion for peripheral device) > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > cause a QEMU crash when trying to use HMP device_del auto-completion. > It can be easily reproduced by: > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > (qemu) device_del > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > Aborted (core dumped) > > The root cause is qdev_build_hotpluggable_device_list going recursively over > all peripherals and their children assuming all are devices. It doesn't work > since PCI devices have at least on child which is a memory region (bus master). > > Solved by observing that all devices appear as direct children of > /machine/peripheral container. No need of going recursively > over all the children. > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/core/qdev.c | 12 ++++++++++-- > include/hw/qdev-core.h | 2 +- > monitor.c | 11 ++++------- > 3 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 413b413..35fd00d 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > } while (class != object_class_by_name(TYPE_DEVICE)); > } > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > { > GSList **list = opaque; > DeviceState *dev = DEVICE(obj); > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > *list = g_slist_append(*list, dev); > } > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > return 0; > } > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > +{ > + GSList *list = NULL; > + > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > + > + return list; > +} > + > static bool device_get_realized(Object *obj, Error **errp) > { > DeviceState *dev = DEVICE(obj); > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index d3a2940..589bbe7 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > char *qdev_get_dev_path(DeviceState *dev); > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > Error **errp); > diff --git a/monitor.c b/monitor.c > index fa00594..f1031a1 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > static void peripheral_device_del_completion(ReadLineState *rs, > const char *str, size_t len) > { > - Object *peripheral; > - GSList *list = NULL, *item; > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > + GSList *list, *item; > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > - if (peripheral == NULL) { > + list = qdev_build_hotpluggable_device_list(peripheral); > + if (!list) { > return; > } > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > - &list); > - > for (item = list; item; item = g_slist_next(item)) { > DeviceState *dev = item->data; >
On Wed, 26 Nov 2014 13:50:01 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > The commits: > - 6a1fa9f5 (monitor: add del completion for peripheral device) > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > cause a QEMU crash when trying to use HMP device_del auto-completion. > It can be easily reproduced by: > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > (qemu) device_del > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > Aborted (core dumped) > > The root cause is qdev_build_hotpluggable_device_list going recursively over > all peripherals and their children assuming all are devices. It doesn't work > since PCI devices have at least on child which is a memory region (bus master). > > Solved by observing that all devices appear as direct children of > /machine/peripheral container. No need of going recursively > over all the children. > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> Peter, can you apply this patch directly to master to avoid me a pull request? Maybe it's a good idea to wait until tomorrow for more reviewers though. Marcel, thanks a lot for taking care of this! > --- > hw/core/qdev.c | 12 ++++++++++-- > include/hw/qdev-core.h | 2 +- > monitor.c | 11 ++++------- > 3 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 413b413..35fd00d 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > } while (class != object_class_by_name(TYPE_DEVICE)); > } > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > { > GSList **list = opaque; > DeviceState *dev = DEVICE(obj); > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > *list = g_slist_append(*list, dev); > } > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > return 0; > } > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > +{ > + GSList *list = NULL; > + > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > + > + return list; > +} > + > static bool device_get_realized(Object *obj, Error **errp) > { > DeviceState *dev = DEVICE(obj); > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index d3a2940..589bbe7 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > char *qdev_get_dev_path(DeviceState *dev); > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > Error **errp); > diff --git a/monitor.c b/monitor.c > index fa00594..f1031a1 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > static void peripheral_device_del_completion(ReadLineState *rs, > const char *str, size_t len) > { > - Object *peripheral; > - GSList *list = NULL, *item; > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > + GSList *list, *item; > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > - if (peripheral == NULL) { > + list = qdev_build_hotpluggable_device_list(peripheral); > + if (!list) { > return; > } > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > - &list); > - > for (item = list; item; item = g_slist_next(item)) { > DeviceState *dev = item->data; >
On 26 November 2014 at 18:05, Luiz Capitulino <lcapitulino@redhat.com> wrote: > Peter, can you apply this patch directly to master to avoid me a pull > request? Maybe it's a good idea to wait until tomorrow for more > reviewers though. Sure. I'll apply it to master some time tomorrow afternoon I expect. -- PMM
On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > On Wed, 26 Nov 2014 13:50:01 +0200 > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > The commits: > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > It can be easily reproduced by: > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > (qemu) device_del > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > Aborted (core dumped) > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > all peripherals and their children assuming all are devices. It doesn't work > > since PCI devices have at least on child which is a memory region (bus master). > > > > Solved by observing that all devices appear as direct children of > > /machine/peripheral container. No need of going recursively > > over all the children. > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > Peter, can you apply this patch directly to master to avoid me a pull > request? Maybe it's a good idea to wait until tomorrow for more > reviewers though. Yes, another review would be welcomed, especially a QOM reviewer. > > Marcel, thanks a lot for taking care of this! No problem, glad to do it. I forgot to add: Reported-by: Gal Hammer <ghammer@redhat.com> Thanks, Marcel > > > --- > > hw/core/qdev.c | 12 ++++++++++-- > > include/hw/qdev-core.h | 2 +- > > monitor.c | 11 ++++------- > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 413b413..35fd00d 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > } while (class != object_class_by_name(TYPE_DEVICE)); > > } > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > { > > GSList **list = opaque; > > DeviceState *dev = DEVICE(obj); > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > *list = g_slist_append(*list, dev); > > } > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > return 0; > > } > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > +{ > > + GSList *list = NULL; > > + > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > + > > + return list; > > +} > > + > > static bool device_get_realized(Object *obj, Error **errp) > > { > > DeviceState *dev = DEVICE(obj); > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > index d3a2940..589bbe7 100644 > > --- a/include/hw/qdev-core.h > > +++ b/include/hw/qdev-core.h > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > Error **errp); > > diff --git a/monitor.c b/monitor.c > > index fa00594..f1031a1 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > static void peripheral_device_del_completion(ReadLineState *rs, > > const char *str, size_t len) > > { > > - Object *peripheral; > > - GSList *list = NULL, *item; > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > + GSList *list, *item; > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > - if (peripheral == NULL) { > > + list = qdev_build_hotpluggable_device_list(peripheral); > > + if (!list) { > > return; > > } > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > - &list); > > - > > for (item = list; item; item = g_slist_next(item)) { > > DeviceState *dev = item->data; > > >
On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > On Wed, 26 Nov 2014 13:50:01 +0200 > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > The commits: > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > It can be easily reproduced by: > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > (qemu) device_del > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > Aborted (core dumped) > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > all peripherals and their children assuming all are devices. It doesn't work > > since PCI devices have at least on child which is a memory region (bus master). > > > > Solved by observing that all devices appear as direct children of > > /machine/peripheral container. No need of going recursively > > over all the children. > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > Peter, can you apply this patch directly to master to avoid me a pull > request? Maybe it's a good idea to wait until tomorrow for more > reviewers though. Speaking of reviewers, I double checked the patch and indeed it solves the crash, but the original patch has another semantic error. It looks for hot-pluggable device and not *hot-plugged* ones. I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... Thanks, Marcel > > Marcel, thanks a lot for taking care of this! > > > --- > > hw/core/qdev.c | 12 ++++++++++-- > > include/hw/qdev-core.h | 2 +- > > monitor.c | 11 ++++------- > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index 413b413..35fd00d 100644 > > --- a/hw/core/qdev.c > > +++ b/hw/core/qdev.c > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > } while (class != object_class_by_name(TYPE_DEVICE)); > > } > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > { > > GSList **list = opaque; > > DeviceState *dev = DEVICE(obj); > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > *list = g_slist_append(*list, dev); > > } > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > return 0; > > } > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > +{ > > + GSList *list = NULL; > > + > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > + > > + return list; > > +} > > + > > static bool device_get_realized(Object *obj, Error **errp) > > { > > DeviceState *dev = DEVICE(obj); > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > index d3a2940..589bbe7 100644 > > --- a/include/hw/qdev-core.h > > +++ b/include/hw/qdev-core.h > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > Error **errp); > > diff --git a/monitor.c b/monitor.c > > index fa00594..f1031a1 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > static void peripheral_device_del_completion(ReadLineState *rs, > > const char *str, size_t len) > > { > > - Object *peripheral; > > - GSList *list = NULL, *item; > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > + GSList *list, *item; > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > - if (peripheral == NULL) { > > + list = qdev_build_hotpluggable_device_list(peripheral); > > + if (!list) { > > return; > > } > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > - &list); > > - > > for (item = list; item; item = g_slist_next(item)) { > > DeviceState *dev = item->data; > > > >
On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > On Wed, 26 Nov 2014 13:50:01 +0200 > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > The commits: > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > It can be easily reproduced by: > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > (qemu) device_del > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > Aborted (core dumped) > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > all peripherals and their children assuming all are devices. It doesn't work > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > Solved by observing that all devices appear as direct children of > > > /machine/peripheral container. No need of going recursively > > > over all the children. > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > Peter, can you apply this patch directly to master to avoid me a pull > > request? Maybe it's a good idea to wait until tomorrow for more > > reviewers though. > Speaking of reviewers, I double checked the patch and indeed it solves > the crash, but the original patch has another semantic error. > It looks for hot-pluggable device and not *hot-plugged* ones. Thinking further, this is not fully true, we can hot-unplugged devices that weren't hot-plugged. We have only a specific problem/bug with pci-2-pci bridge that is hotpluggable, but can be hot-unpluged *only if* was hot-plugged. Other devices do not have this limitation. > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... No need for a solution now, the above bug can wait for 2.3, the crash is more important. Please pull the patch for 2.2 Thanks, Marcel > > Thanks, > Marcel > > > > > > Marcel, thanks a lot for taking care of this! > > > > > --- > > > hw/core/qdev.c | 12 ++++++++++-- > > > include/hw/qdev-core.h | 2 +- > > > monitor.c | 11 ++++------- > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > index 413b413..35fd00d 100644 > > > --- a/hw/core/qdev.c > > > +++ b/hw/core/qdev.c > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > } > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > { > > > GSList **list = opaque; > > > DeviceState *dev = DEVICE(obj); > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > *list = g_slist_append(*list, dev); > > > } > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > return 0; > > > } > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > +{ > > > + GSList *list = NULL; > > > + > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > + > > > + return list; > > > +} > > > + > > > static bool device_get_realized(Object *obj, Error **errp) > > > { > > > DeviceState *dev = DEVICE(obj); > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > index d3a2940..589bbe7 100644 > > > --- a/include/hw/qdev-core.h > > > +++ b/include/hw/qdev-core.h > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > Error **errp); > > > diff --git a/monitor.c b/monitor.c > > > index fa00594..f1031a1 100644 > > > --- a/monitor.c > > > +++ b/monitor.c > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > const char *str, size_t len) > > > { > > > - Object *peripheral; > > > - GSList *list = NULL, *item; > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > + GSList *list, *item; > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > - if (peripheral == NULL) { > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > + if (!list) { > > > return; > > > } > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > - &list); > > > - > > > for (item = list; item; item = g_slist_next(item)) { > > > DeviceState *dev = item->data; > > > > > > > > > > >
On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > On Wed, 26 Nov 2014 13:50:01 +0200 > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > The commits: > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > It can be easily reproduced by: > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > (qemu) device_del > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > Aborted (core dumped) > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > all peripherals and their children assuming all are devices. It doesn't work > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > Solved by observing that all devices appear as direct children of > > > /machine/peripheral container. No need of going recursively > > > over all the children. > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > Peter, can you apply this patch directly to master to avoid me a pull > > request? Maybe it's a good idea to wait until tomorrow for more > > reviewers though. > Speaking of reviewers, I double checked the patch and indeed it solves > the crash, but the original patch has another semantic error. > It looks for hot-pluggable device and not *hot-plugged* ones. > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > Hi Marcel, May you give an example for a hot-plugged but non-hot-pluggable device? Regards, Zhu > Thanks, > Marcel > > > > > > Marcel, thanks a lot for taking care of this! > > > > > --- > > > hw/core/qdev.c | 12 ++++++++++-- > > > include/hw/qdev-core.h | 2 +- > > > monitor.c | 11 ++++------- > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > index 413b413..35fd00d 100644 > > > --- a/hw/core/qdev.c > > > +++ b/hw/core/qdev.c > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > } > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > { > > > GSList **list = opaque; > > > DeviceState *dev = DEVICE(obj); > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > *list = g_slist_append(*list, dev); > > > } > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > return 0; > > > } > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > +{ > > > + GSList *list = NULL; > > > + > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > + > > > + return list; > > > +} > > > + > > > static bool device_get_realized(Object *obj, Error **errp) > > > { > > > DeviceState *dev = DEVICE(obj); > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > index d3a2940..589bbe7 100644 > > > --- a/include/hw/qdev-core.h > > > +++ b/include/hw/qdev-core.h > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > Error **errp); > > > diff --git a/monitor.c b/monitor.c > > > index fa00594..f1031a1 100644 > > > --- a/monitor.c > > > +++ b/monitor.c > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > const char *str, size_t len) > > > { > > > - Object *peripheral; > > > - GSList *list = NULL, *item; > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > + GSList *list, *item; > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > - if (peripheral == NULL) { > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > + if (!list) { > > > return; > > > } > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > - &list); > > > - > > > for (item = list; item; item = g_slist_next(item)) { > > > DeviceState *dev = item->data; > > > > > > > > > >
On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > The commits: > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > It can be easily reproduced by: > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > (qemu) device_del > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > Aborted (core dumped) > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > Solved by observing that all devices appear as direct children of > > > > /machine/peripheral container. No need of going recursively > > > > over all the children. > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > request? Maybe it's a good idea to wait until tomorrow for more > > > reviewers though. > > Speaking of reviewers, I double checked the patch and indeed it solves > > the crash, but the original patch has another semantic error. > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > Hi Marcel, > > May you give an example for a hot-plugged but non-hot-pluggable device? I was talking about something different: A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. This is not true for pci-2-pci device. But this is another issue and can wait for 2.3. So you patch was *almost* correct for looking hotpluggable devices, the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). But for now, we should get the crash fix in and handle that separately for 2.3. If anybody has a better idea, pleas advice. Thanks, Marcel > > Regards, > Zhu > > > Thanks, > > Marcel > > > > > > > > > > Marcel, thanks a lot for taking care of this! > > > > > > > --- > > > > hw/core/qdev.c | 12 ++++++++++-- > > > > include/hw/qdev-core.h | 2 +- > > > > monitor.c | 11 ++++------- > > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > index 413b413..35fd00d 100644 > > > > --- a/hw/core/qdev.c > > > > +++ b/hw/core/qdev.c > > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > > } > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > > { > > > > GSList **list = opaque; > > > > DeviceState *dev = DEVICE(obj); > > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > *list = g_slist_append(*list, dev); > > > > } > > > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > > return 0; > > > > } > > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > > +{ > > > > + GSList *list = NULL; > > > > + > > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > > + > > > > + return list; > > > > +} > > > > + > > > > static bool device_get_realized(Object *obj, Error **errp) > > > > { > > > > DeviceState *dev = DEVICE(obj); > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > > index d3a2940..589bbe7 100644 > > > > --- a/include/hw/qdev-core.h > > > > +++ b/include/hw/qdev-core.h > > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > > Error **errp); > > > > diff --git a/monitor.c b/monitor.c > > > > index fa00594..f1031a1 100644 > > > > --- a/monitor.c > > > > +++ b/monitor.c > > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > > const char *str, size_t len) > > > > { > > > > - Object *peripheral; > > > > - GSList *list = NULL, *item; > > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > > + GSList *list, *item; > > > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > > - if (peripheral == NULL) { > > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > > + if (!list) { > > > > return; > > > > } > > > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > > - &list); > > > > - > > > > for (item = list; item; item = g_slist_next(item)) { > > > > DeviceState *dev = item->data; > > > > > > > > > > > > > > > > > >
On Thu, 2014-11-27 at 13:41 +0200, Marcel Apfelbaum wrote: > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > The commits: > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > It can be easily reproduced by: > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > (qemu) device_del > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > Aborted (core dumped) > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > /machine/peripheral container. No need of going recursively > > > > > over all the children. > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > reviewers though. > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > the crash, but the original patch has another semantic error. > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > Hi Marcel, > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > I was talking about something different: > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > This is not true for pci-2-pci device. > I'm sorry to misunderstand your mean. Thanks for your explanation. > But this is another issue and can wait for 2.3. > So you patch was *almost* correct for looking hotpluggable devices, > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). > Yes, I test pci-bridge just now. The command "device_del" will cause core dump. I will investigate other devices further. And if I device_add pci-bridge without property chassis_nr, it will causes core dumped too. Regards, Zhu > But for now, we should get the crash fix in and handle that separately > for 2.3. If anybody has a better idea, pleas advice. > > Thanks, > Marcel > > > > > > > > Regards, > > Zhu > > > > > Thanks, > > > Marcel > > > > > > > > > > > > > > Marcel, thanks a lot for taking care of this! > > > > > > > > > --- > > > > > hw/core/qdev.c | 12 ++++++++++-- > > > > > include/hw/qdev-core.h | 2 +- > > > > > monitor.c | 11 ++++------- > > > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > > index 413b413..35fd00d 100644 > > > > > --- a/hw/core/qdev.c > > > > > +++ b/hw/core/qdev.c > > > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > > > } > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > > > { > > > > > GSList **list = opaque; > > > > > DeviceState *dev = DEVICE(obj); > > > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > *list = g_slist_append(*list, dev); > > > > > } > > > > > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > > > return 0; > > > > > } > > > > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > > > +{ > > > > > + GSList *list = NULL; > > > > > + > > > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > > > + > > > > > + return list; > > > > > +} > > > > > + > > > > > static bool device_get_realized(Object *obj, Error **errp) > > > > > { > > > > > DeviceState *dev = DEVICE(obj); > > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > > > index d3a2940..589bbe7 100644 > > > > > --- a/include/hw/qdev-core.h > > > > > +++ b/include/hw/qdev-core.h > > > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > > > Error **errp); > > > > > diff --git a/monitor.c b/monitor.c > > > > > index fa00594..f1031a1 100644 > > > > > --- a/monitor.c > > > > > +++ b/monitor.c > > > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > > > const char *str, size_t len) > > > > > { > > > > > - Object *peripheral; > > > > > - GSList *list = NULL, *item; > > > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > > > + GSList *list, *item; > > > > > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > > > - if (peripheral == NULL) { > > > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > > > + if (!list) { > > > > > return; > > > > > } > > > > > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > > > - &list); > > > > > - > > > > > for (item = list; item; item = g_slist_next(item)) { > > > > > DeviceState *dev = item->data; > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, 2014-11-27 at 20:08 +0800, Zhu Guihua wrote: > On Thu, 2014-11-27 at 13:41 +0200, Marcel Apfelbaum wrote: > > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > > > The commits: > > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > > It can be easily reproduced by: > > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > > > (qemu) device_del > > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > > Aborted (core dumped) > > > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > > /machine/peripheral container. No need of going recursively > > > > > > over all the children. > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > > reviewers though. > > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > > the crash, but the original patch has another semantic error. > > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > > > Hi Marcel, > > > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > > I was talking about something different: > > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > > This is not true for pci-2-pci device. > > > > I'm sorry to misunderstand your mean. > Thanks for your explanation. > > > But this is another issue and can wait for 2.3. > > So you patch was *almost* correct for looking hotpluggable devices, > > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). > > > > Yes, I test pci-bridge just now. The command "device_del" will cause > core dump. I will investigate other devices further. After this patch you will not have a core dump. > > And if I device_add pci-bridge without property chassis_nr, it will > causes core dumped too. Are you sure you use the master branch? This is not happening Anyway, you didn't modify device_add, and this is not happening on the latest master version. Please correct me if I am wrong. Thanks, Marcel > > Regards, > Zhu > > > But for now, we should get the crash fix in and handle that separately > > for 2.3. If anybody has a better idea, pleas advice. > > > > Thanks, > > Marcel > > > > > > > > > > > > > > Regards, > > > Zhu > > > > > > > Thanks, > > > > Marcel > > > > > > > > > > > > > > > > > > Marcel, thanks a lot for taking care of this! > > > > > > > > > > > --- > > > > > > hw/core/qdev.c | 12 ++++++++++-- > > > > > > include/hw/qdev-core.h | 2 +- > > > > > > monitor.c | 11 ++++------- > > > > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > > > index 413b413..35fd00d 100644 > > > > > > --- a/hw/core/qdev.c > > > > > > +++ b/hw/core/qdev.c > > > > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > > > > } > > > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > > > > { > > > > > > GSList **list = opaque; > > > > > > DeviceState *dev = DEVICE(obj); > > > > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > > *list = g_slist_append(*list, dev); > > > > > > } > > > > > > > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > > > > +{ > > > > > > + GSList *list = NULL; > > > > > > + > > > > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > > > > + > > > > > > + return list; > > > > > > +} > > > > > > + > > > > > > static bool device_get_realized(Object *obj, Error **errp) > > > > > > { > > > > > > DeviceState *dev = DEVICE(obj); > > > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > > > > index d3a2940..589bbe7 100644 > > > > > > --- a/include/hw/qdev-core.h > > > > > > +++ b/include/hw/qdev-core.h > > > > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > > > > Error **errp); > > > > > > diff --git a/monitor.c b/monitor.c > > > > > > index fa00594..f1031a1 100644 > > > > > > --- a/monitor.c > > > > > > +++ b/monitor.c > > > > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > > > > const char *str, size_t len) > > > > > > { > > > > > > - Object *peripheral; > > > > > > - GSList *list = NULL, *item; > > > > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > > > > + GSList *list, *item; > > > > > > > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > > > > - if (peripheral == NULL) { > > > > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > > > > + if (!list) { > > > > > > return; > > > > > > } > > > > > > > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > > > > - &list); > > > > > > - > > > > > > for (item = list; item; item = g_slist_next(item)) { > > > > > > DeviceState *dev = item->data; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, 27 Nov 2014 13:41:07 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > The commits: > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > It can be easily reproduced by: > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > (qemu) device_del > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > Aborted (core dumped) > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > /machine/peripheral container. No need of going recursively > > > > > over all the children. > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > reviewers though. > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > the crash, but the original patch has another semantic error. > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > Hi Marcel, > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > I was talking about something different: > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. That's applicable to most of devices. > This is not true for pci-2-pci device. Do you have a reproducer? > > But this is another issue and can wait for 2.3. > So you patch was *almost* correct for looking hotpluggable devices, > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). > > But for now, we should get the crash fix in and handle that separately > for 2.3. If anybody has a better idea, pleas advice. > > Thanks, > Marcel > > > > > > > > Regards, > > Zhu > > > > > Thanks, > > > Marcel > > > > > > > > > > > > > > Marcel, thanks a lot for taking care of this! > > > > > > > > > --- > > > > > hw/core/qdev.c | 12 ++++++++++-- > > > > > include/hw/qdev-core.h | 2 +- > > > > > monitor.c | 11 ++++------- > > > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > > index 413b413..35fd00d 100644 > > > > > --- a/hw/core/qdev.c > > > > > +++ b/hw/core/qdev.c > > > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > > > } > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > > > { > > > > > GSList **list = opaque; > > > > > DeviceState *dev = DEVICE(obj); > > > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > *list = g_slist_append(*list, dev); > > > > > } > > > > > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > > > return 0; > > > > > } > > > > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > > > +{ > > > > > + GSList *list = NULL; > > > > > + > > > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > > > + > > > > > + return list; > > > > > +} > > > > > + > > > > > static bool device_get_realized(Object *obj, Error **errp) > > > > > { > > > > > DeviceState *dev = DEVICE(obj); > > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > > > index d3a2940..589bbe7 100644 > > > > > --- a/include/hw/qdev-core.h > > > > > +++ b/include/hw/qdev-core.h > > > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > > > Error **errp); > > > > > diff --git a/monitor.c b/monitor.c > > > > > index fa00594..f1031a1 100644 > > > > > --- a/monitor.c > > > > > +++ b/monitor.c > > > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > > > const char *str, size_t len) > > > > > { > > > > > - Object *peripheral; > > > > > - GSList *list = NULL, *item; > > > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > > > + GSList *list, *item; > > > > > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > > > - if (peripheral == NULL) { > > > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > > > + if (!list) { > > > > > return; > > > > > } > > > > > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > > > - &list); > > > > > - > > > > > for (item = list; item; item = g_slist_next(item)) { > > > > > DeviceState *dev = item->data; > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, 2014-11-27 at 13:38 +0100, Igor Mammedov wrote: > On Thu, 27 Nov 2014 13:41:07 +0200 > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > > > The commits: > > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > > It can be easily reproduced by: > > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > > > (qemu) device_del > > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > > Aborted (core dumped) > > > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > > /machine/peripheral container. No need of going recursively > > > > > > over all the children. > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > > reviewers though. > > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > > the crash, but the original patch has another semantic error. > > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > > > Hi Marcel, > > > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > > I was talking about something different: > > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > That's applicable to most of devices. > > > This is not true for pci-2-pci device. > Do you have a reproducer? Sure: <qemu-bin> <img> -device pci-bridge,chassis_nr=1,id=bridge On monitor: device_del bridge And nothing happens since pci-2-pci can't be hot unplugged The suspected reason is because it appears in ACPI tables. Thanks, Marcel > > > > > But this is another issue and can wait for 2.3. > > So you patch was *almost* correct for looking hotpluggable devices, > > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). > > > > But for now, we should get the crash fix in and handle that separately > > for 2.3. If anybody has a better idea, pleas advice. > > > > Thanks, > > Marcel > > > > > > > > > > > > > > Regards, > > > Zhu > > > > > > > Thanks, > > > > Marcel > > > > > > > > > > > > > > > > > > Marcel, thanks a lot for taking care of this! > > > > > > > > > > > --- > > > > > > hw/core/qdev.c | 12 ++++++++++-- > > > > > > include/hw/qdev-core.h | 2 +- > > > > > > monitor.c | 11 ++++------- > > > > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > > > index 413b413..35fd00d 100644 > > > > > > --- a/hw/core/qdev.c > > > > > > +++ b/hw/core/qdev.c > > > > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > > > > } > > > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > > > > { > > > > > > GSList **list = opaque; > > > > > > DeviceState *dev = DEVICE(obj); > > > > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > > *list = g_slist_append(*list, dev); > > > > > > } > > > > > > > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > > > > +{ > > > > > > + GSList *list = NULL; > > > > > > + > > > > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > > > > + > > > > > > + return list; > > > > > > +} > > > > > > + > > > > > > static bool device_get_realized(Object *obj, Error **errp) > > > > > > { > > > > > > DeviceState *dev = DEVICE(obj); > > > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > > > > index d3a2940..589bbe7 100644 > > > > > > --- a/include/hw/qdev-core.h > > > > > > +++ b/include/hw/qdev-core.h > > > > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > > > > Error **errp); > > > > > > diff --git a/monitor.c b/monitor.c > > > > > > index fa00594..f1031a1 100644 > > > > > > --- a/monitor.c > > > > > > +++ b/monitor.c > > > > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > > > > const char *str, size_t len) > > > > > > { > > > > > > - Object *peripheral; > > > > > > - GSList *list = NULL, *item; > > > > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > > > > + GSList *list, *item; > > > > > > > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > > > > - if (peripheral == NULL) { > > > > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > > > > + if (!list) { > > > > > > return; > > > > > > } > > > > > > > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > > > > - &list); > > > > > > - > > > > > > for (item = list; item; item = g_slist_next(item)) { > > > > > > DeviceState *dev = item->data; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, 27 Nov 2014 13:41:07 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > The commits: > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > It can be easily reproduced by: > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > (qemu) device_del > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > Aborted (core dumped) > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > /machine/peripheral container. No need of going recursively > > > > > over all the children. > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > reviewers though. > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > the crash, but the original patch has another semantic error. > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > Hi Marcel, > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > I was talking about something different: > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > This is not true for pci-2-pci device. > > But this is another issue and can wait for 2.3. Agreed. We're on blockers phase right now. So, if it's not a blocker, it can wait for -stable. > So you patch was *almost* correct for looking hotpluggable devices, > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). > > But for now, we should get the crash fix in and handle that separately > for 2.3. If anybody has a better idea, pleas advice.
On 27 November 2014 at 11:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote: > I was talking about something different: > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > This is not true for pci-2-pci device. > > But this is another issue and can wait for 2.3. > So you patch was *almost* correct for looking hotpluggable devices, > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). So, to be clear, the effect of this is just that our autocompletion will suggest a completion to the command which will fail if actually executed, right? That doesn't sound very serious, so I would be happy to postpone it to 2.3. -- PMM
On Thu, 2014-11-27 at 14:13 +0000, Peter Maydell wrote: > On 27 November 2014 at 11:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > I was talking about something different: > > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > > This is not true for pci-2-pci device. > > > > But this is another issue and can wait for 2.3. > > So you patch was *almost* correct for looking hotpluggable devices, > > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). > > So, to be clear, the effect of this is just that our autocompletion > will suggest a completion to the command which will fail if actually > executed, right? That doesn't sound very serious, so I would be happy > to postpone it to 2.3. Correct. But only if this patch is applied, before this patch is a QEMU crash. Please pull it to solve the crash and we will deal with the other issues on 2.3 Thanks, Marcel > > -- PMM
On 26 November 2014 at 11:50, Marcel Apfelbaum <marcel.a@redhat.com> wrote: > The commits: > - 6a1fa9f5 (monitor: add del completion for peripheral device) > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > cause a QEMU crash when trying to use HMP device_del auto-completion. > It can be easily reproduced by: > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > (qemu) device_del > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > Aborted (core dumped) > > The root cause is qdev_build_hotpluggable_device_list going recursively over > all peripherals and their children assuming all are devices. It doesn't work > since PCI devices have at least on child which is a memory region (bus master). > > Solved by observing that all devices appear as direct children of > /machine/peripheral container. No need of going recursively > over all the children. > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> Applied, thanks. -- PMM
On Thu, 2014-11-27 at 14:15 +0200, Marcel Apfelbaum wrote: > On Thu, 2014-11-27 at 20:08 +0800, Zhu Guihua wrote: > > On Thu, 2014-11-27 at 13:41 +0200, Marcel Apfelbaum wrote: > > > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > > > > > The commits: > > > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > > > It can be easily reproduced by: > > > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > > > > > (qemu) device_del > > > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > > > Aborted (core dumped) > > > > > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > > > /machine/peripheral container. No need of going recursively > > > > > > > over all the children. > > > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > > > reviewers though. > > > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > > > the crash, but the original patch has another semantic error. > > > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > > > > > Hi Marcel, > > > > > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > > > I was talking about something different: > > > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > > > This is not true for pci-2-pci device. > > > > > > > I'm sorry to misunderstand your mean. > > Thanks for your explanation. > > > > > But this is another issue and can wait for 2.3. > > > So you patch was *almost* correct for looking hotpluggable devices, > > > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). > > > > > > > Yes, I test pci-bridge just now. The command "device_del" will cause > > core dump. I will investigate other devices further. > After this patch you will not have a core dump. > > > > > And if I device_add pci-bridge without property chassis_nr, it will > > causes core dumped too. > Are you sure you use the master branch? This is not happening > Anyway, you didn't modify device_add, and this is not happening > on the latest master version. > > Please correct me if I am wrong. you are right, I am wrong. I used the master branch but not the latest version yesterday. regards, Zhu > > Thanks, > Marcel > > > > > Regards, > > Zhu > > > > > But for now, we should get the crash fix in and handle that separately > > > for 2.3. If anybody has a better idea, pleas advice. > > > > > > Thanks, > > > Marcel > > > > > > > > > > > > > > > > > > > > Regards, > > > > Zhu > > > > > > > > > Thanks, > > > > > Marcel > > > > > > > > > > > > > > > > > > > > > > Marcel, thanks a lot for taking care of this! > > > > > > > > > > > > > --- > > > > > > > hw/core/qdev.c | 12 ++++++++++-- > > > > > > > include/hw/qdev-core.h | 2 +- > > > > > > > monitor.c | 11 ++++------- > > > > > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > > > > index 413b413..35fd00d 100644 > > > > > > > --- a/hw/core/qdev.c > > > > > > > +++ b/hw/core/qdev.c > > > > > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > > > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > > > > > } > > > > > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > > > > > { > > > > > > > GSList **list = opaque; > > > > > > > DeviceState *dev = DEVICE(obj); > > > > > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > > > *list = g_slist_append(*list, dev); > > > > > > > } > > > > > > > > > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > > > > > +{ > > > > > > > + GSList *list = NULL; > > > > > > > + > > > > > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > > > > > + > > > > > > > + return list; > > > > > > > +} > > > > > > > + > > > > > > > static bool device_get_realized(Object *obj, Error **errp) > > > > > > > { > > > > > > > DeviceState *dev = DEVICE(obj); > > > > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > > > > > index d3a2940..589bbe7 100644 > > > > > > > --- a/include/hw/qdev-core.h > > > > > > > +++ b/include/hw/qdev-core.h > > > > > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > > > > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > > > > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > > > > > Error **errp); > > > > > > > diff --git a/monitor.c b/monitor.c > > > > > > > index fa00594..f1031a1 100644 > > > > > > > --- a/monitor.c > > > > > > > +++ b/monitor.c > > > > > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > > > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > > > > > const char *str, size_t len) > > > > > > > { > > > > > > > - Object *peripheral; > > > > > > > - GSList *list = NULL, *item; > > > > > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > > > > > + GSList *list, *item; > > > > > > > > > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > > > > > - if (peripheral == NULL) { > > > > > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > > > > > + if (!list) { > > > > > > > return; > > > > > > > } > > > > > > > > > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > > > > > - &list); > > > > > > > - > > > > > > > for (item = list; item; item = g_slist_next(item)) { > > > > > > > DeviceState *dev = item->data; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, 2014-11-27 at 15:48 +0200, Marcel Apfelbaum wrote: > On Thu, 2014-11-27 at 13:38 +0100, Igor Mammedov wrote: > > On Thu, 27 Nov 2014 13:41:07 +0200 > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > > > > > The commits: > > > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > > > It can be easily reproduced by: > > > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > > > > > (qemu) device_del > > > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > > > Aborted (core dumped) > > > > > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > > > /machine/peripheral container. No need of going recursively > > > > > > > over all the children. > > > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > > > reviewers though. > > > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > > > the crash, but the original patch has another semantic error. > > > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > > > > > Hi Marcel, > > > > > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > > > I was talking about something different: > > > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > > That's applicable to most of devices. > > > > > This is not true for pci-2-pci device. > > Do you have a reproducer? > Sure: > <qemu-bin> <img> -device pci-bridge,chassis_nr=1,id=bridge > It is also not true for pc-dimm. <qemu-bin> <img> -m 512,slots=10,maxmem=100G -object memory-backend-ram,id=ram0,size=128M -device pc-dimm,id=d0,memdev=ram0 Regards, Zhu > On monitor: > device_del bridge > > And nothing happens since pci-2-pci can't be hot unplugged > The suspected reason is because it appears in ACPI tables. > > Thanks, > Marcel > > > > > > > > > > But this is another issue and can wait for 2.3. > > > So you patch was *almost* correct for looking hotpluggable devices, > > > the only problem we have is with pci-2-pci bridge (, and maybe with others that I don't know). > > > > > > But for now, we should get the crash fix in and handle that separately > > > for 2.3. If anybody has a better idea, pleas advice. > > > > > > Thanks, > > > Marcel > > > > > > > > > > > > > > > > > > > > Regards, > > > > Zhu > > > > > > > > > Thanks, > > > > > Marcel > > > > > > > > > > > > > > > > > > > > > > Marcel, thanks a lot for taking care of this! > > > > > > > > > > > > > --- > > > > > > > hw/core/qdev.c | 12 ++++++++++-- > > > > > > > include/hw/qdev-core.h | 2 +- > > > > > > > monitor.c | 11 ++++------- > > > > > > > 3 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > > > > > > index 413b413..35fd00d 100644 > > > > > > > --- a/hw/core/qdev.c > > > > > > > +++ b/hw/core/qdev.c > > > > > > > @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) > > > > > > > } while (class != object_class_by_name(TYPE_DEVICE)); > > > > > > > } > > > > > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > > > +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) > > > > > > > { > > > > > > > GSList **list = opaque; > > > > > > > DeviceState *dev = DEVICE(obj); > > > > > > > @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) > > > > > > > *list = g_slist_append(*list, dev); > > > > > > > } > > > > > > > > > > > > > > - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) > > > > > > > +{ > > > > > > > + GSList *list = NULL; > > > > > > > + > > > > > > > + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); > > > > > > > + > > > > > > > + return list; > > > > > > > +} > > > > > > > + > > > > > > > static bool device_get_realized(Object *obj, Error **errp) > > > > > > > { > > > > > > > DeviceState *dev = DEVICE(obj); > > > > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > > > > > index d3a2940..589bbe7 100644 > > > > > > > --- a/include/hw/qdev-core.h > > > > > > > +++ b/include/hw/qdev-core.h > > > > > > > @@ -365,7 +365,7 @@ extern int qdev_hotplug; > > > > > > > > > > > > > > char *qdev_get_dev_path(DeviceState *dev); > > > > > > > > > > > > > > -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); > > > > > > > +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); > > > > > > > > > > > > > > void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, > > > > > > > Error **errp); > > > > > > > diff --git a/monitor.c b/monitor.c > > > > > > > index fa00594..f1031a1 100644 > > > > > > > --- a/monitor.c > > > > > > > +++ b/monitor.c > > > > > > > @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) > > > > > > > static void peripheral_device_del_completion(ReadLineState *rs, > > > > > > > const char *str, size_t len) > > > > > > > { > > > > > > > - Object *peripheral; > > > > > > > - GSList *list = NULL, *item; > > > > > > > + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); > > > > > > > + GSList *list, *item; > > > > > > > > > > > > > > - peripheral = object_resolve_path("/machine/peripheral/", NULL); > > > > > > > - if (peripheral == NULL) { > > > > > > > + list = qdev_build_hotpluggable_device_list(peripheral); > > > > > > > + if (!list) { > > > > > > > return; > > > > > > > } > > > > > > > > > > > > > > - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, > > > > > > > - &list); > > > > > > > - > > > > > > > for (item = list; item; item = g_slist_next(item)) { > > > > > > > DeviceState *dev = item->data; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Fri, 2014-11-28 at 09:50 +0800, Zhu Guihua wrote: > On Thu, 2014-11-27 at 15:48 +0200, Marcel Apfelbaum wrote: > > On Thu, 2014-11-27 at 13:38 +0100, Igor Mammedov wrote: > > > On Thu, 27 Nov 2014 13:41:07 +0200 > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > > > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > > > > > > > The commits: > > > > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > > > > It can be easily reproduced by: > > > > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > > > > > > > (qemu) device_del > > > > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > > > > Aborted (core dumped) > > > > > > > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > > > > /machine/peripheral container. No need of going recursively > > > > > > > > over all the children. > > > > > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > > > > reviewers though. > > > > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > > > > the crash, but the original patch has another semantic error. > > > > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > > > > > > > Hi Marcel, > > > > > > > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > > > > I was talking about something different: > > > > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > > > That's applicable to most of devices. > > > > > > > This is not true for pci-2-pci device. > > > Do you have a reproducer? > > Sure: > > <qemu-bin> <img> -device pci-bridge,chassis_nr=1,id=bridge > > > > It is also not true for pc-dimm. > <qemu-bin> <img> -m 512,slots=10,maxmem=100G -object > memory-backend-ram,id=ram0,size=128M -device pc-dimm,id=d0,memdev=ram0 Thanks for finding it! I'll try to add a "can be hot-unplugged" flag whose default value is "hotpluggable". This can be overridden by sub-classes to meet their needs. 2.3 material, of course Thanks, Marcel [...]
On Fri, 28 Nov 2014 07:53:41 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Fri, 2014-11-28 at 09:50 +0800, Zhu Guihua wrote: > > On Thu, 2014-11-27 at 15:48 +0200, Marcel Apfelbaum wrote: > > > On Thu, 2014-11-27 at 13:38 +0100, Igor Mammedov wrote: > > > > On Thu, 27 Nov 2014 13:41:07 +0200 > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > On Thu, 2014-11-27 at 19:35 +0800, Zhu Guihua wrote: > > > > > > On Thu, 2014-11-27 at 13:11 +0200, Marcel Apfelbaum wrote: > > > > > > > On Wed, 2014-11-26 at 13:05 -0500, Luiz Capitulino wrote: > > > > > > > > On Wed, 26 Nov 2014 13:50:01 +0200 > > > > > > > > Marcel Apfelbaum <marcel.a@redhat.com> wrote: > > > > > > > > > > > > > > > > > The commits: > > > > > > > > > - 6a1fa9f5 (monitor: add del completion for peripheral device) > > > > > > > > > - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) > > > > > > > > > > > > > > > > > > cause a QEMU crash when trying to use HMP device_del auto-completion. > > > > > > > > > It can be easily reproduced by: > > > > > > > > > <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet > > > > > > > > > > > > > > > > > > (qemu) device_del > > > > > > > > > /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device > > > > > > > > > Aborted (core dumped) > > > > > > > > > > > > > > > > > > The root cause is qdev_build_hotpluggable_device_list going recursively over > > > > > > > > > all peripherals and their children assuming all are devices. It doesn't work > > > > > > > > > since PCI devices have at least on child which is a memory region (bus master). > > > > > > > > > > > > > > > > > > Solved by observing that all devices appear as direct children of > > > > > > > > > /machine/peripheral container. No need of going recursively > > > > > > > > > over all the children. > > > > > > > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > > > > > > > > > > > Peter, can you apply this patch directly to master to avoid me a pull > > > > > > > > request? Maybe it's a good idea to wait until tomorrow for more > > > > > > > > reviewers though. > > > > > > > Speaking of reviewers, I double checked the patch and indeed it solves > > > > > > > the crash, but the original patch has another semantic error. > > > > > > > It looks for hot-pluggable device and not *hot-plugged* ones. > > > > > > > > > > > > > > I'll try to come with a solution fast. It should be a "hot-plugged" property somewhere... > > > > > > > > > > > > > Hi Marcel, > > > > > > > > > > > > May you give an example for a hot-plugged but non-hot-pluggable device? > > > > > I was talking about something different: > > > > > A hot-pluggable device that was not hot-plugged is assumed to be hot-unpluggable. > > > > That's applicable to most of devices. > > > > > > > > > This is not true for pci-2-pci device. > > > > Do you have a reproducer? > > > Sure: > > > <qemu-bin> <img> -device pci-bridge,chassis_nr=1,id=bridge > > > > > > > It is also not true for pc-dimm. > > <qemu-bin> <img> -m 512,slots=10,maxmem=100G -object > > memory-backend-ram,id=ram0,size=128M -device pc-dimm,id=d0,memdev=ram0 It's just that unplug is not implemented for pc-dimm, but eventually it should be unpluggable. > > Thanks for finding it! > I'll try to add a "can be hot-unplugged" flag whose default value > is "hotpluggable". > This can be overridden by sub-classes to meet their needs. > > 2.3 material, of course > > Thanks, > Marcel > > [...] >
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 413b413..35fd00d 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -935,7 +935,7 @@ void qdev_alias_all_properties(DeviceState *target, Object *source) } while (class != object_class_by_name(TYPE_DEVICE)); } -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) +static int qdev_add_hotpluggable_device(Object *obj, void *opaque) { GSList **list = opaque; DeviceState *dev = DEVICE(obj); @@ -944,10 +944,18 @@ int qdev_build_hotpluggable_device_list(Object *obj, void *opaque) *list = g_slist_append(*list, dev); } - object_child_foreach(obj, qdev_build_hotpluggable_device_list, opaque); return 0; } +GSList *qdev_build_hotpluggable_device_list(Object *peripheral) +{ + GSList *list = NULL; + + object_child_foreach(peripheral, qdev_add_hotpluggable_device, &list); + + return list; +} + static bool device_get_realized(Object *obj, Error **errp) { DeviceState *dev = DEVICE(obj); diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index d3a2940..589bbe7 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -365,7 +365,7 @@ extern int qdev_hotplug; char *qdev_get_dev_path(DeviceState *dev); -int qdev_build_hotpluggable_device_list(Object *obj, void *opaque); +GSList *qdev_build_hotpluggable_device_list(Object *peripheral); void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler, Error **errp); diff --git a/monitor.c b/monitor.c index fa00594..f1031a1 100644 --- a/monitor.c +++ b/monitor.c @@ -4321,17 +4321,14 @@ void object_add_completion(ReadLineState *rs, int nb_args, const char *str) static void peripheral_device_del_completion(ReadLineState *rs, const char *str, size_t len) { - Object *peripheral; - GSList *list = NULL, *item; + Object *peripheral = container_get(qdev_get_machine(), "/peripheral"); + GSList *list, *item; - peripheral = object_resolve_path("/machine/peripheral/", NULL); - if (peripheral == NULL) { + list = qdev_build_hotpluggable_device_list(peripheral); + if (!list) { return; } - object_child_foreach(peripheral, qdev_build_hotpluggable_device_list, - &list); - for (item = list; item; item = g_slist_next(item)) { DeviceState *dev = item->data;
The commits: - 6a1fa9f5 (monitor: add del completion for peripheral device) - 66e56b13 (qdev: add qdev_build_hotpluggable_device_list helper) cause a QEMU crash when trying to use HMP device_del auto-completion. It can be easily reproduced by: <qemu-bin> -enable-kvm ~/images/fedora.qcow2 -monitor stdio -device virtio-net-pci,id=vnet (qemu) device_del /home/mapfelba/git/upstream/qemu/hw/core/qdev.c:941:qdev_build_hotpluggable_device_list: Object 0x7f6ce04e4fe0 is not an instance of type device Aborted (core dumped) The root cause is qdev_build_hotpluggable_device_list going recursively over all peripherals and their children assuming all are devices. It doesn't work since PCI devices have at least on child which is a memory region (bus master). Solved by observing that all devices appear as direct children of /machine/peripheral container. No need of going recursively over all the children. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- hw/core/qdev.c | 12 ++++++++++-- include/hw/qdev-core.h | 2 +- monitor.c | 11 ++++------- 3 files changed, 15 insertions(+), 10 deletions(-)