| Message ID | 1409392827-9372-3-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
Hi, > +static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) > +{ > + bool ret = false; > + char *devpath_src = qdev_get_fw_dev_path(src); > + char *devpath_dst = qdev_get_fw_dev_path(dst); > + > + if (!strcmp(devpath_src, devpath_dst)) { > + ret = true; > + } > + > + g_free(devpath_src); > + g_free(devpath_dst); > + return ret; > +} > + > +void del_boot_device_path(DeviceState *dev) > +{ > + FWBootEntry *i; > + > + assert(dev != NULL); > + > + /* remove all entries of the assigned device */ > + QTAILQ_FOREACH(i, &fw_boot_order, link) { > + if (i->dev == NULL) { > + continue; > + } > + if ((i->dev == dev || is_same_fw_dev_path(i->dev, dev))) { Why this is needed? Is there any case where i-->dev != dev but is_same_fw_dev_path() returns true? cheers, Gerd
> From: Gerd Hoffmann [mailto:kraxel@redhat.com] > Sent: Monday, September 01, 2014 2:43 PM > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > Importance: High > > Hi, > > > +static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) > > +{ > > + bool ret = false; > > + char *devpath_src = qdev_get_fw_dev_path(src); > > + char *devpath_dst = qdev_get_fw_dev_path(dst); > > + > > + if (!strcmp(devpath_src, devpath_dst)) { > > + ret = true; > > + } > > + > > + g_free(devpath_src); > > + g_free(devpath_dst); > > + return ret; > > +} > > + > > +void del_boot_device_path(DeviceState *dev) > > +{ > > + FWBootEntry *i; > > + > > + assert(dev != NULL); > > + > > + /* remove all entries of the assigned device */ > > + QTAILQ_FOREACH(i, &fw_boot_order, link) { > > + if (i->dev == NULL) { > > + continue; > > + } > > + if ((i->dev == dev || is_same_fw_dev_path(i->dev, dev))) { > > Why this is needed? Is there any case where i-->dev != dev but > is_same_fw_dev_path() returns true? > Yes, it is needed. At present, the virito-net-pci device compliance with this situation. Please see the qom path about virtio-net-pci and virtio-net device: id: null, /machine/peripheral/nic1/virtio-backend id: nic1, /machine/peripheral/nic1 Best regards, -Gonglei
On Mon, Sep 01, 2014 at 06:47:13AM +0000, Gonglei (Arei) wrote: > > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > > Sent: Monday, September 01, 2014 2:43 PM > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > Importance: High > > > > Hi, > > > > > +static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) > > > +{ > > > + bool ret = false; > > > + char *devpath_src = qdev_get_fw_dev_path(src); > > > + char *devpath_dst = qdev_get_fw_dev_path(dst); > > > + > > > + if (!strcmp(devpath_src, devpath_dst)) { > > > + ret = true; > > > + } > > > + > > > + g_free(devpath_src); > > > + g_free(devpath_dst); > > > + return ret; > > > +} > > > + > > > +void del_boot_device_path(DeviceState *dev) > > > +{ > > > + FWBootEntry *i; > > > + > > > + assert(dev != NULL); > > > + > > > + /* remove all entries of the assigned device */ > > > + QTAILQ_FOREACH(i, &fw_boot_order, link) { > > > + if (i->dev == NULL) { > > > + continue; > > > + } > > > + if ((i->dev == dev || is_same_fw_dev_path(i->dev, dev))) { > > > > Why this is needed? Is there any case where i-->dev != dev but > > is_same_fw_dev_path() returns true? > > > Yes, it is needed. At present, the virito-net-pci device > compliance with this situation. > > Please see the qom path about virtio-net-pci and virtio-net device: > > id: null, /machine/peripheral/nic1/virtio-backend > id: nic1, /machine/peripheral/nic1 And why exactly is the caller passing a different pointer to del_boot_device_path()? Why not simply change the caller to pass the same pointer to add_boot_device_path() and del_boot_device_path()?
Hi, > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Wednesday, September 03, 2014 2:00 AM > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > Importance: High > > On Mon, Sep 01, 2014 at 06:47:13AM +0000, Gonglei (Arei) wrote: > > > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > > > Sent: Monday, September 01, 2014 2:43 PM > > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path > function > > > Importance: High > > > > > > Hi, > > > > > > > +static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) > > > > +{ > > > > + bool ret = false; > > > > + char *devpath_src = qdev_get_fw_dev_path(src); > > > > + char *devpath_dst = qdev_get_fw_dev_path(dst); > > > > + > > > > + if (!strcmp(devpath_src, devpath_dst)) { > > > > + ret = true; > > > > + } > > > > + > > > > + g_free(devpath_src); > > > > + g_free(devpath_dst); > > > > + return ret; > > > > +} > > > > + > > > > +void del_boot_device_path(DeviceState *dev) > > > > +{ > > > > + FWBootEntry *i; > > > > + > > > > + assert(dev != NULL); > > > > + > > > > + /* remove all entries of the assigned device */ > > > > + QTAILQ_FOREACH(i, &fw_boot_order, link) { > > > > + if (i->dev == NULL) { > > > > + continue; > > > > + } > > > > + if ((i->dev == dev || is_same_fw_dev_path(i->dev, dev))) { > > > > > > Why this is needed? Is there any case where i-->dev != dev but > > > is_same_fw_dev_path() returns true? > > > > > Yes, it is needed. At present, the virito-net-pci device > > compliance with this situation. > > > > Please see the qom path about virtio-net-pci and virtio-net device: > > > > id: null, /machine/peripheral/nic1/virtio-backend > > id: nic1, /machine/peripheral/nic1 > > And why exactly is the caller passing a different pointer to > del_boot_device_path()? Why not simply change the caller to pass the > same pointer to add_boot_device_path() and del_boot_device_path()? > 1. When we want to create a virtio-net device, we must use '-device virtio-net-pci'. This device is a pci device, and virtio-net-device is its child: object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL); 2. Both virtio-net-pci and virtio-net-device own bootindex property because they include "DEFINE_NIC_PROPERTIES(VirtIONetPCI, vdev.nic_conf)". 3. Only virtio-net-device will call add_boot_device_path() in virtio_net_device_realize(), which use its own pointer, but not virtio-net-pci's pointer: add_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0"); 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's pointer to del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add a function named is_same_fw_dev_path() to handle this situation. Best regards, -Gonglei
> Subject: RE: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > Hi, > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Wednesday, September 03, 2014 2:00 AM > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > Importance: High > > > > On Mon, Sep 01, 2014 at 06:47:13AM +0000, Gonglei (Arei) wrote: > > > > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > > > > Sent: Monday, September 01, 2014 2:43 PM > > > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path > > function > > > > Importance: High > > > > > > > > Hi, > > > > > > > > > +static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) > > > > > +{ > > > > > + bool ret = false; > > > > > + char *devpath_src = qdev_get_fw_dev_path(src); > > > > > + char *devpath_dst = qdev_get_fw_dev_path(dst); > > > > > + > > > > > + if (!strcmp(devpath_src, devpath_dst)) { > > > > > + ret = true; > > > > > + } > > > > > + > > > > > + g_free(devpath_src); > > > > > + g_free(devpath_dst); > > > > > + return ret; > > > > > +} > > > > > + > > > > > +void del_boot_device_path(DeviceState *dev) > > > > > +{ > > > > > + FWBootEntry *i; > > > > > + > > > > > + assert(dev != NULL); > > > > > + > > > > > + /* remove all entries of the assigned device */ > > > > > + QTAILQ_FOREACH(i, &fw_boot_order, link) { > > > > > + if (i->dev == NULL) { > > > > > + continue; > > > > > + } > > > > > + if ((i->dev == dev || is_same_fw_dev_path(i->dev, dev))) { > > > > > > > > Why this is needed? Is there any case where i-->dev != dev but > > > > is_same_fw_dev_path() returns true? > > > > > > > Yes, it is needed. At present, the virito-net-pci device > > > compliance with this situation. > > > > > > Please see the qom path about virtio-net-pci and virtio-net device: > > > > > > id: null, /machine/peripheral/nic1/virtio-backend > > > id: nic1, /machine/peripheral/nic1 > > > > And why exactly is the caller passing a different pointer to > > del_boot_device_path()? Why not simply change the caller to pass the > > same pointer to add_boot_device_path() and del_boot_device_path()? > > > 1. When we want to create a virtio-net device, we must use '-device > virtio-net-pci'. > This device is a pci device, and virtio-net-device is its child: > object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), > NULL); > 2. Both virtio-net-pci and virtio-net-device own bootindex property because > they > include "DEFINE_NIC_PROPERTIES(VirtIONetPCI, vdev.nic_conf)". > 3. Only virtio-net-device will call add_boot_device_path() in > virtio_net_device_realize(), > which use its own pointer, but not virtio-net-pci's pointer: > add_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0"); > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's pointer Sorry, my typo, s/hotplug/hot-unplug/ > to del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add a > function named is_same_fw_dev_path() to handle this situation. > > > Best regards, > -Gonglei
Hi, > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's pointer to > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add a function > named is_same_fw_dev_path() to handle this situation. When hot-unplugging virtio-net-pci I'd expect we free both virtio-net-pci and virtio-net-device (and therefore call del_boot_device_path twice, once for each device). Can you check that? thanks, Gerd
Best regards, -Gonglei > -----Original Message----- > From: Gerd Hoffmann [mailto:kraxel@redhat.com] > Sent: Wednesday, September 03, 2014 2:25 PM > To: Gonglei (Arei) > Cc: Eduardo Habkost; qemu-devel@nongnu.org; aliguori@amazon.com; > mst@redhat.com; pbonzini@redhat.com; akong@redhat.com; > hutao@cn.fujitsu.com; eblake@redhat.com; afaerber@suse.de; > armbru@redhat.com; imammedo@redhat.com; aik@ozlabs.ru; > peter.crosthwaite@xilinx.com; lcapitulino@redhat.com; hani@linux.com; > stefanha@redhat.com; agraf@suse.de; chenliang (T); Huangweidong (C); > Luonengjun; Huangpeng (Peter); kwolf@redhat.com > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > Importance: High > > > Hi, > > > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's pointer > to > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add a > function > > named is_same_fw_dev_path() to handle this situation. > > When hot-unplugging virtio-net-pci I'd expect we free both > virtio-net-pci and virtio-net-device (and therefore call > del_boot_device_path twice, once for each device). Can you check that? > Yes, I can. The del_boot_device_path() is called only once, just for virtio-net-pci. For its child, virtio-net-devcie's resource is cleaned by qbus->child unrealizing process, will not call device_finalize(). Command line: # ./qemu-system-x86_64 -enable-kvm -m 4096 -smp 4 -name redhat6.2 -drive file=/mnt/sdb/gonglei/image/win7_32_2U,if=none,id=drive-ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 -drive file=/mnt/sdb/gonglei/iso/rhel-server-7.0-x86_64-dvd.iso,if=none,id=drive-ide0-0-1 -device ide-cd,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1,bootindex=4 -vnc 0.0.0.0:10 -netdev type=user,id=net0 -device virtio-net-pci,netdev=net0,bootindex=3,id=nic1 QEMU 2.1.50 monitor - type 'help' for more information (qemu) device_del nic1 (qemu) The below is the backtrace by gdb: Breakpoint 1, virtio_net_device_unrealize (dev=0x7f1b1ea87d58, errp=0x7f1b17d8c350) at /mnt/sdb/gonglei/qemu.git/qemu/hw/net/virtio-net.c:1649 1649 { (gdb) bt #0 virtio_net_device_unrealize (dev=0x7f1b1ea87d58, errp=0x7f1b17d8c350) at /mnt/sdb/gonglei/qemu.git/qemu/hw/net/virtio-net.c:1649 #1 0x00007f1b1dd69b71 in virtio_device_unrealize (dev=0x7f1b1ea87d58, errp=0x7f1b17d8c3c8) at /mnt/sdb/gonglei/qemu.git/qemu/hw/virtio/virtio.c:1312 #2 0x00007f1b1de8a1a4 in device_set_realized (obj=0x7f1b1ea87d58, value=false, errp=0x7f1b17d8c568) at hw/core/qdev.c:885 #3 0x00007f1b1dfa01f7 in property_set_bool (obj=0x7f1b1ea87d58, v=0x7f1b1eb29de0, opaque=0x7f1b1eaa72c0, name= 0x7f1b1e0c2b69 "realized", errp=0x7f1b17d8c568) at qom/object.c:1467 #4 0x00007f1b1df9e82f in object_property_set (obj=0x7f1b1ea87d58, v=0x7f1b1eb29de0, name=0x7f1b1e0c2b69 "realized", errp= 0x7f1b17d8c568) at qom/object.c:814 #5 0x00007f1b1dfa0c45 in object_property_set_qobject (obj=0x7f1b1ea87d58, value=0x7f1b1eb09c80, name=0x7f1b1e0c2b69 "realized", errp=0x7f1b17d8c568) at qom/qom-qobject.c:24 #6 0x00007f1b1df9ebd7 in object_property_set_bool (obj=0x7f1b1ea87d58, value=false, name=0x7f1b1e0c2b69 "realized", errp= 0x7f1b17d8c568) at qom/object.c:878 #7 0x00007f1b1de893fa in bus_set_realized (obj=0x7f1b1ea87ce0, value=false, errp=0x7f1b17d8c718) at hw/core/qdev.c:583 #8 0x00007f1b1dfa01f7 in property_set_bool (obj=0x7f1b1ea87ce0, v=0x7f1b1ead5a10, opaque=0x7f1b1eaa1640, name= 0x7f1b1e0c2b69 "realized", errp=0x7f1b17d8c718) at qom/object.c:1467 #9 0x00007f1b1df9e82f in object_property_set (obj=0x7f1b1ea87ce0, v=0x7f1b1ead5a10, name=0x7f1b1e0c2b69 "realized", errp= 0x7f1b17d8c718) at qom/object.c:814 #10 0x00007f1b1dfa0c45 in object_property_set_qobject (obj=0x7f1b1ea87ce0, value=0x7f1b1eb09c60, name=0x7f1b1e0c2b69 "realized", errp=0x7f1b17d8c718) at qom/qom-qobject.c:24 #11 0x00007f1b1df9ebd7 in object_property_set_bool (obj=0x7f1b1ea87ce0, value=false, name=0x7f1b1e0c2b69 "realized", errp= 0x7f1b17d8c718) at qom/object.c:878 #12 0x00007f1b1de8a127 in device_set_realized (obj=0x7f1b1ea87380, value=false, errp=0x0) at hw/core/qdev.c:875 #13 0x00007f1b1dfa01f7 in property_set_bool (obj=0x7f1b1ea87380, v=0x7f1b1eacf940, opaque=0x7f1b1ea89f20, name= 0x7f1b1e0c2b69 "realized", errp=0x0) at qom/object.c:1467 #14 0x00007f1b1df9e82f in object_property_set (obj=0x7f1b1ea87380, v=0x7f1b1eacf940, name=0x7f1b1e0c2b69 "realized", errp=0x0) at qom/object.c:814 #15 0x00007f1b1dfa0c45 in object_property_set_qobject (obj=0x7f1b1ea87380, value=0x7f1b1eb085e0, name=0x7f1b1e0c2b69 "realized", errp=0x0) at qom/qom-qobject.c:24 #16 0x00007f1b1df9ebd7 in object_property_set_bool (obj=0x7f1b1ea87380, value=false, name=0x7f1b1e0c2b69 "realized", errp=0x0) at qom/object.c:878 #17 0x00007f1b1de8a7d0 in device_unparent (obj=0x7f1b1ea87380) at hw/core/qdev.c:1010 #18 0x00007f1b1df9f26b in object_finalize_child_property (obj=0x7f1b1e9f4f30, name=0x7f1b1eaa7ba0 "nic1", opaque=0x7f1b1ea87380) at qom/object.c:1036 #19 0x00007f1b1df9e671 in object_property_del (obj=0x7f1b1e9f4f30, name=0x7f1b1eaa7ba0 "nic1", errp=0x0) at qom/object.c:778 #20 0x00007f1b1df9d692 in object_property_del_child (obj=0x7f1b1e9f4f30, child=0x7f1b1ea87380, errp=0x0) at qom/object.c:382 #21 0x00007f1b1df9d74b in object_unparent (obj=0x7f1b1ea87380) at qom/object.c:391 #22 0x00007f1b1de52317 in acpi_pcihp_eject_slot (s=0x7f1b1eaf3438, bsel=0, slots=8) at hw/acpi/pcihp.c:139 #23 0x00007f1b1de529b5 in pci_write (opaque=0x7f1b1eaf3438, addr=8, data=8, size=4) at hw/acpi/pcihp.c:277 #24 0x00007f1b1dd1e958 in memory_region_write_accessor (mr=0x7f1b1eaf4048, addr=8, value=0x7f1b17d8cb38, size=4, shift=0, mask= 4294967295) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:443 #25 0x00007f1b1dd1ea94 in access_with_adjusted_size (addr=8, value=0x7f1b17d8cb38, size=4, access_size_min=1, access_size_max=4, access=0x7f1b1dd1e8cb <memory_region_write_accessor>, mr=0x7f1b1eaf4048) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:480 #26 0x00007f1b1dd22061 in memory_region_dispatch_write (mr=0x7f1b1eaf4048, addr=8, data=8, size=4) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1137 #27 0x00007f1b1dd25c10 in io_mem_write (mr=0x7f1b1eaf4048, addr=8, val=8, size=4) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1973 #28 0x00007f1b1dcd01a9 in address_space_rw (as=0x7f1b1e4ed180 <address_space_io>, addr=44552, buf=0x7f1b1dc1f000 "\b", len=4, is_write=true) at /mnt/sdb/gonglei/qemu.git/qemu/exec.c:2042 #29 0x00007f1b1dd1b186 in kvm_handle_io (port=44552, data=0x7f1b1dc1f000, direction=1, size=4, count=1) at /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1597 #30 0x00007f1b1dd1b7c4 in kvm_cpu_exec (cpu=0x7f1b1e9f85b0) at /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1748 #31 0x00007f1b1dd02c88 in qemu_kvm_cpu_thread_fn (arg=0x7f1b1e9f85b0) at /mnt/sdb/gonglei/qemu.git/qemu/cpus.c:940 #32 0x00007f1b1af3b7f6 in start_thread () from /lib64/libpthread.so.0 #33 0x00007f1b1ac9709d in clone () from /lib64/libc.so.6 #34 0x0000000000000000 in ?? () (gdb) c Continuing. Breakpoint 2, device_finalize (obj=0x7f1b1ea87380) at hw/core/qdev.c:971 971 { (gdb) bt #0 device_finalize (obj=0x7f1b1ea87380) at hw/core/qdev.c:971 #1 0x00007f1b1df9d79b in object_deinit (obj=0x7f1b1ea87380, type=0x7f1b1e99a970) at qom/object.c:398 #2 0x00007f1b1df9d7bd in object_deinit (obj=0x7f1b1ea87380, type=0x7f1b1e990a40) at qom/object.c:402 #3 0x00007f1b1df9d7bd in object_deinit (obj=0x7f1b1ea87380, type=0x7f1b1e987700) at qom/object.c:402 #4 0x00007f1b1df9d7bd in object_deinit (obj=0x7f1b1ea87380, type=0x7f1b1e987fc0) at qom/object.c:402 #5 0x00007f1b1df9d81a in object_finalize (data=0x7f1b1ea87380) at qom/object.c:412 #6 0x00007f1b1df9e3d9 in object_unref (obj=0x7f1b1ea87380) at qom/object.c:719 #7 0x00007f1b1df9f280 in object_finalize_child_property (obj=0x7f1b1e9f4f30, name=0x7f1b1eaa7ba0 "nic1", opaque=0x7f1b1ea87380) at qom/object.c:1039 #8 0x00007f1b1df9e671 in object_property_del (obj=0x7f1b1e9f4f30, name=0x7f1b1eaa7ba0 "nic1", errp=0x0) at qom/object.c:778 #9 0x00007f1b1df9d692 in object_property_del_child (obj=0x7f1b1e9f4f30, child=0x7f1b1ea87380, errp=0x0) at qom/object.c:382 #10 0x00007f1b1df9d74b in object_unparent (obj=0x7f1b1ea87380) at qom/object.c:391 #11 0x00007f1b1de52317 in acpi_pcihp_eject_slot (s=0x7f1b1eaf3438, bsel=0, slots=8) at hw/acpi/pcihp.c:139 #12 0x00007f1b1de529b5 in pci_write (opaque=0x7f1b1eaf3438, addr=8, data=8, size=4) at hw/acpi/pcihp.c:277 #13 0x00007f1b1dd1e958 in memory_region_write_accessor (mr=0x7f1b1eaf4048, addr=8, value=0x7f1b17d8cb38, size=4, shift=0, mask= 4294967295) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:443 #14 0x00007f1b1dd1ea94 in access_with_adjusted_size (addr=8, value=0x7f1b17d8cb38, size=4, access_size_min=1, access_size_max=4, access=0x7f1b1dd1e8cb <memory_region_write_accessor>, mr=0x7f1b1eaf4048) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:480 #15 0x00007f1b1dd22061 in memory_region_dispatch_write (mr=0x7f1b1eaf4048, addr=8, data=8, size=4) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1137 #16 0x00007f1b1dd25c10 in io_mem_write (mr=0x7f1b1eaf4048, addr=8, val=8, size=4) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1973 #17 0x00007f1b1dcd01a9 in address_space_rw (as=0x7f1b1e4ed180 <address_space_io>, addr=44552, buf=0x7f1b1dc1f000 "\b", len=4, is_write=true) at /mnt/sdb/gonglei/qemu.git/qemu/exec.c:2042 #18 0x00007f1b1dd1b186 in kvm_handle_io (port=44552, data=0x7f1b1dc1f000, direction=1, size=4, count=1) at /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1597 #19 0x00007f1b1dd1b7c4 in kvm_cpu_exec (cpu=0x7f1b1e9f85b0) at /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1748 #20 0x00007f1b1dd02c88 in qemu_kvm_cpu_thread_fn (arg=0x7f1b1e9f85b0) at /mnt/sdb/gonglei/qemu.git/qemu/cpus.c:940 #21 0x00007f1b1af3b7f6 in start_thread () from /lib64/libpthread.so.0 #22 0x00007f1b1ac9709d in clone () from /lib64/libc.so.6 #23 0x0000000000000000 in ?? () (gdb) n 974 DeviceState *dev = DEVICE(obj); (gdb) 977 del_boot_device_path(dev); (gdb) p *dev $1 = {parent_obj = {class = 0x7f1b1e9b7270, free = 0x7f1b1caf1a30 <g_free>, properties = {tqh_first = 0x0, tqh_last = 0x7f1b1ea87390}, ref = 0, parent = 0x0}, id = 0x7f1b1e9cbb30 "nic1", realized = false, pending_deleted_event = true, opts = 0x7f1b1e9cb2f0, hotplugged = 0, parent_bus = 0x0, gpios = {lh_first = 0x0}, child_bus = {lh_first = 0x0}, num_child_bus = 0, instance_id_alias = -1, alias_required_for_version = 0} (gdb) n [Thread 0x7f1b18770700 (LWP 13982) exited] 979 if (dev->opts) { (gdb) info b Num Type Disp Enb Address What 1 breakpoint keep y 0x00007f1b1dd5def2 in virtio_net_device_unrealize at /mnt/sdb/gonglei/qemu.git/qemu/hw/net/virtio-net.c:1649 breakpoint already hit 1 time 2 breakpoint keep y 0x00007f1b1de8a608 in device_finalize at hw/core/qdev.c:971 breakpoint already hit 1 time (gdb) c Continuing. [New Thread 0x7f1b18770700 (LWP 15096)] [Thread 0x7f1b18770700 (LWP 15096) exited] [New Thread 0x7f1b18770700 (LWP 16434)] [Thread 0x7f1b18770700 (LWP 16434) exited]
On Wed, Sep 03, 2014 at 06:45:56AM +0000, Gonglei (Arei) wrote: [...] > > > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's pointer > > to > > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add a > > function > > > named is_same_fw_dev_path() to handle this situation. > > > > When hot-unplugging virtio-net-pci I'd expect we free both > > virtio-net-pci and virtio-net-device (and therefore call > > del_boot_device_path twice, once for each device). Can you check that? > > > Yes, I can. > > The del_boot_device_path() is called only once, just for virtio-net-pci. > For its child, virtio-net-devcie's resource is cleaned by qbus->child unrealizing > process, will not call device_finalize(). Then we need to fix this to make sure there's a corresponding del_boot_device_path() call (with the same pointer) to every add_boot_device_path() call, instead of adding a hack to del_boot_device_path().
Hi, > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Thursday, September 04, 2014 2:13 AM > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > On Wed, Sep 03, 2014 at 06:45:56AM +0000, Gonglei (Arei) wrote: > [...] > > > > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's > pointer > > > to > > > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add a > > > function > > > > named is_same_fw_dev_path() to handle this situation. > > > > > > When hot-unplugging virtio-net-pci I'd expect we free both > > > virtio-net-pci and virtio-net-device (and therefore call > > > del_boot_device_path twice, once for each device). Can you check that? > > > > > Yes, I can. > > > > The del_boot_device_path() is called only once, just for virtio-net-pci. > > For its child, virtio-net-devcie's resource is cleaned by qbus->child unrealizing > > process, will not call device_finalize(). > > Then we need to fix this to make sure there's a corresponding > del_boot_device_path() call (with the same pointer) to every > add_boot_device_path() call, instead of adding a hack to > del_boot_device_path(). > Good idea. We can add functions named $device_finalize_bootindex(), such as: static void virtio_net_set_bootindex(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) { VirtIONet *n = VIRTIO_NET(obj); set_bootindex(&n->nic_conf, v, name, errp); add_boot_device_path(n->nic_conf.bootindex, DEVICE(obj), "/ethernet-phy@0"); } static void virtio_net_finalize_bootindex(Object *obj, const char *name, void *opaque) { del_boot_device_path(DEVICE(obj)); } ... object_property_add(obj, "bootindex", "int", virtio_net_get_bootindex, virtio_net_set_bootindex, virtio_net_finalize_bootindex, dev, NULL); as the previous email, we lay add_boot_device_path() in $device_set_bootindex, and lay del_boot_device_path() in $device_finalize_bootindex is a good idea IMO. Do you like it? Thanks. Best regards, -Gonglei
Hi, > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > > > On Wed, Sep 03, 2014 at 06:45:56AM +0000, Gonglei (Arei) wrote: > > [...] > > > > > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's > > pointer > > > > to > > > > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add a > > > > function > > > > > named is_same_fw_dev_path() to handle this situation. > > > > > > > > When hot-unplugging virtio-net-pci I'd expect we free both > > > > virtio-net-pci and virtio-net-device (and therefore call > > > > del_boot_device_path twice, once for each device). Can you check that? > > > > > > > Yes, I can. > > > > > > The del_boot_device_path() is called only once, just for virtio-net-pci. > > > For its child, virtio-net-devcie's resource is cleaned by qbus->child > unrealizing > > > process, will not call device_finalize(). > > > > Then we need to fix this to make sure there's a corresponding > > del_boot_device_path() call (with the same pointer) to every > > add_boot_device_path() call, instead of adding a hack to > > del_boot_device_path(). > > > Good idea. We can add functions named $device_finalize_bootindex(), such as: > > static void virtio_net_set_bootindex(Object *obj, Visitor *v, void *opaque, > const char *name, Error **errp) > { > VirtIONet *n = VIRTIO_NET(obj); > > set_bootindex(&n->nic_conf, v, name, errp); > add_boot_device_path(n->nic_conf.bootindex, > DEVICE(obj), "/ethernet-phy@0"); > } > > static void virtio_net_finalize_bootindex(Object *obj, const char *name, > void *opaque) > { > del_boot_device_path(DEVICE(obj)); > } > ... Oops, I found an issue that when we hot-unplug a virtio-net-pci device, the virtio_net_finalize_bootindex function will not be called too. Maybe this is a potential *bug* which will cause the virtio-net-device's property resource leak. Paolo, would you have a look at it? Thanks a lot! Backtrace as below: #0 object_unref (obj=0x7f207c640468) at qom/object.c:711 #1 0x00007f207b9850da in bus_remove_child (bus=0x7f207c6403f0, child=0x7f207c640468) at hw/core/qdev.c:76 #2 0x00007f207b987cbc in device_unparent (obj=0x7f207c640468) at hw/core/qdev.c:1013 #3 0x00007f207ba9c5a3 in object_finalize_child_property (obj=0x7f207c63fa90, name=0x7f207c6027c0 "virtio-backend", opaque= 0x7f207c640468) at qom/object.c:1036 #4 0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c63fa90, name=0x7f207c6027c0 "virtio-backend", errp=0x0) at qom/object.c:778 #5 0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c63fa90, child=0x7f207c640468, errp=0x0) at qom/object.c:382 #6 0x00007f207ba9aa83 in object_unparent (obj=0x7f207c640468) at qom/object.c:391 #7 0x00007f207b986666 in bus_unparent (obj=0x7f207c6403f0) at hw/core/qdev.c:548 #8 0x00007f207ba9c5a3 in object_finalize_child_property (obj=0x7f207c63fa90, name=0x7f207c5db7a0 "virtio-bus", opaque= 0x7f207c6403f0) at qom/object.c:1036 #9 0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c63fa90, name=0x7f207c5db7a0 "virtio-bus", errp=0x0) at qom/object.c:778 #10 0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c63fa90, child=0x7f207c6403f0, errp=0x0) at qom/object.c:382 #11 0x00007f207ba9aa83 in object_unparent (obj=0x7f207c6403f0) at qom/object.c:391 #12 0x00007f207b987c8f in device_unparent (obj=0x7f207c63fa90) at hw/core/qdev.c:1010 #13 0x00007f207ba9c5a3 in object_finalize_child_property (obj=0x7f207c4f2f90, name=0x7f207c604580 "nic1", opaque=0x7f207c63fa90) at qom/object.c:1036 #14 0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c4f2f90, name=0x7f207c604580 "nic1", errp=0x0) at qom/object.c:778 #15 0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c4f2f90, child=0x7f207c63fa90, errp=0x0) at qom/object.c:382 #16 0x00007f207ba9aa83 in object_unparent (obj=0x7f207c63fa90) at qom/object.c:391 #17 0x00007f207b94f7c7 in acpi_pcihp_eject_slot (s=0x7f207c5801e8, bsel=0, slots=8) at hw/acpi/pcihp.c:139 #18 0x00007f207b94fe65 in pci_write (opaque=0x7f207c5801e8, addr=8, data=8, size=4) at hw/acpi/pcihp.c:277 #19 0x00007f207b818a38 in memory_region_write_accessor (mr=0x7f207c580df8, addr=8, value=0x7f2075886b38, size=4, shift=0, mask= 4294967295) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:443 #20 0x00007f207b818b74 in access_with_adjusted_size (addr=8, value=0x7f2075886b38, size=4, access_size_min=1, access_size_max=4, access=0x7f207b8189ab <memory_region_write_accessor>, mr=0x7f207c580df8) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:480 #21 0x00007f207b81c141 in memory_region_dispatch_write (mr=0x7f207c580df8, addr=8, data=8, size=4) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1137 #22 0x00007f207b81fcf0 in io_mem_write (mr=0x7f207c580df8, addr=8, val=8, size=4) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1973 #23 0x00007f207b7ca289 in address_space_rw (as=0x7f207bfeb1e0 <address_space_io>, addr=44552, buf=0x7f207b719000 "\b", len=4, is_write=true) at /mnt/sdb/gonglei/qemu.git/qemu/exec.c:2042 #24 0x00007f207b815266 in kvm_handle_io (port=44552, data=0x7f207b719000, direction=1, size=4, count=1) at /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1597 #25 0x00007f207b8158a4 in kvm_cpu_exec (cpu=0x7f207c4f6610) at /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1748 #26 0x00007f207b7fcd68 in qemu_kvm_cpu_thread_fn (arg=0x7f207c4f6610) at /mnt/sdb/gonglei/qemu.git/qemu/cpus.c:940 #27 0x00007f2078a357f6 in start_thread () from /lib64/libpthread.so.0 #28 0x00007f207879109d in clone () from /lib64/libc.so.6 #29 0x0000000000000000 in ?? () (gdb) p *obj $64 = {class = 0x7f207c4bc5f0, free = 0x0, properties = {tqh_first = 0x7f207c591fd0, tqh_last = 0x7f207c6027a8}, ref = 3, parent = 0x7f207c63fa90} (gdb) n 712 if (!obj) { (gdb) 715 g_assert(obj->ref > 0); (gdb) 718 if (atomic_fetch_dec(&obj->ref) == 1) { (gdb) 721 } (gdb) p *obj $65 = {class = 0x7f207c4bc5f0, free = 0x0, properties = {tqh_first = 0x7f207c591fd0, tqh_last = 0x7f207c6027a8}, ref = 2, parent = 0x7f207c63fa90} Because of the virtio-net-device object's ref is 3, will not enter object_finailze(), *leak resource*. Am I wrong? Best regards, -Gonglei > object_property_add(obj, "bootindex", "int", > virtio_net_get_bootindex, > virtio_net_set_bootindex, > virtio_net_finalize_bootindex, dev, NULL); > > as the previous email, we lay add_boot_device_path() in > $device_set_bootindex, > and lay del_boot_device_path() in $device_finalize_bootindex is a good idea > IMO. > > Do you like it? Thanks. > > Best regards, > -Gonglei
> Subject: RE: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > Hi, > > > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path > function > > > > > > On Wed, Sep 03, 2014 at 06:45:56AM +0000, Gonglei (Arei) wrote: > > > [...] > > > > > > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's > > > pointer > > > > > to > > > > > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add > a > > > > > function > > > > > > named is_same_fw_dev_path() to handle this situation. > > > > > > > > > > When hot-unplugging virtio-net-pci I'd expect we free both > > > > > virtio-net-pci and virtio-net-device (and therefore call > > > > > del_boot_device_path twice, once for each device). Can you check > that? > > > > > > > > > Yes, I can. > > > > > > > > The del_boot_device_path() is called only once, just for virtio-net-pci. > > > > For its child, virtio-net-devcie's resource is cleaned by qbus->child > > unrealizing > > > > process, will not call device_finalize(). > > > > > > Then we need to fix this to make sure there's a corresponding > > > del_boot_device_path() call (with the same pointer) to every > > > add_boot_device_path() call, instead of adding a hack to > > > del_boot_device_path(). > > > > > Good idea. We can add functions named $device_finalize_bootindex(), such > as: > > > > static void virtio_net_set_bootindex(Object *obj, Visitor *v, void *opaque, > > const char *name, Error > **errp) > > { > > VirtIONet *n = VIRTIO_NET(obj); > > > > set_bootindex(&n->nic_conf, v, name, errp); > > add_boot_device_path(n->nic_conf.bootindex, > > DEVICE(obj), "/ethernet-phy@0"); > > } > > > > static void virtio_net_finalize_bootindex(Object *obj, const char *name, > > void *opaque) > > { > > del_boot_device_path(DEVICE(obj)); > > } > > ... > > Oops, I found an issue that when we hot-unplug a virtio-net-pci device, > the virtio_net_finalize_bootindex function will not be called too. > Maybe this is a potential *bug* which will cause the virtio-net-device's property > resource leak. > > Paolo, would you have a look at it? Thanks a lot! > > Backtrace as below: > > #0 object_unref (obj=0x7f207c640468) at qom/object.c:711 > #1 0x00007f207b9850da in bus_remove_child (bus=0x7f207c6403f0, > child=0x7f207c640468) at hw/core/qdev.c:76 > #2 0x00007f207b987cbc in device_unparent (obj=0x7f207c640468) at > hw/core/qdev.c:1013 > #3 0x00007f207ba9c5a3 in object_finalize_child_property > (obj=0x7f207c63fa90, name=0x7f207c6027c0 "virtio-backend", opaque= > 0x7f207c640468) at qom/object.c:1036 > #4 0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c63fa90, > name=0x7f207c6027c0 "virtio-backend", errp=0x0) > at qom/object.c:778 > #5 0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c63fa90, > child=0x7f207c640468, errp=0x0) at qom/object.c:382 > #6 0x00007f207ba9aa83 in object_unparent (obj=0x7f207c640468) at > qom/object.c:391 > #7 0x00007f207b986666 in bus_unparent (obj=0x7f207c6403f0) at > hw/core/qdev.c:548 > #8 0x00007f207ba9c5a3 in object_finalize_child_property > (obj=0x7f207c63fa90, name=0x7f207c5db7a0 "virtio-bus", opaque= > 0x7f207c6403f0) at qom/object.c:1036 > #9 0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c63fa90, > name=0x7f207c5db7a0 "virtio-bus", errp=0x0) at qom/object.c:778 > #10 0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c63fa90, > child=0x7f207c6403f0, errp=0x0) at qom/object.c:382 > #11 0x00007f207ba9aa83 in object_unparent (obj=0x7f207c6403f0) at > qom/object.c:391 > #12 0x00007f207b987c8f in device_unparent (obj=0x7f207c63fa90) at > hw/core/qdev.c:1010 > #13 0x00007f207ba9c5a3 in object_finalize_child_property > (obj=0x7f207c4f2f90, name=0x7f207c604580 "nic1", opaque=0x7f207c63fa90) > at qom/object.c:1036 > #14 0x00007f207ba9b9a9 in object_property_del (obj=0x7f207c4f2f90, > name=0x7f207c604580 "nic1", errp=0x0) at qom/object.c:778 > #15 0x00007f207ba9a9ca in object_property_del_child (obj=0x7f207c4f2f90, > child=0x7f207c63fa90, errp=0x0) at qom/object.c:382 > #16 0x00007f207ba9aa83 in object_unparent (obj=0x7f207c63fa90) at > qom/object.c:391 > #17 0x00007f207b94f7c7 in acpi_pcihp_eject_slot (s=0x7f207c5801e8, bsel=0, > slots=8) at hw/acpi/pcihp.c:139 > #18 0x00007f207b94fe65 in pci_write (opaque=0x7f207c5801e8, addr=8, > data=8, size=4) at hw/acpi/pcihp.c:277 > #19 0x00007f207b818a38 in memory_region_write_accessor > (mr=0x7f207c580df8, addr=8, value=0x7f2075886b38, size=4, shift=0, mask= > 4294967295) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:443 > #20 0x00007f207b818b74 in access_with_adjusted_size (addr=8, > value=0x7f2075886b38, size=4, access_size_min=1, access_size_max=4, > access=0x7f207b8189ab <memory_region_write_accessor>, > mr=0x7f207c580df8) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:480 > #21 0x00007f207b81c141 in memory_region_dispatch_write > (mr=0x7f207c580df8, addr=8, data=8, size=4) > at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1137 > #22 0x00007f207b81fcf0 in io_mem_write (mr=0x7f207c580df8, addr=8, val=8, > size=4) at /mnt/sdb/gonglei/qemu.git/qemu/memory.c:1973 > #23 0x00007f207b7ca289 in address_space_rw (as=0x7f207bfeb1e0 > <address_space_io>, addr=44552, buf=0x7f207b719000 "\b", len=4, > is_write=true) at /mnt/sdb/gonglei/qemu.git/qemu/exec.c:2042 > #24 0x00007f207b815266 in kvm_handle_io (port=44552, > data=0x7f207b719000, direction=1, size=4, count=1) > at /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1597 > #25 0x00007f207b8158a4 in kvm_cpu_exec (cpu=0x7f207c4f6610) at > /mnt/sdb/gonglei/qemu.git/qemu/kvm-all.c:1748 > #26 0x00007f207b7fcd68 in qemu_kvm_cpu_thread_fn (arg=0x7f207c4f6610) > at /mnt/sdb/gonglei/qemu.git/qemu/cpus.c:940 > #27 0x00007f2078a357f6 in start_thread () from /lib64/libpthread.so.0 > #28 0x00007f207879109d in clone () from /lib64/libc.so.6 > #29 0x0000000000000000 in ?? () > (gdb) p *obj > $64 = {class = 0x7f207c4bc5f0, free = 0x0, properties = {tqh_first = > 0x7f207c591fd0, tqh_last = 0x7f207c6027a8}, ref = 3, parent = > 0x7f207c63fa90} > (gdb) n > 712 if (!obj) { > (gdb) > 715 g_assert(obj->ref > 0); > (gdb) > 718 if (atomic_fetch_dec(&obj->ref) == 1) { > (gdb) > 721 } > (gdb) p *obj > $65 = {class = 0x7f207c4bc5f0, free = 0x0, properties = {tqh_first = > 0x7f207c591fd0, tqh_last = 0x7f207c6027a8}, ref = 2, parent = > 0x7f207c63fa90} > > Because of the virtio-net-device object's ref is 3, will not enter object_finailze(), > *leak resource*. Am I wrong? > I've confirmed that this is a bug, and have posted a patch fix it. Please review, thanks! [PATCH] virtio-pci: fix virtio-net child refcount in transports Best regards, -Gonglei
Hi, > > I've confirmed that this is a bug, and have posted a patch fix it. > > Please review, thanks! > > [PATCH] virtio-pci: fix virtio-net child refcount in transports > > Best regards, > -Gonglei So, about Gerd's previous question: > When hot-unplugging virtio-net-pci I'd expect we free both > virtio-net-pci and virtio-net-device (and therefore call > del_boot_device_path twice, once for each device). Can you check that? > After fix that bug, you are right. The del_boot_device_path will be called twice. I can rework del_boot_device_path(), remove is_same_fw_dev_path(). The new del_boot_device_path function as below: void del_boot_device_path(DeviceState *dev) { FWBootEntry *i; assert(dev != NULL); /* remove all entries of the assigned device */ QTAILQ_FOREACH(i, &fw_boot_order, link) { if (i->dev == NULL) { continue; } if (i->dev == dev) { QTAILQ_REMOVE(&fw_boot_order, i, link); g_free(i->suffix); g_free(i); break; } } } Best regards, -Gonglei
On Thu, Sep 04, 2014 at 03:01:41AM +0000, Gonglei (Arei) wrote: > Hi, > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Thursday, September 04, 2014 2:13 AM > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > > > On Wed, Sep 03, 2014 at 06:45:56AM +0000, Gonglei (Arei) wrote: > > [...] > > > > > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's > > pointer > > > > to > > > > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add a > > > > function > > > > > named is_same_fw_dev_path() to handle this situation. > > > > > > > > When hot-unplugging virtio-net-pci I'd expect we free both > > > > virtio-net-pci and virtio-net-device (and therefore call > > > > del_boot_device_path twice, once for each device). Can you check that? > > > > > > > Yes, I can. > > > > > > The del_boot_device_path() is called only once, just for virtio-net-pci. > > > For its child, virtio-net-devcie's resource is cleaned by qbus->child unrealizing > > > process, will not call device_finalize(). > > > > Then we need to fix this to make sure there's a corresponding > > del_boot_device_path() call (with the same pointer) to every > > add_boot_device_path() call, instead of adding a hack to > > del_boot_device_path(). > > > Good idea. We can add functions named $device_finalize_bootindex(), such as: > > static void virtio_net_set_bootindex(Object *obj, Visitor *v, void *opaque, > const char *name, Error **errp) > { > VirtIONet *n = VIRTIO_NET(obj); > > set_bootindex(&n->nic_conf, v, name, errp); > add_boot_device_path(n->nic_conf.bootindex, > DEVICE(obj), "/ethernet-phy@0"); > } > > static void virtio_net_finalize_bootindex(Object *obj, const char *name, > void *opaque) > { > del_boot_device_path(DEVICE(obj)); > } > ... > object_property_add(obj, "bootindex", "int", > virtio_net_get_bootindex, > virtio_net_set_bootindex, > virtio_net_finalize_bootindex, dev, NULL); > > as the previous email, we lay add_boot_device_path() in $device_set_bootindex, > and lay del_boot_device_path() in $device_finalize_bootindex is a good idea IMO. Whatever the approach we use, can we have a wrapper to reduce code duplication? e.g. a: void device_add_bootindex_property(DeviceState *dev, int32_t *field, const char *suffix) function. Then instead of reimplementing set/get/finalize functions, device code could just call something like: device_add_bootindex_property(dev, &n->nic_conf.bootindex, "/ethernet-phy@0");
> Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > On Thu, Sep 04, 2014 at 03:01:41AM +0000, Gonglei (Arei) wrote: > > Hi, > > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Thursday, September 04, 2014 2:13 AM > > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path > function > > > > > > On Wed, Sep 03, 2014 at 06:45:56AM +0000, Gonglei (Arei) wrote: > > > [...] > > > > > > 4. When we hotplug the virtio-net-pci device, only pass virtio-net-pci's > > > pointer > > > > > to > > > > > > del_boot_device_path(). But virtio-net-pci != virtio-net-device, so I add > a > > > > > function > > > > > > named is_same_fw_dev_path() to handle this situation. > > > > > > > > > > When hot-unplugging virtio-net-pci I'd expect we free both > > > > > virtio-net-pci and virtio-net-device (and therefore call > > > > > del_boot_device_path twice, once for each device). Can you check > that? > > > > > > > > > Yes, I can. > > > > > > > > The del_boot_device_path() is called only once, just for virtio-net-pci. > > > > For its child, virtio-net-devcie's resource is cleaned by qbus->child > unrealizing > > > > process, will not call device_finalize(). > > > > > > Then we need to fix this to make sure there's a corresponding > > > del_boot_device_path() call (with the same pointer) to every > > > add_boot_device_path() call, instead of adding a hack to > > > del_boot_device_path(). > > > > > Good idea. We can add functions named $device_finalize_bootindex(), such > as: > > > > static void virtio_net_set_bootindex(Object *obj, Visitor *v, void *opaque, > > const char *name, Error > **errp) > > { > > VirtIONet *n = VIRTIO_NET(obj); > > > > set_bootindex(&n->nic_conf, v, name, errp); > > add_boot_device_path(n->nic_conf.bootindex, > > DEVICE(obj), "/ethernet-phy@0"); > > } > > > > static void virtio_net_finalize_bootindex(Object *obj, const char *name, > > void *opaque) > > { > > del_boot_device_path(DEVICE(obj)); > > } > > ... > > object_property_add(obj, "bootindex", "int", > > virtio_net_get_bootindex, > > virtio_net_set_bootindex, > > virtio_net_finalize_bootindex, dev, NULL); > > > > as the previous email, we lay add_boot_device_path() in > $device_set_bootindex, > > and lay del_boot_device_path() in $device_finalize_bootindex is a good idea > IMO. > > Whatever the approach we use, Hmm... we just call this function in device_finalize() as I have used. My above explanation is wrong because of a virtio-net's bug. Thanks. > can we have a wrapper to reduce code > duplication? e.g. a: > void device_add_bootindex_property(DeviceState *dev, int32_t *field, const > char *suffix) > function. > > Then instead of reimplementing set/get/finalize functions, device code > could just call something like: > device_add_bootindex_property(dev, &n->nic_conf.bootindex, > "/ethernet-phy@0"); > This way we cannot attach our target that changing bootindex and take effect during vm rebooting. > -- > Eduardo Best regards, -Gonglei
On Fri, Sep 05, 2014 at 12:44:56AM +0000, Gonglei (Arei) wrote: [...] > > can we have a wrapper to reduce code > > duplication? e.g. a: > > void device_add_bootindex_property(DeviceState *dev, int32_t *field, const > > char *suffix) > > function. > > > > Then instead of reimplementing set/get/finalize functions, device code > > could just call something like: > > device_add_bootindex_property(dev, &n->nic_conf.bootindex, > > "/ethernet-phy@0"); > > > > This way we cannot attach our target that changing bootindex and take effect > during vm rebooting. I don't understand what you mean, here. Whatever you are planning to do on the device-specific setter/getters, if they all look the same, you can write a common getter/setter to do exactly the same steps, and register it inside device_add_bootindex_property(). This way, patches 08/27 to 26/27 can become one-liners, instead of adding more 20 lines each. I mean something like this: typedef struct BootIndexProperty { int32_t *field; const char *suffix; } BootIndexProperty; static void device_get_bootindex(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) { BootIndexProperty *prop = opaque; visit_type_int32(v, prop->field, name, errp); } static void device_set_bootindex(Object *obj, Visitor *v, void *opaque, const char *name, Error **errp) { DeviceState *dev = DEVICE(obj); BootIndexProperty *prop = opaque; int32_t boot_index; Error *local_err = NULL; visit_type_int32(v, &boot_index, name, &local_err); if (local_err) { goto out; } check_boot_index(boot_index, &local_err); if (local_err) { goto out; } *prop->field = boot_index; add_boot_device_path(boot_index, dev, prop->suffix); out: if (local_err) { error_propagate(errp, local_err); } } static void property_release_bootindex(Object *obj, const char *name, void *opaque) { BootIndexProperty *prop = opaque; DeviceState *dev = DEVICE(obj); del_boot_device_path(dev); g_free(prop); } void device_add_bootindex_property(DeviceState *dev, int32_t *field, const char *suffix, Error **errp) { Error *local_err = NULL; BootIndexProperty *prop = g_malloc0(sizeof(*prop)); prop->field = field; prop->suffix = suffix; object_property_add(OBJECT(dev), "bootindex", "int32", device_get_bootindex, device_set_bootindex, property_release_bootindex, prop, &local_err); if (local_err) { error_propagate(errp, local_err); g_free(prop); } }
> From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Friday, September 05, 2014 10:20 AM > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > On Fri, Sep 05, 2014 at 12:44:56AM +0000, Gonglei (Arei) wrote: > [...] > > > can we have a wrapper to reduce code > > > duplication? e.g. a: > > > void device_add_bootindex_property(DeviceState *dev, int32_t *field, > const > > > char *suffix) > > > function. > > > > > > Then instead of reimplementing set/get/finalize functions, device code > > > could just call something like: > > > device_add_bootindex_property(dev, &n->nic_conf.bootindex, > > > "/ethernet-phy@0"); > > > > > > > This way we cannot attach our target that changing bootindex and take > effect > > during vm rebooting. > > I don't understand what you mean, here. Whatever you are planning to do on > the > device-specific setter/getters, if they all look the same, you can write a > common getter/setter to do exactly the same steps, and register it inside > device_add_bootindex_property(). This way, patches 08/27 to 26/27 can > become > one-liners, instead of adding more 20 lines each. > > I mean something like this: > OK. Thanks for explanation. :) I have two questions: 1) virtio-net-pci device do need special handle in device_set_bootindex(). 2) isa-fdc' property is bootindexA and bootindexB, maybe we can add more a parameter for device_add_bootindex_property()? Best regards, -Gonglei > typedef struct BootIndexProperty > { > int32_t *field; > const char *suffix; > } BootIndexProperty; > > static void device_get_bootindex(Object *obj, Visitor *v, void *opaque, > const char *name, Error **errp) > { > BootIndexProperty *prop = opaque; > visit_type_int32(v, prop->field, name, errp); > } > > static void device_set_bootindex(Object *obj, Visitor *v, void *opaque, > const char *name, Error **errp) > { > DeviceState *dev = DEVICE(obj); > BootIndexProperty *prop = opaque; > int32_t boot_index; > Error *local_err = NULL; > > visit_type_int32(v, &boot_index, name, &local_err); > if (local_err) { > goto out; > } > > check_boot_index(boot_index, &local_err); > if (local_err) { > goto out; > } > > *prop->field = boot_index; > add_boot_device_path(boot_index, dev, prop->suffix); > > out: > if (local_err) { > error_propagate(errp, local_err); > } > } > > static void property_release_bootindex(Object *obj, const char *name, > void *opaque) > > { > BootIndexProperty *prop = opaque; > DeviceState *dev = DEVICE(obj); > del_boot_device_path(dev); > g_free(prop); > } > > void device_add_bootindex_property(DeviceState *dev, int32_t *field, > const char *suffix, Error **errp) > { > Error *local_err = NULL; > BootIndexProperty *prop = g_malloc0(sizeof(*prop)); > > prop->field = field; > prop->suffix = suffix; > object_property_add(OBJECT(dev), "bootindex", "int32", > device_get_bootindex, > device_set_bootindex, > property_release_bootindex, > prop, &local_err); > > if (local_err) { > error_propagate(errp, local_err); > g_free(prop); > } > } > > -- > Eduardo
On Fri, Sep 05, 2014 at 02:42:48AM +0000, Gonglei (Arei) wrote: > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Friday, September 05, 2014 10:20 AM > > Subject: Re: [PATCH v6 02/27] bootindex: add del_boot_device_path function > > > > On Fri, Sep 05, 2014 at 12:44:56AM +0000, Gonglei (Arei) wrote: > > [...] > > > > can we have a wrapper to reduce code > > > > duplication? e.g. a: > > > > void device_add_bootindex_property(DeviceState *dev, int32_t *field, > > const > > > > char *suffix) > > > > function. > > > > > > > > Then instead of reimplementing set/get/finalize functions, device code > > > > could just call something like: > > > > device_add_bootindex_property(dev, &n->nic_conf.bootindex, > > > > "/ethernet-phy@0"); > > > > > > > > > > This way we cannot attach our target that changing bootindex and take > > effect > > > during vm rebooting. > > > > I don't understand what you mean, here. Whatever you are planning to do on > > the > > device-specific setter/getters, if they all look the same, you can write a > > common getter/setter to do exactly the same steps, and register it inside > > device_add_bootindex_property(). This way, patches 08/27 to 26/27 can > > become > > one-liners, instead of adding more 20 lines each. > > > > I mean something like this: > > > OK. Thanks for explanation. :) > > I have two questions: > > 1) virtio-net-pci device do need special handle in device_set_bootindex(). > 2) isa-fdc' property is bootindexA and bootindexB, maybe we > can add more a parameter for device_add_bootindex_property()? I see that you added two extra parameters to device_add_bootindex_property(). Looks like a good solution, to me.
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index b343df5..672984c 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -208,6 +208,7 @@ void do_usb_del(Monitor *mon, const QDict *qdict); void usb_info(Monitor *mon, const QDict *qdict); void check_boot_index(int32_t bootindex, Error **errp); +void del_boot_device_path(DeviceState *dev); void add_boot_device_path(int32_t bootindex, DeviceState *dev, const char *suffix); char *get_boot_devices_list(size_t *size, bool ignore_suffixes); diff --git a/vl.c b/vl.c index 76fb008..5e57541 100644 --- a/vl.c +++ b/vl.c @@ -1252,6 +1252,42 @@ void check_boot_index(int32_t bootindex, Error **errp) } } +static bool is_same_fw_dev_path(DeviceState *src, DeviceState *dst) +{ + bool ret = false; + char *devpath_src = qdev_get_fw_dev_path(src); + char *devpath_dst = qdev_get_fw_dev_path(dst); + + if (!strcmp(devpath_src, devpath_dst)) { + ret = true; + } + + g_free(devpath_src); + g_free(devpath_dst); + return ret; +} + +void del_boot_device_path(DeviceState *dev) +{ + FWBootEntry *i; + + assert(dev != NULL); + + /* remove all entries of the assigned device */ + QTAILQ_FOREACH(i, &fw_boot_order, link) { + if (i->dev == NULL) { + continue; + } + if ((i->dev == dev || is_same_fw_dev_path(i->dev, dev))) { + QTAILQ_REMOVE(&fw_boot_order, i, link); + g_free(i->suffix); + g_free(i); + + break; + } + } +} + void add_boot_device_path(int32_t bootindex, DeviceState *dev, const char *suffix) {