[Qemu-devel] [PATCH v2 0/7] modify boot order of guest, and take effect after rebooting

[Qemu-devel] [PATCH v2 0/7] modify boot order of guest, and take effect after rebooting

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Sometimes, we want to modify boot order of a guest, but no need to
shutdown it. We can call dynamic changing bootindex of a guest, which
can be assured taking effect just after the guest rebooting.

For example, in P2V scene, we boot a guest and then attach a
new system disk, for copying some thing. We want to assign the
new disk as the booting disk, which means its bootindex=1.

Different nics can be assigen different bootindex dynamically
also make sense.

The patchsets add one qmp interface, and add an fw_cfg_machine_reset()
to achieve it. Please review, Thanks.

Steps of testing:

./qemu-system-x86_64 -enable-kvm -m 4096 -smp 4 -name redhat6.2 -drive \ 
file=/home/redhat6.2.img,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=/home/RH-DVD1.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 \
-netdev type=user,id=net1 -device e1000,netdev=net1,bootindex=2,id=nic2 -monitor stdio
QEMU 2.0.93 monitor - type 'help' for more information
(qemu) set-bootindex ide0-0-1 1
The bootindex 1 has already been used
(qemu) set-bootindex ide0-0-1 5 "/disk@1"
(qemu) set-bootindex ide0-0-1 0 "/disk@1"
(qemu) system_reset
(qemu) set-bootindex nic2 0
The bootindex 0 has already been used
(qemu) set-bootindex ide0-0-1 -1 "/disk@1"
(qemu) set-bootindex nic2 0
(qemu) system_reset
(qemu) 

Changes since v1:
 *rework by Gerd's suggestion:
 - split modify and del fw_boot_order for single function.
 - change modify bootindex's realization which simply lookup
   the device and modify the bootindex. if the new bootindex
   has already used by another device just throw an error.
 - change to del_boot_device_path(DeviceState *dev) and simply delete all
   entries belonging to the device.

  Thanks Gerd.
 
Gonglei (7):
  bootindex: add modify_boot_device_path function
  bootindex: add del_boot_device_path function
  fw_cfg: add fw_cfg_machine_reset function
  bootindex: delete bootindex when device is removed
  qmp: add set-bootindex command
  qemu-monitor: HMP set-bootindex wrapper
  spapr: fix possible memory leak

 hmp-commands.hx           | 15 ++++++++++++
 hmp.c                     | 13 +++++++++++
 hmp.h                     |  1 +
 hw/block/virtio-blk.c     |  1 +
 hw/i386/kvm/pci-assign.c  |  1 +
 hw/misc/vfio.c            |  1 +
 hw/net/e1000.c            |  1 +
 hw/net/eepro100.c         |  1 +
 hw/net/ne2000.c           |  1 +
 hw/net/rtl8139.c          |  1 +
 hw/net/virtio-net.c       |  1 +
 hw/net/vmxnet3.c          |  1 +
 hw/nvram/fw_cfg.c         | 54 +++++++++++++++++++++++++++++++++++++------
 hw/ppc/spapr.c            |  1 +
 hw/scsi/scsi-generic.c    |  1 +
 hw/usb/dev-network.c      |  1 +
 hw/usb/host-libusb.c      |  1 +
 hw/usb/redirect.c         |  1 +
 include/hw/nvram/fw_cfg.h |  2 ++
 include/sysemu/sysemu.h   |  3 +++
 qapi-schema.json          | 16 +++++++++++++
 qmp-commands.hx           | 24 +++++++++++++++++++
 qmp.c                     | 17 ++++++++++++++
 vl.c                      | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 24 files changed, 211 insertions(+), 7 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

When we want to change one device's bootindex, we
should lookup the device and change the bootindex.
it is simply that remove it from the global boot list,
then re-add it, sorted by new bootindex. If the new
bootindex has already used by another device just
throw an error.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 include/sysemu/sysemu.h |  2 ++
 vl.c                    | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index d8539fd..e1b0659 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -209,6 +209,8 @@ void usb_info(Monitor *mon, const QDict *qdict);
 
 void add_boot_device_path(int32_t bootindex, DeviceState *dev,
                           const char *suffix);
+void modify_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 fe451aa..83d7dc4 100644
--- a/vl.c
+++ b/vl.c
@@ -1248,6 +1248,48 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
     QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
 }
 
+void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
+                          const char *suffix)
+{
+    FWBootEntry *node, *i;
+
+    assert(dev != NULL || suffix != NULL);
+
+    QTAILQ_FOREACH(i, &fw_boot_order, link) {
+        if (i->bootindex == bootindex) {
+            qerror_report(ERROR_CLASS_GENERIC_ERROR,
+               "The bootindex %d has already been used", bootindex);
+            return;
+        }
+        /* delete the same original dev */
+        if (i->dev->id && !strcmp(i->dev->id, dev->id)) {
+            QTAILQ_REMOVE(&fw_boot_order, i, link);
+            g_free(i->suffix);
+            g_free(i);
+
+            break;
+        }
+    }
+
+    if (bootindex >= 0) {
+        node = g_malloc0(sizeof(FWBootEntry));
+        node->bootindex = bootindex;
+        node->suffix = g_strdup(suffix);
+        node->dev = dev;
+
+        /* add to the global boot list */
+        QTAILQ_FOREACH(i, &fw_boot_order, link) {
+            if (i->bootindex < bootindex) {
+                continue;
+            }
+            QTAILQ_INSERT_BEFORE(i, node, link);
+            return;
+        }
+
+        QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
+    }
+}
+
 DeviceState *get_boot_device(uint32_t position)
 {
     uint32_t counter = 0;
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 2/7] bootindex: add del_boot_device_path function

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Introduce a del_boot_device_path() cleanup fw_cfg content
when hot-unplugging devcie refer to bootindex.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 include/sysemu/sysemu.h |  1 +
 vl.c                    | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e1b0659..7a79ff4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -209,6 +209,7 @@ void usb_info(Monitor *mon, const QDict *qdict);
 
 void add_boot_device_path(int32_t bootindex, DeviceState *dev,
                           const char *suffix);
+void del_boot_device_path(DeviceState *dev);
 void modify_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 83d7dc4..ce943ec 100644
--- a/vl.c
+++ b/vl.c
@@ -1248,6 +1248,23 @@ void add_boot_device_path(int32_t bootindex, DeviceState *dev,
     QTAILQ_INSERT_TAIL(&fw_boot_order, node, link);
 }
 
+void del_boot_device_path(DeviceState *dev)
+{
+    FWBootEntry *i;
+
+    assert(dev != NULL);
+
+    QTAILQ_FOREACH(i, &fw_boot_order, link) {
+        if (!strcmp(i->dev->id, dev->id)) {
+            /* remove all entries of the assigend dev */
+            QTAILQ_REMOVE(&fw_boot_order, i, link);
+            g_free(i->suffix);
+            g_free(i);
+            break;
+        }
+    }
+}
+
 void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
                           const char *suffix)
 {
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 3/7] fw_cfg: add fw_cfg_machine_reset function

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

We must assure that the changed bootindex can take effect
when guest is rebooted. So we introduce fw_cfg_machine_reset(),
which change the fw_cfg file's bootindex data using the new
global fw_boot_order list.

Signed-off-by: Chenliang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/nvram/fw_cfg.c         | 54 +++++++++++++++++++++++++++++++++++++++++------
 include/hw/nvram/fw_cfg.h |  2 ++
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b71d251..a24a44d 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -56,7 +56,6 @@ struct FWCfgState {
     FWCfgFiles *files;
     uint16_t cur_entry;
     uint32_t cur_offset;
-    Notifier machine_ready;
 };
 
 #define JPG_FILE 0
@@ -402,6 +401,26 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
     s->entries[arch][key].callback_opaque = callback_opaque;
 }
 
+static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
+                                              void *data, size_t len)
+{
+    void *ptr;
+    int arch = !!(key & FW_CFG_ARCH_LOCAL);
+
+    key &= FW_CFG_ENTRY_MASK;
+
+    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
+
+    /* return the old data to the function caller, avoid memory leak */
+    ptr = s->entries[arch][key].data;
+    s->entries[arch][key].data = data;
+    s->entries[arch][key].len = len;
+    s->entries[arch][key].callback_opaque = NULL;
+    s->entries[arch][key].callback = NULL;
+
+    return ptr;
+}
+
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
 {
     fw_cfg_add_bytes_read_callback(s, key, NULL, NULL, data, len);
@@ -499,13 +518,36 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
     fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len);
 }
 
-static void fw_cfg_machine_ready(struct Notifier *n, void *data)
+void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
+                        void *data, size_t len)
+{
+    int i, index;
+
+    assert(s->files);
+
+    index = be32_to_cpu(s->files->count);
+    assert(index < FW_CFG_FILE_SLOTS);
+
+    for (i = 0; i < index; i++) {
+        if (strcmp(filename, s->files->f[i].name) == 0) {
+            return fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
+                                     data, len);
+        }
+    }
+    /* add new one */
+    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len);
+    return NULL;
+}
+
+static void fw_cfg_machine_reset(void *opaque)
 {
+    void *ptr;
     size_t len;
-    FWCfgState *s = container_of(n, FWCfgState, machine_ready);
+    FWCfgState *s = opaque;
     char *bootindex = get_boot_devices_list(&len, false);
 
-    fw_cfg_add_file(s, "bootorder", (uint8_t*)bootindex, len);
+    ptr = fw_cfg_modify_file(s, "bootorder", (uint8_t *)bootindex, len);
+    g_free(ptr);
 }
 
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
@@ -542,9 +584,7 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
     fw_cfg_bootsplash(s);
     fw_cfg_reboot(s);
 
-    s->machine_ready.notify = fw_cfg_machine_ready;
-    qemu_add_machine_init_done_notifier(&s->machine_ready);
-
+    qemu_register_reset(fw_cfg_machine_reset, s);
     return s;
 }
 
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 72b1549..56e1ed7 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -76,6 +76,8 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len);
+void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
+                         size_t len);
 FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
                         hwaddr crl_addr, hwaddr data_addr);
 
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device is removed

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Device should be removed from global boot list when
it is hot-unplugged.

Signed-off-by: Chenliang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/block/virtio-blk.c    | 1 +
 hw/i386/kvm/pci-assign.c | 1 +
 hw/misc/vfio.c           | 1 +
 hw/net/e1000.c           | 1 +
 hw/net/eepro100.c        | 1 +
 hw/net/ne2000.c          | 1 +
 hw/net/rtl8139.c         | 1 +
 hw/net/virtio-net.c      | 1 +
 hw/net/vmxnet3.c         | 1 +
 hw/scsi/scsi-generic.c   | 1 +
 hw/usb/dev-network.c     | 1 +
 hw/usb/host-libusb.c     | 1 +
 hw/usb/redirect.c        | 1 +
 13 files changed, 13 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index c241c50..49813d5 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -786,6 +786,7 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBlock *s = VIRTIO_BLK(dev);
 
