Introduce qemuDomainChangeBootIndex api to support update device's bootindex. These function will be used in following patches to support change device's (support cdrom, disk and net) bootindex with virsh command like 'virsh update-device <domain> <file> [--flag]'. Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com&gt; --- src/qemu/qemu_conf.c | 40 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 5 +++++ src/qemu/qemu_monitor.c | 20 ++++++++++++++++++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 33 +++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++++ 6 files changed, 110 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ae5bbcd138..0071a95cb6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1641,3 +1641,43 @@ qemuHugepageMakeBasedir(virQEMUDriver *driver, return 0; } + +/** + * qemuDomainChangeBootIndex: + * @vm: domain object + * @devInfo: origin device info + * @newBootIndex: new bootIndex + * + * check the alias and bootindex before send the command + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +qemuDomainChangeBootIndex(virDomainObj *vm, + virDomainDeviceInfo *devInfo, + int newBootIndex) +{ + int ret = -1; + int dummyIndex = -1; + qemuDomainObjPrivate *priv = vm->privateData; + + if (!devInfo->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("cannot change boot index: device alias not found")); + return -1; + } + + VIR_DEBUG("Change dev: %s boot index from %d to %d", devInfo->alias, + devInfo->bootIndex, newBootIndex); + + qemuDomainObjEnterMonitor(vm); + /* newBootIndex = 0 means cancel the specified bootIndex, send -1 to qemu instead */ + if (!newBootIndex) + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, dummyIndex); + else + ret = qemuMonitorSetBootIndex(priv->mon, devInfo->alias, newBootIndex); + qemuDomainObjExitMonitor(vm); + + return ret; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 8cf2dd2ec5..e7447191df 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -375,3 +375,8 @@ int qemuGetMemoryBackingPath(virQEMUDriver *driver, int qemuHugepageMakeBasedir(virQEMUDriver *driver, virHugeTLBFS *hugepage); + +int qemuDomainChangeBootIndex(virDomainObj *vm, + virDomainDeviceInfo *devInfo, + int newBootIndex) + ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 80f262cec7..2b89d9bdae 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4491,3 +4491,23 @@ qemuMonitorGetStatsByQOMPath(virJSONValue *arr, return NULL; } + +/** + * qemuMonitorSetBootIndex: + * @mon: monitor object + * @alias: device's alias + * @bootIndex: new boot index + * + * Returns data from a call to 'qom-set' + */ +int +qemuMonitorSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) +{ + VIR_DEBUG("name=%s, bootIndex=%d", alias, bootIndex); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetBootIndex(mon, alias, bootIndex); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c690fc3655..0b9943e1f7 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1568,3 +1568,9 @@ qemuMonitorExtractQueryStats(virJSONValue *info); virJSONValue * qemuMonitorGetStatsByQOMPath(virJSONValue *arr, char *qom_path); + +int +qemuMonitorSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index f4e942a32f..75de73e61b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8890,3 +8890,36 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon, return virJSONValueObjectStealArray(reply, "return"); } + +/** + * qemuMonitorSetBootIndex: + * @mon: monitor object + * @alias: device's alias + * @bootIndex: new boot index + * + * Set the bootindex of device to new bootIndex + * + * Returns: 0 on success, + * -1 otherwise. + */ +int +qemuMonitorJSONSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-set", "s:path", alias, + "s:property", "bootindex", + "i:value", bootIndex, NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 93789480c5..30cd92f232 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -824,3 +824,9 @@ qemuMonitorJSONQueryStats(qemuMonitor *mon, qemuMonitorQueryStatsTargetType target, char **vcpus, GPtrArray *providers); + +int +qemuMonitorJSONSetBootIndex(qemuMonitor *mon, + const char *alias, + int bootIndex) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 2.33.0

On 2022/11/22 22:36, Peter Krempa Wrote: I see, I'll better describe my functions in the next patch. I have tried it on qemu-4.1 and qemu-6.2 and they both work will. And the alias of device is unique to the domain, so I think it's okay to use alias here. Thanks for your suggestion, I will modify it in next version.

