Re: [PATCHv2.5 05/10] qemu: memdev: Add infrastructure to load memory device information

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]