+    del_boot_device_path(dev);
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     remove_migration_state_change_notifier(&s->migration_state_notifier);
     virtio_blk_data_plane_destroy(s->dataplane);
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index de33657..1322479 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1853,6 +1853,7 @@ static void assigned_exitfn(struct PCIDevice *pci_dev)
 {
     AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
 
+    del_boot_device_path(&pci_dev->qdev);
     deassign_device(dev);
     free_assigned_device(dev);
 }
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 0b9eba0..f891312 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -4304,6 +4304,7 @@ static void vfio_exitfn(PCIDevice *pdev)
     VFIODevice *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
     VFIOGroup *group = vdev->group;
 
+    del_boot_device_path(&pdev->qdev);
     vfio_unregister_err_notifier(vdev);
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_disable_interrupts(vdev);
diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0fc29a0..fa4e858 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1492,6 +1492,7 @@ pci_e1000_uninit(PCIDevice *dev)
 {
     E1000State *d = E1000(dev);
 
+    del_boot_device_path(DEVICE(dev));
     timer_del(d->autoneg_timer);
     timer_free(d->autoneg_timer);
     timer_del(d->mit_timer);
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index 3263e3f..62951e4 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1843,6 +1843,7 @@ static void pci_nic_uninit(PCIDevice *pci_dev)
 {
     EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
 
+    del_boot_device_path(&pci_dev->qdev);
     memory_region_destroy(&s->mmio_bar);
     memory_region_destroy(&s->io_bar);
     memory_region_destroy(&s->flash_bar);
diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index d558b8c..62afa65 100644
--- a/hw/net/ne2000.c
+++ b/hw/net/ne2000.c
@@ -748,6 +748,7 @@ static void pci_ne2000_exit(PCIDevice *pci_dev)
     PCINE2000State *d = DO_UPCAST(PCINE2000State, dev, pci_dev);
     NE2000State *s = &d->ne2000;
 
+    del_boot_device_path(&pci_dev->qdev);
     memory_region_destroy(&s->io);
     qemu_del_nic(s->nic);
     qemu_free_irq(s->irq);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 90bc5ec..b34c91a 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -3462,6 +3462,7 @@ static void pci_rtl8139_uninit(PCIDevice *dev)
 {
     RTL8139State *s = RTL8139(dev);
 
+    del_boot_device_path(DEVICE(dev));
     memory_region_destroy(&s->bar_io);
     memory_region_destroy(&s->bar_mem);
     if (s->cplus_txbuffer) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 268eff9..0419575 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1654,6 +1654,7 @@ static void virtio_net_device_unrealize(DeviceState *dev, Error **errp)
     virtio_net_set_status(vdev, 0);
 
     unregister_savevm(dev, "virtio-net", n);
+    del_boot_device_path(dev);
 
     g_free(n->netclient_name);
     n->netclient_name = NULL;
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 77bea6f..c10d84e 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2176,6 +2176,7 @@ static void vmxnet3_pci_uninit(PCIDevice *pci_dev)
     VMW_CBPRN("Starting uninit...");
 
     unregister_savevm(dev, "vmxnet3-msix", s);
+    del_boot_device_path(dev);
 
     vmxnet3_net_uninit(s);
 
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 3733d2c..b270bc7 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -388,6 +388,7 @@ static void scsi_generic_reset(DeviceState *dev)
 
 static void scsi_destroy(SCSIDevice *s)
 {
+    del_boot_device_path(&s->qdev);
     scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE));
     blockdev_mark_auto_del(s->conf.bs);
 }
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 518d536..be39802 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1331,6 +1331,7 @@ static void usb_net_handle_destroy(USBDevice *dev)
     /* TODO: remove the nd_table[] entry */
     rndis_clear_responsequeue(s);
     qemu_del_nic(s->nic);
+    del_boot_device_path(&dev->qdev);
 }
 
 static NetClientInfo net_usbnet_info = {
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index c189147..919a0d2 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -984,6 +984,7 @@ static void usb_host_handle_destroy(USBDevice *udev)
 {
     USBHostDevice *s = USB_HOST_DEVICE(udev);
 
+    del_boot_device_path(&udev->qdev);
     qemu_remove_exit_notifier(&s->exit);
     QTAILQ_REMOVE(&hostdevs, s, next);
     usb_host_close(s);
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 44522d9..33df3af 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1416,6 +1416,7 @@ static void usbredir_handle_destroy(USBDevice *udev)
 {
     USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
 
+    del_boot_device_path(&udev->qdev);
     qemu_chr_delete(dev->cs);
     dev->cs = NULL;
     /* Note must be done after qemu_chr_close, as that causes a close event */
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.

Example QMP command:
-> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1", "bootindex": 1, "suffix": "/disk@0"}}
<- { "return": {} }

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 qapi-schema.json | 16 ++++++++++++++++
 qmp-commands.hx  | 24 ++++++++++++++++++++++++
 qmp.c            | 17 +++++++++++++++++
 3 files changed, 57 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index b11aad2..a9ef0be 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1704,6 +1704,22 @@
 { 'command': 'device_del', 'data': {'id': 'str'} }
 
 ##
+# @set-bootindex:
+#
+# set bootindex of a devcie
+#
+# @id: the name of the device
+# @bootindex: the bootindex of the device
+# @suffix: #optional a suffix of the device
+#
+# Returns: Nothing on success
+#          If @id is not a valid device, DeviceNotFound
+#
+# Since: 2.2
+##
+{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex': 'int', '*suffix': 'str'} }
+
+##
 # @DumpGuestMemoryFormat:
 #
 # An enumeration of guest-memory-dump's format.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 4be4765..2c89a97 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -330,6 +330,30 @@ Example:
 <- { "return": {} }
 
 EQMP
+    {
+        .name       = "set-bootindex",
+        .args_type  = "id:s,bootindex:l,suffix:s?",
+        .mhandler.cmd_new = qmp_marshal_input_set_bootindex,
+    },
+
+SQMP
+set-bootindex
+--------------------
+
+Set bootindex of a device
+
+Arguments:
+
+- "id": the device's ID (json-string)
+- "bootindex": the device's bootindex (json-int)
+- "suffix": the device's suffix in global boot list (json-string, optional)
+
+Example:
+
+-> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1", "bootindex": 1, "suffix": "/disk@0"}}
+<- { "return": {} }
+
+EQMP
 
     {
         .name       = "send-key",
diff --git a/qmp.c b/qmp.c
index 0d2553a..f2c3c14 100644
--- a/qmp.c
+++ b/qmp.c
@@ -684,6 +684,23 @@ void qmp_object_del(const char *id, Error **errp)
     object_unparent(obj);
 }
 
+void qmp_set_bootindex(const char *id, int64_t bootindex,
+                     bool has_suffix, const char *suffix, Error **errp)
+{
+    DeviceState *dev;
+
+    dev = qdev_find_recursive(sysbus_get_default(), id);
+    if (NULL == dev) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, id);
+        return;
+    }
+    if (has_suffix) {
+        modify_boot_device_path(bootindex, dev, suffix);
+    } else {
+        modify_boot_device_path(bootindex, dev, NULL);
+    }
+}
+
 MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
 {
     MemoryDeviceInfoList *head = NULL;
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 6/7] qemu-monitor: HMP set-bootindex wrapper

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Add HMP set-bootindex wrapper to allow setting
devcie's bootindex via monitor.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Chenliang <chenliang88@huawei.com>
---
 hmp-commands.hx | 15 +++++++++++++++
 hmp.c           | 13 +++++++++++++
 hmp.h           |  1 +
 3 files changed, 29 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d0943b1..31ef24e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -688,6 +688,21 @@ Remove device @var{id}.
 ETEXI
 
     {
+        .name       = "set-bootindex",
+        .args_type  = "id:s,bootindex:l,suffix:s?",
+        .params     = "device bootindex [suffix]",
+        .help       = "set bootindex of a device(e.g. set-bootindex disk0 1 '/disk@0')",
+        .mhandler.cmd = hmp_set_bootindex,
+    },
+
+STEXI
+@item set-bootindex @var{id} @var{bootindex}
+@findex set-bootindex
+
+Set bootindex of a device.
+ETEXI
+
+    {
         .name       = "cpu",
         .args_type  = "index:i",
         .params     = "index",
diff --git a/hmp.c b/hmp.c
index 4d1838e..95f7eeb 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1714,3 +1714,16 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
 
     monitor_printf(mon, "\n");
 }
+
+void hmp_set_bootindex(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+
+    const char *id = qdict_get_str(qdict, "id");
+    int64_t bootindex = qdict_get_int(qdict, "bootindex");
+    bool has_suffix = qdict_haskey(qdict, "suffix");
+    const char *suffix = qdict_get_try_str(qdict, "suffix");
+
+    qmp_set_bootindex(id, bootindex, has_suffix, suffix, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 4fd3c4a..eb2641a 100644
--- a/hmp.h
+++ b/hmp.h
@@ -94,6 +94,7 @@ void hmp_cpu_add(Monitor *mon, const QDict *qdict);
 void hmp_object_add(Monitor *mon, const QDict *qdict);
 void hmp_object_del(Monitor *mon, const QDict *qdict);
 void hmp_info_memdev(Monitor *mon, const QDict *qdict);
+void hmp_set_bootindex(Monitor *mon, const QDict *qdict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
 void device_add_completion(ReadLineState *rs, int nb_args, const char *str);
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 7/7] spapr: fix possible memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

get_boot_devices_list() will malloc memory, spapr_finalize_fdt
doesn't free it.

Signed-off-by: Chenliang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/ppc/spapr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d01978f..edff5ce 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -745,6 +745,7 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
 
     cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt));
 
+    g_free(bootlist);
     g_free(fdt);
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gerd Hoffmann]

  Hi,

> +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> +                          const char *suffix)
> +{
> +    FWBootEntry *node, *i;
> +
> +    assert(dev != NULL || suffix != NULL);
> +
> +    QTAILQ_FOREACH(i, &fw_boot_order, link) {
> +        if (i->bootindex == bootindex) {
> +            qerror_report(ERROR_CLASS_GENERIC_ERROR,
> +               "The bootindex %d has already been used", bootindex);
> +            return;
> +        }
> +        /* delete the same original dev */
> +        if (i->dev->id && !strcmp(i->dev->id, dev->id)) {
> +            QTAILQ_REMOVE(&fw_boot_order, i, link);

Ok ...

> +            g_free(i->suffix);
> +            g_free(i);

... but you should free the old entry later ...

> +
> +            break;
> +        }
> +    }
> +
> +    if (bootindex >= 0) {
> +        node = g_malloc0(sizeof(FWBootEntry));
> +        node->bootindex = bootindex;
> +        node->suffix = g_strdup(suffix);

... because you can just copy the suffix from the old entry here,
instead of expecting the caller pass it in.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device is removed

[Gerd Hoffmann]

 Hi,

> +    del_boot_device_path(dev);

You can call this from device_finalize() instead of placing it into each
individual device.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device is removed

[Gonglei (Arei)]

Hi, Gerd

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Friday, July 25, 2014 5:52 PM
> Subject: Re: [PATCH v2 4/7] bootindex: delete bootindex when device is
> removed
> 
>  Hi,
> 
> > +    del_boot_device_path(dev);
> 
> You can call this from device_finalize() instead of placing it into each
> individual device.
> 
Good idea! Thanks very much.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi, Gerd

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Friday, July 25, 2014 5:46 PM
> 
>   Hi,
> 
> > +void modify_boot_device_path(int32_t bootindex, DeviceState *dev,
> > +                          const char *suffix)
> > +{
> > +    FWBootEntry *node, *i;
> > +
> > +    assert(dev != NULL || suffix != NULL);
> > +
> > +    QTAILQ_FOREACH(i, &fw_boot_order, link) {
> > +        if (i->bootindex == bootindex) {
> > +            qerror_report(ERROR_CLASS_GENERIC_ERROR,
> > +               "The bootindex %d has already been used", bootindex);
> > +            return;
> > +        }
> > +        /* delete the same original dev */
> > +        if (i->dev->id && !strcmp(i->dev->id, dev->id)) {
> > +            QTAILQ_REMOVE(&fw_boot_order, i, link);
> 
> Ok ...
> 
> > +            g_free(i->suffix);
> > +            g_free(i);
> 
> ... but you should free the old entry later ...
> 
> > +
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (bootindex >= 0) {
> > +        node = g_malloc0(sizeof(FWBootEntry));
> > +        node->bootindex = bootindex;
> > +        node->suffix = g_strdup(suffix);
> 
> ... because you can just copy the suffix from the old entry here,
> instead of expecting the caller pass it in.
> 
Okay, agreed.

But we should also think about the situation which a device don't have
old entry in global fw_boot_order list. So we can do like this:

if (suffix) {
    node->suffix = g_strdup(suffix);
} else if (has_old_entry) {
	node->suffix = g_strdup(old_entry->suffix);
} else {
	node->suffix = NULL;
}

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gerd Hoffmann]

  Hi,

> > ... because you can just copy the suffix from the old entry here,
> > instead of expecting the caller pass it in.
> > 
> Okay, agreed.
> 
> But we should also think about the situation which a device don't have
> old entry in global fw_boot_order list.

Throw an error?

I think it is ok to allow only *changing* the bootindex.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Monday, July 28, 2014 4:31 PM
> Subject: Re: [PATCH v2 1/7] bootindex: add modify_boot_device_path function
> 
>   Hi,
> 
> > > ... because you can just copy the suffix from the old entry here,
> > > instead of expecting the caller pass it in.
> > >
> > Okay, agreed.
> >
> > But we should also think about the situation which a device don't have
> > old entry in global fw_boot_order list.
> 
> Throw an error?
> 
No. I should firstly use the suffix which the caller pass in IMHO,
just like V3 I have posted, Thanks.

> I think it is ok to allow only *changing* the bootindex.
> 
Yes, that's no problem.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gerd Hoffmann]

> > I think it is ok to allow only *changing* the bootindex.
> > 
> Yes, that's no problem.

But then yoy always  will have a old entry where you can take the suffix
from, and you don't need the suffix as parameter for the monitor
command.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Monday, July 28, 2014 5:28 PM
> Subject: Re: [PATCH v2 1/7] bootindex: add modify_boot_device_path function
> 
> > > I think it is ok to allow only *changing* the bootindex.
> > >
> > Yes, that's no problem.
> 
> But then yoy always  will have a old entry where you can take the suffix
> from, and you don't need the suffix as parameter for the monitor
> command.
> 
No, optional. 

Because the bootindex property is not a necessary property for devices.
If a device, such as IDE cdrom haven't attach the bootindex in qemu booting 
command line, the global fw_boot_order will not have its entry. So, we should
save the suffix as parameter.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gerd Hoffmann]

  Hi,

> > > > I think it is ok to allow only *changing* the bootindex.
> > > >
> > > Yes, that's no problem.
> > 
> > But then yoy always  will have a old entry where you can take the suffix
> > from, and you don't need the suffix as parameter for the monitor
> > command.
> > 
> No, optional. 

> Because the bootindex property is not a necessary property for devices.
> If a device, such as IDE cdrom haven't attach the bootindex in qemu booting 
> command line, the global fw_boot_order will not have its entry.

I'd suggest to simply not support this and throw an error then.

> So, we should
> save the suffix as parameter.

I think it is a bad idea to have the suffix as monitor command
parameter.  It is a implementation detail which the monitor users should
not have to worry about.

Easiest way to do this is to allow *changing* an existing bootindex
entry only, and not support *adding* new boot entries.  The user has to
set a bootindex for any device it might want to boot from in the future
then.  I think this is acceptable.

If you want support adding bootentries at runtime (and it probably makes
sense to support removing entries too in that case) the suffix handling
should be reworked.  The suffix could be stored in DeviceState instead
of being passed to the add_bootentry function, so you can add boot
entries later on without expecting the user to know what the correct
suffix is.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Monday, July 28, 2014 6:02 PM
> Subject: Re: [PATCH v2 1/7] bootindex: add modify_boot_device_path function
> 
>   Hi,
> 
> > > > > I think it is ok to allow only *changing* the bootindex.
> > > > >
> > > > Yes, that's no problem.
> > >
> > > But then yoy always  will have a old entry where you can take the suffix
> > > from, and you don't need the suffix as parameter for the monitor
> > > command.
> > >
> > No, optional.
> 
> > Because the bootindex property is not a necessary property for devices.
> > If a device, such as IDE cdrom haven't attach the bootindex in qemu booting
> > command line, the global fw_boot_order will not have its entry.
> 
> I'd suggest to simply not support this and throw an error then.
> 
Ok.

> > So, we should
> > save the suffix as parameter.
> 
> I think it is a bad idea to have the suffix as monitor command
> parameter.  It is a implementation detail which the monitor users should
> not have to worry about.
> 
Yes. Actually I also have this misgivings.

> Easiest way to do this is to allow *changing* an existing bootindex
> entry only, and not support *adding* new boot entries.  The user has to
> set a bootindex for any device it might want to boot from in the future
> then.  I think this is acceptable.
> 
Hmm..

> If you want support adding bootentries at runtime (and it probably makes
> sense to support removing entries too in that case) the suffix handling
> should be reworked.  The suffix could be stored in DeviceState instead
> of being passed to the add_bootentry function, so you can add boot
> entries later on without expecting the user to know what the correct
> suffix is.
> 
That's a good idea! I think this is a good improvement. 

Any other comments? Thanks!

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 7/7] spapr: fix possible memory leak

[Alexander Graf]

On 25.07.14 08:52, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> get_boot_devices_list() will malloc memory, spapr_finalize_fdt
> doesn't free it.
>
> Signed-off-by: Chenliang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

Thanks, applied to ppc-next-2.2.


Alex

Re: [Qemu-devel] [PATCH v2 7/7] spapr: fix possible memory leak

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Monday, July 28, 2014 6:45 PM
> Subject: Re: [PATCH v2 7/7] spapr: fix possible memory leak
> 
> 
> On 25.07.14 08:52, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > get_boot_devices_list() will malloc memory, spapr_finalize_fdt
> > doesn't free it.
> >
> > Signed-off-by: Chenliang <chenliang88@huawei.com>
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> 
> Thanks, applied to ppc-next-2.2.
> 
Thanks, Alex.

Then I will not include this patch in my next version of the
patch serials about bootindex.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Monday, July 28, 2014 6:16 PM
> Subject: RE: [PATCH v2 1/7] bootindex: add modify_boot_device_path function
> 
> > -----Original Message-----
> > From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> > Sent: Monday, July 28, 2014 6:02 PM
> > Subject: Re: [PATCH v2 1/7] bootindex: add modify_boot_device_path function
> >
> >   Hi,
> >
> > > > > > I think it is ok to allow only *changing* the bootindex.
> > > > > >
> > > > > Yes, that's no problem.
> > > >
> > > > But then yoy always  will have a old entry where you can take the suffix
> > > > from, and you don't need the suffix as parameter for the monitor
> > > > command.
> > > >
> > > No, optional.
> >
> > > Because the bootindex property is not a necessary property for devices.
> > > If a device, such as IDE cdrom haven't attach the bootindex in qemu booting
> > > command line, the global fw_boot_order will not have its entry.
> >
> > I'd suggest to simply not support this and throw an error then.
> >
> Ok.
> 
> > > So, we should
> > > save the suffix as parameter.
> >
> > I think it is a bad idea to have the suffix as monitor command
> > parameter.  It is a implementation detail which the monitor users should
> > not have to worry about.
> >
> Yes. Actually I also have this misgivings.
> 
> > Easiest way to do this is to allow *changing* an existing bootindex
> > entry only, and not support *adding* new boot entries.  The user has to
> > set a bootindex for any device it might want to boot from in the future
> > then.  I think this is acceptable.
> >
> Hmm..
> 
> > If you want support adding bootentries at runtime (and it probably makes
> > sense to support removing entries too in that case) the suffix handling
> > should be reworked.  The suffix could be stored in DeviceState instead
> > of being passed to the add_bootentry function, so you can add boot
> > entries later on without expecting the user to know what the correct
> > suffix is.
> >
> That's a good idea! I think this is a good improvement.
> 
> Any other comments? Thanks!
> 
Maybe we should save the suffix as parameter in add_boot_device_path()

There are two reasons:

1. the floppy device may have two bootindex, which configure as below:
 " -global isa-fdc.driveA=drive-fdc0-0-0, bootindexA=xxx \ 
  -global isa-fdc.driveB=drive-fdc0-0-1 bootindexB=xxx \"
 We can see it in isabus_fdc_realize() [hw/block/fdc.c]

2. The option rom don't need pass dev to add_bootentry, 
  which realized in rom_add_file() [hw/core/loader.c]
  
in those situation, we have to pass suffix to add_bootentry function.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 1/7] bootindex: add modify_boot_device_path function

[Gonglei (Arei)]

Hi, Gerd

> > > > > > > I think it is ok to allow only *changing* the bootindex.
> > > > > > >
> > > > > > Yes, that's no problem.
> > > > >
> > > > > But then yoy always  will have a old entry where you can take the suffix
> > > > > from, and you don't need the suffix as parameter for the monitor
> > > > > command.
> > > > >
> > > > No, optional.
> > >
> > > > Because the bootindex property is not a necessary property for devices.
> > > > If a device, such as IDE cdrom haven't attach the bootindex in qemu
> booting
> > > > command line, the global fw_boot_order will not have its entry.
> > >
> > > I'd suggest to simply not support this and throw an error then.
> > >
> > Ok.
> >
> > > > So, we should
> > > > save the suffix as parameter.
> > >
> > > I think it is a bad idea to have the suffix as monitor command
> > > parameter.  It is a implementation detail which the monitor users should
> > > not have to worry about.
> > >
> > Yes. Actually I also have this misgivings.
> >
> > > Easiest way to do this is to allow *changing* an existing bootindex
> > > entry only, and not support *adding* new boot entries.  The user has to
> > > set a bootindex for any device it might want to boot from in the future
> > > then.  I think this is acceptable.
> > >
> > Hmm..
> >
> > > If you want support adding bootentries at runtime (and it probably makes
> > > sense to support removing entries too in that case) the suffix handling
> > > should be reworked.  The suffix could be stored in DeviceState instead
> > > of being passed to the add_bootentry function, so you can add boot
> > > entries later on without expecting the user to know what the correct
> > > suffix is.
> > >
> > That's a good idea! I think this is a good improvement.
> >
> > Any other comments? Thanks!
> >
> Maybe we should save the suffix as parameter in add_boot_device_path()
> 
> There are two reasons:
> 
> 1. the floppy device may have two bootindex, which configure as below:
>  " -global isa-fdc.driveA=drive-fdc0-0-0, bootindexA=xxx \
>   -global isa-fdc.driveB=drive-fdc0-0-1 bootindexB=xxx \"
>  We can see it in isabus_fdc_realize() [hw/block/fdc.c]
> 
> 2. The option rom don't need pass dev to add_bootentry,
>   which realized in rom_add_file() [hw/core/loader.c]
> 
> in those situation, we have to pass suffix to add_bootentry function.
> 
In addition, because an isa-fdc device can be configured two drive,
we have to pass the suffix for changing floppy's bootindex. Both del_bootentry
and add_bootentry need suffix to confirm the unique device.

Because the suffix of bootindex is invisible for common user. Maybe we 
can introduce a query interface for user. Such as hmp monitor command
"info bootindex", which show the information of bootindex have configured,
includeing suffix. But the suffix is optional, maybe just useful for floppy disk,
that will be not a serious or trouble problem IMHO. 

So, I want to do those in the next version:
- save the suffix as optional continue.
- allow changing the existing bootindex entry only, 
 not support adding new boot entries.
- rework modify_boot_device_path(), add checking logic for the suffix
 when remove old bootindex entry from global boot order list. 

I'm looking forward to your reply, thanks!

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device is removed

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Friday, July 25, 2014 5:52 PM
> 
> > +    del_boot_device_path(dev);
> 
> You can call this from device_finalize() instead of placing it into each
> individual device.
> 
Maybe put this call in device_finalize is not a good idea. 
I have three reasons:

1. the device's some memory have been freed before call device_finalize,
 such as device->id. It is too later.
2. not every kinds of device can configure bootindex property, such as usb
 host adapters. It is a waste and useless for those devices. This is the
 main reason.
3. virtio-net device's parent is virtio-pci device, which configured id property,
 But the device saved in global fw_boot_order list is virtio-net device have not
 id property. If we put call del_boot_device_path(dev) in virtio_net_device_unrealize
 we can delete it from fw_boot_order directly.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device is removed

[Eduardo Habkost]

On Wed, Jul 30, 2014 at 07:29:56AM +0000, Gonglei (Arei) wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> > Sent: Friday, July 25, 2014 5:52 PM
> > 
> > > +    del_boot_device_path(dev);
> > 
> > You can call this from device_finalize() instead of placing it into each
> > individual device.
> > 
> Maybe put this call in device_finalize is not a good idea. 
> I have three reasons:
> 
> 1. the device's some memory have been freed before call device_finalize,
>  such as device->id. It is too later.

I don't think you even need id. See my reply to v4 2/8.

But you have a point about being too late: some devices call
add_boot_device_path() on realize, so those would need to revert the
operation on unrealize; others do it on init, so they need to do it on
finalize.

On either case, I believe an extra check inside device_finalize()
wouldn't hurt, even if it becomes redundant on some devices.


> 2. not every kinds of device can configure bootindex property, such as usb
>  host adapters. It is a waste and useless for those devices. This is the
>  main reason.

I would prefer to waste a few cycles scanning the boot index list every
time a device is removed, than risking crashing QEMU in case somebody
forget to add a del_boot_device_path() call.


> 3. virtio-net device's parent is virtio-pci device, which configured id property,
>  But the device saved in global fw_boot_order list is virtio-net device have not
>  id property. If we put call del_boot_device_path(dev) in virtio_net_device_unrealize
>  we can delete it from fw_boot_order directly.

Sorry, I don't understand what you mean here. If virtio-net doesn't have
an id property, would the current version of del_boot_device_path() even
work?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Eduardo Habkost]

On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
> 
> Example QMP command:
> -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1", "bootindex": 1, "suffix": "/disk@0"}}
> <- { "return": {} }
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Chenliang <chenliang88@huawei.com>
> ---
>  qapi-schema.json | 16 ++++++++++++++++
>  qmp-commands.hx  | 24 ++++++++++++++++++++++++
>  qmp.c            | 17 +++++++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b11aad2..a9ef0be 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1704,6 +1704,22 @@
>  { 'command': 'device_del', 'data': {'id': 'str'} }
>  
>  ##
> +# @set-bootindex:
> +#
> +# set bootindex of a devcie
> +#
> +# @id: the name of the device
> +# @bootindex: the bootindex of the device
> +# @suffix: #optional a suffix of the device
> +#
> +# Returns: Nothing on success
> +#          If @id is not a valid device, DeviceNotFound
> +#
> +# Since: 2.2
> +##
> +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex': 'int', '*suffix': 'str'} }
> +
> +##

I wonder if we could simply use qom-set for that. How many devices
actually support having multiple bootindex entries with different
suffixes?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device is removed

[Gonglei (Arei)]

Hi,

> > >
> > > > +    del_boot_device_path(dev);
> > >
> > > You can call this from device_finalize() instead of placing it into each
> > > individual device.
> > >
> > Maybe put this call in device_finalize is not a good idea.
> > I have three reasons:
> >
> > 1. the device's some memory have been freed before call device_finalize,
> >  such as device->id. It is too later.
> 
> I don't think you even need id. See my reply to v4 2/8.
> 
> But you have a point about being too late: some devices call
> add_boot_device_path() on realize, so those would need to revert the
> operation on unrealize; others do it on init, so they need to do it on
> finalize.
> 
> On either case, I believe an extra check inside device_finalize()
> wouldn't hurt, even if it becomes redundant on some devices.
> 
> 
OK. And I copy your review from v3 2/7, as follows:

> 
> What if the device doesn't have any ID set? I don't see anything on
> add_boot_device_path() ensuring that dev->id is always set.
> 
Yes, the id is not always set. So, I add a check in V4.

> Why you don't just check if i->dev == dev?
>
No, if we check directly i->dev == dev, we will not handle the virtio devices.

For example, the common user configure a virtio-net nic using command line
like " -device virtio-net-pci,netdev=net0,bootindex=3,id=nic1 ". Then the id property
will be added for virtio-net-pci device, not virtio-net device which stored in the global
fw_boot_order list. So, the i->dev

When common users want to change the bootindex of virtio-net. They only are concerned 
that they have configured an id for virtio-net nic card. So, they can pass the id to QEMU. But
we should handle those scenes, meanwhile the device object gained by id is virtio-net-pci device
not equals i->dev.

> > 2. not every kinds of device can configure bootindex property, such as usb
> >  host adapters. It is a waste and useless for those devices. This is the
> >  main reason.
> 
> I would prefer to waste a few cycles scanning the boot index list every
> time a device is removed, than risking crashing QEMU in case somebody
> forget to add a del_boot_device_path() call.
> 
OK, fine!

Maybe I should do this in device_finalize() as Gerd's previous suggestion, 
like yours. Thanks.

> 
> > 3. virtio-net device's parent is virtio-pci device, which configured id property,
> >  But the device saved in global fw_boot_order list is virtio-net device have
> not
> >  id property. If we put call del_boot_device_path(dev) in
> virtio_net_device_unrealize
> >  we can delete it from fw_boot_order directly.
> 
> Sorry, I don't understand what you mean here. If virtio-net doesn't have
> an id property, would the current version of del_boot_device_path() even
> work?
> 
Please see above comments.

Thanks for your review!

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Gonglei (Arei)]