On Thu, Nov 17, 2022 at 10:05:28 +0800, Jiang Jiacheng wrote: You'll need to note in the documentation the special 0 value and how it behaves when updating a device. In case where you disagree with my comment below that the bootindex should not be in the output XML you'll also need to add XML2XML test cases. I'm not persuaded that the logic here needs changing. If the boot index is 0 and the device is not bootable we should avoid the element. This includes cases where the device was updated in order co clear the boot index.

Introduce qemuCheckBootIndex to check the new bootindex and is it nessary to update the bootindex. Introduce qemuChangeDiskBootIndex to support update disk's bootindex according to different disks' type. Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com&gt; --- src/qemu/qemu_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 7 +++++ 2 files changed, 74 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 0071a95cb6..9a7992db01 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1681,3 +1681,70 @@ qemuDomainChangeBootIndex(virDomainObj *vm, return ret; } + +/** + * qemuCheckBootIndex: + * @devInfo: origin device info + * @new_bootindex: new bootIndex + * + * check whether the device's bootIndex could be changed or neet to + * be changed + * + * Returns: 1 on need to change + * 0 on don't need to change + * -1 on could not change with an error + */ +int +qemuCheckBootIndex(virDomainDeviceInfo *devInfo, + const int new_bootindex) +{ + if (!devInfo->bootIndexSpecified) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("this device does not set boot index, cannot change it.")); + return -1; + } + + /* if the new bootindex is different from the old bootindex, + * we will update the bootindex. */ + if (devInfo->bootIndex != new_bootindex) { + return 1; + } + + return 0; +} + +/** + * qemuChangeDiskBootIndex: + * @vm: domain object + * @orig_disk: the origin disk + * @disk: the new disk to be updated to + * + * check device's type and if its type support update bootIndex, update it. + * + * Returns: 0 on success + * -1 on fail + */ +int +qemuChangeDiskBootIndex(virDomainObj *vm, + virDomainDiskDef *orig_disk, + virDomainDiskDef *disk) +{ + switch (disk->device) { + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: + if (qemuDomainChangeBootIndex(vm, &orig_disk->info, + disk->info.bootIndex) < 0) + return -1; + + orig_disk->info.bootIndex = disk->info.bootIndex; + break; + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + case VIR_DOMAIN_DISK_DEVICE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The boot index of disk bus '%s' cannot be changed."), + virDomainDiskBusTypeToString(disk->bus)); + return -1; + } + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e7447191df..5b05a7392b 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -380,3 +380,10 @@ int qemuDomainChangeBootIndex(virDomainObj *vm, virDomainDeviceInfo *devInfo, int newBootIndex) ATTRIBUTE_NONNULL(2); + +int qemuCheckBootIndex(virDomainDeviceInfo *devInfo, + const int new_bootindex); + +int qemuChangeDiskBootIndex(virDomainObj *vm, + virDomainDiskDef *orig_disk, + virDomainDiskDef *disk); -- 2.33.0

On 2022/11/22 23:06, Peter Krempa wrote: Thank for your suggestion, I will move it to qemu_domain in my next patch. The bootindex changable devices is fixed when start the domain, so it is not supported to add a boot index, this check is used to prevent them. However, it do have problems as you mentioned in next patch, the logical here should be optimized.

On Thu, Nov 17, 2022 at 10:05:30 +0800, Jiang Jiacheng wrote: [...] Due to the weird check I complained about in previous patch, this actively breaks media change (and other parameter change) for disks which don't have bootindex. This also doesn't make sense. You don't have the same issue updating the bootindex of a floppy. Similarly, the qemuCheckBootIndex check is also not applicable as the next boot config willr esult in a new boot where you can change anything.

Support to update the net's bootindex using 'virsh update-device'. Using flag --config or --persistent to change the boot index and the change will be affected after reboot. With --persistent, we can get the result of change immediently, but it still takes effect after reboot. Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com&gt; --- src/qemu/qemu_driver.c | 4 ++++ src/qemu/qemu_hotplug.c | 17 ++++++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65abc04998..863b779514 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7568,6 +7568,10 @@ qemuDomainUpdateDeviceConfig(virDomainDef *vmdef, false) < 0) return -1; + if (qemuCheckBootIndex(&vmdef->nets[pos]->info, + net->info.bootIndex) < 0) + return -1; + if (virDomainNetUpdate(vmdef, pos, net)) return -1; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index da92ced2f4..5d1913d0c7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3468,6 +3468,7 @@ qemuDomainChangeNet(virQEMUDriver *driver, bool needCoalesceChange = false; bool needVlanUpdate = false; bool needIsolatedPortChange = false; + int needChangeIndex = 0; int ret = -1; int changeidx = -1; g_autoptr(virConnect) conn = NULL; @@ -3633,11 +3634,8 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; } - if (newdev->info.bootIndex == 0) - newdev->info.bootIndex = olddev->info.bootIndex; - if (olddev->info.bootIndex != newdev->info.bootIndex) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot modify network device boot index setting")); + if ((needChangeIndex = qemuCheckBootIndex(&olddev->info, + newdev->info.bootIndex)) < 0) { goto cleanup; } @@ -3918,6 +3916,15 @@ qemuDomainChangeNet(virQEMUDriver *driver, needReplaceDevDef = true; } + if (needChangeIndex) { + if (qemuDomainChangeBootIndex(vm, &olddev->info, newdev->info.bootIndex) < 0) + goto cleanup; + /* we successfully switched to the new boot index, and we've + * determined that the rest of newdev is equivalent to olddev, + * so move newdev into place */ + needReplaceDevDef = true; + } + if (needReplaceDevDef) { /* the changes above warrant replacing olddev with newdev in * the domain's nets list. -- 2.33.0

On 2022/11/22 23:40, Peter Krempa wrote: I use 'boot order = 0' or new bootindex was not specified to cancel the bootindex setting. It seems not specified bootindex means inherit the old bootindex? But I can't find similiar code with disk, is this only used for net? See above.

If the 'boot order' is not specified in the xml, we cancel its bootindex setting and set it to '0'. However, the bootIndexSpecified will be set to false because we cannot parse boot order from XML, so copy bootIndexSpecified from origin device when updating it. Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com&gt; --- src/qemu/qemu_conf.c | 33 +++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_driver.c | 3 +++ 3 files changed, 40 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 9a7992db01..517fc813f2 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1748,3 +1748,36 @@ qemuChangeDiskBootIndex(virDomainObj *vm, } return 0; } + +/** + * qemuDomainDefCopyBootState: + * @dev: pointer to device parsed from xml + * @vmdef: pointer to vm definition structure + * + * copy the bootIndexSpecified from vmDef's device info + * + * Returns: 0 on success or no need to copy + * -1 on fail to find the device + */ +int +qemuDomainDefCopyBootState(virDomainDeviceDef *dev, virDomainDef *vmDef) +{ + int idx = -1; + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + if ((idx = virDomainDiskIndexByName(vmDef, dev->data.disk->dst, false)) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("target %s doesn't exist."), dev->data.disk->info.alias); + return -1; + } + dev->data.disk->info.bootIndexSpecified = vmDef->disks[idx]->info.bootIndexSpecified; + } + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + if ((idx = virDomainNetFindIdx(vmDef, dev->data.net)) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("target %s doesn't exist."), dev->data.net->info.alias); + return -1; + } + dev->data.net->info.bootIndexSpecified = vmDef->nets[idx]->info.bootIndexSpecified; + } + return 0; +} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 5b05a7392b..c1e9a2120f 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -387,3 +387,7 @@ int qemuCheckBootIndex(virDomainDeviceInfo *devInfo, int qemuChangeDiskBootIndex(virDomainObj *vm, virDomainDiskDef *orig_disk, virDomainDiskDef *disk); + + +int qemuDomainDefCopyBootState(virDomainDeviceDef *dev, + virDomainDef *vmDef); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 863b779514..3dbf95041e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7841,6 +7841,9 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (dev == NULL) goto endjob; + if (qemuDomainDefCopyBootState(dev, vm->def) < 0) + goto endjob; + if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { /* If we are affecting both CONFIG and LIVE -- 2.33.0