On 03/04/2015 11:24 AM, Peter Krempa wrote:
> When using 'dimm' memory devices with qemu, some of the information
> like the slot number and base address need to be reloaded from qemu
> after process start so that it reflects the actual state. The state then
> allows to use memory devices across migrations.
> ---
> src/qemu/qemu_domain.c | 49 +++++++++++++++++
> src/qemu/qemu_domain.h | 4 ++
> src/qemu/qemu_monitor.c | 42 +++++++++++++++
> src/qemu/qemu_monitor.h | 14 +++++
> src/qemu/qemu_monitor_json.c | 122 +++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_monitor_json.h | 5 ++
> src/qemu/qemu_process.c | 4 ++
> 7 files changed, 240 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4225f38..61b0fc8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2784,6 +2784,55 @@ qemuDomainUpdateDeviceList(virQEMUDriverPtr driver,
> return 0;
> }
>
> +
> +int
> +qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + int asyncJob)
> +{
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + virHashTablePtr meminfo = NULL;
> + int rc;
> + size_t i;
> +
> + if (vm->def->nmems == 0)
> + return 0;
> +
> + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> + return -1;
> +
> + rc = qemuMonitorGetMemoryDeviceInfo(priv->mon, &meminfo);
> +
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + return -1;
> +
> + /* if qemu doesn't support the info request, just carry on */
> + if (rc == -2)
> + rc = 0;
[1] hmmm
Cannot remember if we agreed previously that not having a message was ok
(e.g. "requires json" or "requires query-memory-devices command"). I
guess I figure if we get this far, then some other check has failed us.
But, but returning 0 we say - yep it worked, which of course isn't true.
I'm conflicted since future patches become affected...
> +
> + if (rc < 0)
> + return -1;
> +
> + for (i = 0; i < vm->def->nmems; i++) {
[1] If "the real" rc == -2, then vm->def->nmems > 0 and we enter this
loop which is probably not a good idea.
> + virDomainMemoryDefPtr mem = vm->def->mems[i];
> + qemuMonitorMemoryDeviceInfoPtr dimm;
> +
> + if (!mem->info.alias)
> + continue;
> +
> + if (!(dimm = virHashLookup(meminfo, mem->info.alias)))
> + continue;
> +
> + mem->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM;
> + mem->info.addr.dimm.slot = dimm->slot;
> + mem->info.addr.dimm.base = dimm->address;
> + }
> +
> + virHashFree(meminfo);
> + return 0;
> +}
> +
> +
> bool
> qemuDomainDefCheckABIStability(virQEMUDriverPtr driver,
> virDomainDefPtr src,
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 9760095..b2be323 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -391,6 +391,10 @@ extern virDomainDefParserConfig virQEMUDriverDomainDefParserConfig;
> int qemuDomainUpdateDeviceList(virQEMUDriverPtr driver,
> virDomainObjPtr vm, int asyncJob);
>
> +int qemuDomainUpdateMemoryDeviceInfo(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + int asyncJob);
> +
> bool qemuDomainDefCheckABIStability(virQEMUDriverPtr driver,
> virDomainDefPtr src,
> virDomainDefPtr dst);
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 94495cd..34673e1 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -4359,3 +4359,45 @@ void qemuMonitorIOThreadsInfoFree(qemuMonitorIOThreadsInfoPtr iothread)
> VIR_FREE(iothread->name);
> VIR_FREE(iothread);
> }
> +
> +
> +/**
> + * qemuMonitorGetMemoryDeviceInfo:
> + * @mon: pointer to the monitor
> + * @info: Location to return the hash of qemuMonitorMemoryDeviceInfo
> + *
> + * Retrieve state and addresses of frontend memory devices present in
> + * the guest.
> + *
> + * Returns 0 on success and fills @info with a newly allocated struct; if the
> + * data can't be retrieved due to lack of support in qemu, returns -2. On
> + * other errors returns -1.
> + */
> +int
> +qemuMonitorGetMemoryDeviceInfo(qemuMonitorPtr mon,
> + virHashTablePtr *info)
> +{
> + VIR_DEBUG("mon=%p info=%p", mon, info);
> + int ret;
> +
> + *info = NULL;
> +
> + if (!mon) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("monitor must not be NULL"));
> + return -1;
> + }
> +
> + if (!mon->json)
> + return -2;
> +
> + if (!(*info = virHashCreate(10, virHashValueFree)))
> + return -1;
> +
> + if ((ret = qemuMonitorJSONGetMemoryDeviceInfo(mon, *info)) < 0) {
> + virHashFree(*info);
> + *info = NULL;
> + }
> +
> + return ret;
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 133d42d..811ce49 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -892,6 +892,20 @@ int qemuMonitorGetIOThreads(qemuMonitorPtr mon,
>
> void qemuMonitorIOThreadsInfoFree(qemuMonitorIOThreadsInfoPtr iothread);
>
> +typedef struct _qemuMonitorMemoryDeviceInfo qemuMonitorMemoryDeviceInfo;
> +typedef qemuMonitorMemoryDeviceInfo *qemuMonitorMemoryDeviceInfoPtr;
> +
> +struct _qemuMonitorMemoryDeviceInfo {
> + unsigned long long address;
> + unsigned int slot;
> + bool hotplugged;
> + bool hotpluggable;
> +};
> +
> +int qemuMonitorGetMemoryDeviceInfo(qemuMonitorPtr mon,
> + virHashTablePtr *info)
> + ATTRIBUTE_NONNULL(2);
> +
> /**
> * When running two dd process and using <> redirection, we need a
> * shell that will not truncate files. These two strings serve that
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 832f589..e463988 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -6569,3 +6569,125 @@ qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon,
> virJSONValueFree(reply);
> return ret;
> }
> +
> +
> +int
> +qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
> + virHashTablePtr info)
> +{
> + int ret = -1;
> + virJSONValuePtr cmd;
> + virJSONValuePtr reply = NULL;
> + virJSONValuePtr data = NULL;
> + qemuMonitorMemoryDeviceInfoPtr meminfo = NULL;
> + ssize_t n;
> + size_t i;
> +
> + if (!(cmd = qemuMonitorJSONMakeCommand("query-memory-devices", NULL)))
> + return -1;
> +
> + ret = qemuMonitorJSONCommand(mon, cmd, &reply);
> +
> + if (ret == 0) {
> + if (qemuMonitorJSONHasError(reply, "CommandNotFound")) {
> + ret = -2;
> + goto cleanup;
> + }
> +
> + ret = qemuMonitorJSONCheckError(cmd, reply);
> + }
> +
> + if (ret < 0)
> + goto cleanup;
> +
> + ret = -1;
> +
> + if (!(data = virJSONValueObjectGet(reply, "return"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("query-memory-devices reply was missing return data"));
> + goto cleanup;
> + }
> +
> + if ((n = virJSONValueArraySize(data)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("query-memory-devices reply data was not an array"));
> + goto cleanup;
> + }
> +
> + for (i = 0; i < n; i++) {
> + virJSONValuePtr elem = virJSONValueArrayGet(data, i);
> + const char *type;
> +
> + if (!(type = virJSONValueObjectGetString(elem, "type"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("query-memory-devices reply data doesn't contain "
> + "enum type discriminator"));
> + goto cleanup;
> + }
> +
> + /* dimm memory devices */
> + if (STREQ(type, "dimm")) {
> + virJSONValuePtr dimminfo;
> + const char *devalias;
> +
> + if (!(dimminfo = virJSONValueObjectGet(elem, "data"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("query-memory-devices reply data doesn't "
> + "contain enum data"));
> + goto cleanup;
> + }
> +
> + if (!(devalias = virJSONValueObjectGetString(dimminfo, "id"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("dimm memory info data is missing 'id'"));
> + goto cleanup;
> + }
> +
> + if (VIR_ALLOC(meminfo) < 0)
> + goto cleanup;
> +
> + if (virJSONValueObjectGetNumberUlong(dimminfo, "addr",
> + &meminfo->address) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed/missing addr in dimm memory info"));
> + goto cleanup;
> + }
> +
> + if (virJSONValueObjectGetNumberUint(dimminfo, "slot",
> + &meminfo->slot) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed/missing slot in dimm memory info"));
> + goto cleanup;
> + }
> +
> + if (virJSONValueObjectGetBoolean(dimminfo, "hotplugged",
> + &meminfo->hotplugged) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed/missing hotplugged in dimm memory info"));
> + goto cleanup;
> +
> + }
> +
> + if (virJSONValueObjectGetBoolean(dimminfo, "hotpluggable",
> + &meminfo->hotpluggable) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed/missing hotpluggable in dimm memory info"));
> + goto cleanup;
> +
> + }
> +
> + if (virHashAddEntry(info, devalias, meminfo) < 0)
> + goto cleanup;
> +
> + meminfo = NULL;
> + }
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(meminfo);
> + virJSONValueFree(cmd);
> + virJSONValueFree(reply);
> + return ret;
> +}
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 1da1a00..eb51a60 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -475,4 +475,9 @@ int qemuMonitorJSONRTCResetReinjection(qemuMonitorPtr mon);
> int qemuMonitorJSONGetIOThreads(qemuMonitorPtr mon,
> qemuMonitorIOThreadsInfoPtr **iothreads)
> ATTRIBUTE_NONNULL(2);
> +
> +int qemuMonitorJSONGetMemoryDeviceInfo(qemuMonitorPtr mon,
> + virHashTablePtr info)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
> #endif /* QEMU_MONITOR_JSON_H */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 019b477..fecc20c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4989,6 +4989,10 @@ int qemuProcessStart(virConnectPtr conn,
> if (qemuDomainUpdateDeviceList(driver, vm, asyncJob) < 0)
> goto cleanup;
>
> + VIR_DEBUG("Updating info of memory devices");
> + if (qemuDomainUpdateMemoryDeviceInfo(driver, vm, asyncJob) < 0)
> + goto cleanup;
> +
> /* Technically, qemuProcessStart can be called from inside
> * QEMU_ASYNC_JOB_MIGRATION_IN, but we are okay treating this like
> * a sync job since no other job can call into the domain until
>
There's nothing through the qemuProcessAttach processing for this data
(although there is balloon info processing)
- Decision on error handling of -2 or not
- Don't drop into the loop to look at returned data if we had -2 returned
- And add some sort of qemuProcessAttach handling...
Just so it doesn't impede progress, I'm fine with a future follow-up
patch for the qemuProcessAttach. Leaving only handling the second point
above for an ACK
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list