Hi,

> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command
> 
> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
> >
> > Example QMP command:
> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1", "bootindex":
> 1, "suffix": "/disk@0"}}
> > <- { "return": {} }
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > Signed-off-by: Chenliang <chenliang88@huawei.com>
> > ---
> >  qapi-schema.json | 16 ++++++++++++++++
> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
> >  qmp.c            | 17 +++++++++++++++++
> >  3 files changed, 57 insertions(+)
> >
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index b11aad2..a9ef0be 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1704,6 +1704,22 @@
> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> >
> >  ##
> > +# @set-bootindex:
> > +#
> > +# set bootindex of a devcie
> > +#
> > +# @id: the name of the device
> > +# @bootindex: the bootindex of the device
> > +# @suffix: #optional a suffix of the device
> > +#
> > +# Returns: Nothing on success
> > +#          If @id is not a valid device, DeviceNotFound
> > +#
> > +# Since: 2.2
> > +##
> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex': 'int', '*suffix':
> 'str'} }
> > +
> > +##
> 
> I wonder if we could simply use qom-set for that. How many devices
> actually support having multiple bootindex entries with different
> suffixes?
> 
AFAICT, the floppy device support two bootindex with different suffixes.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Markus Armbruster]

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

> Hi,
>
>> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command
>> 
>> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com wrote:
>> > From: Gonglei <arei.gonglei@huawei.com>
>> >
>> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
>> >
>> > Example QMP command:
>> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
>> > "bootindex":
>> 1, "suffix": "/disk@0"}}
>> > <- { "return": {} }
>> >
>> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> > Signed-off-by: Chenliang <chenliang88@huawei.com>
>> > ---
>> >  qapi-schema.json | 16 ++++++++++++++++
>> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
>> >  qmp.c            | 17 +++++++++++++++++
>> >  3 files changed, 57 insertions(+)
>> >
>> > diff --git a/qapi-schema.json b/qapi-schema.json
>> > index b11aad2..a9ef0be 100644
>> > --- a/qapi-schema.json
>> > +++ b/qapi-schema.json
>> > @@ -1704,6 +1704,22 @@
>> >  { 'command': 'device_del', 'data': {'id': 'str'} }
>> >
>> >  ##
>> > +# @set-bootindex:
>> > +#
>> > +# set bootindex of a devcie
>> > +#
>> > +# @id: the name of the device
>> > +# @bootindex: the bootindex of the device
>> > +# @suffix: #optional a suffix of the device
>> > +#
>> > +# Returns: Nothing on success
>> > +#          If @id is not a valid device, DeviceNotFound
>> > +#
>> > +# Since: 2.2
>> > +##
>> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
>> > int', '*suffix':
>> 'str'} }
>> > +
>> > +##
>> 
>> I wonder if we could simply use qom-set for that. How many devices
>> actually support having multiple bootindex entries with different
>> suffixes?
>> 
> AFAICT, the floppy device support two bootindex with different suffixes.

