* Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device [not found] ` <1398158504.30055.12.camel@localhost.localdomain> @ 2014-05-11 3:07 ` lijun 2014-05-11 9:17 ` Marcel Apfelbaum 0 siblings, 1 reply; 8+ messages in thread From: lijun @ 2014-05-11 3:07 UTC (permalink / raw) To: marcel.a Cc: kwolf, aliguori, mst, juli, stefanha, armbru, qemu-devel, anthony, pbonzini, afaerber On 04/22/2014 05:21 PM, Marcel Apfelbaum wrote: > On Wed, 2014-04-16 at 22:20 +0800, Jun Li wrote: >> Add remove_boot_device_path() function to remove bootindex when hot-unplug >> a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-generic device. >> So it has fixed bug1086603, ref: >> https://bugzilla.redhat.com/show_bug.cgi?id=1086603 >> >> Make some changes based on Andreas's good suggestion. >> >> Signed-off-by: Jun Li <junmuzi@gmail.com> >> --- >> hw/block/virtio-blk.c | 1 + >> hw/net/virtio-net.c | 1 + >> hw/scsi/scsi-disk.c | 6 ++++-- >> hw/scsi/scsi-generic.c | 3 +++ >> include/sysemu/sysemu.h | 2 ++ >> vl.c | 16 ++++++++++++++++ >> 6 files changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 8a568e5..ecdd266 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) >> unregister_savevm(dev, "virtio-blk", s); >> blockdev_mark_auto_del(s->bs); >> virtio_cleanup(vdev); >> + remove_boot_device_path(s->conf->bootindex, dev, "/disk@0,0"); >> } >> >> static Property virtio_blk_properties[] = { >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 33bd233..520c029 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -1633,6 +1633,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp) >> g_free(n->vqs); >> qemu_del_nic(n->nic); >> virtio_cleanup(vdev); >> + remove_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0"); >> } >> >> static void virtio_net_instance_init(Object *obj) >> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c >> index 48a28ae..bb2176a 100644 >> --- a/hw/scsi/scsi-disk.c >> +++ b/hw/scsi/scsi-disk.c >> @@ -2150,12 +2150,14 @@ static void scsi_disk_reset(DeviceState *dev) >> s->tray_open = 0; >> } >> >> -static void scsi_destroy(SCSIDevice *dev) >> +static void scsi_destroy(SCSIDevice *sdev) >> { >> - SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); >> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, sdev); >> + DeviceState *dev = DEVICE(sdev); >> >> scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE)); >> blockdev_mark_auto_del(s->qdev.conf.bs); >> + remove_boot_device_path(s->qdev.conf.bootindex, dev, NULL); >> } >> >> static void scsi_disk_resize_cb(void *opaque) >> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c >> index 8d92e0d..2531a44 100644 >> --- a/hw/scsi/scsi-generic.c >> +++ b/hw/scsi/scsi-generic.c >> @@ -388,8 +388,11 @@ static void scsi_generic_reset(DeviceState *dev) >> >> static void scsi_destroy(SCSIDevice *s) >> { >> + DeviceState *dev = DEVICE(s); >> + >> scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE)); >> blockdev_mark_auto_del(s->conf.bs); >> + remove_boot_device_path(s->conf.bootindex, dev, NULL); >> } >> >> static int scsi_generic_initfn(SCSIDevice *s) >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >> index ba5c7f8..f7ad1e2 100644 >> --- a/include/sysemu/sysemu.h >> +++ b/include/sysemu/sysemu.h >> @@ -193,6 +193,8 @@ void rtc_change_mon_event(struct tm *tm); >> >> void add_boot_device_path(int32_t bootindex, DeviceState *dev, >> const char *suffix); >> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, >> + const char *suffix); >> char *get_boot_devices_list(size_t *size, bool ignore_suffixes); >> >> DeviceState *get_boot_device(uint32_t position); >> diff --git a/vl.c b/vl.c >> index 9975e5a..1713c68 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, >> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); >> } >> >> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, >> + const char *suffix) >> +{ >> + FWBootEntry *node, *next_node; >> + >> + if (bootindex == -1) { >> + return; >> + } >> + >> + QTAILQ_FOREACH_SAFE(node, &fw_boot_order, link, next_node) >> + if (node->bootindex == bootindex) { >> + QTAILQ_REMOVE(&fw_boot_order, node, link); > Hi Liu. > Sorry for the late response, but I was in vacation :) . > Your patch looks OK, I once wrote another one on the same issue > (it touched the same problem, but it was *completely* different). > > Here is the *real* problem: fw_boot_order list is not queried > again on guest reboot, so 'touching' the list has no effect. > While your code is correct (as far as I can tell), it seems > that his place is after the above problem is solved. > > No one is currently working on this, feel free > to take this challenge. I'll help however I can. Hi Marcel, Sorry, so later response. I got a heavy sick these days. I have see these discussion emails between you and Paolo. After guest reboot, seabios will got devices info from boards(this boards be generated by qemu command line). Just as Paolo's explanations, but I am not familiar with seabios. I will do some research about this. Thank you very much. BTW, My first name is Jun, my last name is Li, my English name is woodpecker, you could call me jun or woodpecker. :) Best Regards, Jun Li > >> + return; >> + } >> +} >> + >> DeviceState *get_boot_device(uint32_t position) >> { >> uint32_t counter = 0; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device 2014-05-11 3:07 ` [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device lijun @ 2014-05-11 9:17 ` Marcel Apfelbaum 2014-05-12 15:51 ` Jun Li 0 siblings, 1 reply; 8+ messages in thread From: Marcel Apfelbaum @ 2014-05-11 9:17 UTC (permalink / raw) To: lijun Cc: kwolf, aliguori, mst, juli, stefanha, armbru, qemu-devel, anthony, pbonzini, afaerber On Sun, 2014-05-11 at 11:07 +0800, lijun wrote: > On 04/22/2014 05:21 PM, Marcel Apfelbaum wrote: > > On Wed, 2014-04-16 at 22:20 +0800, Jun Li wrote: > >> Add remove_boot_device_path() function to remove bootindex when hot-unplug > >> a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-generic device. > >> So it has fixed bug1086603, ref: > >> https://bugzilla.redhat.com/show_bug.cgi?id=1086603 > >> > >> Make some changes based on Andreas's good suggestion. > >> > >> Signed-off-by: Jun Li <junmuzi@gmail.com> > >> --- > >> hw/block/virtio-blk.c | 1 + > >> hw/net/virtio-net.c | 1 + > >> hw/scsi/scsi-disk.c | 6 ++++-- > >> hw/scsi/scsi-generic.c | 3 +++ > >> include/sysemu/sysemu.h | 2 ++ > >> vl.c | 16 ++++++++++++++++ > >> 6 files changed, 27 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > >> index 8a568e5..ecdd266 100644 > >> --- a/hw/block/virtio-blk.c > >> +++ b/hw/block/virtio-blk.c > >> @@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) > >> unregister_savevm(dev, "virtio-blk", s); > >> blockdev_mark_auto_del(s->bs); > >> virtio_cleanup(vdev); > >> + remove_boot_device_path(s->conf->bootindex, dev, "/disk@0,0"); > >> } > >> > >> static Property virtio_blk_properties[] = { > >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >> index 33bd233..520c029 100644 > >> --- a/hw/net/virtio-net.c > >> +++ b/hw/net/virtio-net.c > >> @@ -1633,6 +1633,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp) > >> g_free(n->vqs); > >> qemu_del_nic(n->nic); > >> virtio_cleanup(vdev); > >> + remove_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0"); > >> } > >> > >> static void virtio_net_instance_init(Object *obj) > >> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > >> index 48a28ae..bb2176a 100644 > >> --- a/hw/scsi/scsi-disk.c > >> +++ b/hw/scsi/scsi-disk.c > >> @@ -2150,12 +2150,14 @@ static void scsi_disk_reset(DeviceState *dev) > >> s->tray_open = 0; > >> } > >> > >> -static void scsi_destroy(SCSIDevice *dev) > >> +static void scsi_destroy(SCSIDevice *sdev) > >> { > >> - SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); > >> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, sdev); > >> + DeviceState *dev = DEVICE(sdev); > >> > >> scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE)); > >> blockdev_mark_auto_del(s->qdev.conf.bs); > >> + remove_boot_device_path(s->qdev.conf.bootindex, dev, NULL); > >> } > >> > >> static void scsi_disk_resize_cb(void *opaque) > >> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c > >> index 8d92e0d..2531a44 100644 > >> --- a/hw/scsi/scsi-generic.c > >> +++ b/hw/scsi/scsi-generic.c > >> @@ -388,8 +388,11 @@ static void scsi_generic_reset(DeviceState *dev) > >> > >> static void scsi_destroy(SCSIDevice *s) > >> { > >> + DeviceState *dev = DEVICE(s); > >> + > >> scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE)); > >> blockdev_mark_auto_del(s->conf.bs); > >> + remove_boot_device_path(s->conf.bootindex, dev, NULL); > >> } > >> > >> static int scsi_generic_initfn(SCSIDevice *s) > >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > >> index ba5c7f8..f7ad1e2 100644 > >> --- a/include/sysemu/sysemu.h > >> +++ b/include/sysemu/sysemu.h > >> @@ -193,6 +193,8 @@ void rtc_change_mon_event(struct tm *tm); > >> > >> void add_boot_device_path(int32_t bootindex, DeviceState *dev, > >> const char *suffix); > >> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, > >> + const char *suffix); > >> char *get_boot_devices_list(size_t *size, bool ignore_suffixes); > >> > >> DeviceState *get_boot_device(uint32_t position); > >> diff --git a/vl.c b/vl.c > >> index 9975e5a..1713c68 100644 > >> --- a/vl.c > >> +++ b/vl.c > >> @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, > >> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); > >> } > >> > >> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, > >> + const char *suffix) > >> +{ > >> + FWBootEntry *node, *next_node; > >> + > >> + if (bootindex == -1) { > >> + return; > >> + } > >> + > >> + QTAILQ_FOREACH_SAFE(node, &fw_boot_order, link, next_node) > >> + if (node->bootindex == bootindex) { > >> + QTAILQ_REMOVE(&fw_boot_order, node, link); > > Hi Liu. > > Sorry for the late response, but I was in vacation :) . > > Your patch looks OK, I once wrote another one on the same issue > > (it touched the same problem, but it was *completely* different). > > > > Here is the *real* problem: fw_boot_order list is not queried > > again on guest reboot, so 'touching' the list has no effect. > > While your code is correct (as far as I can tell), it seems > > that his place is after the above problem is solved. > > > > No one is currently working on this, feel free > > to take this challenge. I'll help however I can. > Hi Marcel, > > Sorry, so later response. I got a heavy sick these days. I have see > these discussion emails between you and Paolo. > After guest reboot, seabios will got devices info from boards(this > boards be generated by qemu command line). Just as Paolo's explanations, > but I am not familiar with seabios. I will do some research about this. > Thank you very much. > > BTW, My first name is Jun, my last name is Li, my English name is > woodpecker, you could call me jun or woodpecker. :) Hi Jun, I hope you are feeling better! If you need further assistance, just let us know. Thanks, Marcel > > Best Regards, > Jun Li > > > > >> + return; > >> + } > >> +} > >> + > >> DeviceState *get_boot_device(uint32_t position) > >> { > >> uint32_t counter = 0; > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device 2014-05-11 9:17 ` Marcel Apfelbaum @ 2014-05-12 15:51 ` Jun Li 2014-05-14 13:36 ` Michael S. Tsirkin 0 siblings, 1 reply; 8+ messages in thread From: Jun Li @ 2014-05-12 15:51 UTC (permalink / raw) To: Marcel Apfelbaum Cc: kwolf, aliguori, mst, juli, stefanha, armbru, qemu-devel, anthony, pbonzini, afaerber On 05/11/2014 05:17 PM, Marcel Apfelbaum wrote: > On Sun, 2014-05-11 at 11:07 +0800, lijun wrote: >> On 04/22/2014 05:21 PM, Marcel Apfelbaum wrote: >>> On Wed, 2014-04-16 at 22:20 +0800, Jun Li wrote: >>>> Add remove_boot_device_path() function to remove bootindex when hot-unplug >>>> a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-generic device. >>>> So it has fixed bug1086603, ref: >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1086603 >>>> >>>> Make some changes based on Andreas's good suggestion. >>>> >>>> Signed-off-by: Jun Li <junmuzi@gmail.com> >>>> --- >>>> hw/block/virtio-blk.c | 1 + >>>> hw/net/virtio-net.c | 1 + >>>> hw/scsi/scsi-disk.c | 6 ++++-- >>>> hw/scsi/scsi-generic.c | 3 +++ >>>> include/sysemu/sysemu.h | 2 ++ >>>> vl.c | 16 ++++++++++++++++ >>>> 6 files changed, 27 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >>>> index 8a568e5..ecdd266 100644 >>>> --- a/hw/block/virtio-blk.c >>>> +++ b/hw/block/virtio-blk.c >>>> @@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) >>>> unregister_savevm(dev, "virtio-blk", s); >>>> blockdev_mark_auto_del(s->bs); >>>> virtio_cleanup(vdev); >>>> + remove_boot_device_path(s->conf->bootindex, dev, "/disk@0,0"); >>>> } >>>> >>>> static Property virtio_blk_properties[] = { >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>> index 33bd233..520c029 100644 >>>> --- a/hw/net/virtio-net.c >>>> +++ b/hw/net/virtio-net.c >>>> @@ -1633,6 +1633,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp) >>>> g_free(n->vqs); >>>> qemu_del_nic(n->nic); >>>> virtio_cleanup(vdev); >>>> + remove_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0"); >>>> } >>>> >>>> static void virtio_net_instance_init(Object *obj) >>>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c >>>> index 48a28ae..bb2176a 100644 >>>> --- a/hw/scsi/scsi-disk.c >>>> +++ b/hw/scsi/scsi-disk.c >>>> @@ -2150,12 +2150,14 @@ static void scsi_disk_reset(DeviceState *dev) >>>> s->tray_open = 0; >>>> } >>>> >>>> -static void scsi_destroy(SCSIDevice *dev) >>>> +static void scsi_destroy(SCSIDevice *sdev) >>>> { >>>> - SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); >>>> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, sdev); >>>> + DeviceState *dev = DEVICE(sdev); >>>> >>>> scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE)); >>>> blockdev_mark_auto_del(s->qdev.conf.bs); >>>> + remove_boot_device_path(s->qdev.conf.bootindex, dev, NULL); >>>> } >>>> >>>> static void scsi_disk_resize_cb(void *opaque) >>>> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c >>>> index 8d92e0d..2531a44 100644 >>>> --- a/hw/scsi/scsi-generic.c >>>> +++ b/hw/scsi/scsi-generic.c >>>> @@ -388,8 +388,11 @@ static void scsi_generic_reset(DeviceState *dev) >>>> >>>> static void scsi_destroy(SCSIDevice *s) >>>> { >>>> + DeviceState *dev = DEVICE(s); >>>> + >>>> scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE)); >>>> blockdev_mark_auto_del(s->conf.bs); >>>> + remove_boot_device_path(s->conf.bootindex, dev, NULL); >>>> } >>>> >>>> static int scsi_generic_initfn(SCSIDevice *s) >>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >>>> index ba5c7f8..f7ad1e2 100644 >>>> --- a/include/sysemu/sysemu.h >>>> +++ b/include/sysemu/sysemu.h >>>> @@ -193,6 +193,8 @@ void rtc_change_mon_event(struct tm *tm); >>>> >>>> void add_boot_device_path(int32_t bootindex, DeviceState *dev, >>>> const char *suffix); >>>> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, >>>> + const char *suffix); >>>> char *get_boot_devices_list(size_t *size, bool ignore_suffixes); >>>> >>>> DeviceState *get_boot_device(uint32_t position); >>>> diff --git a/vl.c b/vl.c >>>> index 9975e5a..1713c68 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, >>>> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); >>>> } >>>> >>>> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, >>>> + const char *suffix) >>>> +{ >>>> + FWBootEntry *node, *next_node; >>>> + >>>> + if (bootindex == -1) { >>>> + return; >>>> + } >>>> + >>>> + QTAILQ_FOREACH_SAFE(node, &fw_boot_order, link, next_node) >>>> + if (node->bootindex == bootindex) { >>>> + QTAILQ_REMOVE(&fw_boot_order, node, link); >>> Hi Liu. >>> Sorry for the late response, but I was in vacation :) . >>> Your patch looks OK, I once wrote another one on the same issue >>> (it touched the same problem, but it was *completely* different). >>> >>> Here is the *real* problem: fw_boot_order list is not queried >>> again on guest reboot, so 'touching' the list has no effect. >>> While your code is correct (as far as I can tell), it seems >>> that his place is after the above problem is solved. >>> >>> No one is currently working on this, feel free >>> to take this challenge. I'll help however I can. >> Hi Marcel, >> >> Sorry, so later response. I got a heavy sick these days. I have see >> these discussion emails between you and Paolo. >> After guest reboot, seabios will got devices info from boards(this >> boards be generated by qemu command line). Just as Paolo's explanations, >> but I am not familiar with seabios. I will do some research about this. >> Thank you very much. >> >> BTW, My first name is Jun, my last name is Li, my English name is >> woodpecker, you could call me jun or woodpecker. :) > Hi Jun, > I hope you are feeling better! > If you need further assistance, just let us know. Hi Marcel, I just do a glancing debug, find that, when reboot guest, It will call function qemu_system_reset. So I want to add reset "fw_cfg" in function qemu_system_reset. Maybe this can fix your issue. But I have not try due to the limited time. This is just an ideal. As I am a sparetime "developer", so I can just fix this issue at weekend. So sorry. Thanks, Jun Li ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device 2014-05-12 15:51 ` Jun Li @ 2014-05-14 13:36 ` Michael S. Tsirkin 0 siblings, 0 replies; 8+ messages in thread From: Michael S. Tsirkin @ 2014-05-14 13:36 UTC (permalink / raw) To: Jun Li Cc: kwolf, aliguori, Marcel Apfelbaum, juli, stefanha, armbru, qemu-devel, anthony, pbonzini, afaerber On Mon, May 12, 2014 at 11:51:35PM +0800, Jun Li wrote: > > On 05/11/2014 05:17 PM, Marcel Apfelbaum wrote: > >On Sun, 2014-05-11 at 11:07 +0800, lijun wrote: > >>On 04/22/2014 05:21 PM, Marcel Apfelbaum wrote: > >>>On Wed, 2014-04-16 at 22:20 +0800, Jun Li wrote: > >>>>Add remove_boot_device_path() function to remove bootindex when hot-unplug > >>>>a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-generic device. > >>>>So it has fixed bug1086603, ref: > >>>>https://bugzilla.redhat.com/show_bug.cgi?id=1086603 > >>>> > >>>>Make some changes based on Andreas's good suggestion. > >>>> > >>>>Signed-off-by: Jun Li <junmuzi@gmail.com> > >>>>--- > >>>> hw/block/virtio-blk.c | 1 + > >>>> hw/net/virtio-net.c | 1 + > >>>> hw/scsi/scsi-disk.c | 6 ++++-- > >>>> hw/scsi/scsi-generic.c | 3 +++ > >>>> include/sysemu/sysemu.h | 2 ++ > >>>> vl.c | 16 ++++++++++++++++ > >>>> 6 files changed, 27 insertions(+), 2 deletions(-) > >>>> > >>>>diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > >>>>index 8a568e5..ecdd266 100644 > >>>>--- a/hw/block/virtio-blk.c > >>>>+++ b/hw/block/virtio-blk.c > >>>>@@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) > >>>> unregister_savevm(dev, "virtio-blk", s); > >>>> blockdev_mark_auto_del(s->bs); > >>>> virtio_cleanup(vdev); > >>>>+ remove_boot_device_path(s->conf->bootindex, dev, "/disk@0,0"); > >>>> } > >>>> static Property virtio_blk_properties[] = { > >>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>>index 33bd233..520c029 100644 > >>>>--- a/hw/net/virtio-net.c > >>>>+++ b/hw/net/virtio-net.c > >>>>@@ -1633,6 +1633,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp) > >>>> g_free(n->vqs); > >>>> qemu_del_nic(n->nic); > >>>> virtio_cleanup(vdev); > >>>>+ remove_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0"); > >>>> } > >>>> static void virtio_net_instance_init(Object *obj) > >>>>diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > >>>>index 48a28ae..bb2176a 100644 > >>>>--- a/hw/scsi/scsi-disk.c > >>>>+++ b/hw/scsi/scsi-disk.c > >>>>@@ -2150,12 +2150,14 @@ static void scsi_disk_reset(DeviceState *dev) > >>>> s->tray_open = 0; > >>>> } > >>>>-static void scsi_destroy(SCSIDevice *dev) > >>>>+static void scsi_destroy(SCSIDevice *sdev) > >>>> { > >>>>- SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); > >>>>+ SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, sdev); > >>>>+ DeviceState *dev = DEVICE(sdev); > >>>> scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE)); > >>>> blockdev_mark_auto_del(s->qdev.conf.bs); > >>>>+ remove_boot_device_path(s->qdev.conf.bootindex, dev, NULL); > >>>> } > >>>> static void scsi_disk_resize_cb(void *opaque) > >>>>diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c > >>>>index 8d92e0d..2531a44 100644 > >>>>--- a/hw/scsi/scsi-generic.c > >>>>+++ b/hw/scsi/scsi-generic.c > >>>>@@ -388,8 +388,11 @@ static void scsi_generic_reset(DeviceState *dev) > >>>> static void scsi_destroy(SCSIDevice *s) > >>>> { > >>>>+ DeviceState *dev = DEVICE(s); > >>>>+ > >>>> scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE)); > >>>> blockdev_mark_auto_del(s->conf.bs); > >>>>+ remove_boot_device_path(s->conf.bootindex, dev, NULL); > >>>> } > >>>> static int scsi_generic_initfn(SCSIDevice *s) > >>>>diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > >>>>index ba5c7f8..f7ad1e2 100644 > >>>>--- a/include/sysemu/sysemu.h > >>>>+++ b/include/sysemu/sysemu.h > >>>>@@ -193,6 +193,8 @@ void rtc_change_mon_event(struct tm *tm); > >>>> void add_boot_device_path(int32_t bootindex, DeviceState *dev, > >>>> const char *suffix); > >>>>+void remove_boot_device_path(int32_t bootindex, DeviceState *dev, > >>>>+ const char *suffix); > >>>> char *get_boot_devices_list(size_t *size, bool ignore_suffixes); > >>>> DeviceState *get_boot_device(uint32_t position); > >>>>diff --git a/vl.c b/vl.c > >>>>index 9975e5a..1713c68 100644 > >>>>--- a/vl.c > >>>>+++ b/vl.c > >>>>@@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, > >>>> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); > >>>> } > >>>>+void remove_boot_device_path(int32_t bootindex, DeviceState *dev, > >>>>+ const char *suffix) > >>>>+{ > >>>>+ FWBootEntry *node, *next_node; > >>>>+ > >>>>+ if (bootindex == -1) { > >>>>+ return; > >>>>+ } > >>>>+ > >>>>+ QTAILQ_FOREACH_SAFE(node, &fw_boot_order, link, next_node) > >>>>+ if (node->bootindex == bootindex) { > >>>>+ QTAILQ_REMOVE(&fw_boot_order, node, link); > >>>Hi Liu. > >>>Sorry for the late response, but I was in vacation :) . > >>>Your patch looks OK, I once wrote another one on the same issue > >>>(it touched the same problem, but it was *completely* different). > >>> > >>>Here is the *real* problem: fw_boot_order list is not queried > >>>again on guest reboot, so 'touching' the list has no effect. > >>>While your code is correct (as far as I can tell), it seems > >>>that his place is after the above problem is solved. > >>> > >>>No one is currently working on this, feel free > >>>to take this challenge. I'll help however I can. > >>Hi Marcel, > >> > >>Sorry, so later response. I got a heavy sick these days. I have see > >>these discussion emails between you and Paolo. > >>After guest reboot, seabios will got devices info from boards(this > >>boards be generated by qemu command line). Just as Paolo's explanations, > >>but I am not familiar with seabios. I will do some research about this. > >>Thank you very much. > >> > >>BTW, My first name is Jun, my last name is Li, my English name is > >>woodpecker, you could call me jun or woodpecker. :) > >Hi Jun, > >I hope you are feeling better! > >If you need further assistance, just let us know. > Hi Marcel, > I just do a glancing debug, find that, when reboot guest, It will > call function qemu_system_reset. So I want to add reset "fw_cfg" in > function qemu_system_reset. Maybe this can fix your issue. But I > have not try due to the limited time. This is just an ideal. As I am > a sparetime "developer", so I can just fix this issue at weekend. So > sorry. > > Thanks, > Jun Li Well FW CFG changing across reboots will create all kind of problems. So I think we should think about an alternative mechanism if we want to make it all dynamic, a static boot index number is a problematic api anyway, how do you prepare for ability to add more devices at arbitrary places? number everything 10,20,30, BASIC like? For now your patch fixes a dangling pointer so I'll pick it up, thanks! -- MST ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device [not found] <1397658057-12086-1-git-send-email-junmuzi@gmail.com> [not found] ` <1398158504.30055.12.camel@localhost.localdomain> @ 2014-05-15 15:07 ` Michael S. Tsirkin 2014-05-17 12:09 ` Jun Li 1 sibling, 1 reply; 8+ messages in thread From: Michael S. Tsirkin @ 2014-05-15 15:07 UTC (permalink / raw) To: Jun Li Cc: kwolf, aliguori, juli, stefanha, armbru, qemu-devel, anthony, pbonzini, afaerber On Wed, Apr 16, 2014 at 10:20:57PM +0800, Jun Li wrote: > Add remove_boot_device_path() function to remove bootindex when hot-unplug > a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-generic device. > So it has fixed bug1086603, ref: > https://bugzilla.redhat.com/show_bug.cgi?id=1086603 > > Make some changes based on Andreas's good suggestion. > > Signed-off-by: Jun Li <junmuzi@gmail.com> > --- > hw/block/virtio-blk.c | 1 + > hw/net/virtio-net.c | 1 + > hw/scsi/scsi-disk.c | 6 ++++-- > hw/scsi/scsi-generic.c | 3 +++ > include/sysemu/sysemu.h | 2 ++ > vl.c | 16 ++++++++++++++++ > 6 files changed, 27 insertions(+), 2 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 8a568e5..ecdd266 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) > unregister_savevm(dev, "virtio-blk", s); > blockdev_mark_auto_del(s->bs); > virtio_cleanup(vdev); > + remove_boot_device_path(s->conf->bootindex, dev, "/disk@0,0"); > } > > static Property virtio_blk_properties[] = { > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 33bd233..520c029 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -1633,6 +1633,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp) > g_free(n->vqs); > qemu_del_nic(n->nic); > virtio_cleanup(vdev); > + remove_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0"); > } > > static void virtio_net_instance_init(Object *obj) > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index 48a28ae..bb2176a 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -2150,12 +2150,14 @@ static void scsi_disk_reset(DeviceState *dev) > s->tray_open = 0; > } > > -static void scsi_destroy(SCSIDevice *dev) > +static void scsi_destroy(SCSIDevice *sdev) > { > - SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); > + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, sdev); > + DeviceState *dev = DEVICE(sdev); > > scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE)); > blockdev_mark_auto_del(s->qdev.conf.bs); > + remove_boot_device_path(s->qdev.conf.bootindex, dev, NULL); > } > > static void scsi_disk_resize_cb(void *opaque) > diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c > index 8d92e0d..2531a44 100644 > --- a/hw/scsi/scsi-generic.c > +++ b/hw/scsi/scsi-generic.c > @@ -388,8 +388,11 @@ static void scsi_generic_reset(DeviceState *dev) > > static void scsi_destroy(SCSIDevice *s) > { > + DeviceState *dev = DEVICE(s); > + > scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE)); > blockdev_mark_auto_del(s->conf.bs); > + remove_boot_device_path(s->conf.bootindex, dev, NULL); > } > > static int scsi_generic_initfn(SCSIDevice *s) > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index ba5c7f8..f7ad1e2 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -193,6 +193,8 @@ void rtc_change_mon_event(struct tm *tm); > > void add_boot_device_path(int32_t bootindex, DeviceState *dev, > const char *suffix); > +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, > + const char *suffix); > char *get_boot_devices_list(size_t *size, bool ignore_suffixes); > > DeviceState *get_boot_device(uint32_t position); > diff --git a/vl.c b/vl.c > index 9975e5a..1713c68 100644 > --- a/vl.c > +++ b/vl.c > @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, > QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); > } > > +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, > + const char *suffix) Why do we need suffix here? It seems unused. > +{ > + FWBootEntry *node, *next_node; > + > + if (bootindex == -1) { > + return; > + } > + > + QTAILQ_FOREACH_SAFE(node, &fw_boot_order, link, next_node) > + if (node->bootindex == bootindex) { > + QTAILQ_REMOVE(&fw_boot_order, node, link); > + return; > + } > +} > + > DeviceState *get_boot_device(uint32_t position) > { > uint32_t counter = 0; > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device 2014-05-15 15:07 ` Michael S. Tsirkin @ 2014-05-17 12:09 ` Jun Li 2014-05-19 7:18 ` Markus Armbruster 0 siblings, 1 reply; 8+ messages in thread From: Jun Li @ 2014-05-17 12:09 UTC (permalink / raw) To: Michael S. Tsirkin Cc: kwolf, aliguori, juli, stefanha, armbru, qemu-devel, anthony, pbonzini, afaerber On 05/15/2014 11:07 PM, Michael S. Tsirkin wrote: > On Wed, Apr 16, 2014 at 10:20:57PM +0800, Jun Li wrote: >> Add remove_boot_device_path() function to remove bootindex when hot-unplug >> a device. This patch fixed virtio-blk/virtio-net/scsi-disk/scsi-generic device. >> So it has fixed bug1086603, ref: >> https://bugzilla.redhat.com/show_bug.cgi?id=1086603 >> >> Make some changes based on Andreas's good suggestion. >> >> Signed-off-by: Jun Li <junmuzi@gmail.com> >> --- >> hw/block/virtio-blk.c | 1 + >> hw/net/virtio-net.c | 1 + >> hw/scsi/scsi-disk.c | 6 ++++-- >> hw/scsi/scsi-generic.c | 3 +++ >> include/sysemu/sysemu.h | 2 ++ >> vl.c | 16 ++++++++++++++++ >> 6 files changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 8a568e5..ecdd266 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -752,6 +752,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp) >> unregister_savevm(dev, "virtio-blk", s); >> blockdev_mark_auto_del(s->bs); >> virtio_cleanup(vdev); >> + remove_boot_device_path(s->conf->bootindex, dev, "/disk@0,0"); >> } >> >> static Property virtio_blk_properties[] = { >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 33bd233..520c029 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -1633,6 +1633,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp) >> g_free(n->vqs); >> qemu_del_nic(n->nic); >> virtio_cleanup(vdev); >> + remove_boot_device_path(n->nic_conf.bootindex, dev, "/ethernet-phy@0"); >> } >> >> static void virtio_net_instance_init(Object *obj) >> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c >> index 48a28ae..bb2176a 100644 >> --- a/hw/scsi/scsi-disk.c >> +++ b/hw/scsi/scsi-disk.c >> @@ -2150,12 +2150,14 @@ static void scsi_disk_reset(DeviceState *dev) >> s->tray_open = 0; >> } >> >> -static void scsi_destroy(SCSIDevice *dev) >> +static void scsi_destroy(SCSIDevice *sdev) >> { >> - SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); >> + SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, sdev); >> + DeviceState *dev = DEVICE(sdev); >> >> scsi_device_purge_requests(&s->qdev, SENSE_CODE(NO_SENSE)); >> blockdev_mark_auto_del(s->qdev.conf.bs); >> + remove_boot_device_path(s->qdev.conf.bootindex, dev, NULL); >> } >> >> static void scsi_disk_resize_cb(void *opaque) >> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c >> index 8d92e0d..2531a44 100644 >> --- a/hw/scsi/scsi-generic.c >> +++ b/hw/scsi/scsi-generic.c >> @@ -388,8 +388,11 @@ static void scsi_generic_reset(DeviceState *dev) >> >> static void scsi_destroy(SCSIDevice *s) >> { >> + DeviceState *dev = DEVICE(s); >> + >> scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE)); >> blockdev_mark_auto_del(s->conf.bs); >> + remove_boot_device_path(s->conf.bootindex, dev, NULL); >> } >> >> static int scsi_generic_initfn(SCSIDevice *s) >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >> index ba5c7f8..f7ad1e2 100644 >> --- a/include/sysemu/sysemu.h >> +++ b/include/sysemu/sysemu.h >> @@ -193,6 +193,8 @@ void rtc_change_mon_event(struct tm *tm); >> >> void add_boot_device_path(int32_t bootindex, DeviceState *dev, >> const char *suffix); >> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, >> + const char *suffix); >> char *get_boot_devices_list(size_t *size, bool ignore_suffixes); >> >> DeviceState *get_boot_device(uint32_t position); >> diff --git a/vl.c b/vl.c >> index 9975e5a..1713c68 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, >> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); >> } >> >> +void remove_boot_device_path(int32_t bootindex, DeviceState *dev, >> + const char *suffix) > Why do we need suffix here? > It seems unused. Hi Michael, I just want to keep the same as function "add_boot_device_path". Such as: void add_boot_device_path(int32_t bootindex, DeviceState *dev, const char *suffix) I also find another function has define mon, but not be used in this function. Such as: ---file vl.c--- do_usb_del(Monitor *mon, const QDict *qdict) { const char *devname = qdict_get_str(qdict, "devname"); if (usb_device_del(devname) < 0) { error_report("could not delete USB device '%s'", devname); } } Best Regards, Jun Li >> +{ >> + FWBootEntry *node, *next_node; >> + >> + if (bootindex == -1) { >> + return; >> + } >> + >> + QTAILQ_FOREACH_SAFE(node, &fw_boot_order, link, next_node) >> + if (node->bootindex == bootindex) { >> + QTAILQ_REMOVE(&fw_boot_order, node, link); >> + return; >> + } >> +} >> + >> DeviceState *get_boot_device(uint32_t position) >> { >> uint32_t counter = 0; >> -- >> 1.8.3.1 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device 2014-05-17 12:09 ` Jun Li @ 2014-05-19 7:18 ` Markus Armbruster 2014-05-19 13:44 ` Jun Li 0 siblings, 1 reply; 8+ messages in thread From: Markus Armbruster @ 2014-05-19 7:18 UTC (permalink / raw) To: Jun Li Cc: kwolf, aliguori, Michael S. Tsirkin, juli, qemu-devel, stefanha, anthony, pbonzini, afaerber Jun Li <junmuzi@gmail.com> writes: > On 05/15/2014 11:07 PM, Michael S. Tsirkin wrote: >> On Wed, Apr 16, 2014 at 10:20:57PM +0800, Jun Li wrote: [...] >>> diff --git a/vl.c b/vl.c >>> index 9975e5a..1713c68 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, >>> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); >>> } >>> +void remove_boot_device_path(int32_t bootindex, DeviceState >>> *dev, >>> + const char *suffix) >> Why do we need suffix here? >> It seems unused. dev is unused as well. > Hi Michael, > > I just want to keep the same as function "add_boot_device_path". Such as: > void add_boot_device_path(int32_t bootindex, DeviceState *dev, > const char *suffix) Functions creating something commonly take different arguments than the function to destroy it. > I also find another function has define mon, but not be used in this function. Such as: > ---file vl.c--- > do_usb_del(Monitor *mon, const QDict *qdict) > { > const char *devname = qdict_get_str(qdict, "devname"); > if (usb_device_del(devname) < 0) { > error_report("could not delete USB device '%s'", devname); > } > } This one has a compelling reason: it's a mon_cmd_t mhandler.cmd callback. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device 2014-05-19 7:18 ` Markus Armbruster @ 2014-05-19 13:44 ` Jun Li 0 siblings, 0 replies; 8+ messages in thread From: Jun Li @ 2014-05-19 13:44 UTC (permalink / raw) To: Markus Armbruster Cc: kwolf, aliguori, Michael S. Tsirkin, juli, qemu-devel, stefanha, anthony, pbonzini, afaerber On 05/19/2014 03:18 PM, Markus Armbruster wrote: > Jun Li <junmuzi@gmail.com> writes: > >> On 05/15/2014 11:07 PM, Michael S. Tsirkin wrote: >>> On Wed, Apr 16, 2014 at 10:20:57PM +0800, Jun Li wrote: > [...] >>>> diff --git a/vl.c b/vl.c >>>> index 9975e5a..1713c68 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -1184,6 +1184,22 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev, >>>> QTAILQ_INSERT_TAIL(&fw_boot_order, node, link); >>>> } >>>> +void remove_boot_device_path(int32_t bootindex, DeviceState >>>> *dev, >>>> + const char *suffix) >>> Why do we need suffix here? >>> It seems unused. > dev is unused as well. > >> Hi Michael, >> >> I just want to keep the same as function "add_boot_device_path". Such as: >> void add_boot_device_path(int32_t bootindex, DeviceState *dev, >> const char *suffix) > Functions creating something commonly take different arguments than the > function to destroy it. ok, I will submit a update version for this patch. > >> I also find another function has define mon, but not be used in this function. Such as: >> ---file vl.c--- >> do_usb_del(Monitor *mon, const QDict *qdict) >> { >> const char *devname = qdict_get_str(qdict, "devname"); >> if (usb_device_del(devname) < 0) { >> error_report("could not delete USB device '%s'", devname); >> } >> } > This one has a compelling reason: it's a mon_cmd_t mhandler.cmd > callback. oh, thanks for your explanation. Best Regards, Jun Li ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-05-19 13:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1397658057-12086-1-git-send-email-junmuzi@gmail.com>
[not found] ` <1398158504.30055.12.camel@localhost.localdomain>
2014-05-11 3:07 ` [Qemu-devel] [PATCH v2] Add remove_boot_device_path() function for hot-unplug device lijun
2014-05-11 9:17 ` Marcel Apfelbaum
2014-05-12 15:51 ` Jun Li
2014-05-14 13:36 ` Michael S. Tsirkin
2014-05-15 15:07 ` Michael S. Tsirkin
2014-05-17 12:09 ` Jun Li
2014-05-19 7:18 ` Markus Armbruster
2014-05-19 13:44 ` Jun Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).