| Message ID | 1397658057-12086-1-git-send-email-junmuzi@gmail.com |
|---|---|
| State | New |
| Headers | show |
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. Thanks, Marcel > + return; > + } > +} > + > DeviceState *get_boot_device(uint32_t position) > { > uint32_t counter = 0;
Il 22/04/2014 11:21, Marcel Apfelbaum ha scritto: > 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. Doesn't Jun's patch fix a dangling pointer? If so, that would come first anyway. Paolo
On Mon, 2014-04-28 at 11:22 +0200, Paolo Bonzini wrote: > Il 22/04/2014 11:21, Marcel Apfelbaum ha scritto: > > 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. > > Doesn't Jun's patch fix a dangling pointer? If so, that would come > first anyway. It removes pointers from a "dead" list, and it only deals with some symptom. Even if you remove the device from the list, the guest still uses the previous list. The only difference is that you can now hot-plug/hot-unplug a disk with a boot-order index. However, strange things will happen when you reboot the guest, one of them may be that it actually works :) It is possible that I got it wrong... Thanks, Marcel > > Paolo >
Il 28/04/2014 11:38, Marcel Apfelbaum ha scritto: >> > Doesn't Jun's patch fix a dangling pointer? If so, that would come >> > first anyway. > It removes pointers from a "dead" list, and it only deals with > some symptom. Yeah, but dangling pointers are bad anyway. I think we should include Jun's patch. Paolo > Even if you remove the device from the list, the guest still uses > the previous list. The only difference is that you can now > hot-plug/hot-unplug a disk with a boot-order index. > However, strange things will happen when you reboot the guest, > one of them may be that it actually works :)
On Mon, 2014-04-28 at 13:32 +0200, Paolo Bonzini wrote: > Il 28/04/2014 11:38, Marcel Apfelbaum ha scritto: > >> > Doesn't Jun's patch fix a dangling pointer? If so, that would come > >> > first anyway. > > It removes pointers from a "dead" list, and it only deals with > > some symptom. > > Yeah, but dangling pointers are bad anyway. I think we should include > Jun's patch. I have nothing against it, but maybe you/someone answer this question which I honestly don't have the answer for: After this patch you can replace a boot-able disk at index x. What happens if you do so and the user reboots and selects index x? Until now, the index was 'in use', even if the disk was hot-unplugged. What are the consequences now? If we can live with them, I am all for using this patch. Thanks, Marcel > > Paolo > > > Even if you remove the device from the list, the guest still uses > > the previous list. The only difference is that you can now > > hot-plug/hot-unplug a disk with a boot-order index. > > However, strange things will happen when you reboot the guest, > > one of them may be that it actually works :) >
Il 28/04/2014 14:43, Marcel Apfelbaum ha scritto: >> > Yeah, but dangling pointers are bad anyway. I think we should include >> > Jun's patch. > I have nothing against it, but maybe you/someone answer this question > which I honestly don't have the answer for: > After this patch you can replace a boot-able disk at index x. > What happens if you do so and the user reboots and selects index x? > Until now, the index was 'in use', even if the disk was hot-unplugged. > What are the consequences now? If we can live with them, I am all for using this patch. Firmware does not use indices, it uses paths. If boards do not update the paths on reset, firmware behavior will be unmodified before/after this patch. With or without this patch, firmware will look for a device at the address where the "first" index x was. If there is no device there, either firmware will crash or it will ignore the entry. If there is a device, it will boot from that device no matter what the bootindex was on QEMU's command line. Paolo
On Mon, 2014-04-28 at 14:48 +0200, Paolo Bonzini wrote: > Il 28/04/2014 14:43, Marcel Apfelbaum ha scritto: > >> > Yeah, but dangling pointers are bad anyway. I think we should include > >> > Jun's patch. > > I have nothing against it, but maybe you/someone answer this question > > which I honestly don't have the answer for: > > After this patch you can replace a boot-able disk at index x. > > What happens if you do so and the user reboots and selects index x? > > Until now, the index was 'in use', even if the disk was hot-unplugged. > > What are the consequences now? If we can live with them, I am all for using this patch. > > Firmware does not use indices, it uses paths. If boards do not update > the paths on reset, firmware behavior will be unmodified before/after > this patch. > > With or without this patch, firmware will look for a device at the > address where the "first" index x was. If there is no device there, > either firmware will crash or it will ignore the entry. If there is a > device, it will boot from that device no matter what the bootindex was > on QEMU's command line. Thanks for the explanation! As I previously said, I agree with the patch. Thanks, Marcel > > Paolo
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; > >
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; > > > > >
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
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!
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 >
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 >>
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.
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
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); + return; + } +} + DeviceState *get_boot_device(uint32_t position) { uint32_t counter = 0;
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(-)