Block device models commonly a single block device.  By convention,
property "drive" is the backend, and property "bootindex" the bootindex,
if the device model supports that.

The device ID suffices to identify a block device for such device
models.

The floppy device model is an exception.  It folds multiple real-world
objects into one: the controller and the actual drives.  Have a look at
-device isa-fdc,help:

    isa-fdc.iobase=uint32
    isa-fdc.irq=uint32
    isa-fdc.dma=uint32
    isa-fdc.driveA=drive
    isa-fdc.driveB=drive
    isa-fdc.bootindexA=int32
    isa-fdc.bootindexB=int32
    isa-fdc.check_media_rate=on/off

The properties ending with 'A' or 'B' apply to the first and the second
drive, the others to the controller.

Unfortunately, this means the device ID by itself doesn't identify the
floppy device.

Arguably, this is lousy modeling --- we really should model separate
real-world objects as separate objects.  But making floppies pretty (or
even sane) isn't anyone's priority nowadays.

Since qom-set works on *properties*, I can't see why it couldn't be used
for changing bootindexes.  There is no ambiguity even with floppy.
You either set bootindexA or bootindexB.  No need to fuzz around with
suffixes.  Or am I missing something?

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Gonglei (Arei)]

Hi,

> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command
> >>
> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com wrote:
> >> > From: Gonglei <arei.gonglei@huawei.com>
> >> >
> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
> >> >
> >> > Example QMP command:
> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
> >> > "bootindex":
> >> 1, "suffix": "/disk@0"}}
> >> > <- { "return": {} }
> >> >
> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> > Signed-off-by: Chenliang <chenliang88@huawei.com>
> >> > ---
> >> >  qapi-schema.json | 16 ++++++++++++++++
> >> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
> >> >  qmp.c            | 17 +++++++++++++++++
> >> >  3 files changed, 57 insertions(+)
> >> >
> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> > index b11aad2..a9ef0be 100644
> >> > --- a/qapi-schema.json
> >> > +++ b/qapi-schema.json
> >> > @@ -1704,6 +1704,22 @@
> >> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> >> >
> >> >  ##
> >> > +# @set-bootindex:
> >> > +#
> >> > +# set bootindex of a devcie
> >> > +#
> >> > +# @id: the name of the device
> >> > +# @bootindex: the bootindex of the device
> >> > +# @suffix: #optional a suffix of the device
> >> > +#
> >> > +# Returns: Nothing on success
> >> > +#          If @id is not a valid device, DeviceNotFound
> >> > +#
> >> > +# Since: 2.2
> >> > +##
> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
> >> > int', '*suffix':
> >> 'str'} }
> >> > +
> >> > +##
> >>
> >> I wonder if we could simply use qom-set for that. How many devices
> >> actually support having multiple bootindex entries with different
> >> suffixes?
> >>
> > AFAICT, the floppy device support two bootindex with different suffixes.
> 
> Block device models commonly a single block device.  By convention,
> property "drive" is the backend, and property "bootindex" the bootindex,
> if the device model supports that.
> 
> The device ID suffices to identify a block device for such device
> models.
> 
> The floppy device model is an exception.  It folds multiple real-world
> objects into one: the controller and the actual drives.  Have a look at
> -device isa-fdc,help:
> 
>     isa-fdc.iobase=uint32
>     isa-fdc.irq=uint32
>     isa-fdc.dma=uint32
>     isa-fdc.driveA=drive
>     isa-fdc.driveB=drive
>     isa-fdc.bootindexA=int32
>     isa-fdc.bootindexB=int32
>     isa-fdc.check_media_rate=on/off
> 
> The properties ending with 'A' or 'B' apply to the first and the second
> drive, the others to the controller.
> 
> Unfortunately, this means the device ID by itself doesn't identify the
> floppy device.
> 
Yes.

