On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
> Use the query-gic-capabilities QMP command to probe GIC
> capabilities for a QEMU binary.
>
> The information obtained is stored in virQEMUCaps.
Once finally pushed to the qemu.git repo, perhaps reference the qemu
commit id...
> ---
> src/qemu/qemu_capabilities.c | 24 ++++++++++++
> src/qemu/qemu_monitor.c | 10 +++++
> src/qemu/qemu_monitor.h | 4 ++
> src/qemu/qemu_monitor_json.c | 90 ++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_monitor_json.h | 4 ++
> src/util/virgic.h | 13 +++++++
> 6 files changed, 145 insertions(+)
Is there a way to set a capabilities bit to determine if the
"query-gic-capabilities" command exists? Perhaps virQEMUCapsCommands?
Changes a check later on in qemu_monitor_json.c
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 4b1e750..e54208a 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -354,6 +354,9 @@ struct _virQEMUCaps {
> char **machineTypes;
> char **machineAliases;
> unsigned int *machineMaxCpus;
> +
> + size_t ngicCapabilities;
> + virGICCapability *gicCapabilities;
Any reason to not use virGICCapabilityPtr ?
> };
>
> struct virQEMUCapsSearchData {
> @@ -2690,6 +2693,22 @@ virQEMUCapsProbeQMPMigrationCapabilities(virQEMUCapsPtr qemuCaps,
> return 0;
> }
>
May as well go with the 2 empty lines between functions here.
Need some intro comments, arguments, returns, etc. Not every command
does it, but let's try to add some
> +static int
> +virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps,
> + qemuMonitorPtr mon)
> +{
> + virGICCapability *caps = NULL;
virGICCapabilityPtr ?
> + int ncaps;
> +
> + if ((ncaps = qemuMonitorGetGICCapabilities(mon, &caps)) < 0)
> + return -1;
> +
> + qemuCaps->gicCapabilities = caps;
> + qemuCaps->ngicCapabilities = ncaps;
You will need to VIR_FREE(qemuCaps->gicCapabilities) in
virQEMUCapsDispose and virQEMUCapsReset (looked up where machineTypes
and machineAliases free'd memory).
> +
> + return 0;
> +}
> +
> int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps,
> qemuMonitorPtr mon)
> {
> @@ -3410,6 +3429,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
> if (virQEMUCapsProbeQMPMigrationCapabilities(qemuCaps, mon) < 0)
> goto cleanup;
>
> + /* GIC capabilities, eg. available GIC versions */
> + if (ARCH_IS_ARM(qemuCaps->arch) &&
I note that patch 5 only fills in the domain feature caps based on
VIR_ARCH_ARMV7L and VIR_ARCH_AARCH64... Does that perhaps mean we need
some sort of "second" ARCH_IS_ARM type macro? There are some other
places where the two are checked...
> + virQEMUCapsProbeQMPGICCapabilities(qemuCaps, mon) < 0)
> + goto cleanup;
> +
> ret = 0;
> cleanup:
> return ret;
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index ace3bb4..ac5fe8c 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3552,6 +3552,16 @@ qemuMonitorSetMigrationCapability(qemuMonitorPtr mon,
>
>
Although not always done, would be nice to have some intro comments,
arguments, returns, etc. Especially since this returns 'n' capabilities
that were found and **capabilities is allocated and must be free'd by
caller.
> int
> +qemuMonitorGetGICCapabilities(qemuMonitorPtr mon,
> + virGICCapability **capabilities)
virGICCapabilityPtr ?
> +{
> + QEMU_CHECK_MONITOR_JSON(mon);
> +
> + return qemuMonitorJSONGetGICCapabilities(mon, capabilities);
> +}
> +
> +
> +int
> qemuMonitorNBDServerStart(qemuMonitorPtr mon,
> const char *host,
> unsigned int port)
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 4467a41..550eae0 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -34,6 +34,7 @@
> # include "virnetdev.h"
> # include "device_conf.h"
> # include "cpu/cpu.h"
> +# include "util/virgic.h"
>
> typedef struct _qemuMonitor qemuMonitor;
> typedef qemuMonitor *qemuMonitorPtr;
> @@ -545,6 +546,9 @@ int qemuMonitorSetMigrationCapability(qemuMonitorPtr mon,
> qemuMonitorMigrationCaps capability,
> bool state);
>
> +int qemuMonitorGetGICCapabilities(qemuMonitorPtr mon,
> + virGICCapability **capabilities);
virGICCapabilityPtr ?
> +
> typedef enum {
> QEMU_MONITOR_MIGRATE_BACKGROUND = 1 << 0,
> QEMU_MONITOR_MIGRATE_NON_SHARED_DISK = 1 << 1, /* migration with non-shared storage with full disk copy */
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 8352e53..dee2ee4 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5744,6 +5744,96 @@ qemuMonitorJSONSetMigrationCapability(qemuMonitorPtr mon,
> return ret;
> }
>
Although not always followed in this module, add two blank lines between
API's...
Also would be nice to have a few comments here, especially the stuff put
into the qemu submit regarding what gets returned (something else we're
not very good at). e.g.:
{"return": [{"emulated": false, "version": 3, "kernel": true},
{"emulated": true, "version": 2, "kernel": false}]}
This API returns the number of GIC capabilities... -1 on failure... 0 if
command not found (although that could change) and if nothing returned
from command query.
> +int
> +qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon,
> + virGICCapability **capabilities)
virGICCapabilityPtr ?
> +{
> + int ret;
> + virJSONValuePtr cmd;
> + virJSONValuePtr reply = NULL;
> + virJSONValuePtr caps;
> + virGICCapability *list = NULL;
virGICCapabilityPtr ?
> + size_t i;
> + ssize_t n;
> +
> + *capabilities = NULL;
> +
> + if (!(cmd = qemuMonitorJSONMakeCommand("query-gic-capabilities",
> + NULL)))
> + return -1;
> +
> + ret = qemuMonitorJSONCommand(mon, cmd, &reply);
> +
> + if (ret == 0) {
> + if (qemuMonitorJSONHasError(reply, "CommandNotFound"))
> + goto cleanup;
One would hope that if we checked the capability to have the command
before calling this, then this check wouldn't be necessary.
In any case, if it is necessary to keep, then we don't fail back to the
original caller (virQEMUCapsProbeQMPGICCapabilities). Not a problem I
suppose, the caller would get a 'ncaps == 0' and "caps == NULL"...
> + ret = qemuMonitorJSONCheckError(cmd, reply);
> + }
> +
> + if (ret < 0)
> + goto cleanup;
> +
> + ret = -1;
> +
> + if (!(caps = virJSONValueObjectGetArray(reply, "return")) ||
> + (n = virJSONValueArraySize(caps)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing GIC capabilities"));
> + goto cleanup;
> + }
> +
Should n == 0 here? Would that be an error? e.g., change the < 0 to <=
0 above.
> + if (VIR_ALLOC_N(list, n + 1) < 0)
> + goto cleanup;
why + 1? (it's not done in patch 6, virQEMUCapsLoadCache)
> +
> + for (i = 0; i < n; i++) {
> + virJSONValuePtr cap = virJSONValueArrayGet(caps, i);
> + int version;
> + bool kernel;
> + bool emulated;
> +
> + if (!cap || cap->type != VIR_JSON_TYPE_OBJECT) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing entry in GIC capabilities list"));
> + goto cleanup;
> + }
> +
> + if (virJSONValueObjectGetNumberInt(cap, "version", &version) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing GIC version"));
> + goto cleanup;
> + }
> +
> + if (virJSONValueObjectGetBoolean(cap, "kernel", &kernel) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing in-kernel GIC information"));
> + goto cleanup;
> + }
> +
> + if (virJSONValueObjectGetBoolean(cap, "emulated", &emulated) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing emulated GIC information"));
> + goto cleanup;
> + }
> +
> + list[i].version = version;
> + if (kernel)
> + list[i].implementation |= VIR_GIC_IMPLEMENTATION_KERNEL;
> + if (emulated)
> + list[i].implementation |= VIR_GIC_IMPLEMENTATION_EMULATED;
> + }
> +
> + ret = n;
> + *capabilities = list;
> +
> + cleanup:
> + if (ret < 0)
> + VIR_FREE(list);
> + virJSONValueFree(cmd);
> + virJSONValueFree(reply);
> +
> + return ret;
> +}
> +
> static virJSONValuePtr
> qemuMonitorJSONBuildInetSocketAddress(const char *host,
> const char *port)
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 4068187..09a8269 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -30,6 +30,7 @@
> # include "qemu_monitor.h"
> # include "virbitmap.h"
> # include "cpu/cpu.h"
> +# include "util/virgic.h"
>
> int qemuMonitorJSONIOProcess(qemuMonitorPtr mon,
> const char *data,
> @@ -137,6 +138,9 @@ int qemuMonitorJSONSetMigrationCapability(qemuMonitorPtr mon,
> qemuMonitorMigrationCaps capability,
> bool state);
>
> +int qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon,
> + virGICCapability **capabilities);
virGICCapabilityPtr ?
John
> +
> int qemuMonitorJSONMigrate(qemuMonitorPtr mon,
> unsigned int flags,
> const char *uri);
> diff --git a/src/util/virgic.h b/src/util/virgic.h
> index 470ce95..1c9efd6 100644
> --- a/src/util/virgic.h
> +++ b/src/util/virgic.h
> @@ -38,4 +38,17 @@ VIR_ENUM_DECL(virGICVersion);
> /* Consider GIC v2 the default */
> # define VIR_GIC_VERSION_DEFAULT VIR_GIC_VERSION_2
>
> +typedef enum {
> + VIR_GIC_IMPLEMENTATION_NONE = 0,
> + VIR_GIC_IMPLEMENTATION_KERNEL = (1 << 1),
> + VIR_GIC_IMPLEMENTATION_EMULATED = (1 << 2)
> +} virGICImplementation;
> +
> +typedef struct _virGICCapability virGICCapability;
> +typedef virGICCapability *virGICCapabilityPtr;
> +struct _virGICCapability {
> + virGICVersion version;
> + virGICImplementation implementation;
> +};
> +
> #endif /* __VIR_GIC_H__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list