[libvirt] [RFC 0/6] Probe and expose GIC capabilities
This series implements support for asking QEMU what GIC versions can be used for guests, eg: <features> <gic version='2'/> </features> and exposing such information to users via domain capabilities. QEMU patches that implement the query-gic-capabilities QMP command: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04465.html Cheers. Andrea Bolognani (6): conf: Get rid of virDomainCapsDevice qemu: Probe GIC capabilities schema: Validate GIC capabilities conf: Expose GIC capabilities qemu: Fill in GIC capabilities qemu: Cache GIC capabilities docs/schemas/domaincaps.rng | 18 +++ src/conf/domain_capabilities.c | 26 +++- src/conf/domain_capabilities.h | 24 ++-- src/qemu/qemu_capabilities.c | 157 ++++++++++++++++++++- 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 ++ tests/domaincapsschemadata/domaincaps-basic.xml | 3 + tests/domaincapsschemadata/domaincaps-full.xml | 3 + .../domaincaps-qemu_1.6.50-1.xml | 3 + tests/domaincapstest.c | 8 +- 13 files changed, 341 insertions(+), 22 deletions(-) -- 2.5.5
The struct contains a single boolean field which can be applied to domain capabilities that do not represent device availability. Instead of trying to come up with a more generic name just get rid of the struct altogether. --- src/conf/domain_capabilities.c | 6 +++--- src/conf/domain_capabilities.h | 14 ++++---------- src/qemu/qemu_capabilities.c | 8 ++++---- tests/domaincapstest.c | 8 ++++---- 4 files changed, 15 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 0e32f52..466c0c6 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -187,9 +187,9 @@ virDomainCapsStringValuesFormat(virBufferPtr buf, #define FORMAT_PROLOGUE(item) \ do { \ virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ - item->device.supported ? "yes" : "no", \ - item->device.supported ? ">" : "/>"); \ - if (!item->device.supported) \ + item->supported ? "yes" : "no", \ + item->supported ? ">" : "/>"); \ + if (!item->supported) \ return; \ virBufferAdjustIndent(buf, 2); \ } while (0) diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 597ac75..3eacb35 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -44,16 +44,10 @@ struct _virDomainCapsStringValues { size_t nvalues; /* number of strings */ }; -typedef struct _virDomainCapsDevice virDomainCapsDevice; -typedef virDomainCapsDevice *virDomainCapsDevicePtr; -struct _virDomainCapsDevice { - bool supported; /* true if <devtype> is supported by hypervisor */ -}; - typedef struct _virDomainCapsLoader virDomainCapsLoader; typedef virDomainCapsLoader *virDomainCapsLoaderPtr; struct _virDomainCapsLoader { - virDomainCapsDevice device; + bool supported; virDomainCapsStringValues values; /* Info about values for the element */ virDomainCapsEnum type; /* Info about virDomainLoader */ virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */ @@ -62,14 +56,14 @@ struct _virDomainCapsLoader { typedef struct _virDomainCapsOS virDomainCapsOS; typedef virDomainCapsOS *virDomainCapsOSPtr; struct _virDomainCapsOS { - virDomainCapsDevice device; + bool supported; virDomainCapsLoader loader; /* Info about virDomainLoaderDef */ }; typedef struct _virDomainCapsDeviceDisk virDomainCapsDeviceDisk; typedef virDomainCapsDeviceDisk *virDomainCapsDeviceDiskPtr; struct _virDomainCapsDeviceDisk { - virDomainCapsDevice device; + bool supported; virDomainCapsEnum diskDevice; /* Info about virDomainDiskDevice enum values */ virDomainCapsEnum bus; /* Info about virDomainDiskBus enum values */ /* add new fields here */ @@ -78,7 +72,7 @@ struct _virDomainCapsDeviceDisk { typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev; typedef virDomainCapsDeviceHostdev *virDomainCapsDeviceHostdevPtr; struct _virDomainCapsDeviceHostdev { - virDomainCapsDevice device; + bool supported; virDomainCapsEnum mode; /* Info about virDomainHostdevMode */ virDomainCapsEnum startupPolicy; /* Info about virDomainStartupPolicy */ virDomainCapsEnum subsysType; /* Info about virDomainHostdevSubsysType */ diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b223837..4b1e750 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3909,7 +3909,7 @@ virQEMUCapsFillDomainLoaderCaps(virQEMUCapsPtr qemuCaps, { size_t i; - capsLoader->device.supported = true; + capsLoader->supported = true; if (VIR_ALLOC_N(capsLoader->values.values, nloader) < 0) return -1; @@ -3951,7 +3951,7 @@ virQEMUCapsFillDomainOSCaps(virQEMUCapsPtr qemuCaps, { virDomainCapsLoaderPtr capsLoader = &os->loader; - os->device.supported = true; + os->supported = true; if (virQEMUCapsFillDomainLoaderCaps(qemuCaps, capsLoader, loader, nloader) < 0) return -1; @@ -3964,7 +3964,7 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps, const char *machine, virDomainCapsDeviceDiskPtr disk) { - disk->device.supported = true; + disk->supported = true; /* QEMU supports all of these */ VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, VIR_DOMAIN_DISK_DEVICE_DISK, @@ -4000,7 +4000,7 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps, bool supportsPassthroughKVM = qemuHostdevHostSupportsPassthroughLegacy(); bool supportsPassthroughVFIO = qemuHostdevHostSupportsPassthroughVFIO(); - hostdev->device.supported = true; + hostdev->supported = true; /* VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES is for containers only */ VIR_DOMAIN_CAPS_ENUM_SET(hostdev->mode, VIR_DOMAIN_HOSTDEV_MODE_SUBSYS); diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index ecefdb9..b6f6ac8 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -65,9 +65,9 @@ fillAll(virDomainCapsPtr domCaps, virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev; domCaps->maxvcpus = 255; - os->device.supported = true; + os->supported = true; - loader->device.supported = true; + loader->supported = true; SET_ALL_BITS(loader->type); SET_ALL_BITS(loader->readonly); if (fillStringValues(&loader->values, @@ -76,11 +76,11 @@ fillAll(virDomainCapsPtr domCaps, NULL) < 0) return -1; - disk->device.supported = true; + disk->supported = true; SET_ALL_BITS(disk->diskDevice); SET_ALL_BITS(disk->bus); - hostdev->device.supported = true; + hostdev->supported = true; SET_ALL_BITS(hostdev->mode); SET_ALL_BITS(hostdev->startupPolicy); SET_ALL_BITS(hostdev->subsysType); -- 2.5.5
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
The struct contains a single boolean field which can be applied to domain capabilities that do not represent device availability.
Instead of trying to come up with a more generic name just get rid of the struct altogether. --- src/conf/domain_capabilities.c | 6 +++--- src/conf/domain_capabilities.h | 14 ++++---------- src/qemu/qemu_capabilities.c | 8 ++++---- tests/domaincapstest.c | 8 ++++---- 4 files changed, 15 insertions(+), 21 deletions(-)
The construct was added as part of commit id '614581f32' - not sure if Michal felt more bool's were going to be added to virDomainCapsDevice. I see this affects patch 4 - I think it would be a good idea to see if Michal had "other designs" in mind before changing this. That could be separate too... John
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 0e32f52..466c0c6 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -187,9 +187,9 @@ virDomainCapsStringValuesFormat(virBufferPtr buf, #define FORMAT_PROLOGUE(item) \ do { \ virBufferAsprintf(buf, "<" #item " supported='%s'%s\n", \ - item->device.supported ? "yes" : "no", \ - item->device.supported ? ">" : "/>"); \ - if (!item->device.supported) \ + item->supported ? "yes" : "no", \ + item->supported ? ">" : "/>"); \ + if (!item->supported) \ return; \ virBufferAdjustIndent(buf, 2); \ } while (0) diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 597ac75..3eacb35 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -44,16 +44,10 @@ struct _virDomainCapsStringValues { size_t nvalues; /* number of strings */ };
-typedef struct _virDomainCapsDevice virDomainCapsDevice; -typedef virDomainCapsDevice *virDomainCapsDevicePtr; -struct _virDomainCapsDevice { - bool supported; /* true if <devtype> is supported by hypervisor */ -}; - typedef struct _virDomainCapsLoader virDomainCapsLoader; typedef virDomainCapsLoader *virDomainCapsLoaderPtr; struct _virDomainCapsLoader { - virDomainCapsDevice device; + bool supported; virDomainCapsStringValues values; /* Info about values for the element */ virDomainCapsEnum type; /* Info about virDomainLoader */ virDomainCapsEnum readonly; /* Info about readonly:virTristateBool */ @@ -62,14 +56,14 @@ struct _virDomainCapsLoader { typedef struct _virDomainCapsOS virDomainCapsOS; typedef virDomainCapsOS *virDomainCapsOSPtr; struct _virDomainCapsOS { - virDomainCapsDevice device; + bool supported; virDomainCapsLoader loader; /* Info about virDomainLoaderDef */ };
typedef struct _virDomainCapsDeviceDisk virDomainCapsDeviceDisk; typedef virDomainCapsDeviceDisk *virDomainCapsDeviceDiskPtr; struct _virDomainCapsDeviceDisk { - virDomainCapsDevice device; + bool supported; virDomainCapsEnum diskDevice; /* Info about virDomainDiskDevice enum values */ virDomainCapsEnum bus; /* Info about virDomainDiskBus enum values */ /* add new fields here */ @@ -78,7 +72,7 @@ struct _virDomainCapsDeviceDisk { typedef struct _virDomainCapsDeviceHostdev virDomainCapsDeviceHostdev; typedef virDomainCapsDeviceHostdev *virDomainCapsDeviceHostdevPtr; struct _virDomainCapsDeviceHostdev { - virDomainCapsDevice device; + bool supported; virDomainCapsEnum mode; /* Info about virDomainHostdevMode */ virDomainCapsEnum startupPolicy; /* Info about virDomainStartupPolicy */ virDomainCapsEnum subsysType; /* Info about virDomainHostdevSubsysType */ diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b223837..4b1e750 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3909,7 +3909,7 @@ virQEMUCapsFillDomainLoaderCaps(virQEMUCapsPtr qemuCaps, { size_t i;
- capsLoader->device.supported = true; + capsLoader->supported = true;
if (VIR_ALLOC_N(capsLoader->values.values, nloader) < 0) return -1; @@ -3951,7 +3951,7 @@ virQEMUCapsFillDomainOSCaps(virQEMUCapsPtr qemuCaps, { virDomainCapsLoaderPtr capsLoader = &os->loader;
- os->device.supported = true; + os->supported = true; if (virQEMUCapsFillDomainLoaderCaps(qemuCaps, capsLoader, loader, nloader) < 0) return -1; @@ -3964,7 +3964,7 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps, const char *machine, virDomainCapsDeviceDiskPtr disk) { - disk->device.supported = true; + disk->supported = true; /* QEMU supports all of these */ VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, VIR_DOMAIN_DISK_DEVICE_DISK, @@ -4000,7 +4000,7 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps, bool supportsPassthroughKVM = qemuHostdevHostSupportsPassthroughLegacy(); bool supportsPassthroughVFIO = qemuHostdevHostSupportsPassthroughVFIO();
- hostdev->device.supported = true; + hostdev->supported = true; /* VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES is for containers only */ VIR_DOMAIN_CAPS_ENUM_SET(hostdev->mode, VIR_DOMAIN_HOSTDEV_MODE_SUBSYS); diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index ecefdb9..b6f6ac8 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -65,9 +65,9 @@ fillAll(virDomainCapsPtr domCaps, virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev; domCaps->maxvcpus = 255;
- os->device.supported = true; + os->supported = true;
- loader->device.supported = true; + loader->supported = true; SET_ALL_BITS(loader->type); SET_ALL_BITS(loader->readonly); if (fillStringValues(&loader->values, @@ -76,11 +76,11 @@ fillAll(virDomainCapsPtr domCaps, NULL) < 0) return -1;
- disk->device.supported = true; + disk->supported = true; SET_ALL_BITS(disk->diskDevice); SET_ALL_BITS(disk->bus);
- hostdev->device.supported = true; + hostdev->supported = true; SET_ALL_BITS(hostdev->mode); SET_ALL_BITS(hostdev->startupPolicy); SET_ALL_BITS(hostdev->subsysType);
On Wed, 2016-03-30 at 13:19 -0400, John Ferlan wrote:
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
The struct contains a single boolean field which can be applied to domain capabilities that do not represent device availability. Instead of trying to come up with a more generic name just get rid of the struct altogether. --- src/conf/domain_capabilities.c | 6 +++--- src/conf/domain_capabilities.h | 14 ++++---------- src/qemu/qemu_capabilities.c | 8 ++++---- tests/domaincapstest.c | 8 ++++---- 4 files changed, 15 insertions(+), 21 deletions(-) The construct was added as part of commit id '614581f32' - not sure if Michal felt more bool's were going to be added to virDomainCapsDevice.
The problem is that, as you noted, patch 4 adds a virDomainCapsFeatureGIC structure - if I were to follow the pattern established with eg. virDomainCapsDeviceDisk, I would have to introduce something like struct _virDomainCapsFeature { bool supported; }; and then use it as the first member of virDomainCapsFeatureGIC; however, that would clash with the virDomainCapsFeature that's already being defined in domain_conf.h. Moreover, the existing FORMAT_PROLOGUE() macro would not be usable for virDomainCapsFeatureGIC, because it checks for item->device.supported and we would probably use something like item->feature.supported instead. Last but not least, the current usage is questionable: neither virDomainCapsOS nor virDomainCapsLoader end up being inside the <devices> element, yet both include virDomainCapsDevice as their first member...
I see this affects patch 4 - I think it would be a good idea to see if Michal had "other designs" in mind before changing this. That could be separate too...
CCing Michal so he can voice his opinion on the matter. Cheers. PS: Don't worry, I have no idea what I was trying to say with the first paragraph of the commit message either :) -- Andrea Bolognani Software Engineer - Virtualization Team
Use the query-gic-capabilities QMP command to probe GIC capabilities for a QEMU binary. The information obtained is stored in virQEMUCaps. --- 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(+) 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; }; struct virQEMUCapsSearchData { @@ -2690,6 +2693,22 @@ virQEMUCapsProbeQMPMigrationCapabilities(virQEMUCapsPtr qemuCaps, return 0; } +static int +virQEMUCapsProbeQMPGICCapabilities(virQEMUCapsPtr qemuCaps, + qemuMonitorPtr mon) +{ + virGICCapability *caps = NULL; + int ncaps; + + if ((ncaps = qemuMonitorGetGICCapabilities(mon, &caps)) < 0) + return -1; + + qemuCaps->gicCapabilities = caps; + qemuCaps->ngicCapabilities = ncaps; + + 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) && + 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, int +qemuMonitorGetGICCapabilities(qemuMonitorPtr mon, + virGICCapability **capabilities) +{ + 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); + 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; } +int +qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon, + virGICCapability **capabilities) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr caps; + virGICCapability *list = NULL; + 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; + 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; + } + + if (VIR_ALLOC_N(list, n + 1) < 0) + goto cleanup; + + 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); + 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__ */ -- 2.5.5
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__ */
On Wed, 2016-03-30 at 13:26 -0400, John Ferlan wrote:
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
Sure, but would that be useful? We should already be handling the case where the QMP command is not available gracefully, eg. returning an empty list of GIC capabilities. How would adding a capabilities bit improve the situation?
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 ?
virGICCapabilityPtr would be appropriate if this was a pointer to a single virGICCapability instance; here we have an array of virGICCapability instances, so I think virGICCapability* is more suitable. Same goes for the other instances you've noted later on.
struct virQEMUCapsSearchData { @@ -2690,6 +2693,22 @@ virQEMUCapsProbeQMPMigrationCapabilities(virQEMUCapsPtr qemuCaps, return 0; } May as well go with the 2 empty lines between functions here.
I really wish we had a standard for how many empty lines there should be between functions. It seems to vary wildly between modules, and even inside the same module :(
Need some intro comments, arguments, returns, etc. Not every command does it, but let's try to add some
Right, I'll add some.
+ 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).
Good catch.
+ + 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...
Again, good catch. There's no reason to be inconsistent here. As for adding a new ARCH_IS_*() macro, I'm honestly not sure what such a macro would look like. I'll try to poke around and come up with some guidelines on which sub-architectures we should check for, instead of just going with whatever's already there.
+ 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"...
'caps == NULL' and 'ncaps == 0' is perfectly fine, that's how the caller can tell the QEMU binary has no GIC capabilities.
+ 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.
Well, 'n < 0' means that virJSONValueArraySize() failed, ie. the QMP command didn't return an array. 'n == 0' means that the returned array is empty, which is perfectly fine. But I realize now that such case is not handled properly: we should return NULL caps, instead we return an empty 1-element array.
+ if (VIR_ALLOC_N(list, n + 1) < 0) + goto cleanup; why + 1? (it's not done in patch 6, virQEMUCapsLoadCache)
Yeah, having '+ 1' here is just a waste of memory. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
We need to expose GIC capabilities in the domain capabilities XML: update the schema to validate documents that contain the new information. --- docs/schemas/domaincaps.rng | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 35d3745..0d2777b 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -31,6 +31,9 @@ <optional> <ref name='devices'/> </optional> + <optional> + <ref name='features'/> + </optional> </interleave> </element> </define> @@ -88,6 +91,21 @@ </element> </define> + <define name='features'> + <element name='features'> + <interleave> + <ref name='gic'/> + </interleave> + </element> + </define> + + <define name='gic'> + <element name='gic'> + <ref name='supported'/> + <ref name='enum'/> + </element> + </define> + <define name='value'> <zeroOrMore> <element name='value'> -- 2.5.5
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
We need to expose GIC capabilities in the domain capabilities XML: update the schema to validate documents that contain the new information. --- docs/schemas/domaincaps.rng | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
I think patches 3 and 4 should be combined and 5 is not too far behind! "features" is fairly generic for something so specific, but I'm not sure I have other great suggestions for a "domain capabilities" section that has path, domain, machine, arch, vcpu, os, and devices already. It is a cpu interrupt controller - so maybe it's an "interrupt" device. When the XML is created what is it going to look like? <features> <gic supported='yes'/> <enum name='version'> <value>2</value> <value>3</value> </enum> </gic> </features> Is putting it after "<devices>" a back compat thing? I guess I would think it was more logical after the <arch> or even more radical as part of it: <arch gic_version='%s' gic_emulated='%s' git_kernel='%s'>armv7l</arch> Additionally, docs/formatdomaincaps.html.in will need an update to describe this... And then there's virsh.pod - not sure if it needs an update... John
diff --git a/docs/schemas/domaincaps.rng b/docs/schemas/domaincaps.rng index 35d3745..0d2777b 100644 --- a/docs/schemas/domaincaps.rng +++ b/docs/schemas/domaincaps.rng @@ -31,6 +31,9 @@ <optional> <ref name='devices'/> </optional> + <optional> + <ref name='features'/> + </optional> </interleave> </element> </define> @@ -88,6 +91,21 @@ </element> </define>
+ <define name='features'> + <element name='features'> + <interleave> + <ref name='gic'/> + </interleave> + </element> + </define> + + <define name='gic'> + <element name='gic'> + <ref name='supported'/> + <ref name='enum'/> + </element> + </define> + <define name='value'> <zeroOrMore> <element name='value'>
On Wed, 2016-03-30 at 15:36 -0400, John Ferlan wrote:
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
We need to expose GIC capabilities in the domain capabilities XML: update the schema to validate documents that contain the new information. --- docs/schemas/domaincaps.rng | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
I think patches 3 and 4 should be combined and 5 is not too far behind!
I usually try to keep patches as small and self-contained as possible, provided doing so doesn't break the check and syntax-check targets and the changes represent some sort of meaningful unit. The balance is of course very hard to get right, so I fully expect other people to disagree with the way I've orgainzed them :) If you feel like squashing a bunch of commits together will help you during review, then I'll definitely do that.
"features" is fairly generic for something so specific, but I'm not sure I have other great suggestions for a "domain capabilities" section that has path, domain, machine, arch, vcpu, os, and devices already. It is a cpu interrupt controller - so maybe it's an "interrupt" device.
In the domain XML, the <gic> element will be a child of the <features> element, so this name seemed like a natural choice. The documentation[1] claims The available features can be found by asking for the capabilities XML so it will have to be updated, because in this case the information would be in the *domain* capabilities and not in the *host* capabilities. At least the name of the containing element will be the same.
When the XML is created what is it going to look like? <features> <gic supported='yes'/> <enum name='version'> <value>2</value> <value>3</value> </enum> </gic> </features>
Yes, it's going to look exactly like that. As you mentioned later on, the documentation will have to contain examples.
Is putting it after "<devices>" a back compat thing? I guess I would think it was more logical after the <arch> or even more radical as part of it: <arch gic_version='%s' gic_emulated='%s' git_kernel='%s'>armv7l</arch>
Users should not be relying on the order of elements anyway, so if we want it to appear right after <arch> I guess there's nothing stopping us from doing so. Merging this into <arch> would not work, because you have to report information about more than a single version... As for the other attributes, the reason why this is in domain capabilities in the first place is because that way the user doesn't need to worry about the difference between 'kernel' and 'emulated': in the capabilities for a kvm domain we will only report the GIC versions that are implemented in kernel, whereas for a qemu domain we will only report those that can be emulated. The user only needs to look at the XML and pick one of the versions listed.
Additionally, docs/formatdomaincaps.html.in will need an update to describe this...
Absolutely.
And then there's virsh.pod - not sure if it needs an update...
Don't think so. Cheers. [1] https://libvirt.org/formatdomain.html#elementsFeatures -- Andrea Bolognani Software Engineer - Virtualization Team
Add information about GIC capabilities to virDomainCaps and update the formatter to include them in the XML output. --- src/conf/domain_capabilities.c | 20 ++++++++++++++++++++ src/conf/domain_capabilities.h | 10 ++++++++++ tests/domaincapsschemadata/domaincaps-basic.xml | 3 +++ tests/domaincapsschemadata/domaincaps-full.xml | 3 +++ .../domaincaps-qemu_1.6.50-1.xml | 3 +++ 5 files changed, 39 insertions(+) diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 466c0c6..d29ed81 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -262,6 +262,18 @@ virDomainCapsDeviceHostdevFormat(virBufferPtr buf, } +static void +virDomainCapsFeatureGICFormat(virBufferPtr buf, + virDomainCapsFeatureGICPtr const gic) +{ + FORMAT_PROLOGUE(gic); + + ENUM_PROCESS(gic, version, virGICVersionTypeToString); + + FORMAT_EPILOGUE(gic); +} + + static int virDomainCapsFormatInternal(virBufferPtr buf, virDomainCapsPtr const caps) @@ -291,6 +303,14 @@ virDomainCapsFormatInternal(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</devices>\n"); + virBufferAddLit(buf, "<features>\n"); + virBufferAdjustIndent(buf, 2); + + virDomainCapsFeatureGICFormat(buf, &caps->gic); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</features>\n"); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</domainCapabilities>\n"); return 0; diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 3eacb35..95afe5e 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -81,6 +81,13 @@ struct _virDomainCapsDeviceHostdev { /* add new fields here */ }; +typedef struct _virDomainCapsFeatureGIC virDomainCapsFeatureGIC; +typedef virDomainCapsFeatureGIC *virDomainCapsFeatureGICPtr; +struct _virDomainCapsFeatureGIC { + bool supported; + virDomainCapsEnum version; /* Info about virGICVersion */ +}; + struct _virDomainCaps { virObjectLockable parent; @@ -96,6 +103,9 @@ struct _virDomainCaps { virDomainCapsDeviceDisk disk; virDomainCapsDeviceHostdev hostdev; /* add new domain devices here */ + + virDomainCapsFeatureGIC gic; + /* add new domain features here */ }; virDomainCapsPtr virDomainCapsNew(const char *path, diff --git a/tests/domaincapsschemadata/domaincaps-basic.xml b/tests/domaincapsschemadata/domaincaps-basic.xml index 6171393..6587d56 100644 --- a/tests/domaincapsschemadata/domaincaps-basic.xml +++ b/tests/domaincapsschemadata/domaincaps-basic.xml @@ -8,4 +8,7 @@ <disk supported='no'/> <hostdev supported='no'/> </devices> + <features> + <gic supported='no'/> + </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/domaincaps-full.xml b/tests/domaincapsschemadata/domaincaps-full.xml index 96202bc..d4f91fa 100644 --- a/tests/domaincapsschemadata/domaincaps-full.xml +++ b/tests/domaincapsschemadata/domaincaps-full.xml @@ -68,4 +68,7 @@ </enum> </hostdev> </devices> + <features> + <gic supported='no'/> + </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml index 37d2102..990661b 100644 --- a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml +++ b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml @@ -56,4 +56,7 @@ </enum> </hostdev> </devices> + <features> + <gic supported='no'/> + </features> </domainCapabilities> -- 2.5.5
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
Add information about GIC capabilities to virDomainCaps and update the formatter to include them in the XML output. --- src/conf/domain_capabilities.c | 20 ++++++++++++++++++++ src/conf/domain_capabilities.h | 10 ++++++++++ tests/domaincapsschemadata/domaincaps-basic.xml | 3 +++ tests/domaincapsschemadata/domaincaps-full.xml | 3 +++ .../domaincaps-qemu_1.6.50-1.xml | 3 +++ 5 files changed, 39 insertions(+)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 466c0c6..d29ed81 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -262,6 +262,18 @@ virDomainCapsDeviceHostdevFormat(virBufferPtr buf, }
+static void +virDomainCapsFeatureGICFormat(virBufferPtr buf, + virDomainCapsFeatureGICPtr const gic) +{ + FORMAT_PROLOGUE(gic); + + ENUM_PROCESS(gic, version, virGICVersionTypeToString); + + FORMAT_EPILOGUE(gic); +} + + static int virDomainCapsFormatInternal(virBufferPtr buf, virDomainCapsPtr const caps) @@ -291,6 +303,14 @@ virDomainCapsFormatInternal(virBufferPtr buf, virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</devices>\n");
+ virBufferAddLit(buf, "<features>\n"); + virBufferAdjustIndent(buf, 2); + + virDomainCapsFeatureGICFormat(buf, &caps->gic); + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</features>\n"); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</domainCapabilities>\n"); return 0; diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 3eacb35..95afe5e 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -81,6 +81,13 @@ struct _virDomainCapsDeviceHostdev { /* add new fields here */ };
+typedef struct _virDomainCapsFeatureGIC virDomainCapsFeatureGIC; +typedef virDomainCapsFeatureGIC *virDomainCapsFeatureGICPtr; +struct _virDomainCapsFeatureGIC { + bool supported;
Hmmm.. I see - I guess depends on how patch 1/6 is handled...
+ virDomainCapsEnum version; /* Info about virGICVersion */ +}; + struct _virDomainCaps { virObjectLockable parent;
@@ -96,6 +103,9 @@ struct _virDomainCaps { virDomainCapsDeviceDisk disk; virDomainCapsDeviceHostdev hostdev; /* add new domain devices here */ + + virDomainCapsFeatureGIC gic; + /* add new domain features here */ };
virDomainCapsPtr virDomainCapsNew(const char *path, diff --git a/tests/domaincapsschemadata/domaincaps-basic.xml b/tests/domaincapsschemadata/domaincaps-basic.xml index 6171393..6587d56 100644 --- a/tests/domaincapsschemadata/domaincaps-basic.xml +++ b/tests/domaincapsschemadata/domaincaps-basic.xml @@ -8,4 +8,7 @@ <disk supported='no'/> <hostdev supported='no'/> </devices> + <features> + <gic supported='no'/> + </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/domaincaps-full.xml b/tests/domaincapsschemadata/domaincaps-full.xml index 96202bc..d4f91fa 100644 --- a/tests/domaincapsschemadata/domaincaps-full.xml +++ b/tests/domaincapsschemadata/domaincaps-full.xml @@ -68,4 +68,7 @@ </enum> </hostdev> </devices> + <features> + <gic supported='no'/> + </features> </domainCapabilities> diff --git a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml index 37d2102..990661b 100644 --- a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml +++ b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml @@ -56,4 +56,7 @@ </enum> </hostdev> </devices> + <features> + <gic supported='no'/> + </features> </domainCapabilities>
I first wondered why the XML file was modified, but later understood - sort of... Still not fully clear on how what this schema output should look like... For prior to 2.6 should it even be printed? For 2.6 and beyond what would it look like? John
On Wed, 2016-03-30 at 16:10 -0400, John Ferlan wrote:
diff --git a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml b/tests/domaincapsschemadata/domaincaps- qemu_1.6.50-1.xml index 37d2102..990661b 100644 --- a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml +++ b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml @@ -56,4 +56,7 @@ </enum> </hostdev> </devices> + <features> + <gic supported='no'/> + </features> </domainCapabilities> I first wondered why the XML file was modified, but later understood - sort of...
We will need more tests for the new feature, of course. I wanted to gather feedback on the interface with the RFC before I spent too much time testing or documenting it.
Still not fully clear on how what this schema output should look like... For prior to 2.6 should it even be printed? For 2.6 and beyond what would it look like?
There might be a case for not printing it, but we don't perform such optimization for the rest of the information that's part of domain capabilities, so I'm not sure whether we should bother in this case. Versions prior to 2.6, and 2.6 itself on non-ARM architectures would yield <features> <gic supported='no'/> </features> whereas 2.6 onward on ARM would yield something like <features> <gic supported='yes'> <enum name='version'> <value>2</value> </enum> </gic> </features> which mirrors exactly the type of <gic> element the user can expect to be able to use inside the <feature> element in the domain XML, eg. <features> <gic version='2'/> </features> or none at all. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 04/06/2016 10:39 AM, Andrea Bolognani wrote:
On Wed, 2016-03-30 at 16:10 -0400, John Ferlan wrote:
diff --git a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml b/tests/domaincapsschemadata/domaincaps- qemu_1.6.50-1.xml index 37d2102..990661b 100644 --- a/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml +++ b/tests/domaincapsschemadata/domaincaps-qemu_1.6.50-1.xml @@ -56,4 +56,7 @@ </enum> </hostdev> </devices> + <features> + <gic supported='no'/> + </features> </domainCapabilities>
I first wondered why the XML file was modified, but later understood - sort of...
We will need more tests for the new feature, of course.
I wanted to gather feedback on the interface with the RFC before I spent too much time testing or documenting it.
Still not fully clear on how what this schema output should look like... For prior to 2.6 should it even be printed? For 2.6 and beyond what would it look like?
There might be a case for not printing it, but we don't perform such optimization for the rest of the information that's part of domain capabilities, so I'm not sure whether we should bother in this case.
Versions prior to 2.6, and 2.6 itself on non-ARM architectures would yield
<features> <gic supported='no'/> </features>
whereas 2.6 onward on ARM would yield something like
<features> <gic supported='yes'> <enum name='version'> <value>2</value> </enum> </gic> </features>
which mirrors exactly the type of <gic> element the user can expect to be able to use inside the <feature> element in the domain XML, eg.
<features> <gic version='2'/> </features>
Unfortunately there's not a ton of prior art WRT domcapabilities to go off of, but FWIW with my virt-manager/libvirt consumer hat on that all seems sensible for the domcaps schema. It mirrors the domain XML, and reports the actual values we will want to pass to the domain XML, so we don't need to translate between magic strings or anything. - Cole
Take the GIC capabilities stored in a virQEMUCaps instance and update a virDomainCaps instance appropriately. --- src/qemu/qemu_capabilities.c | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e54208a..64007f0 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4065,6 +4065,41 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps, } +static int +virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps, + virDomainCapsPtr domCaps) +{ + virDomainCapsFeatureGICPtr gic = &domCaps->gic; + size_t i; + + if (domCaps->arch != VIR_ARCH_ARMV7L && + domCaps->arch != VIR_ARCH_AARCH64) + return 0; + + if (STRNEQ(domCaps->machine, "virt") && + !STRPREFIX(domCaps->machine, "virt-")) + return 0; + + for (i = 0; i < qemuCaps->ngicCapabilities; i++) { + virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i]; + + if (domCaps->virttype == VIR_DOMAIN_VIRT_KVM && + !(cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL)) + continue; + + if (domCaps->virttype == VIR_DOMAIN_VIRT_QEMU && + !(cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED)) + continue; + + gic->supported = true; + VIR_DOMAIN_CAPS_ENUM_SET(gic->version, + cap->version); + } + + return 0; +} + + int virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, virQEMUCapsPtr qemuCaps, @@ -4081,7 +4116,8 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, if (virQEMUCapsFillDomainOSCaps(qemuCaps, os, loader, nloader) < 0 || virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, domCaps->machine, disk) < 0 || - virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0) + virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 || + virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0) return -1; return 0; } -- 2.5.5
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
Take the GIC capabilities stored in a virQEMUCaps instance and update a virDomainCaps instance appropriately. --- src/qemu/qemu_capabilities.c | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-)
I think domaincapstest.c should be modified to add a (new) 2.6 version of the *.caps file. One that has the supported='yes' set. and this is probably where the docs get modified to add the new elements...
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e54208a..64007f0 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4065,6 +4065,41 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps, }
+static int +virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps, + virDomainCapsPtr domCaps) +{ + virDomainCapsFeatureGICPtr gic = &domCaps->gic; + size_t i; + + if (domCaps->arch != VIR_ARCH_ARMV7L && + domCaps->arch != VIR_ARCH_AARCH64) + return 0; + + if (STRNEQ(domCaps->machine, "virt") && + !STRPREFIX(domCaps->machine, "virt-")) + return 0; + + for (i = 0; i < qemuCaps->ngicCapabilities; i++) { + virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i]; + + if (domCaps->virttype == VIR_DOMAIN_VIRT_KVM && + !(cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL)) + continue; + + if (domCaps->virttype == VIR_DOMAIN_VIRT_QEMU && + !(cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED)) + continue;
For these, patch 6 would need to be already in place I think if circumstances were "just right" (so to speak).
+ + gic->supported = true; + VIR_DOMAIN_CAPS_ENUM_SET(gic->version, + cap->version);
Can there be more than one? How is that handled! IOW: Once we print, do we just break;? John
+ } + + return 0; +} + + int virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, virQEMUCapsPtr qemuCaps, @@ -4081,7 +4116,8 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, if (virQEMUCapsFillDomainOSCaps(qemuCaps, os, loader, nloader) < 0 || virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, domCaps->machine, disk) < 0 || - virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0) + virQEMUCapsFillDomainDeviceHostdevCaps(qemuCaps, hostdev) < 0 || + virQEMUCapsFillDomainFeatureGICCaps(qemuCaps, domCaps) < 0) return -1; return 0; }
On Wed, 2016-03-30 at 16:12 -0400, John Ferlan wrote:
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
+static int +virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps, + virDomainCapsPtr domCaps) +{ + virDomainCapsFeatureGICPtr gic = &domCaps->gic; + size_t i; + + if (domCaps->arch != VIR_ARCH_ARMV7L && + domCaps->arch != VIR_ARCH_AARCH64) + return 0; + + if (STRNEQ(domCaps->machine, "virt") && + !STRPREFIX(domCaps->machine, "virt-")) + return 0; + + for (i = 0; i < qemuCaps->ngicCapabilities; i++) { + virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i]; + + if (domCaps->virttype == VIR_DOMAIN_VIRT_KVM && + !(cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL)) + continue; + + if (domCaps->virttype == VIR_DOMAIN_VIRT_QEMU && + !(cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED)) + continue; For these, patch 6 would need to be already in place I think if circumstances were "just right" (so to speak).
The feature is not really expected to work properly until the entire series has been applied, right? We can still shuffle patches around to implement proper caching of the capabilities even before such capabilities are actually filled in, but I don't think it's a blocker per se.
+ + gic->supported = true; + VIR_DOMAIN_CAPS_ENUM_SET(gic->version, + cap->version); Can there be more than one?
Definitely! An example is hardware supporting both GIC v2 and GIC v3; another one is QEMU eventually implementing an emulated GIC v3. In both cases we'll end up with <features> <gic supported='yes'> <enum name='version'> <value>2</value> <value>3</value> </enum> </gic> </features>
How is that handled!
With a bit of magic, really ;) Basically virDomainCapsEnumSet() packs enum values very tightly, as if they were declared as flags to begin with, and virDomainCapsFormat() later unpacks them and presents them as above. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
Implement support for saving GIC capabilities in the cache and read them back. --- src/qemu/qemu_capabilities.c | 87 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 64007f0..23c3740 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2906,6 +2906,77 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename, } VIR_FREE(nodes); + if ((n = virXPathNodeSet("./gic", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities gic")); + goto cleanup; + } + if (n > 0) { + unsigned int uintValue; + bool boolValue; + + qemuCaps->ngicCapabilities = n; + if (VIR_ALLOC_N(qemuCaps->gicCapabilities, n) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i]; + + if (!(str = virXMLPropString(nodes[i], "version"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing GIC version " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + virStrToLong_ui(str, NULL, 10, &uintValue) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed GIC version " + "in QEMU capabilities cache")); + goto cleanup; + } + cap->version = uintValue; + VIR_FREE(str); + + if (!(str = virXMLPropString(nodes[i], "kernel"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing in-kernel GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + !(boolValue = STREQ(str, "true")) && + STRNEQ(str, "false")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed in-kernel GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (boolValue) + cap->implementation |= VIR_GIC_IMPLEMENTATION_KERNEL; + VIR_FREE(str); + + if (!(str = virXMLPropString(nodes[i], "emulated"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing emulated GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + !(boolValue = STREQ(str, "true")) && + STRNEQ(str, "false")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed emulated GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (boolValue) + cap->implementation |= VIR_GIC_IMPLEMENTATION_EMULATED; + VIR_FREE(str); + } + } + VIR_FREE(nodes); + ret = 0; cleanup: VIR_FREE(str); @@ -2972,6 +3043,22 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) qemuCaps->machineMaxCpus[i]); } + for (i = 0; i < qemuCaps->ngicCapabilities; i++) { + virGICCapabilityPtr cap; + bool kernel; + bool emulated; + + cap = &qemuCaps->gicCapabilities[i]; + kernel = (cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL); + emulated = (cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED); + + virBufferAsprintf(&buf, + "<gic version='%d' kernel='%s' emulated='%s'/>\n", + cap->version, + kernel ? "true" : "false", + emulated ? "true" : "false"); + } + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</qemuCaps>\n"); -- 2.5.5
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
Implement support for saving GIC capabilities in the cache and read them back. --- src/qemu/qemu_capabilities.c | 87 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)
This is more related to patch 2 and I think would need to be in place for patch 5 to work properly given the right circumstances...
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 64007f0..23c3740 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2906,6 +2906,77 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename, } VIR_FREE(nodes);
+ if ((n = virXPathNodeSet("./gic", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities gic")); + goto cleanup; + } + if (n > 0) { + unsigned int uintValue; + bool boolValue; + + qemuCaps->ngicCapabilities = n; + if (VIR_ALLOC_N(qemuCaps->gicCapabilities, n) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i]; + + if (!(str = virXMLPropString(nodes[i], "version"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing GIC version " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + virStrToLong_ui(str, NULL, 10, &uintValue) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed GIC version " + "in QEMU capabilities cache")); + goto cleanup; + } + cap->version = uintValue; + VIR_FREE(str); + + if (!(str = virXMLPropString(nodes[i], "kernel"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing in-kernel GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + !(boolValue = STREQ(str, "true")) && + STRNEQ(str, "false")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed in-kernel GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (boolValue) + cap->implementation |= VIR_GIC_IMPLEMENTATION_KERNEL; + VIR_FREE(str); + + if (!(str = virXMLPropString(nodes[i], "emulated"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing emulated GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + !(boolValue = STREQ(str, "true")) && + STRNEQ(str, "false")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed emulated GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (boolValue) + cap->implementation |= VIR_GIC_IMPLEMENTATION_EMULATED;
Since these are processed as either/or in patch 5 based on virttype, I'm beginning to think perhaps you'd have : <gic version='#' type='{kernel|emulated}'/> but this works, I'm still trying to process the whole domaincaps code and what the use case would be. It looks like merely the output to domcapabilities. In which case, I'm wonder if printing the gic_version inside <arch> would be sufficient. John
+ VIR_FREE(str); + } + } + VIR_FREE(nodes); + ret = 0; cleanup: VIR_FREE(str); @@ -2972,6 +3043,22 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) qemuCaps->machineMaxCpus[i]); }
+ for (i = 0; i < qemuCaps->ngicCapabilities; i++) { + virGICCapabilityPtr cap; + bool kernel; + bool emulated; + + cap = &qemuCaps->gicCapabilities[i]; + kernel = (cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL); + emulated = (cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED); + + virBufferAsprintf(&buf, + "<gic version='%d' kernel='%s' emulated='%s'/>\n", + cap->version, + kernel ? "true" : "false", + emulated ? "true" : "false"); + } + virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</qemuCaps>\n");
On Wed, 2016-03-30 at 16:29 -0400, John Ferlan wrote:
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 64007f0..23c3740 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2906,6 +2906,77 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename, } VIR_FREE(nodes); + if ((n = virXPathNodeSet("./gic", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities gic")); + goto cleanup; + } + if (n > 0) { + unsigned int uintValue; + bool boolValue; + + qemuCaps->ngicCapabilities = n; + if (VIR_ALLOC_N(qemuCaps->gicCapabilities, n) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i]; + + if (!(str = virXMLPropString(nodes[i], "version"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing GIC version " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + virStrToLong_ui(str, NULL, 10, &uintValue) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed GIC version " + "in QEMU capabilities cache")); + goto cleanup; + } + cap->version = uintValue; + VIR_FREE(str); + + if (!(str = virXMLPropString(nodes[i], "kernel"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing in-kernel GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + !(boolValue = STREQ(str, "true")) && + STRNEQ(str, "false")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed in-kernel GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (boolValue) + cap->implementation |= VIR_GIC_IMPLEMENTATION_KERNEL; + VIR_FREE(str); + + if (!(str = virXMLPropString(nodes[i], "emulated"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing emulated GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + !(boolValue = STREQ(str, "true")) && + STRNEQ(str, "false")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed emulated GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (boolValue) + cap->implementation |= VIR_GIC_IMPLEMENTATION_EMULATED; Since these are processed as either/or in patch 5 based on virttype, I'm beginning to think perhaps you'd have : <gic version='#' type='{kernel|emulated}'/> but this works, I'm still trying to process the whole domaincaps code and what the use case would be. It looks like merely the output to domcapabilities. In which case, I'm wonder if printing the gic_version inside <arch> would be sufficient.
I believe all of these were addressed in my previous comments. Let me know if that was actually not the case :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 04/06/2016 11:29 AM, Andrea Bolognani wrote:
On Wed, 2016-03-30 at 16:29 -0400, John Ferlan wrote:
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 64007f0..23c3740 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2906,6 +2906,77 @@ virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename, } VIR_FREE(nodes);
+ if ((n = virXPathNodeSet("./gic", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities gic")); + goto cleanup; + } + if (n > 0) { + unsigned int uintValue; + bool boolValue; + + qemuCaps->ngicCapabilities = n; + if (VIR_ALLOC_N(qemuCaps->gicCapabilities, n) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i]; + + if (!(str = virXMLPropString(nodes[i], "version"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing GIC version " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + virStrToLong_ui(str, NULL, 10, &uintValue) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed GIC version " + "in QEMU capabilities cache")); + goto cleanup; + } + cap->version = uintValue; + VIR_FREE(str); + + if (!(str = virXMLPropString(nodes[i], "kernel"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing in-kernel GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + !(boolValue = STREQ(str, "true")) && + STRNEQ(str, "false")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed in-kernel GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (boolValue) + cap->implementation |= VIR_GIC_IMPLEMENTATION_KERNEL; + VIR_FREE(str); + + if (!(str = virXMLPropString(nodes[i], "emulated"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing emulated GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (str && + !(boolValue = STREQ(str, "true")) && + STRNEQ(str, "false")) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed emulated GIC information " + "in QEMU capabilities cache")); + goto cleanup; + } + if (boolValue) + cap->implementation |= VIR_GIC_IMPLEMENTATION_EMULATED;
Since these are processed as either/or in patch 5 based on virttype, I'm beginning to think perhaps you'd have :
<gic version='#' type='{kernel|emulated}'/>
but this works, I'm still trying to process the whole domaincaps code and what the use case would be. It looks like merely the output to domcapabilities. In which case, I'm wonder if printing the gic_version inside <arch> would be sufficient.
I believe all of these were addressed in my previous comments.
Let me know if that was actually not the case :)
I tried going through the patches and the comments but it was a bit hard to follow, there was quite a few points/counterpoints :) Andrea can you send a v2 addressing the obvious bits that John pointed out, and anything you're still unsure of/disagreed with just highlight in v2 comments? Thanks, Cole
On Thu, 2016-04-14 at 18:14 -0400, Cole Robinson wrote:
On 04/06/2016 11:29 AM, Andrea Bolognani wrote:
On Wed, 2016-03-30 at 16:29 -0400, John Ferlan wrote:
I believe all of these were addressed in my previous comments.
Let me know if that was actually not the case :)
I tried going through the patches and the comments but it was a bit hard to follow, there was quite a few points/counterpoints :)
Andrea can you send a v2 addressing the obvious bits that John pointed out, and anything you're still unsure of/disagreed with just highlight in v2 comments?
I've just posted a non-RFC version[1] that does exactly what you're asking. Cheers. [1] https://www.redhat.com/archives/libvir-list/2016-April/msg01143.html -- Andrea Bolognani Software Engineer - Virtualization Team
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
This series implements support for asking QEMU what GIC versions can be used for guests, eg:
<features> <gic version='2'/> </features>
and exposing such information to users via domain capabilities.
QEMU patches that implement the query-gic-capabilities QMP command:
https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04465.html
I know there's been a bunch of discussions around this... what's the current plan for how apps handle this? A pointer is fine Thanks, Cole
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
This series implements support for asking QEMU what GIC versions can be used for guests, eg:
<features> <gic version='2'/> </features>
and exposing such information to users via domain capabilities.
QEMU patches that implement the query-gic-capabilities QMP command:
https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04465.html
Cheers.
Andrea Bolognani (6): conf: Get rid of virDomainCapsDevice qemu: Probe GIC capabilities schema: Validate GIC capabilities conf: Expose GIC capabilities qemu: Fill in GIC capabilities qemu: Cache GIC capabilities
Code looks good to me but I skimmed it. You labelled this an RFC... are you waiting for design comments from other people or should I do a full review? Thanks, Cole
On Tue, 2016-03-22 at 08:57 -0400, Cole Robinson wrote:
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
This series implements support for asking QEMU what GIC versions can be used for guests, eg: <features> <gic version='2'/> </features> and exposing such information to users via domain capabilities. QEMU patches that implement the query-gic-capabilities QMP command: https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04465.html Cheers. Andrea Bolognani (6): conf: Get rid of virDomainCapsDevice qemu: Probe GIC capabilities schema: Validate GIC capabilities conf: Expose GIC capabilities qemu: Fill in GIC capabilities qemu: Cache GIC capabilities Code looks good to me but I skimmed it. You labelled this an RFC... are you waiting for design comments from other people or should I do a full review?
Design comments are definitely good, it's my first time implementing something like this so I want to be sure I got it right. If you feel like doing a full review that would be lovely, but keep in mind that the QEMU bits haven't landed yet, so merging right away would be out of the question. The idea was pretty much to get the code out there instead of sitting on it forever, so that interested parties could take it for a spin with a patched QEMU and spot design / implementation issues early. I still need to add at least a couple of test cases, but that needs further digging. Any ideas about how to generate new files for tests/qemucapabilitiesdata? :) To answer your other question - the thread starts at [1], and it's a long one. The take away is that, once this information is exposed, management applications can let the user decide whether they want to give the guest a GIC v2 or v3, and warn if they try to run a v3 guest on a host that only supports v2. Cheers. [1] https://www.redhat.com/archives/libvir-list/2015-December/msg00502.html -- Andrea Bolognani Software Engineer - Virtualization Team
On Tue, Mar 22, 2016 at 1:57 PM, Cole Robinson <crobinso@redhat.com> wrote:
On 03/21/2016 01:28 PM, Andrea Bolognani wrote:
This series implements support for asking QEMU what GIC versions can be used for guests, eg:
<features> <gic version='2'/> </features>
and exposing such information to users via domain capabilities.
QEMU patches that implement the query-gic-capabilities QMP command:
https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg04465.html
Cheers.
Andrea Bolognani (6): conf: Get rid of virDomainCapsDevice qemu: Probe GIC capabilities schema: Validate GIC capabilities conf: Expose GIC capabilities qemu: Fill in GIC capabilities qemu: Cache GIC capabilities
Code looks good to me but I skimmed it. You labelled this an RFC... are you waiting for design comments from other people or should I do a full review?
Looking through some of the comments [1] on the previous versions of the QEMU patch set for the QMP command, it seems one essential thing to get a lock on would be if the interface is sufficient that the QEMU code should be merged. So if you or others can verify that part, then that's more urgent than fine combing through the libvirt code at this point, in my humble opinion. [1]: https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05272.html Thanks, -Christoffer
On Wed, 2016-03-23 at 12:38 +0100, Christoffer Dall wrote:
Code looks good to me but I skimmed it. You labelled this an RFC... are you waiting for design comments from other people or should I do a full review? Looking through some of the comments [1] on the previous versions of
On Tue, Mar 22, 2016 at 1:57 PM, Cole Robinson <crobinso@redhat.com> wrote: the QEMU patch set for the QMP command, it seems one essential thing to get a lock on would be if the interface is sufficient that the QEMU code should be merged. So if you or others can verify that part, then that's more urgent than fine combing through the libvirt code at this point, in my humble opinion. [1]: https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05272.html
Christoffer, thanks a lot for bringing this up! While I failed to mention it in the cover letter (my bad), the fact that Peter had asked for an ACK to the interface from libvirt's side was one of the reasons for sending out this series sooner rather than later. Maybe Cole, having skimmed my RFC patches, can provide an explicit ACK to the QMP interface? I don't feel confident enough to provide anything more than a weak ACK myself. Note that v6, posted by Peter earlier today[1], changes the QMP interface a bit... I believe the changes are not big enough that updating this series to deal with the changes would be an issue though. Cheers. [1] https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg05441.html -- Andrea Bolognani Software Engineer - Virtualization Team
participants (4)
-
Andrea Bolognani -
Christoffer Dall -
Cole Robinson -
John Ferlan