> Arguably, this is lousy modeling --- we really should model separate
> real-world objects as separate objects.  But making floppies pretty (or
> even sane) isn't anyone's priority nowadays.
> 
Hmm... Agreed.

> Since qom-set works on *properties*, I can't see why it couldn't be used
> for changing bootindexes.  There is no ambiguity even with floppy.

Sorry? 

> You either set bootindexA or bootindexB.  No need to fuzz around with
> suffixes.  Or am I missing something?

Your mean that we just need to think about change one bootindex? Either 
set bootindexA or bootindexB, do not need pass suffix for identifying?

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Markus Armbruster]

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

> Hi,
>
>> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command
>> >>
>> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com wrote:
>> >> > From: Gonglei <arei.gonglei@huawei.com>
>> >> >
>> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
>> >> >
>> >> > Example QMP command:
>> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
>> >> > "bootindex":
>> >> 1, "suffix": "/disk@0"}}
>> >> > <- { "return": {} }
>> >> >
>> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> >> > Signed-off-by: Chenliang <chenliang88@huawei.com>
>> >> > ---
>> >> >  qapi-schema.json | 16 ++++++++++++++++
>> >> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
>> >> >  qmp.c            | 17 +++++++++++++++++
>> >> >  3 files changed, 57 insertions(+)
>> >> >
>> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> > index b11aad2..a9ef0be 100644
>> >> > --- a/qapi-schema.json
>> >> > +++ b/qapi-schema.json
>> >> > @@ -1704,6 +1704,22 @@
>> >> >  { 'command': 'device_del', 'data': {'id': 'str'} }
>> >> >
>> >> >  ##
>> >> > +# @set-bootindex:
>> >> > +#
>> >> > +# set bootindex of a devcie
>> >> > +#
>> >> > +# @id: the name of the device
>> >> > +# @bootindex: the bootindex of the device
>> >> > +# @suffix: #optional a suffix of the device
>> >> > +#
>> >> > +# Returns: Nothing on success
>> >> > +#          If @id is not a valid device, DeviceNotFound
>> >> > +#
>> >> > +# Since: 2.2
>> >> > +##
>> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
>> >> > int', '*suffix':
>> >> 'str'} }
>> >> > +
>> >> > +##
>> >>
>> >> I wonder if we could simply use qom-set for that. How many devices
>> >> actually support having multiple bootindex entries with different
>> >> suffixes?
>> >>
>> > AFAICT, the floppy device support two bootindex with different suffixes.
>> 
>> Block device models commonly a single block device.  By convention,
>> property "drive" is the backend, and property "bootindex" the bootindex,
>> if the device model supports that.
>> 
>> The device ID suffices to identify a block device for such device
>> models.
>> 
>> The floppy device model is an exception.  It folds multiple real-world
>> objects into one: the controller and the actual drives.  Have a look at
>> -device isa-fdc,help:
>> 
>>     isa-fdc.iobase=uint32
>>     isa-fdc.irq=uint32
>>     isa-fdc.dma=uint32
>>     isa-fdc.driveA=drive
>>     isa-fdc.driveB=drive
>>     isa-fdc.bootindexA=int32
>>     isa-fdc.bootindexB=int32
>>     isa-fdc.check_media_rate=on/off
>> 
>> The properties ending with 'A' or 'B' apply to the first and the second
>> drive, the others to the controller.
>> 
>> Unfortunately, this means the device ID by itself doesn't identify the
>> floppy device.
>> 
> Yes.
>
>> Arguably, this is lousy modeling --- we really should model separate
>> real-world objects as separate objects.  But making floppies pretty (or
>> even sane) isn't anyone's priority nowadays.
>> 
> Hmm... Agreed.
>
>> Since qom-set works on *properties*, I can't see why it couldn't be used
>> for changing bootindexes.  There is no ambiguity even with floppy.
>
> Sorry? 
>
>> You either set bootindexA or bootindexB.  No need to fuzz around with
>> suffixes.  Or am I missing something?
>
> Your mean that we just need to think about change one bootindex? Either 
> set bootindexA or bootindexB, do not need pass suffix for identifying?

Eduardo suggested to think about using qom-set to update the bootindex.

Could look like

    { "execute": "qom-set",
      "arguments": { "path": "/machine/unattached/device[15]",
                     "property": "bootindexA", "value": 1 } }

Demonstrates an existing, well-defined way to identify the bootindex to
change.  Do we really want to invent another one, based on suffix?

The value of "path" is the QOM path.  I can't remember offhand how to go
from qdev ID to QOM path.  Onboard devices like isa-fdc don't have one
anyway.

I also don't remember whether there's a nicer QOM path than this one.

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Gonglei (Arei)]

Hi, Markus

> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex
> command
> >> >>
> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com
> wrote:
> >> >> > From: Gonglei <arei.gonglei@huawei.com>
> >> >> >
> >> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
> >> >> >
> >> >> > Example QMP command:
> >> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
> >> >> > "bootindex":
> >> >> 1, "suffix": "/disk@0"}}
> >> >> > <- { "return": {} }
> >> >> >
> >> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> >> > Signed-off-by: Chenliang <chenliang88@huawei.com>
> >> >> > ---
> >> >> >  qapi-schema.json | 16 ++++++++++++++++
> >> >> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
> >> >> >  qmp.c            | 17 +++++++++++++++++
> >> >> >  3 files changed, 57 insertions(+)
> >> >> >
> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> >> > index b11aad2..a9ef0be 100644
> >> >> > --- a/qapi-schema.json
> >> >> > +++ b/qapi-schema.json
> >> >> > @@ -1704,6 +1704,22 @@
> >> >> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> >> >> >
> >> >> >  ##
> >> >> > +# @set-bootindex:
> >> >> > +#
> >> >> > +# set bootindex of a devcie
> >> >> > +#
> >> >> > +# @id: the name of the device
> >> >> > +# @bootindex: the bootindex of the device
> >> >> > +# @suffix: #optional a suffix of the device
> >> >> > +#
> >> >> > +# Returns: Nothing on success
> >> >> > +#          If @id is not a valid device, DeviceNotFound
> >> >> > +#
> >> >> > +# Since: 2.2
> >> >> > +##
> >> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
> >> >> > int', '*suffix':
> >> >> 'str'} }
> >> >> > +
> >> >> > +##
> >> >>
> >> >> I wonder if we could simply use qom-set for that. How many devices
> >> >> actually support having multiple bootindex entries with different
> >> >> suffixes?
> >> >>
> >> > AFAICT, the floppy device support two bootindex with different suffixes.
> >>
> >> Block device models commonly a single block device.  By convention,
> >> property "drive" is the backend, and property "bootindex" the bootindex,
> >> if the device model supports that.
> >>
> >> The device ID suffices to identify a block device for such device
> >> models.
> >>
> >> The floppy device model is an exception.  It folds multiple real-world
> >> objects into one: the controller and the actual drives.  Have a look at
> >> -device isa-fdc,help:
> >>
> >>     isa-fdc.iobase=uint32
> >>     isa-fdc.irq=uint32
> >>     isa-fdc.dma=uint32
> >>     isa-fdc.driveA=drive
> >>     isa-fdc.driveB=drive
> >>     isa-fdc.bootindexA=int32
> >>     isa-fdc.bootindexB=int32
> >>     isa-fdc.check_media_rate=on/off
> >>
> >> The properties ending with 'A' or 'B' apply to the first and the second
> >> drive, the others to the controller.
> >>
> >> Unfortunately, this means the device ID by itself doesn't identify the
> >> floppy device.
> >>
> > Yes.
> >
> >> Arguably, this is lousy modeling --- we really should model separate
> >> real-world objects as separate objects.  But making floppies pretty (or
> >> even sane) isn't anyone's priority nowadays.
> >>
> > Hmm... Agreed.
> >
> >> Since qom-set works on *properties*, I can't see why it couldn't be used
> >> for changing bootindexes.  There is no ambiguity even with floppy.
> >
> > Sorry?
> >
> >> You either set bootindexA or bootindexB.  No need to fuzz around with
> >> suffixes.  Or am I missing something?
> >
> > Your mean that we just need to think about change one bootindex? Either
> > set bootindexA or bootindexB, do not need pass suffix for identifying?
> 
> Eduardo suggested to think about using qom-set to update the bootindex.
> 
> Could look like
> 
>     { "execute": "qom-set",
>       "arguments": { "path": "/machine/unattached/device[15]",
>                      "property": "bootindexA", "value": 1 } }
> 
> Demonstrates an existing, well-defined way to identify the bootindex to
> change.  Do we really want to invent another one, based on suffix?
> 
> The value of "path" is the QOM path.  I can't remember offhand how to go
> from qdev ID to QOM path.  Onboard devices like isa-fdc don't have one
> anyway.
> 
> I also don't remember whether there's a nicer QOM path than this one.

