On Fri, Apr 23, 2021 at 15:24:29 +0200, Michal Privoznik wrote:
> As advertised in one of previous commits, we want to be able to
> change 'requested-size' attribute of virtio-mem on the fly. This
> commit does exactly that. Changing anything else is checked for
> and forbidden.
>
> Once guest has changed the allocation, QEMU emits an event which
> we will use to track the allocation. In the next commit.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 36 ++++++++
> src/conf/domain_conf.h | 4 +
> src/libvirt_private.syms | 1 +
> src/qemu/qemu_driver.c | 172 ++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_hotplug.c | 18 ++++
> src/qemu/qemu_hotplug.h | 5 +
> src/qemu/qemu_monitor.c | 13 +++
> src/qemu/qemu_monitor.h | 5 +
> src/qemu/qemu_monitor_json.c | 15 +++
> src/qemu/qemu_monitor_json.h | 5 +
> 10 files changed, 273 insertions(+), 1 deletion(-)
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d908e95ba7..9a241ad551 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7130,6 +7130,165 @@ qemuDomainChangeDiskLive(virDomainObj *vm,
> return 0;
> }
>
> +
> +static bool
> +qemuDomainChangeMemoryLiveValidateChange(const virDomainMemoryDef *oldDef,
> + const virDomainMemoryDef *newDef)
> +{
I must say this function feels almost as the ABI stability checker.
> + /* The only thing that is allowed to change is 'requestedsize' for virtio
> + * model. */
Also this is a bit weird. You mention that we are dealing with
virtio-mem ...
> + if (oldDef->model != newDef->model) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory model from '%s' to '%s'"),
> + virDomainMemoryModelTypeToString(oldDef->model),
> + virDomainMemoryModelTypeToString(newDef->model));
> + return false;
> + }
> +
> + if (oldDef->access != newDef->access) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory access from '%s' to '%s'"),
> + virDomainMemoryAccessTypeToString(oldDef->access),
> + virDomainMemoryAccessTypeToString(newDef->access));
> + return false;
> + }
> +
> + if (oldDef->discard != newDef->discard) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory discard from '%s' to '%s'"),
> + virTristateBoolTypeToString(oldDef->discard),
> + virTristateBoolTypeToString(newDef->discard));
> + return false;
> + }
> +
> + if (!virBitmapEqual(oldDef->sourceNodes,
> + newDef->sourceNodes)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("cannot modify memory source nodes"));
> + return false;
> + }
> +
> + if (oldDef->pagesize != newDef->pagesize) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory pagesize from '%llu' to '%llu'"),
> + oldDef->pagesize,
> + newDef->pagesize);
> + return false;
> + }
> +
> + if (STRNEQ_NULLABLE(oldDef->nvdimmPath, newDef->nvdimmPath)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory path from '%s' to '%s'"),
> + NULLSTR(oldDef->nvdimmPath),
> + NULLSTR(newDef->nvdimmPath));
> + return false;
> + }
... but also check stuff not relevant to virtio-mem ...
> +
> + if (oldDef->alignsize != newDef->alignsize) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory align size from '%llu' to '%llu'"),
> + oldDef->alignsize, newDef->alignsize);
> + return false;
> + }
> +
> + if (oldDef->nvdimmPmem != newDef->nvdimmPmem) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory pmem from '%d' to '%d'"),
> + oldDef->nvdimmPmem, newDef->nvdimmPmem);
> + return false;
> + }
> +
> + if (oldDef->targetNode != newDef->targetNode) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory targetNode from '%d' to '%d'"),
> + oldDef->targetNode, newDef->targetNode);
> + return false;
> + }
> +
> + if (oldDef->size != newDef->size) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory size from '%llu' to '%llu'"),
> + oldDef->size, newDef->size);
> + return false;
> + }
> +
> + if (oldDef->labelsize != newDef->labelsize) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory label size from '%llu' to '%llu'"),
> + oldDef->labelsize, newDef->labelsize);
> + return false;
> + }
> + if (oldDef->blocksize != newDef->blocksize) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory block size from '%llu' to '%llu'"),
> + oldDef->blocksize, newDef->blocksize);
> + return false;
> + }
> +
> + /* requestedsize can change */
> +
> + if (oldDef->readonly != newDef->readonly) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("cannot modify memory pmem flag"));
> + return false;
> + }
> +
> + if ((oldDef->uuid || newDef->uuid) &&
> + !(oldDef->uuid && newDef->uuid &&
> + memcmp(oldDef->uuid, newDef->uuid, VIR_UUID_BUFLEN) == 0)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("cannot modify memory UUID"));
> + return false;
> + }
> +
> + switch (oldDef->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_MEM:
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM:
> + case VIR_DOMAIN_MEMORY_MODEL_VIRTIO_PMEM:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("cannot modify memory of model '%s'"),
> + virDomainMemoryModelTypeToString(oldDef->model));
> + return false;
... but at the end only allow virtio-mem, so the irrelevant checks are
actually not even necessary.
> + break;
> + }
> +
> + return true;
> +}
> +
> +
> +static int
> +qemuDomainChangeMemoryLive(virQEMUDriver *driver G_GNUC_UNUSED,
> + virDomainObj *vm,
> + virDomainDeviceDef *dev)
> +{
> + virDomainMemoryDef *newDef = dev->data.memory;
> + virDomainMemoryDef *oldDef = NULL;
> +
> + oldDef = virDomainMemoryFindByDeviceInfo(vm->def, &dev->data.memory->info);
> + if (!oldDef) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("memory '%s' not found"), dev->data.memory->info.alias);
[1]
> + return -1;
> + }
> +
> + if (!qemuDomainChangeMemoryLiveValidateChange(oldDef, newDef))
> + return -1;
> +
> + if (qemuDomainChangeMemoryRequestedSize(driver, vm,
> + newDef->info.alias,
> + newDef->requestedsize) < 0)
> + return -1;
> +
> + oldDef->requestedsize = newDef->requestedsize;
> + return 0;
> +}
> +
> +
> static int
> qemuDomainUpdateDeviceLive(virDomainObj *vm,
> virDomainDeviceDef *dev,
> @@ -7171,6 +7330,18 @@ qemuDomainUpdateDeviceLive(virDomainObj *vm,
> ret = qemuDomainChangeNet(driver, vm, dev);
> break;
>
> + case VIR_DOMAIN_DEVICE_MEMORY:
> + oldDev.data.memory = virDomainMemoryFindByDeviceInfo(vm->def, &dev->data.memory->info);
> + if (oldDev.data.memory) {
Also this is a bit weird too. Here you do this optionally if
virDomainMemoryFindByDeviceInfo finds the device ...
> + if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
> + VIR_DOMAIN_DEVICE_ACTION_UPDATE,
> + true) < 0)
> + return -1;
> + }
> +
> + ret = qemuDomainChangeMemoryLive(driver, vm, dev);
... but here the first thing that is done is that virDomainMemoryFindByDeviceInfo
is called again and a NULL return is fatal.
> + break;
> +
> case VIR_DOMAIN_DEVICE_FS:
> case VIR_DOMAIN_DEVICE_INPUT:
> case VIR_DOMAIN_DEVICE_SOUND:
With the wierdness reduced:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