Thanks for your explaining. TBH I haven't used "qom-set" before, so I
don't know what your mean (like Eduardo, sorry again). 

But now, I have a look at the implement of "qom-set" command, and 
I find the command is just change a device's property value, do not have
any other logic. For my case, we really change value of the global fw_boot_order list, 
which the device's bootindex property worked for actually. Only in this way,
we can attach our original target.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Markus Armbruster]

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

> Hi, Markus
>
>> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex
>> command
>> >> >>
>> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com
>> wrote:
>> >> >> > From: Gonglei <arei.gonglei@huawei.com>
>> >> >> >
>> >> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
>> >> >> >
>> >> >> > Example QMP command:
>> >> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
>> >> >> > "bootindex":
>> >> >> 1, "suffix": "/disk@0"}}
>> >> >> > <- { "return": {} }
>> >> >> >
>> >> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> >> >> > Signed-off-by: Chenliang <chenliang88@huawei.com>
>> >> >> > ---
>> >> >> >  qapi-schema.json | 16 ++++++++++++++++
>> >> >> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
>> >> >> >  qmp.c            | 17 +++++++++++++++++
>> >> >> >  3 files changed, 57 insertions(+)
>> >> >> >
>> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> >> > index b11aad2..a9ef0be 100644
>> >> >> > --- a/qapi-schema.json
>> >> >> > +++ b/qapi-schema.json
>> >> >> > @@ -1704,6 +1704,22 @@
>> >> >> >  { 'command': 'device_del', 'data': {'id': 'str'} }
>> >> >> >
>> >> >> >  ##
>> >> >> > +# @set-bootindex:
>> >> >> > +#
>> >> >> > +# set bootindex of a devcie
>> >> >> > +#
>> >> >> > +# @id: the name of the device
>> >> >> > +# @bootindex: the bootindex of the device
>> >> >> > +# @suffix: #optional a suffix of the device
>> >> >> > +#
>> >> >> > +# Returns: Nothing on success
>> >> >> > +#          If @id is not a valid device, DeviceNotFound
>> >> >> > +#
>> >> >> > +# Since: 2.2
>> >> >> > +##
>> >> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
>> >> >> > int', '*suffix':
>> >> >> 'str'} }
>> >> >> > +
>> >> >> > +##
>> >> >>
>> >> >> I wonder if we could simply use qom-set for that. How many devices
>> >> >> actually support having multiple bootindex entries with different
>> >> >> suffixes?
>> >> >>
>> >> > AFAICT, the floppy device support two bootindex with different suffixes.
>> >>
>> >> Block device models commonly a single block device.  By convention,
>> >> property "drive" is the backend, and property "bootindex" the bootindex,
>> >> if the device model supports that.
>> >>
>> >> The device ID suffices to identify a block device for such device
>> >> models.
>> >>
>> >> The floppy device model is an exception.  It folds multiple real-world
>> >> objects into one: the controller and the actual drives.  Have a look at
>> >> -device isa-fdc,help:
>> >>
>> >>     isa-fdc.iobase=uint32
>> >>     isa-fdc.irq=uint32
>> >>     isa-fdc.dma=uint32
>> >>     isa-fdc.driveA=drive
>> >>     isa-fdc.driveB=drive
>> >>     isa-fdc.bootindexA=int32
>> >>     isa-fdc.bootindexB=int32
>> >>     isa-fdc.check_media_rate=on/off
>> >>
>> >> The properties ending with 'A' or 'B' apply to the first and the second
>> >> drive, the others to the controller.
>> >>
>> >> Unfortunately, this means the device ID by itself doesn't identify the
>> >> floppy device.
>> >>
>> > Yes.
>> >
>> >> Arguably, this is lousy modeling --- we really should model separate
>> >> real-world objects as separate objects.  But making floppies pretty (or
>> >> even sane) isn't anyone's priority nowadays.
>> >>
>> > Hmm... Agreed.
>> >
>> >> Since qom-set works on *properties*, I can't see why it couldn't be used
>> >> for changing bootindexes.  There is no ambiguity even with floppy.
>> >
>> > Sorry?
>> >
>> >> You either set bootindexA or bootindexB.  No need to fuzz around with
>> >> suffixes.  Or am I missing something?
>> >
>> > Your mean that we just need to think about change one bootindex? Either
>> > set bootindexA or bootindexB, do not need pass suffix for identifying?
>> 
>> Eduardo suggested to think about using qom-set to update the bootindex.
>> 
>> Could look like
>> 
>>     { "execute": "qom-set",
>>       "arguments": { "path": "/machine/unattached/device[15]",
>>                      "property": "bootindexA", "value": 1 } }
>> 
>> Demonstrates an existing, well-defined way to identify the bootindex to
>> change.  Do we really want to invent another one, based on suffix?
>> 
>> The value of "path" is the QOM path.  I can't remember offhand how to go
>> from qdev ID to QOM path.  Onboard devices like isa-fdc don't have one
>> anyway.
>> 
>> I also don't remember whether there's a nicer QOM path than this one.
>
> Thanks for your explaining. TBH I haven't used "qom-set" before, so I

Few people have :)

> don't know what your mean (like Eduardo, sorry again). 
>
> But now, I have a look at the implement of "qom-set" command, and 
> I find the command is just change a device's property value, do not have
> any other logic. For my case, we really change value of the global
> fw_boot_order list,
> which the device's bootindex property worked for actually. Only in this way,
> we can attach our original target.

Why can't we delay the logic to the next reboot?  Let me explain.

Current code starts with empty fw_boot_order, then lets device realize
add to it.  Unplugging a device leaves a dangling DeviceState pointer in
the list (I think).

Could we instead build, use and free the list during reboot?  That way,
qom-set of bootindex affects the next reboot without additional logic.

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Gonglei (Arei)]

Hi, Markus
> >
> >> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex
> >> command
> >> >> >>
> >> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com
> >> wrote:
> >> >> >> > From: Gonglei <arei.gonglei@huawei.com>
> >> >> >> >
> >> >> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
> >> >> >> >
> >> >> >> > Example QMP command:
> >> >> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
> >> >> >> > "bootindex":
> >> >> >> 1, "suffix": "/disk@0"}}
> >> >> >> > <- { "return": {} }
> >> >> >> >
> >> >> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> >> >> > Signed-off-by: Chenliang <chenliang88@huawei.com>
> >> >> >> > ---
> >> >> >> >  qapi-schema.json | 16 ++++++++++++++++
> >> >> >> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
> >> >> >> >  qmp.c            | 17 +++++++++++++++++
> >> >> >> >  3 files changed, 57 insertions(+)
> >> >> >> >
> >> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> >> >> > index b11aad2..a9ef0be 100644
> >> >> >> > --- a/qapi-schema.json
> >> >> >> > +++ b/qapi-schema.json
> >> >> >> > @@ -1704,6 +1704,22 @@
> >> >> >> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> >> >> >> >
> >> >> >> >  ##
> >> >> >> > +# @set-bootindex:
> >> >> >> > +#
> >> >> >> > +# set bootindex of a devcie
> >> >> >> > +#
> >> >> >> > +# @id: the name of the device
> >> >> >> > +# @bootindex: the bootindex of the device
> >> >> >> > +# @suffix: #optional a suffix of the device
> >> >> >> > +#
> >> >> >> > +# Returns: Nothing on success
> >> >> >> > +#          If @id is not a valid device, DeviceNotFound
> >> >> >> > +#
> >> >> >> > +# Since: 2.2
> >> >> >> > +##
> >> >> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
> >> >> >> > int', '*suffix':
> >> >> >> 'str'} }
> >> >> >> > +
> >> >> >> > +##
> >> >> >>
> >> >> >> I wonder if we could simply use qom-set for that. How many devices
> >> >> >> actually support having multiple bootindex entries with different
> >> >> >> suffixes?
> >> >> >>
> >> >> > AFAICT, the floppy device support two bootindex with different suffixes.
> >> >>
> >> >> Block device models commonly a single block device.  By convention,
> >> >> property "drive" is the backend, and property "bootindex" the bootindex,
> >> >> if the device model supports that.
> >> >>
> >> >> The device ID suffices to identify a block device for such device
> >> >> models.
> >> >>
> >> >> The floppy device model is an exception.  It folds multiple real-world
> >> >> objects into one: the controller and the actual drives.  Have a look at
> >> >> -device isa-fdc,help:
> >> >>
> >> >>     isa-fdc.iobase=uint32
> >> >>     isa-fdc.irq=uint32
> >> >>     isa-fdc.dma=uint32
> >> >>     isa-fdc.driveA=drive
> >> >>     isa-fdc.driveB=drive
> >> >>     isa-fdc.bootindexA=int32
> >> >>     isa-fdc.bootindexB=int32
> >> >>     isa-fdc.check_media_rate=on/off
> >> >>
> >> >> The properties ending with 'A' or 'B' apply to the first and the second
> >> >> drive, the others to the controller.
> >> >>
> >> >> Unfortunately, this means the device ID by itself doesn't identify the
> >> >> floppy device.
> >> >>
> >> > Yes.
> >> >
> >> >> Arguably, this is lousy modeling --- we really should model separate
> >> >> real-world objects as separate objects.  But making floppies pretty (or
> >> >> even sane) isn't anyone's priority nowadays.
> >> >>
> >> > Hmm... Agreed.
> >> >
> >> >> Since qom-set works on *properties*, I can't see why it couldn't be used
> >> >> for changing bootindexes.  There is no ambiguity even with floppy.
> >> >
> >> > Sorry?
> >> >
> >> >> You either set bootindexA or bootindexB.  No need to fuzz around with
> >> >> suffixes.  Or am I missing something?
> >> >
> >> > Your mean that we just need to think about change one bootindex? Either
> >> > set bootindexA or bootindexB, do not need pass suffix for identifying?
> >>
> >> Eduardo suggested to think about using qom-set to update the bootindex.
> >>
> >> Could look like
> >>
> >>     { "execute": "qom-set",
> >>       "arguments": { "path": "/machine/unattached/device[15]",
> >>                      "property": "bootindexA", "value": 1 } }
> >>
> >> Demonstrates an existing, well-defined way to identify the bootindex to
> >> change.  Do we really want to invent another one, based on suffix?
> >>
> >> The value of "path" is the QOM path.  I can't remember offhand how to go
> >> from qdev ID to QOM path.  Onboard devices like isa-fdc don't have one
> >> anyway.
> >>
> >> I also don't remember whether there's a nicer QOM path than this one.
> >
> > Thanks for your explaining. TBH I haven't used "qom-set" before, so I
> 
> Few people have :)
> 
> > don't know what your mean (like Eduardo, sorry again).
> >
> > But now, I have a look at the implement of "qom-set" command, and
> > I find the command is just change a device's property value, do not have
> > any other logic. For my case, we really change value of the global
> > fw_boot_order list,
> > which the device's bootindex property worked for actually. Only in this way,
> > we can attach our original target.
> 
> Why can't we delay the logic to the next reboot?  Let me explain.
> 
> Current code starts with empty fw_boot_order, then lets device realize
> add to it.  Unplugging a device leaves a dangling DeviceState pointer in
> the list (I think).
> 
Yes.

> Could we instead build, use and free the list during reboot?  That way,
> qom-set of bootindex affects the next reboot without additional logic.

Yes, we can do this, but it will be too complicated. Firstly the device realize
will not reattach during reboot. So, we should check all the device of the global list during reboot,
but the DeviceState haven't bootindex property, we should cast it into idiographic
device for getting the changed bootindex, such as " VirtIONet *n = VIRTIO_NET(dev)" ( this will be a trouble 
for different devices),  then we can change fw_boot_order list's bootindex using new bootindex,
then reorder all bootindexes in list. Am I wrong? 

So, I don't think it is a good idea.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Markus Armbruster]

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

> Hi, Markus
>> >
>> >> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex
>> >> command
>> >> >> >>
>> >> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com
>> >> wrote:
>> >> >> >> > From: Gonglei <arei.gonglei@huawei.com>
>> >> >> >> >
>> >> >> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
>> >> >> >> >
>> >> >> >> > Example QMP command:
>> >> >> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
>> >> >> >> > "bootindex":
>> >> >> >> 1, "suffix": "/disk@0"}}
>> >> >> >> > <- { "return": {} }
>> >> >> >> >
>> >> >> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> >> >> >> > Signed-off-by: Chenliang <chenliang88@huawei.com>
>> >> >> >> > ---
>> >> >> >> >  qapi-schema.json | 16 ++++++++++++++++
>> >> >> >> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
>> >> >> >> >  qmp.c            | 17 +++++++++++++++++
>> >> >> >> >  3 files changed, 57 insertions(+)
>> >> >> >> >
>> >> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> >> >> > index b11aad2..a9ef0be 100644
>> >> >> >> > --- a/qapi-schema.json
>> >> >> >> > +++ b/qapi-schema.json
>> >> >> >> > @@ -1704,6 +1704,22 @@
>> >> >> >> >  { 'command': 'device_del', 'data': {'id': 'str'} }
>> >> >> >> >
>> >> >> >> >  ##
>> >> >> >> > +# @set-bootindex:
>> >> >> >> > +#
>> >> >> >> > +# set bootindex of a devcie
>> >> >> >> > +#
>> >> >> >> > +# @id: the name of the device
>> >> >> >> > +# @bootindex: the bootindex of the device
>> >> >> >> > +# @suffix: #optional a suffix of the device
>> >> >> >> > +#
>> >> >> >> > +# Returns: Nothing on success
>> >> >> >> > +#          If @id is not a valid device, DeviceNotFound
>> >> >> >> > +#
>> >> >> >> > +# Since: 2.2
>> >> >> >> > +##
>> >> >> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
>> >> >> >> > int', '*suffix':
>> >> >> >> 'str'} }
>> >> >> >> > +
>> >> >> >> > +##
>> >> >> >>
>> >> >> >> I wonder if we could simply use qom-set for that. How many devices
>> >> >> >> actually support having multiple bootindex entries with different
>> >> >> >> suffixes?
>> >> >> >>
>> >> >> > AFAICT, the floppy device support two bootindex with
>> >> >> different suffixes.
>> >> >>
>> >> >> Block device models commonly a single block device.  By convention,
>> >> >> property "drive" is the backend, and property "bootindex" the bootindex,
>> >> >> if the device model supports that.
>> >> >>
>> >> >> The device ID suffices to identify a block device for such device
>> >> >> models.
>> >> >>
>> >> >> The floppy device model is an exception.  It folds multiple real-world
>> >> >> objects into one: the controller and the actual drives.  Have a look at
>> >> >> -device isa-fdc,help:
>> >> >>
>> >> >>     isa-fdc.iobase=uint32
>> >> >>     isa-fdc.irq=uint32
>> >> >>     isa-fdc.dma=uint32
>> >> >>     isa-fdc.driveA=drive
>> >> >>     isa-fdc.driveB=drive
>> >> >>     isa-fdc.bootindexA=int32
>> >> >>     isa-fdc.bootindexB=int32
>> >> >>     isa-fdc.check_media_rate=on/off
>> >> >>
>> >> >> The properties ending with 'A' or 'B' apply to the first and the second
>> >> >> drive, the others to the controller.
>> >> >>
>> >> >> Unfortunately, this means the device ID by itself doesn't identify the
>> >> >> floppy device.
>> >> >>
>> >> > Yes.
>> >> >
>> >> >> Arguably, this is lousy modeling --- we really should model separate
>> >> >> real-world objects as separate objects.  But making floppies pretty (or
>> >> >> even sane) isn't anyone's priority nowadays.
>> >> >>
>> >> > Hmm... Agreed.
>> >> >
>> >> >> Since qom-set works on *properties*, I can't see why it couldn't be used
>> >> >> for changing bootindexes.  There is no ambiguity even with floppy.
>> >> >
>> >> > Sorry?
>> >> >
>> >> >> You either set bootindexA or bootindexB.  No need to fuzz around with
>> >> >> suffixes.  Or am I missing something?
>> >> >
>> >> > Your mean that we just need to think about change one bootindex? Either
>> >> > set bootindexA or bootindexB, do not need pass suffix for identifying?
>> >>
>> >> Eduardo suggested to think about using qom-set to update the bootindex.
>> >>
>> >> Could look like
>> >>
>> >>     { "execute": "qom-set",
>> >>       "arguments": { "path": "/machine/unattached/device[15]",
>> >>                      "property": "bootindexA", "value": 1 } }
>> >>
>> >> Demonstrates an existing, well-defined way to identify the bootindex to
>> >> change.  Do we really want to invent another one, based on suffix?
>> >>
>> >> The value of "path" is the QOM path.  I can't remember offhand how to go
>> >> from qdev ID to QOM path.  Onboard devices like isa-fdc don't have one
>> >> anyway.
>> >>
>> >> I also don't remember whether there's a nicer QOM path than this one.
>> >
>> > Thanks for your explaining. TBH I haven't used "qom-set" before, so I
>> 
>> Few people have :)
>> 
>> > don't know what your mean (like Eduardo, sorry again).
>> >
>> > But now, I have a look at the implement of "qom-set" command, and
>> > I find the command is just change a device's property value, do not have
>> > any other logic. For my case, we really change value of the global
>> > fw_boot_order list,
>> > which the device's bootindex property worked for actually. Only in this way,
>> > we can attach our original target.
>> 
>> Why can't we delay the logic to the next reboot?  Let me explain.
>> 
>> Current code starts with empty fw_boot_order, then lets device realize
>> add to it.  Unplugging a device leaves a dangling DeviceState pointer in
>> the list (I think).
>> 
> Yes.
>
>> Could we instead build, use and free the list during reboot?  That way,
>> qom-set of bootindex affects the next reboot without additional logic.
>
> Yes, we can do this, but it will be too complicated. Firstly the device realize
> will not reattach during reboot. So, we should check all the device of
> the global list during reboot,
> but the DeviceState haven't bootindex property, we should cast it into
> idiographic
> device for getting the changed bootindex, such as " VirtIONet *n =
> VIRTIO_NET(dev)" ( this will be a trouble
> for different devices), then we can change fw_boot_order list's
> bootindex using new bootindex,
> then reorder all bootindexes in list. Am I wrong? 
>
> So, I don't think it is a good idea.

Moving add_boot_device_path() from realize to reset could do the trick.

Anyway, delaying the logic is not necessary for use of qom-set.
Ordinary qdev properties use common setter functions.  QOM is more
general: it lets you define your own setter.  For example,
device_initfn() defines property "realized" like this:

    object_property_add_bool(obj, "realized",
                             device_get_realized, device_set_realized, NULL);

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Eduardo Habkost]

On Mon, Aug 04, 2014 at 12:00:59PM +0200, Markus Armbruster wrote:
> "Gonglei (Arei)" <arei.gonglei@huawei.com> writes:
> 
> > Hi,
> >
> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command
> >> >>
> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800, arei.gonglei@huawei.com wrote:
> >> >> > From: Gonglei <arei.gonglei@huawei.com>
> >> >> >
> >> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP command.
> >> >> >
> >> >> > Example QMP command:
> >> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
> >> >> > "bootindex":
> >> >> 1, "suffix": "/disk@0"}}
> >> >> > <- { "return": {} }
> >> >> >
> >> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> >> > Signed-off-by: Chenliang <chenliang88@huawei.com>
> >> >> > ---
> >> >> >  qapi-schema.json | 16 ++++++++++++++++
> >> >> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
> >> >> >  qmp.c            | 17 +++++++++++++++++
> >> >> >  3 files changed, 57 insertions(+)
> >> >> >
> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> >> > index b11aad2..a9ef0be 100644
> >> >> > --- a/qapi-schema.json
> >> >> > +++ b/qapi-schema.json
> >> >> > @@ -1704,6 +1704,22 @@
> >> >> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> >> >> >
> >> >> >  ##
> >> >> > +# @set-bootindex:
> >> >> > +#
> >> >> > +# set bootindex of a devcie
> >> >> > +#
> >> >> > +# @id: the name of the device
> >> >> > +# @bootindex: the bootindex of the device
> >> >> > +# @suffix: #optional a suffix of the device
> >> >> > +#
> >> >> > +# Returns: Nothing on success
> >> >> > +#          If @id is not a valid device, DeviceNotFound
> >> >> > +#
> >> >> > +# Since: 2.2
> >> >> > +##
> >> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
> >> >> > int', '*suffix':
> >> >> 'str'} }
> >> >> > +
> >> >> > +##
> >> >>
> >> >> I wonder if we could simply use qom-set for that. How many devices
> >> >> actually support having multiple bootindex entries with different
> >> >> suffixes?
> >> >>
> >> > AFAICT, the floppy device support two bootindex with different suffixes.
> >> 
> >> Block device models commonly a single block device.  By convention,
> >> property "drive" is the backend, and property "bootindex" the bootindex,
> >> if the device model supports that.
> >> 
> >> The device ID suffices to identify a block device for such device
> >> models.
> >> 
> >> The floppy device model is an exception.  It folds multiple real-world
> >> objects into one: the controller and the actual drives.  Have a look at
> >> -device isa-fdc,help:
> >> 
> >>     isa-fdc.iobase=uint32
> >>     isa-fdc.irq=uint32
> >>     isa-fdc.dma=uint32
> >>     isa-fdc.driveA=drive
> >>     isa-fdc.driveB=drive
> >>     isa-fdc.bootindexA=int32
> >>     isa-fdc.bootindexB=int32
> >>     isa-fdc.check_media_rate=on/off
> >> 
> >> The properties ending with 'A' or 'B' apply to the first and the second
> >> drive, the others to the controller.
> >> 
> >> Unfortunately, this means the device ID by itself doesn't identify the
> >> floppy device.
> >> 
> > Yes.
> >
> >> Arguably, this is lousy modeling --- we really should model separate
> >> real-world objects as separate objects.  But making floppies pretty (or
> >> even sane) isn't anyone's priority nowadays.
> >> 
> > Hmm... Agreed.
> >
> >> Since qom-set works on *properties*, I can't see why it couldn't be used
> >> for changing bootindexes.  There is no ambiguity even with floppy.
> >
> > Sorry? 
> >
> >> You either set bootindexA or bootindexB.  No need to fuzz around with
> >> suffixes.  Or am I missing something?
> >
> > Your mean that we just need to think about change one bootindex? Either 
> > set bootindexA or bootindexB, do not need pass suffix for identifying?
> 
> Eduardo suggested to think about using qom-set to update the bootindex.
> 
> Could look like
> 
>     { "execute": "qom-set",
>       "arguments": { "path": "/machine/unattached/device[15]",
>                      "property": "bootindexA", "value": 1 } }
> 
> Demonstrates an existing, well-defined way to identify the bootindex to
> change.  Do we really want to invent another one, based on suffix?
> 
> The value of "path" is the QOM path.  I can't remember offhand how to go
> from qdev ID to QOM path.  Onboard devices like isa-fdc don't have one
> anyway.
> 
> I also don't remember whether there's a nicer QOM path than this one.

The "path" argument may be a "partial path". From qapi-schema.json:

    Partial paths look like relative filenames.  They do not begin
    with a prefix.  The matching rules for partial paths are subtle but
    designed to make specifying objects easy.  At each level of the
    composition tree, the partial path is matched as an absolute path.
    The first match is not returned.  At least two matches are searched
    for.  A successful result is only returned if only one match is
    found.  If more than one match is found, a flag is return to
    indicate that the match was ambiguous.

I haven't tested this. Is the qdev ID visible in device paths somewhere,
allowing it to be used for partial path matching?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex command

[Gonglei (Arei)]

Hi,

> >> >> >> >> Subject: Re: [Qemu-devel] [PATCH v2 5/7] qmp: add set-bootindex
> >> >> command
> >> >> >> >>
> >> >> >> >> On Fri, Jul 25, 2014 at 02:52:50PM +0800,
> arei.gonglei@huawei.com
> >> >> wrote:
> >> >> >> >> > From: Gonglei <arei.gonglei@huawei.com>
> >> >> >> >> >
> >> >> >> >> > Adds "set-bootindex id=xx,bootindex=xx,suffix=xx" QMP
> command.
> >> >> >> >> >
> >> >> >> >> > Example QMP command:
> >> >> >> >> > -> { "execute": "set-bootindex", "arguments": { "id": "ide0-0-1",
> >> >> >> >> > "bootindex":
> >> >> >> >> 1, "suffix": "/disk@0"}}
> >> >> >> >> > <- { "return": {} }
> >> >> >> >> >
> >> >> >> >> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> >> >> >> > Signed-off-by: Chenliang <chenliang88@huawei.com>
> >> >> >> >> > ---
> >> >> >> >> >  qapi-schema.json | 16 ++++++++++++++++
> >> >> >> >> >  qmp-commands.hx  | 24 ++++++++++++++++++++++++
> >> >> >> >> >  qmp.c            | 17 +++++++++++++++++
> >> >> >> >> >  3 files changed, 57 insertions(+)
> >> >> >> >> >
> >> >> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> >> >> >> > index b11aad2..a9ef0be 100644
> >> >> >> >> > --- a/qapi-schema.json
> >> >> >> >> > +++ b/qapi-schema.json
> >> >> >> >> > @@ -1704,6 +1704,22 @@
> >> >> >> >> >  { 'command': 'device_del', 'data': {'id': 'str'} }
> >> >> >> >> >
> >> >> >> >> >  ##
> >> >> >> >> > +# @set-bootindex:
> >> >> >> >> > +#
> >> >> >> >> > +# set bootindex of a devcie
> >> >> >> >> > +#
> >> >> >> >> > +# @id: the name of the device
> >> >> >> >> > +# @bootindex: the bootindex of the device
> >> >> >> >> > +# @suffix: #optional a suffix of the device
> >> >> >> >> > +#
> >> >> >> >> > +# Returns: Nothing on success
> >> >> >> >> > +#          If @id is not a valid device, DeviceNotFound
> >> >> >> >> > +#
> >> >> >> >> > +# Since: 2.2
> >> >> >> >> > +##
> >> >> >> >> > +{ 'command': 'set-bootindex', 'data': {'id': 'str', 'bootindex':
> >> >> >> >> > int', '*suffix':
> >> >> >> >> 'str'} }
> >> >> >> >> > +
> >> >> >> >> > +##
> >> >> >> >>
> >> >> >> >> I wonder if we could simply use qom-set for that. How many
> devices
> >> >> >> >> actually support having multiple bootindex entries with different
> >> >> >> >> suffixes?
> >> >> >> >>
> >> >> >> > AFAICT, the floppy device support two bootindex with
> >> >> >> different suffixes.
> >> >> >>
> >> >> >> Block device models commonly a single block device.  By convention,
> >> >> >> property "drive" is the backend, and property "bootindex" the
> bootindex,
> >> >> >> if the device model supports that.
> >> >> >>
> >> >> >> The device ID suffices to identify a block device for such device
> >> >> >> models.
> >> >> >>
> >> >> >> The floppy device model is an exception.  It folds multiple real-world
> >> >> >> objects into one: the controller and the actual drives.  Have a look at
> >> >> >> -device isa-fdc,help:
> >> >> >>
> >> >> >>     isa-fdc.iobase=uint32
> >> >> >>     isa-fdc.irq=uint32
> >> >> >>     isa-fdc.dma=uint32
> >> >> >>     isa-fdc.driveA=drive
> >> >> >>     isa-fdc.driveB=drive
> >> >> >>     isa-fdc.bootindexA=int32
> >> >> >>     isa-fdc.bootindexB=int32
> >> >> >>     isa-fdc.check_media_rate=on/off
> >> >> >>
> >> >> >> The properties ending with 'A' or 'B' apply to the first and the second
> >> >> >> drive, the others to the controller.
> >> >> >>
> >> >> >> Unfortunately, this means the device ID by itself doesn't identify the
> >> >> >> floppy device.
> >> >> >>
> >> >> > Yes.
> >> >> >
> >> >> >> Arguably, this is lousy modeling --- we really should model separate
> >> >> >> real-world objects as separate objects.  But making floppies pretty
> (or
> >> >> >> even sane) isn't anyone's priority nowadays.
> >> >> >>
> >> >> > Hmm... Agreed.
> >> >> >
> >> >> >> Since qom-set works on *properties*, I can't see why it couldn't be
> used
> >> >> >> for changing bootindexes.  There is no ambiguity even with floppy.
> >> >> >
> >> >> > Sorry?
> >> >> >
> >> >> >> You either set bootindexA or bootindexB.  No need to fuzz around
> with
> >> >> >> suffixes.  Or am I missing something?
> >> >> >
> >> >> > Your mean that we just need to think about change one bootindex?
> Either
> >> >> > set bootindexA or bootindexB, do not need pass suffix for identifying?
> >> >>
> >> >> Eduardo suggested to think about using qom-set to update the
> bootindex.
> >> >>
> >> >> Could look like
> >> >>
> >> >>     { "execute": "qom-set",
> >> >>       "arguments": { "path": "/machine/unattached/device[15]",
> >> >>                      "property": "bootindexA", "value": 1 } }
> >> >>
> >> >> Demonstrates an existing, well-defined way to identify the bootindex to
> >> >> change.  Do we really want to invent another one, based on suffix?
> >> >>
> >> >> The value of "path" is the QOM path.  I can't remember offhand how to
> go
> >> >> from qdev ID to QOM path.  Onboard devices like isa-fdc don't have one
> >> >> anyway.
> >> >>
> >> >> I also don't remember whether there's a nicer QOM path than this one.
> >> >
> >> > Thanks for your explaining. TBH I haven't used "qom-set" before, so I
> >>
> >> Few people have :)
> >>
> >> > don't know what your mean (like Eduardo, sorry again).
> >> >
> >> > But now, I have a look at the implement of "qom-set" command, and
> >> > I find the command is just change a device's property value, do not have
> >> > any other logic. For my case, we really change value of the global
> >> > fw_boot_order list,
> >> > which the device's bootindex property worked for actually. Only in this way,
> >> > we can attach our original target.
> >>
> >> Why can't we delay the logic to the next reboot?  Let me explain.
> >>
> >> Current code starts with empty fw_boot_order, then lets device realize
> >> add to it.  Unplugging a device leaves a dangling DeviceState pointer in
> >> the list (I think).
> >>
> > Yes.
> >
> >> Could we instead build, use and free the list during reboot?  That way,
> >> qom-set of bootindex affects the next reboot without additional logic.
> >
> > Yes, we can do this, but it will be too complicated. Firstly the device realize
> > will not reattach during reboot. So, we should check all the device of
> > the global list during reboot,
> > but the DeviceState haven't bootindex property, we should cast it into
> > idiographic
> > device for getting the changed bootindex, such as " VirtIONet *n =
> > VIRTIO_NET(dev)" ( this will be a trouble
> > for different devices), then we can change fw_boot_order list's
> > bootindex using new bootindex,
> > then reorder all bootindexes in list. Am I wrong?
> >
> > So, I don't think it is a good idea.
> 
> Moving add_boot_device_path() from realize to reset could do the trick.
> 
Hmm... but not every device has reset function. 
This changing will be a big surgery IMO.

> Anyway, delaying the logic is not necessary for use of qom-set.
> Ordinary qdev properties use common setter functions.  QOM is more
> general: it lets you define your own setter.  For example,
> device_initfn() defines property "realized" like this:
> 
>     object_property_add_bool(obj, "realized",
>                              device_get_realized, device_set_realized,
> NULL);

Agreed.

Best regards,
-Gonglei