[libvirt] [PATCHv2 0/8] Introduce paravirtual spinlock support
Version 2 fixes stuff pointed out in Dan's review Jiri Denemark (2): cpu: Export few x86-specific APIs qemu: Add monitor APIs to fetch CPUID data from QEMU Peter Krempa (6): cpu_x86: Refactor storage of CPUID data to add support for KVM features cpu: x86: Parse the CPU feature map only once cpu: x86: Add internal CPUID features support and KVM feature bits conf: Refactor storing and usage of feature flags qemu: Add support for paravirtual spinlocks in the guest qemu: process: Validate specific CPUID flags of a guest docs/formatdomain.html.in | 8 + docs/schemas/domaincommon.rng | 10 +- src/conf/domain_conf.c | 195 +++++++++--- src/conf/domain_conf.h | 3 +- src/cpu/cpu_x86.c | 350 +++++++++++---------- src/cpu/cpu_x86.h | 9 + src/cpu/cpu_x86_data.h | 20 +- src/libvirt_private.syms | 6 + src/libxl/libxl_conf.c | 9 +- src/lxc/lxc_container.c | 6 +- src/qemu/qemu_command.c | 28 +- src/qemu/qemu_monitor.c | 35 +++ src/qemu/qemu_monitor.h | 5 + src/qemu/qemu_monitor_json.c | 145 +++++++++ src/qemu/qemu_monitor_json.h | 2 + src/qemu/qemu_process.c | 53 ++++ src/vbox/vbox_tmpl.c | 45 ++- src/xenapi/xenapi_driver.c | 10 +- src/xenapi/xenapi_utils.c | 22 +- src/xenxs/xen_sxpr.c | 20 +- src/xenxs/xen_xm.c | 30 +- tests/Makefile.am | 1 + .../qemumonitorjson-getcpu-full.data | 5 + .../qemumonitorjson-getcpu-full.json | 46 +++ .../qemumonitorjson-getcpu-host.data | 6 + .../qemumonitorjson-getcpu-host.json | 45 +++ tests/qemumonitorjsontest.c | 76 +++++ .../qemuxml2argv-pv-spinlock-disabled.args | 5 + .../qemuxml2argv-pv-spinlock-disabled.xml | 26 ++ .../qemuxml2argv-pv-spinlock-enabled.args | 5 + .../qemuxml2argv-pv-spinlock-enabled.xml | 26 ++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 2 + 33 files changed, 952 insertions(+), 304 deletions(-) create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.data create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.json create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.data create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.json create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml -- 1.8.3.2
From: Jiri Denemark <jdenemar@redhat.com> This makes virCPUx86DataAddCPUID, virCPUx86DataFree, and virCPUx86MakeData available for direct usage outside of cpu driver in tests and the new qemu monitor that will request the actual CPU definition from a running qemu instance. --- Notes: Version 2: - clarified scope of the export in the commit message src/cpu/cpu_x86.c | 6 +++--- src/cpu/cpu_x86.h | 9 +++++++++ src/libvirt_private.syms | 6 ++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 8427555..539d8b2 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -203,7 +203,7 @@ x86DataCpuid(const virCPUx86Data *data, } -static void +void virCPUx86DataFree(virCPUx86Data *data) { if (data == NULL) @@ -215,7 +215,7 @@ virCPUx86DataFree(virCPUx86Data *data) } -static virCPUDataPtr +virCPUDataPtr virCPUx86MakeData(virArch arch, virCPUx86Data **data) { virCPUDataPtr cpuData; @@ -295,7 +295,7 @@ x86DataExpand(virCPUx86Data *data, } -static int +int virCPUx86DataAddCPUID(virCPUx86Data *data, const virCPUx86CPUID *cpuid) { diff --git a/src/cpu/cpu_x86.h b/src/cpu/cpu_x86.h index 77965b7..af0fa23 100644 --- a/src/cpu/cpu_x86.h +++ b/src/cpu/cpu_x86.h @@ -25,7 +25,16 @@ # define __VIR_CPU_X86_H__ # include "cpu.h" +# include "cpu_x86_data.h" extern struct cpuArchDriver cpuDriverX86; +int virCPUx86DataAddCPUID(virCPUx86Data *data, + const virCPUx86CPUID *cpuid); + +void virCPUx86DataFree(virCPUx86Data *data); + +virCPUDataPtr virCPUx86MakeData(virArch arch, + virCPUx86Data **data); + #endif /* __VIR_CPU_X86_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 84c1c28..cb80700 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -733,6 +733,12 @@ cpuNodeData; cpuUpdate; +# cpu/cpu_x86.h +virCPUx86DataAddCPUID; +virCPUx86DataFree; +virCPUx86MakeData; + + # datatypes.h virConnectClass; virDomainClass; -- 1.8.3.2
On Thu, Oct 17, 2013 at 03:10:15PM +0200, Peter Krempa wrote:
From: Jiri Denemark <jdenemar@redhat.com>
This makes virCPUx86DataAddCPUID, virCPUx86DataFree, and virCPUx86MakeData available for direct usage outside of cpu driver in tests and the new qemu monitor that will request the actual CPU definition from a running qemu instance. ---
Notes: Version 2: - clarified scope of the export in the commit message
src/cpu/cpu_x86.c | 6 +++--- src/cpu/cpu_x86.h | 9 +++++++++ src/libvirt_private.syms | 6 ++++++ 3 files changed, 18 insertions(+), 3 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/30/13 16:17, Daniel P. Berrange wrote:
On Thu, Oct 17, 2013 at 03:10:15PM +0200, Peter Krempa wrote:
From: Jiri Denemark <jdenemar@redhat.com>
This makes virCPUx86DataAddCPUID, virCPUx86DataFree, and virCPUx86MakeData available for direct usage outside of cpu driver in tests and the new qemu monitor that will request the actual CPU definition from a running qemu instance. ---
Notes: Version 2: - clarified scope of the export in the commit message
src/cpu/cpu_x86.c | 6 +++--- src/cpu/cpu_x86.h | 9 +++++++++ src/libvirt_private.syms | 6 ++++++ 3 files changed, 18 insertions(+), 3 deletions(-)
ACK
Pushed; Thanks. Peter
The CPUID functions were stored in multiple arrays according to a specified prefix of those. This made it very hard to add another prefix to store KVM CPUID features (0x40000000). Instead of hardcoding a third array this patch changes the approach used: The code is refactored to use a single array where the CPUID functions are stored ordered by the cpuid function so that they don't depend on the specific prefix and don't waste memory. The code is also less complex using this approach. A tradeoff to this is the change from O(N) complexity to O(N^2) in x86DataAdd and x86DataSubtract. The rest of the functions were already using O(N^2) algorithms. --- Notes: Version 2: - no change, weak ACK from Daniel - fixed typo in commit message src/cpu/cpu_x86.c | 213 ++++++++++++++++++------------------------------- src/cpu/cpu_x86_data.h | 8 +- 2 files changed, 80 insertions(+), 141 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 539d8b2..1b1f2b4 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -80,14 +80,13 @@ enum compare_result { struct virCPUx86DataIterator { - virCPUx86Data *data; + const virCPUx86Data *data; int pos; - bool extended; }; #define virCPUx86DataIteratorInit(data) \ - { data, -1, false } + { data, -1 } static bool @@ -116,6 +115,9 @@ static void x86cpuidSetBits(virCPUx86CPUID *cpuid, const virCPUx86CPUID *mask) { + if (!mask) + return; + cpuid->eax |= mask->eax; cpuid->ebx |= mask->ebx; cpuid->ecx |= mask->ecx; @@ -127,6 +129,9 @@ static void x86cpuidClearBits(virCPUx86CPUID *cpuid, const virCPUx86CPUID *mask) { + if (!mask) + return; + cpuid->eax &= ~mask->eax; cpuid->ebx &= ~mask->ebx; cpuid->ecx &= ~mask->ecx; @@ -138,42 +143,45 @@ static void x86cpuidAndBits(virCPUx86CPUID *cpuid, const virCPUx86CPUID *mask) { + if (!mask) + return; + cpuid->eax &= mask->eax; cpuid->ebx &= mask->ebx; cpuid->ecx &= mask->ecx; cpuid->edx &= mask->edx; } +static int +virCPUx86CPUIDSorter(const void *a, const void *b) +{ + virCPUx86CPUID *da = (virCPUx86CPUID *) a; + virCPUx86CPUID *db = (virCPUx86CPUID *) b; + + if (da->function > db->function) + return 1; + else if (da->function < db->function) + return -1; + + return 0; +} + /* skips all zero CPUID leafs */ static virCPUx86CPUID * x86DataCpuidNext(struct virCPUx86DataIterator *iterator) { - virCPUx86CPUID *ret; - virCPUx86Data *data = iterator->data; + const virCPUx86Data *data = iterator->data; if (!data) return NULL; - do { - ret = NULL; - iterator->pos++; - - if (!iterator->extended) { - if (iterator->pos < data->basic_len) - ret = data->basic + iterator->pos; - else { - iterator->extended = true; - iterator->pos = 0; - } - } - - if (iterator->extended && iterator->pos < data->extended_len) { - ret = data->extended + iterator->pos; - } - } while (ret && x86cpuidMatch(ret, &cpuidNull)); + while (++iterator->pos < data->len) { + if (!x86cpuidMatch(data->data + iterator->pos, &cpuidNull)) + return data->data + iterator->pos; + } - return ret; + return NULL; } @@ -181,36 +189,23 @@ static virCPUx86CPUID * x86DataCpuid(const virCPUx86Data *data, uint32_t function) { - virCPUx86CPUID *cpuids; - int len; size_t i; - if (function < CPUX86_EXTENDED) { - cpuids = data->basic; - len = data->basic_len; - i = function; - } - else { - cpuids = data->extended; - len = data->extended_len; - i = function - CPUX86_EXTENDED; + for (i = 0; i < data->len; i++) { + if (data->data[i].function == function) + return data->data + i; } - if (i < len && !x86cpuidMatch(cpuids + i, &cpuidNull)) - return cpuids + i; - else - return NULL; + return NULL; } - void virCPUx86DataFree(virCPUx86Data *data) { if (data == NULL) return; - VIR_FREE(data->basic); - VIR_FREE(data->extended); + VIR_FREE(data->data); VIR_FREE(data); } @@ -247,77 +242,36 @@ x86DataCopy(const virCPUx86Data *data) virCPUx86Data *copy = NULL; size_t i; - if (VIR_ALLOC(copy) < 0 - || VIR_ALLOC_N(copy->basic, data->basic_len) < 0 - || VIR_ALLOC_N(copy->extended, data->extended_len) < 0) { + if (VIR_ALLOC(copy) < 0 || + VIR_ALLOC_N(copy->data, data->len) < 0) { virCPUx86DataFree(copy); return NULL; } - copy->basic_len = data->basic_len; - for (i = 0; i < data->basic_len; i++) - copy->basic[i] = data->basic[i]; - - copy->extended_len = data->extended_len; - for (i = 0; i < data->extended_len; i++) - copy->extended[i] = data->extended[i]; + copy->len = data->len; + for (i = 0; i < data->len; i++) + copy->data[i] = data->data[i]; return copy; } -static int -x86DataExpand(virCPUx86Data *data, - int basic_by, - int extended_by) -{ - size_t i; - - if (basic_by > 0) { - size_t len = data->basic_len; - if (VIR_EXPAND_N(data->basic, data->basic_len, basic_by) < 0) - return -1; - - for (i = 0; i < basic_by; i++) - data->basic[len + i].function = len + i; - } - - if (extended_by > 0) { - size_t len = data->extended_len; - if (VIR_EXPAND_N(data->extended, data->extended_len, extended_by) < 0) - return -1; - - for (i = 0; i < extended_by; i++) - data->extended[len + i].function = len + i + CPUX86_EXTENDED; - } - - return 0; -} - - int virCPUx86DataAddCPUID(virCPUx86Data *data, const virCPUx86CPUID *cpuid) { - unsigned int basic_by = 0; - unsigned int extended_by = 0; - virCPUx86CPUID **cpuids; - unsigned int pos; - - if (cpuid->function < CPUX86_EXTENDED) { - pos = cpuid->function; - basic_by = pos + 1 - data->basic_len; - cpuids = &data->basic; - } else { - pos = cpuid->function - CPUX86_EXTENDED; - extended_by = pos + 1 - data->extended_len; - cpuids = &data->extended; - } + virCPUx86CPUID *existing; - if (x86DataExpand(data, basic_by, extended_by) < 0) - return -1; + if ((existing = x86DataCpuid(data, cpuid->function))) { + x86cpuidSetBits(existing, cpuid); + } else { + if (VIR_APPEND_ELEMENT_COPY(data->data, data->len, + *((virCPUx86CPUID *)cpuid)) < 0) + return -1; - x86cpuidSetBits((*cpuids) + pos, cpuid); + qsort(data->data, data->len, + sizeof(virCPUx86CPUID), virCPUx86CPUIDSorter); + } return 0; } @@ -327,21 +281,19 @@ static int x86DataAdd(virCPUx86Data *data1, const virCPUx86Data *data2) { - size_t i; - - if (x86DataExpand(data1, - data2->basic_len - data1->basic_len, - data2->extended_len - data1->extended_len) < 0) - return -1; + struct virCPUx86DataIterator iter = virCPUx86DataIteratorInit(data2); + virCPUx86CPUID *cpuid1; + virCPUx86CPUID *cpuid2; - for (i = 0; i < data2->basic_len; i++) { - x86cpuidSetBits(data1->basic + i, - data2->basic + i); - } + while ((cpuid2 = x86DataCpuidNext(&iter))) { + cpuid1 = x86DataCpuid(data1, cpuid2->function); - for (i = 0; i < data2->extended_len; i++) { - x86cpuidSetBits(data1->extended + i, - data2->extended + i); + if (cpuid1) { + x86cpuidSetBits(cpuid1, cpuid2); + } else { + if (virCPUx86DataAddCPUID(data1, cpuid2) < 0) + return -1; + } } return 0; @@ -352,19 +304,13 @@ static void x86DataSubtract(virCPUx86Data *data1, const virCPUx86Data *data2) { - size_t i; - unsigned int len; - - len = MIN(data1->basic_len, data2->basic_len); - for (i = 0; i < len; i++) { - x86cpuidClearBits(data1->basic + i, - data2->basic + i); - } + struct virCPUx86DataIterator iter = virCPUx86DataIteratorInit(data1); + virCPUx86CPUID *cpuid1; + virCPUx86CPUID *cpuid2; - len = MIN(data1->extended_len, data2->extended_len); - for (i = 0; i < len; i++) { - x86cpuidClearBits(data1->extended + i, - data2->extended + i); + while ((cpuid1 = x86DataCpuidNext(&iter))) { + cpuid2 = x86DataCpuid(data2, cpuid1->function); + x86cpuidClearBits(cpuid1, cpuid2); } } @@ -1747,25 +1693,23 @@ cpuidCall(virCPUx86CPUID *cpuid) static int -cpuidSet(uint32_t base, virCPUx86CPUID **set) +cpuidSet(uint32_t base, virCPUx86Data *data) { uint32_t max; uint32_t i; virCPUx86CPUID cpuid = { base, 0, 0, 0, 0 }; cpuidCall(&cpuid); - max = cpuid.eax - base; + max = cpuid.eax; - if (VIR_ALLOC_N(*set, max + 1) < 0) - return -1; - - for (i = 0; i <= max; i++) { - cpuid.function = base | i; + for (i = base; i <= max; i++) { + cpuid.function = i; cpuidCall(&cpuid); - (*set)[i] = cpuid; + if (virCPUx86DataAddCPUID(data, &cpuid) < 0) + return -1; } - return max + 1; + return 0; } @@ -1774,18 +1718,15 @@ x86NodeData(virArch arch) { virCPUDataPtr cpuData = NULL; virCPUx86Data *data; - int ret; if (VIR_ALLOC(data) < 0) return NULL; - if ((ret = cpuidSet(CPUX86_BASIC, &data->basic)) < 0) + if (cpuidSet(CPUX86_BASIC, data) < 0) goto error; - data->basic_len = ret; - if ((ret = cpuidSet(CPUX86_EXTENDED, &data->extended)) < 0) + if (cpuidSet(CPUX86_EXTENDED, data) < 0) goto error; - data->extended_len = ret; if (!(cpuData = virCPUx86MakeData(arch, &data))) goto error; diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h index 5910ea9..69066f1 100644 --- a/src/cpu/cpu_x86_data.h +++ b/src/cpu/cpu_x86_data.h @@ -1,7 +1,7 @@ /* * cpu_x86_data.h: x86 specific CPU data * - * Copyright (C) 2009-2010 Red Hat, Inc. + * Copyright (C) 2009-2010, 2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -40,10 +40,8 @@ struct _virCPUx86CPUID { typedef struct _virCPUx86Data virCPUx86Data; struct _virCPUx86Data { - size_t basic_len; - virCPUx86CPUID *basic; - size_t extended_len; - virCPUx86CPUID *extended; + size_t len; + virCPUx86CPUID *data; }; #endif /* __VIR_CPU_X86_DATA_H__ */ -- 1.8.3.2
On Thu, Oct 17, 2013 at 03:10:16PM +0200, Peter Krempa wrote:
The CPUID functions were stored in multiple arrays according to a specified prefix of those. This made it very hard to add another prefix to store KVM CPUID features (0x40000000). Instead of hardcoding a third array this patch changes the approach used:
The code is refactored to use a single array where the CPUID functions are stored ordered by the cpuid function so that they don't depend on the specific prefix and don't waste memory. The code is also less complex using this approach. A tradeoff to this is the change from O(N) complexity to O(N^2) in x86DataAdd and x86DataSubtract. The rest of the functions were already using O(N^2) algorithms. ---
Notes: Version 2: - no change, weak ACK from Daniel - fixed typo in commit message
src/cpu/cpu_x86.c | 213 ++++++++++++++++++------------------------------- src/cpu/cpu_x86_data.h | 8 +- 2 files changed, 80 insertions(+), 141 deletions(-)
ACK, but as before, I'd still prefer if someone (ideally Jiri) double checked this particular patch. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
From: Jiri Denemark <jdenemar@redhat.com> The qemu monitor supports retrieval of actual CPUID bits presented to the guest using QMP monitor. Add APIs to extract these information and tests for them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- Notes: Version 2: - unified global and JSON monitor func signatures - changed return values so that errors can be differenitated from lack of support - code is conditionally run only when architecture matches src/qemu/qemu_monitor.c | 35 +++++ src/qemu/qemu_monitor.h | 5 + src/qemu/qemu_monitor_json.c | 145 +++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 2 + tests/Makefile.am | 1 + .../qemumonitorjson-getcpu-full.data | 5 + .../qemumonitorjson-getcpu-full.json | 46 +++++++ .../qemumonitorjson-getcpu-host.data | 6 + .../qemumonitorjson-getcpu-host.json | 45 +++++++ tests/qemumonitorjsontest.c | 76 +++++++++++ 10 files changed, 366 insertions(+) create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.data create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.json create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.data create mode 100644 tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.json diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 2bafe28..e2339cd 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3926,3 +3926,38 @@ qemuMonitorSetDomainLog(qemuMonitorPtr mon, int logfd) return 0; } + + +/** + * qemuMonitorJSONGetGuestCPU: + * @mon: Pointer to the monitor + * @arch: arch of the guest + * @cpuData: Returned cpu definition of the gues + * + * Retrieve the definition of the guest CPU from a running qemu instance. + * + * Returns 0 on success; -1 on error; -2 if error may be caused by qemu not + * supporting reporting of CPU definition. + */ +int +qemuMonitorGetGuestCPU(qemuMonitorPtr mon, + virArch arch, + virCPUDataPtr *cpuData) +{ + VIR_DEBUG("mon=%p, arch='%s' cpuData=%p", + mon, virArchToString(arch), cpuData); + + if (!mon) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("monitor must not be NULL")); + return -1; + } + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("JSON monitor is required")); + return -2; + } + + return qemuMonitorJSONGetGuestCPU(mon, arch, cpuData); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 06ba7e8..f893b1f 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -32,6 +32,7 @@ # include "virhash.h" # include "virjson.h" # include "device_conf.h" +# include "cpu/cpu.h" typedef struct _qemuMonitor qemuMonitor; typedef qemuMonitor *qemuMonitorPtr; @@ -763,6 +764,10 @@ int qemuMonitorGetDeviceAliases(qemuMonitorPtr mon, int qemuMonitorSetDomainLog(qemuMonitorPtr mon, int logfd); +int qemuMonitorGetGuestCPU(qemuMonitorPtr mon, + virArch arch, + virCPUDataPtr *data); + /** * 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 05f8aa6..342d8f0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -42,6 +42,7 @@ #include "virerror.h" #include "virjson.h" #include "virstring.h" +#include "cpu/cpu_x86.h" #ifdef WITH_DTRACE_PROBES # include "libvirt_qemu_probes.h" @@ -49,6 +50,7 @@ #define VIR_FROM_THIS VIR_FROM_QEMU +#define QOM_CPU_PATH "/machine/unattached/device[0]" #define LINE_ENDING "\r\n" @@ -5454,3 +5456,146 @@ cleanup: VIR_FREE(paths); return ret; } + + +static int +qemuMonitorJSONParseCPUx86FeatureWord(virJSONValuePtr data, + virCPUx86CPUID *cpuid) +{ + const char *reg; + unsigned long long fun; + unsigned long long features; + + memset(cpuid, 0, sizeof(*cpuid)); + + if (!(reg = virJSONValueObjectGetString(data, "cpuid-register"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing cpuid-register in CPU data")); + return -1; + } + if (virJSONValueObjectGetNumberUlong(data, "cpuid-input-eax", &fun)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing or invalid cpuid-input-eax in CPU data")); + return -1; + } + if (virJSONValueObjectGetNumberUlong(data, "features", &features) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing or invalid features in CPU data")); + return -1; + } + + cpuid->function = fun; + if (STREQ(reg, "EAX")) { + cpuid->eax = features; + } else if (STREQ(reg, "EBX")) { + cpuid->ebx = features; + } else if (STREQ(reg, "ECX")) { + cpuid->ecx = features; + } else if (STREQ(reg, "EDX")) { + cpuid->edx = features; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown CPU register '%s'"), reg); + return -1; + } + + return 0; +} + + +static int +qemuMonitorJSONGetCPUx86Data(qemuMonitorPtr mon, + const char *property, + virCPUDataPtr *cpuData) +{ + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + virCPUx86Data *x86Data = NULL; + virCPUx86CPUID cpuid; + size_t i; + int ret = -1; + int n; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-get", + "s:path", QOM_CPU_PATH, + "s:property", property, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) { + goto cleanup; + } + + if (qemuMonitorJSONCheckError(cmd, reply)) { + ret = -2; + goto cleanup; + } + + + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qom-get reply was missing return data")); + goto cleanup; + } + + if ((n = virJSONValueArraySize(data)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s CPU property did not return an array"), + property); + goto cleanup; + } + + if (VIR_ALLOC(x86Data) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + if (qemuMonitorJSONParseCPUx86FeatureWord(virJSONValueArrayGet(data, i), + &cpuid) < 0 || + virCPUx86DataAddCPUID(x86Data, &cpuid) < 0) + goto cleanup; + } + + if (!(*cpuData = virCPUx86MakeData(VIR_ARCH_X86_64, &x86Data))) + goto cleanup; + + ret = 0; + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + virCPUx86DataFree(x86Data); + return ret; +} + + +/** + * qemuMonitorJSONGetGuestCPU: + * @mon: Pointer to the monitor + * @arch: arch of the guest + * @cpuData: Returned cpu definition of the gues + * + * Retrieve the definition of the guest CPU from a running qemu instance. + * + * Returns 0 on success; -1 on error; -2 if error may be caused by qemu not + * supporting reporting of CPU definition. + */ +int +qemuMonitorJSONGetGuestCPU(qemuMonitorPtr mon, + virArch arch, + virCPUDataPtr *cpuData) +{ + *cpuData = NULL; + + switch (arch) { + case VIR_ARCH_X86_64: + case VIR_ARCH_I686: + return qemuMonitorJSONGetCPUx86Data(mon, "feature-words", cpuData); + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("CPU definition retrieval insn't supported for '%s'"), + virArchToString(arch)); + return -2; + } +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 51cf19c..035c714 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -29,6 +29,7 @@ # include "qemu_monitor.h" # include "virbitmap.h" +# include "cpu/cpu.h" int qemuMonitorJSONIOProcess(qemuMonitorPtr mon, const char *data, @@ -426,4 +427,5 @@ int qemuMonitorJSONDetachCharDev(qemuMonitorPtr mon, int qemuMonitorJSONGetDeviceAliases(qemuMonitorPtr mon, char ***aliases); +int qemuMonitorJSONGetGuestCPU(qemuMonitorPtr mon, virArch arch, virCPUDataPtr *data); #endif /* QEMU_MONITOR_JSON_H */ diff --git a/tests/Makefile.am b/tests/Makefile.am index a47d376..4338f74 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -86,6 +86,7 @@ EXTRA_DIST = \ qemucapabilitiesdata \ qemuhelpdata \ qemuhotplugtestdata \ + qemumonitorjsondata \ qemuxml2argvdata \ qemuxml2xmloutdata \ qemuxmlnsdata \ diff --git a/tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.data b/tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.data new file mode 100644 index 0000000..bba8d31 --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.data @@ -0,0 +1,5 @@ +<cpudata arch='x86'> + <cpuid function='0x00000001' eax='0x00000000' ebx='0x00000000' ecx='0x97ba2223' edx='0x078bfbfd'/> + <cpuid function='0x40000001' eax='0x0100003b' ebx='0x00000000' ecx='0x00000000' edx='0x00000000'/> + <cpuid function='0x80000001' eax='0x00000000' ebx='0x00000000' ecx='0x00000001' edx='0x28100800'/> +</cpudata> diff --git a/tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.json b/tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.json new file mode 100644 index 0000000..29c00b4 --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-getcpu-full.json @@ -0,0 +1,46 @@ +{ + "return": [ + { + "cpuid-register": "EDX", + "cpuid-input-eax": 2147483658, + "features": 0 + }, + { + "cpuid-register": "EAX", + "cpuid-input-eax": 1073741825, + "features": 16777275 + }, + { + "cpuid-register": "EDX", + "cpuid-input-eax": 3221225473, + "features": 0 + }, + { + "cpuid-register": "ECX", + "cpuid-input-eax": 2147483649, + "features": 1 + }, + { + "cpuid-register": "EDX", + "cpuid-input-eax": 2147483649, + "features": 672139264 + }, + { + "cpuid-register": "EBX", + "cpuid-input-ecx": 0, + "cpuid-input-eax": 7, + "features": 0 + }, + { + "cpuid-register": "ECX", + "cpuid-input-eax": 1, + "features": 2545558051 + }, + { + "cpuid-register": "EDX", + "cpuid-input-eax": 1, + "features": 126614525 + } + ], + "id": "libvirt-6" +} diff --git a/tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.data b/tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.data new file mode 100644 index 0000000..98b9e7c --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.data @@ -0,0 +1,6 @@ +<cpudata arch='x86'> + <cpuid function='0x00000001' eax='0x00000000' ebx='0x00000000' ecx='0x97ba2223' edx='0x0f8bfbff'/> + <cpuid function='0x00000007' eax='0x00000000' ebx='0x00000002' ecx='0x00000000' edx='0x00000000'/> + <cpuid function='0x40000001' eax='0x0100007b' ebx='0x00000000' ecx='0x00000000' edx='0x00000000'/> + <cpuid function='0x80000001' eax='0x00000000' ebx='0x00000000' ecx='0x00000001' edx='0x2993fbff'/> +</cpudata> diff --git a/tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.json b/tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.json new file mode 100644 index 0000000..b5fb9f3 --- /dev/null +++ b/tests/qemumonitorjsondata/qemumonitorjson-getcpu-host.json @@ -0,0 +1,45 @@ +{ + "return": [ + { + "cpuid-register": "EDX", + "cpuid-input-eax": 2147483658, + "features": 0 + }, + { + "cpuid-register": "EAX", + "cpuid-input-eax": 1073741825, + "features": 16777339 + }, + { + "cpuid-register": "EDX", + "cpuid-input-eax": 3221225473, + "features": 0 + }, + { + "cpuid-register": "ECX", + "cpuid-input-eax": 2147483649, + "features": 1 + }, + { + "cpuid-register": "EDX", + "cpuid-input-eax": 2147483649, + "features": 697564159 + }, + { + "cpuid-register": "EBX", + "cpuid-input-ecx": 0, + "cpuid-input-eax": 7, + "features": 2 + }, + { + "cpuid-register": "ECX", + "cpuid-input-eax": 1, + "features": 2545558051 + }, + { + "cpuid-register": "EDX", + "cpuid-input-eax": 1, + "features": 260832255 + } + ] +} diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index de907ae..333b83d 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -27,6 +27,7 @@ #include "virthread.h" #include "virerror.h" #include "virstring.h" +#include "cpu/cpu.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -1958,6 +1959,70 @@ cleanup: return ret; } + +struct testCPUData { + const char *name; + virDomainXMLOptionPtr xmlopt; +}; + + +static int +testQemuMonitorJSONGetCPUData(const void *opaque) +{ + const struct testCPUData *data = opaque; + qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, data->xmlopt); + virCPUDataPtr cpuData = NULL; + char *jsonFile = NULL; + char *dataFile = NULL; + char *jsonStr = NULL; + char *expected = NULL; + char *actual = NULL; + int ret = -1; + + if (!test) + return -1; + + if (virAsprintf(&jsonFile, + "%s/qemumonitorjsondata/qemumonitorjson-getcpu-%s.json", + abs_srcdir, data->name) < 0 || + virAsprintf(&dataFile, + "%s/qemumonitorjsondata/qemumonitorjson-getcpu-%s.data", + abs_srcdir, data->name) < 0) + goto cleanup; + + if (virtTestLoadFile(jsonFile, &jsonStr) < 0 || + virtTestLoadFile(dataFile, &expected) < 0) + goto cleanup; + + if (qemuMonitorTestAddItem(test, "qom-get", jsonStr) < 0) + goto cleanup; + + if (qemuMonitorJSONGetGuestCPU(qemuMonitorTestGetMonitor(test), + VIR_ARCH_X86_64, + &cpuData) < 0) + goto cleanup; + + if (!cpuData || !(actual = cpuDataFormat(cpuData))) + goto cleanup; + + if (STRNEQ(expected, actual)) { + virtTestDifference(stderr, expected, actual); + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FREE(jsonFile); + VIR_FREE(dataFile); + VIR_FREE(jsonStr); + VIR_FREE(expected); + VIR_FREE(actual); + cpuDataFree(cpuData); + qemuMonitorTestFree(test); + return ret; +} + + static int mymain(void) { @@ -1991,6 +2056,14 @@ mymain(void) if (virtTestRun(# name, testQemuMonitorJSON ## name, &simpleFunc) < 0) \ ret = -1 +#define DO_TEST_CPU_DATA(name) \ + do { \ + struct testCPUData data = { name, xmlopt }; \ + const char *label = "GetCPUData(" name ")"; \ + if (virtTestRun(label, testQemuMonitorJSONGetCPUData, &data) < 0) \ + ret = -1; \ + } while (0) + DO_TEST(GetStatus); DO_TEST(GetVersion); DO_TEST(GetMachines); @@ -2055,6 +2128,9 @@ mymain(void) DO_TEST(qemuMonitorJSONGetVirtType); DO_TEST(qemuMonitorJSONSendKey); + DO_TEST_CPU_DATA("host"); + DO_TEST_CPU_DATA("full"); + virObjectUnref(xmlopt); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; -- 1.8.3.2
On Thu, Oct 17, 2013 at 03:10:17PM +0200, Peter Krempa wrote:
From: Jiri Denemark <jdenemar@redhat.com>
The qemu monitor supports retrieval of actual CPUID bits presented to the guest using QMP monitor. Add APIs to extract these information and tests for them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
+/** + * qemuMonitorJSONGetGuestCPU: + * @mon: Pointer to the monitor + * @arch: arch of the guest + * @cpuData: Returned cpu definition of the gues + * + * Retrieve the definition of the guest CPU from a running qemu instance. + * + * Returns 0 on success; -1 on error; -2 if error may be caused by qemu not + * supporting reporting of CPU definition.
I'm not convinced we need the '-2' case here - I'd wrap it into the regular '-1' case. I'll point out why in patch 8.
+ */ +int +qemuMonitorJSONGetGuestCPU(qemuMonitorPtr mon, + virArch arch, + virCPUDataPtr *cpuData) +{ + *cpuData = NULL; + + switch (arch) { + case VIR_ARCH_X86_64: + case VIR_ARCH_I686: + return qemuMonitorJSONGetCPUx86Data(mon, "feature-words", cpuData); + + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("CPU definition retrieval insn't supported for '%s'"),
s/insn't/isn't/
+ virArchToString(arch)); + return -2; + } +}
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Until now the map was loaded from the XML definition file every time a operation on the flags was requested. With the introduciton of one shot initializers we can store the definition forever (as it will never change) instead of parsing it over and over again. --- Notes: Version 2: - kept the map loading function separate so that tests can use it in the future src/cpu/cpu_x86.c | 67 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 27 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 1b1f2b4..ba6a2b0 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -70,6 +70,10 @@ struct x86_map { struct x86_model *models; }; +static struct x86_map* virCPUx86Map = NULL; +int virCPUx86MapOnceInit(void); +VIR_ONCE_GLOBAL_INIT(virCPUx86Map); + enum compare_result { SUBSET, @@ -1065,7 +1069,7 @@ x86MapLoadCallback(enum cpuMapElement element, static struct x86_map * -x86LoadMap(void) +virCPUx86LoadMap(void) { struct x86_map *map; @@ -1083,6 +1087,26 @@ error: } +int +virCPUx86MapOnceInit(void) +{ + if (!(virCPUx86Map = virCPUx86LoadMap())) + return -1; + + return 0; +} + + +static const struct x86_map * +virCPUx86GetMap(void) +{ + if (virCPUx86MapInitialize() < 0) + return NULL; + + return virCPUx86Map; +} + + static char * x86CPUDataFormat(const virCPUData *data) { @@ -1194,7 +1218,7 @@ x86Compute(virCPUDefPtr host, virCPUDataPtr *guest, char **message) { - struct x86_map *map = NULL; + const struct x86_map *map = NULL; struct x86_model *host_model = NULL; struct x86_model *cpu_force = NULL; struct x86_model *cpu_require = NULL; @@ -1247,7 +1271,7 @@ x86Compute(virCPUDefPtr host, return VIR_CPU_COMPARE_INCOMPATIBLE; } - if (!(map = x86LoadMap()) || + if (!(map = virCPUx86GetMap()) || !(host_model = x86ModelFromCPU(host, map, VIR_CPU_FEATURE_REQUIRE)) || !(cpu_force = x86ModelFromCPU(cpu, map, VIR_CPU_FEATURE_FORCE)) || !(cpu_require = x86ModelFromCPU(cpu, map, VIR_CPU_FEATURE_REQUIRE)) || @@ -1323,7 +1347,6 @@ x86Compute(virCPUDefPtr host, } cleanup: - x86MapFree(map); x86ModelFree(host_model); x86ModelFree(diff); x86ModelFree(cpu_force); @@ -1362,7 +1385,7 @@ x86GuestData(virCPUDefPtr host, static int x86AddFeatures(virCPUDefPtr cpu, - struct x86_map *map) + const struct x86_map *map) { const struct x86_model *candidate; const struct x86_feature *feature = map->features; @@ -1398,7 +1421,7 @@ x86Decode(virCPUDefPtr cpu, unsigned int flags) { int ret = -1; - struct x86_map *map; + const struct x86_map *map; const struct x86_model *candidate; virCPUDefPtr cpuCandidate; virCPUDefPtr cpuModel = NULL; @@ -1406,7 +1429,7 @@ x86Decode(virCPUDefPtr cpu, virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, -1); - if (data == NULL || (map = x86LoadMap()) == NULL) + if (!data || !(map = virCPUx86GetMap())) return -1; candidate = map->models; @@ -1490,7 +1513,6 @@ x86Decode(virCPUDefPtr cpu, ret = 0; out: - x86MapFree(map); virCPUDefFree(cpuModel); return ret; @@ -1537,14 +1559,13 @@ x86Encode(virArch arch, virCPUDataPtr *forbidden, virCPUDataPtr *vendor) { - struct x86_map *map = NULL; + const struct x86_map *map = NULL; virCPUx86Data *data_forced = NULL; virCPUx86Data *data_required = NULL; virCPUx86Data *data_optional = NULL; virCPUx86Data *data_disabled = NULL; virCPUx86Data *data_forbidden = NULL; virCPUx86Data *data_vendor = NULL; - int ret = -1; if (forced) *forced = NULL; @@ -1559,7 +1580,7 @@ x86Encode(virArch arch, if (vendor) *vendor = NULL; - if ((map = x86LoadMap()) == NULL) + if ((map = virCPUx86GetMap()) == NULL) goto error; if (forced) { @@ -1627,12 +1648,7 @@ x86Encode(virArch arch, !(*vendor = virCPUx86MakeData(arch, &data_vendor))) goto error; - ret = 0; - -cleanup: - x86MapFree(map); - - return ret; + return 0; error: virCPUx86DataFree(data_forced); @@ -1653,7 +1669,7 @@ error: x86FreeCPUData(*forbidden); if (vendor) x86FreeCPUData(*vendor); - goto cleanup; + return -1; } @@ -1748,7 +1764,7 @@ x86Baseline(virCPUDefPtr *cpus, unsigned int nmodels, unsigned int flags) { - struct x86_map *map = NULL; + const struct x86_map *map = NULL; struct x86_model *base_model = NULL; virCPUDefPtr cpu = NULL; size_t i; @@ -1756,7 +1772,7 @@ x86Baseline(virCPUDefPtr *cpus, struct x86_model *model = NULL; bool outputVendor = true; - if (!(map = x86LoadMap())) + if (!(map = virCPUx86GetMap())) goto error; if (!(base_model = x86ModelFromCPU(cpus[0], map, VIR_CPU_FEATURE_REQUIRE))) @@ -1837,7 +1853,6 @@ x86Baseline(virCPUDefPtr *cpus, cleanup: x86ModelFree(base_model); - x86MapFree(map); return cpu; @@ -1855,10 +1870,10 @@ x86UpdateCustom(virCPUDefPtr guest, { int ret = -1; size_t i; - struct x86_map *map; + const struct x86_map *map; struct x86_model *host_model = NULL; - if (!(map = x86LoadMap()) || + if (!(map = virCPUx86GetMap()) || !(host_model = x86ModelFromCPU(host, map, VIR_CPU_FEATURE_REQUIRE))) goto cleanup; @@ -1890,7 +1905,6 @@ x86UpdateCustom(virCPUDefPtr guest, ret = 0; cleanup: - x86MapFree(map); x86ModelFree(host_model); return ret; } @@ -1960,11 +1974,11 @@ static int x86HasFeature(const virCPUData *data, const char *name) { - struct x86_map *map; + const struct x86_map *map; struct x86_feature *feature; int ret = -1; - if (!(map = x86LoadMap())) + if (!(map = virCPUx86GetMap())) return -1; if (!(feature = x86FeatureFind(map, name))) @@ -1973,7 +1987,6 @@ x86HasFeature(const virCPUData *data, ret = x86DataIsSubset(data->data.x86, feature->data) ? 1 : 0; cleanup: - x86MapFree(map); return ret; } -- 1.8.3.2
On Thu, Oct 17, 2013 at 03:10:18PM +0200, Peter Krempa wrote:
Until now the map was loaded from the XML definition file every time a operation on the flags was requested. With the introduciton of one shot initializers we can store the definition forever (as it will never change) instead of parsing it over and over again. ---
Notes: Version 2: - kept the map loading function separate so that tests can use it in the future
src/cpu/cpu_x86.c | 67 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 27 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/30/13 16:50, Daniel P. Berrange wrote:
On Thu, Oct 17, 2013 at 03:10:18PM +0200, Peter Krempa wrote:
Until now the map was loaded from the XML definition file every time a operation on the flags was requested. With the introduciton of one shot initializers we can store the definition forever (as it will never change) instead of parsing it over and over again. ---
Notes: Version 2: - kept the map loading function separate so that tests can use it in the future
src/cpu/cpu_x86.c | 67 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 27 deletions(-)
ACK
Pushed; Thanks. Peter
Some of the emulator features are presented in the <features> element in the domain XML although they are virtual CPUID feature bits when presented to the guest. To avoid confusing the users with these features, as they are not configurable via the <cpu> element, this patch adds an internal array where those can be stored privately instead of exposing them in the XML. Additionaly KVM feature bits are added as example usage of this code. --- Notes: Version 2: -no change, ACK'd in V1, didn't make sense to push src/cpu/cpu_x86.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu_x86_data.h | 12 ++++++++++ 2 files changed, 76 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index ba6a2b0..ce75562 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -56,6 +56,25 @@ struct x86_feature { struct x86_feature *next; }; +struct x86_kvm_feature { + const char *name; + const virCPUx86CPUID cpuid; +}; + +static const struct x86_kvm_feature x86_kvm_features[] = +{ + {VIR_CPU_x86_KVM_CLOCKSOURCE, { .function = 0x40000001, .eax = 0x00000001 }}, + {VIR_CPU_x86_KVM_NOP_IO_DELAY, { .function = 0x40000001, .eax = 0x00000002 }}, + {VIR_CPU_x86_KVM_MMU_OP, { .function = 0x40000001, .eax = 0x00000004 }}, + {VIR_CPU_x86_KVM_CLOCKSOURCE2, { .function = 0x40000001, .eax = 0x00000008 }}, + {VIR_CPU_x86_KVM_ASYNC_PF, { .function = 0x40000001, .eax = 0x00000010 }}, + {VIR_CPU_x86_KVM_STEAL_TIME, { .function = 0x40000001, .eax = 0x00000020 }}, + {VIR_CPU_x86_KVM_PV_EOI, { .function = 0x40000001, .eax = 0x00000040 }}, + {VIR_CPU_x86_KVM_PV_UNHALT, { .function = 0x40000001, .eax = 0x00000080 }}, + {VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT, + { .function = 0x40000001, .eax = 0x01000000 }}, +}; + struct x86_model { char *name; const struct x86_vendor *vendor; @@ -1068,6 +1087,48 @@ x86MapLoadCallback(enum cpuMapElement element, } +static int +x86MapLoadInternalFeatures(struct x86_map *map) +{ + size_t i; + struct x86_feature *feature = NULL; + + for (i = 0; i < ARRAY_CARDINALITY(x86_kvm_features); i++) { + const char *name = x86_kvm_features[i].name; + + if (x86FeatureFind(map, name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("CPU feature %s already defined"), name); + return -1; + } + + if (!(feature = x86FeatureNew())) + goto error; + + if (VIR_STRDUP(feature->name, name) < 0) + goto error; + + if (virCPUx86DataAddCPUID(feature->data, &x86_kvm_features[i].cpuid)) + goto error; + + if (map->features == NULL) { + map->features = feature; + } else { + feature->next = map->features; + map->features = feature; + } + + feature = NULL; + } + + return 0; + +error: + x86FeatureFree(feature); + return -1; +} + + static struct x86_map * virCPUx86LoadMap(void) { @@ -1079,6 +1140,9 @@ virCPUx86LoadMap(void) if (cpuMapLoad("x86", x86MapLoadCallback, map) < 0) goto error; + if (x86MapLoadInternalFeatures(map) < 0) + goto error; + return map; error: diff --git a/src/cpu/cpu_x86_data.h b/src/cpu/cpu_x86_data.h index 69066f1..88dccf6 100644 --- a/src/cpu/cpu_x86_data.h +++ b/src/cpu/cpu_x86_data.h @@ -36,8 +36,20 @@ struct _virCPUx86CPUID { }; # define CPUX86_BASIC 0x0 +# define CPUX86_KVM 0x40000000 # define CPUX86_EXTENDED 0x80000000 +# define VIR_CPU_x86_KVM_CLOCKSOURCE "__kvm_clocksource" +# define VIR_CPU_x86_KVM_NOP_IO_DELAY "__kvm_no_io_delay" +# define VIR_CPU_x86_KVM_MMU_OP "__kvm_mmu_op" +# define VIR_CPU_x86_KVM_CLOCKSOURCE2 "__kvm_clocksource2" +# define VIR_CPU_x86_KVM_ASYNC_PF "__kvm_async_pf" +# define VIR_CPU_x86_KVM_STEAL_TIME "__kvm_steal_time" +# define VIR_CPU_x86_KVM_PV_EOI "__kvm_pv_eoi" +# define VIR_CPU_x86_KVM_PV_UNHALT "__kvm_pv_unhalt" +# define VIR_CPU_x86_KVM_CLOCKSOURCE_STABLE_BIT "__kvm_clocksource_stable" + + typedef struct _virCPUx86Data virCPUx86Data; struct _virCPUx86Data { size_t len; -- 1.8.3.2
On Thu, Oct 17, 2013 at 03:10:19PM +0200, Peter Krempa wrote:
Some of the emulator features are presented in the <features> element in the domain XML although they are virtual CPUID feature bits when presented to the guest. To avoid confusing the users with these features, as they are not configurable via the <cpu> element, this patch adds an internal array where those can be stored privately instead of exposing them in the XML.
Additionaly KVM feature bits are added as example usage of this code. ---
Notes: Version 2: -no change, ACK'd in V1, didn't make sense to push
src/cpu/cpu_x86.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu_x86_data.h | 12 ++++++++++ 2 files changed, 76 insertions(+)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Currently we were storing domain feature flags in a bit field as the they were either enabled or disabled. New features such as paravirtual spinlocks however can be tri-state as the default option may depend on hypervisor version. To allow storing tri-state feature state in the same place instead of having to declare dedicated variables for each feature this patch refactors the bit field to an array. --- Notes: Version 2: - clarified the intent of this patch in the commit message src/conf/domain_conf.c | 159 ++++++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 2 +- src/libxl/libxl_conf.c | 9 ++- src/lxc/lxc_container.c | 6 +- src/qemu/qemu_command.c | 15 ++--- src/vbox/vbox_tmpl.c | 45 +++++++------ src/xenapi/xenapi_driver.c | 10 +-- src/xenapi/xenapi_utils.c | 22 +++---- src/xenxs/xen_sxpr.c | 20 +++--- src/xenxs/xen_xm.c | 30 ++++----- 10 files changed, 187 insertions(+), 131 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 562d98b..6a99e3f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11411,10 +11411,10 @@ virDomainDefParseXML(xmlDocPtr xml, _("unexpected feature '%s'"), nodes[i]->name); goto error; } - def->features |= (1 << val); - if (val == VIR_DOMAIN_FEATURE_APIC) { - tmp = virXPathString("string(./features/apic/@eoi)", ctxt); - if (tmp) { + + switch ((enum virDomainFeature) val) { + case VIR_DOMAIN_FEATURE_APIC: + if ((tmp = virXPathString("string(./features/apic/@eoi)", ctxt))) { int eoi; if ((eoi = virDomainFeatureStateTypeFromString(tmp)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -11425,11 +11425,23 @@ virDomainDefParseXML(xmlDocPtr xml, def->apic_eoi = eoi; VIR_FREE(tmp); } + /* fallthrough */ + case VIR_DOMAIN_FEATURE_ACPI: + case VIR_DOMAIN_FEATURE_PAE: + case VIR_DOMAIN_FEATURE_HAP: + case VIR_DOMAIN_FEATURE_VIRIDIAN: + case VIR_DOMAIN_FEATURE_PRIVNET: + case VIR_DOMAIN_FEATURE_HYPERV: + def->features[val] = VIR_DOMAIN_FEATURE_STATE_ON; + break; + + case VIR_DOMAIN_FEATURE_LAST: + break; } } VIR_FREE(nodes); - if (def->features & (1 << VIR_DOMAIN_FEATURE_HYPERV)) { + if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_DOMAIN_FEATURE_STATE_ON) { int feature; int value; node = ctxt->node; @@ -13401,12 +13413,16 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, { size_t i; - /* basic check */ - if (src->features != dst->features) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target domain features %d does not match source %d"), - dst->features, src->features); - return false; + for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) { + if (src->features[i] != dst->features[i]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s' differs: " + "source: '%s', destination: '%s'"), + virDomainFeatureTypeToString(i), + virDomainFeatureStateTypeToString(src->features[i]), + virDomainFeatureStateTypeToString(dst->features[i])); + return false; + } } /* APIC EOI */ @@ -13420,7 +13436,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } /* hyperv */ - if (src->features & (1 << VIR_DOMAIN_FEATURE_HYPERV)) { + if (src->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_DOMAIN_FEATURE_STATE_ON) { for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { switch ((enum virDomainHyperv) i) { case VIR_DOMAIN_HYPERV_RELAXED: @@ -16743,58 +16759,99 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, " </idmap>\n"); } + for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) { + if (def->features[i] != VIR_DOMAIN_FEATURE_STATE_DEFAULT) + break; + } - if (def->features) { + if (i != VIR_DOMAIN_FEATURE_LAST) { virBufferAddLit(buf, " <features>\n"); + for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) { - if (def->features & (1 << i) && i != VIR_DOMAIN_FEATURE_HYPERV) { - const char *name = virDomainFeatureTypeToString(i); - if (!name) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected feature %zu"), i); - goto error; - } - virBufferAsprintf(buf, " <%s", name); - if (i == VIR_DOMAIN_FEATURE_APIC && def->apic_eoi) { - virBufferAsprintf(buf, - " eoi='%s'", - virDomainFeatureStateTypeToString(def->apic_eoi)); - } - virBufferAddLit(buf, "/>\n"); + const char *name = virDomainFeatureTypeToString(i); + size_t j; + + if (!name) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected feature %zu"), i); + goto error; } - } - if (def->features & (1 << VIR_DOMAIN_FEATURE_HYPERV)) { - virBufferAddLit(buf, " <hyperv>\n"); - for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { - switch ((enum virDomainHyperv) i) { - case VIR_DOMAIN_HYPERV_RELAXED: - case VIR_DOMAIN_HYPERV_VAPIC: - if (def->hyperv_features[i]) - virBufferAsprintf(buf, " <%s state='%s'/>\n", - virDomainHypervTypeToString(i), - virDomainFeatureStateTypeToString( - def->hyperv_features[i])); + switch ((enum virDomainFeature) i) { + case VIR_DOMAIN_FEATURE_ACPI: + case VIR_DOMAIN_FEATURE_PAE: + case VIR_DOMAIN_FEATURE_HAP: + case VIR_DOMAIN_FEATURE_VIRIDIAN: + case VIR_DOMAIN_FEATURE_PRIVNET: + switch ((enum virDomainFeatureState) def->features[i]) { + case VIR_DOMAIN_FEATURE_STATE_DEFAULT: break; - case VIR_DOMAIN_HYPERV_SPINLOCKS: - if (def->hyperv_features[i] == 0) - break; + case VIR_DOMAIN_FEATURE_STATE_ON: + virBufferAsprintf(buf, " <%s/>\n", name); + break; + + case VIR_DOMAIN_FEATURE_STATE_LAST: + case VIR_DOMAIN_FEATURE_STATE_OFF: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected state of feature '%s'"), name); + + goto error; + break; + } + + break; - virBufferAsprintf(buf, " <spinlocks state='%s'", - virDomainFeatureStateTypeToString( - def->hyperv_features[i])); - if (def->hyperv_features[i] == VIR_DOMAIN_FEATURE_STATE_ON) - virBufferAsprintf(buf, " retries='%d'", - def->hyperv_spinlocks); + case VIR_DOMAIN_FEATURE_APIC: + if (def->features[i] == VIR_DOMAIN_FEATURE_STATE_ON) { + virBufferAddLit(buf, " <apic"); + if (def->apic_eoi) { + virBufferAsprintf(buf, " eoi='%s'", + virDomainFeatureStateTypeToString(def->apic_eoi)); + } virBufferAddLit(buf, "/>\n"); - break; + } + break; - case VIR_DOMAIN_HYPERV_LAST: + case VIR_DOMAIN_FEATURE_HYPERV: + if (def->features[i] != VIR_DOMAIN_FEATURE_STATE_ON) break; + + virBufferAddLit(buf, " <hyperv>\n"); + for (j = 0; j < VIR_DOMAIN_HYPERV_LAST; j++) { + switch ((enum virDomainHyperv) j) { + case VIR_DOMAIN_HYPERV_RELAXED: + case VIR_DOMAIN_HYPERV_VAPIC: + if (def->hyperv_features[j]) + virBufferAsprintf(buf, " <%s state='%s'/>\n", + virDomainHypervTypeToString(j), + virDomainFeatureStateTypeToString( + def->hyperv_features[j])); + break; + + case VIR_DOMAIN_HYPERV_SPINLOCKS: + if (def->hyperv_features[j] == 0) + break; + + virBufferAsprintf(buf, " <spinlocks state='%s'", + virDomainFeatureStateTypeToString( + def->hyperv_features[j])); + if (def->hyperv_features[j] == VIR_DOMAIN_FEATURE_STATE_ON) + virBufferAsprintf(buf, " retries='%d'", + def->hyperv_spinlocks); + virBufferAddLit(buf, "/>\n"); + break; + + case VIR_DOMAIN_HYPERV_LAST: + break; + } } + virBufferAddLit(buf, " </hyperv>\n"); + break; + + case VIR_DOMAIN_FEATURE_LAST: + break; } - virBufferAddLit(buf, " </hyperv>\n"); } virBufferAddLit(buf, " </features>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4520ae4..ffd7e9d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1988,7 +1988,7 @@ struct _virDomainDef { virDomainOSDef os; char *emulator; - int features; + int features[VIR_DOMAIN_FEATURE_LAST]; /* enum virDomainFeatureState */ int apic_eoi; /* These options are of type virDomainFeatureState */ diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index d4226b8..79cf2b6 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -603,11 +603,14 @@ libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config) char bootorder[VIR_DOMAIN_BOOT_LAST + 1]; libxl_defbool_set(&b_info->u.hvm.pae, - def->features & (1 << VIR_DOMAIN_FEATURE_PAE)); + def->features[VIR_DOMAIN_FEATURE_PAE] == + VIR_DOMAIN_FEATURE_STATE_ON); libxl_defbool_set(&b_info->u.hvm.apic, - def->features & (1 << VIR_DOMAIN_FEATURE_APIC)); + def->features[VIR_DOMAIN_FEATURE_APIC] == + VIR_DOMAIN_FEATURE_STATE_ON); libxl_defbool_set(&b_info->u.hvm.acpi, - def->features & (1 << VIR_DOMAIN_FEATURE_ACPI)); + def->features[VIR_DOMAIN_FEATURE_ACPI] == + VIR_DOMAIN_FEATURE_STATE_ON); for (i = 0; i < def->clock.ntimers; i++) { if (def->clock.timers[i]->name == VIR_DOMAIN_TIMER_NAME_HPET && def->clock.timers[i]->present == 1) { diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 56df69e..610d131 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1867,8 +1867,8 @@ static int lxcContainerChild(void *data) } /* rename and enable interfaces */ - if (lxcContainerRenameAndEnableInterfaces(!!(vmDef->features & - (1 << VIR_DOMAIN_FEATURE_PRIVNET)), + if (lxcContainerRenameAndEnableInterfaces(vmDef->features[VIR_DOMAIN_FEATURE_PRIVNET] == + VIR_DOMAIN_FEATURE_STATE_ON, argv->nveths, argv->veths) < 0) { goto cleanup; @@ -1958,7 +1958,7 @@ lxcNeedNetworkNamespace(virDomainDefPtr def) size_t i; if (def->nets != NULL) return true; - if (def->features & (1 << VIR_DOMAIN_FEATURE_PRIVNET)) + if (def->features[VIR_DOMAIN_FEATURE_PRIVNET] == VIR_DOMAIN_FEATURE_STATE_ON) return true; for (i = 0; i < def->nhostdevs; i++) { if (def->hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES && diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index abb62e9..91ffacc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6718,7 +6718,7 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, have_cpu = true; } - if (def->features & (1 << VIR_DOMAIN_FEATURE_HYPERV)) { + if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_DOMAIN_FEATURE_STATE_ON) { if (!have_cpu) { virBufferAdd(&buf, default_model, -1); have_cpu = true; @@ -8053,7 +8053,7 @@ qemuBuildCommandLine(virConnectPtr conn, } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI)) { - if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) + if (def->features[VIR_DOMAIN_FEATURE_ACPI] != VIR_DOMAIN_FEATURE_STATE_ON) virCommandAddArg(cmd, "-no-acpi"); } @@ -10852,7 +10852,7 @@ qemuParseCommandLineCPU(virDomainDefPtr dom, if (*feature == '\0') goto syntax; - dom->features |= (1 << VIR_DOMAIN_FEATURE_HYPERV); + dom->features[VIR_DOMAIN_FEATURE_HYPERV] = VIR_DOMAIN_FEATURE_STATE_ON; if ((f = virDomainHypervTypeFromString(feature)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -11106,7 +11106,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, goto error; if (strstr(path, "kvm")) { def->virtType = VIR_DOMAIN_VIRT_KVM; - def->features |= (1 << VIR_DOMAIN_FEATURE_PAE); + def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_DOMAIN_FEATURE_STATE_ON; } } @@ -11119,8 +11119,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, if ((def->os.arch == VIR_ARCH_I686) || (def->os.arch == VIR_ARCH_X86_64)) - def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI) - /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/; + def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_ON; #define WANT_VALUE() \ const char *val = progargv[++i]; \ @@ -11414,7 +11413,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, def->disks[def->ndisks++] = disk; disk = NULL; } else if (STREQ(arg, "-no-acpi")) { - def->features &= ~(1 << VIR_DOMAIN_FEATURE_ACPI); + def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_DEFAULT; } else if (STREQ(arg, "-no-reboot")) { def->onReboot = VIR_DOMAIN_LIFECYCLE_DESTROY; } else if (STREQ(arg, "-no-kvm")) { @@ -11533,7 +11532,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, def->mem.nosharepages = true; } else if (STRPREFIX(param, "accel=kvm")) { def->virtType = VIR_DOMAIN_VIRT_KVM; - def->features |= (1 << VIR_DOMAIN_FEATURE_PAE); + def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_DOMAIN_FEATURE_STATE_ON; } } virStringFreeList(list); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index cf34f5c..187bb6d 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2338,7 +2338,6 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { } } - def->features = 0; #if VBOX_API_VERSION < 3001 machine->vtbl->GetPAEEnabled(machine, &PAEEnabled); #elif VBOX_API_VERSION == 3001 @@ -2346,21 +2345,18 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { #elif VBOX_API_VERSION >= 3002 machine->vtbl->GetCPUProperty(machine, CPUPropertyType_PAE, &PAEEnabled); #endif - if (PAEEnabled) { - def->features = def->features | (1 << VIR_DOMAIN_FEATURE_PAE); - } + if (PAEEnabled) + def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_DOMAIN_FEATURE_STATE_ON; machine->vtbl->GetBIOSSettings(machine, &bios); if (bios) { bios->vtbl->GetACPIEnabled(bios, &ACPIEnabled); - if (ACPIEnabled) { - def->features = def->features | (1 << VIR_DOMAIN_FEATURE_ACPI); - } + if (ACPIEnabled) + def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_ON; bios->vtbl->GetIOAPICEnabled(bios, &IOAPICEnabled); - if (IOAPICEnabled) { - def->features = def->features | (1 << VIR_DOMAIN_FEATURE_APIC); - } + if (IOAPICEnabled) + def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_DOMAIN_FEATURE_STATE_ON; VBOX_RELEASE(bios); } @@ -5076,40 +5072,43 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml) { } #if VBOX_API_VERSION < 3001 - rc = machine->vtbl->SetPAEEnabled(machine, (def->features) & - (1 << VIR_DOMAIN_FEATURE_PAE)); + rc = machine->vtbl->SetPAEEnabled(machine, + def->features[VIR_DOMAIN_FEATURE_PAE] == + VIR_DOMAIN_FEATURE_STATE_ON); #elif VBOX_API_VERSION == 3001 rc = machine->vtbl->SetCpuProperty(machine, CpuPropertyType_PAE, - (def->features) & - (1 << VIR_DOMAIN_FEATURE_PAE)); + def->features[VIR_DOMAIN_FEATURE_PAE] == + VIR_DOMAIN_FEATURE_STATE_ON); #elif VBOX_API_VERSION >= 3002 rc = machine->vtbl->SetCPUProperty(machine, CPUPropertyType_PAE, - (def->features) & - (1 << VIR_DOMAIN_FEATURE_PAE)); + def->features[VIR_DOMAIN_FEATURE_PAE] == + VIR_DOMAIN_FEATURE_STATE_ON); #endif if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("could not change PAE status to: %s, rc=%08x"), - ((def->features) & (1 << VIR_DOMAIN_FEATURE_PAE)) + (def->features[VIR_DOMAIN_FEATURE_PAE] == VIR_DOMAIN_FEATURE_STATE_ON) ? _("Enabled") : _("Disabled"), (unsigned)rc); } machine->vtbl->GetBIOSSettings(machine, &bios); if (bios) { - rc = bios->vtbl->SetACPIEnabled(bios, (def->features) & - (1 << VIR_DOMAIN_FEATURE_ACPI)); + rc = bios->vtbl->SetACPIEnabled(bios, + def->features[VIR_DOMAIN_FEATURE_ACPI] == + VIR_DOMAIN_FEATURE_STATE_ON); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("could not change ACPI status to: %s, rc=%08x"), - ((def->features) & (1 << VIR_DOMAIN_FEATURE_ACPI)) + (def->features[VIR_DOMAIN_FEATURE_ACPI] == VIR_DOMAIN_FEATURE_STATE_ON) ? _("Enabled") : _("Disabled"), (unsigned)rc); } - rc = bios->vtbl->SetIOAPICEnabled(bios, (def->features) & - (1 << VIR_DOMAIN_FEATURE_APIC)); + rc = bios->vtbl->SetIOAPICEnabled(bios, + def->features[VIR_DOMAIN_FEATURE_APIC] == + VIR_DOMAIN_FEATURE_STATE_ON); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("could not change APIC status to: %s, rc=%08x"), - ((def->features) & (1 << VIR_DOMAIN_FEATURE_APIC)) + (def->features[VIR_DOMAIN_FEATURE_APIC] == VIR_DOMAIN_FEATURE_STATE_ON) ? _("Enabled") : _("Disabled"), (unsigned)rc); } VBOX_RELEASE(bios); diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 4b522c0..df926a6 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1474,15 +1474,15 @@ xenapiDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) for (i = 0; i < result->size; i++) { if (STREQ(result->contents[i].val, "true")) { if (STREQ(result->contents[i].key, "acpi")) - defPtr->features = defPtr->features | (1<<VIR_DOMAIN_FEATURE_ACPI); + defPtr->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_ON; else if (STREQ(result->contents[i].key, "apic")) - defPtr->features = defPtr->features | (1<<VIR_DOMAIN_FEATURE_APIC); + defPtr->features[VIR_DOMAIN_FEATURE_APIC] = VIR_DOMAIN_FEATURE_STATE_ON; else if (STREQ(result->contents[i].key, "pae")) - defPtr->features = defPtr->features | (1<<VIR_DOMAIN_FEATURE_PAE); + defPtr->features[VIR_DOMAIN_FEATURE_PAE] = VIR_DOMAIN_FEATURE_STATE_ON; else if (STREQ(result->contents[i].key, "hap")) - defPtr->features = defPtr->features | (1<<VIR_DOMAIN_FEATURE_HAP); + defPtr->features[VIR_DOMAIN_FEATURE_HAP] = VIR_DOMAIN_FEATURE_STATE_ON; else if (STREQ(result->contents[i].key, "viridian")) - defPtr->features = defPtr->features | (1<<VIR_DOMAIN_FEATURE_VIRIDIAN); + defPtr->features[VIR_DOMAIN_FEATURE_VIRIDIAN] = VIR_DOMAIN_FEATURE_STATE_ON; } } xen_string_string_map_free(result); diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 91ae54b..02d4885 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -511,18 +511,16 @@ createVMRecordFromXml(virConnectPtr conn, virDomainDefPtr def, if (def->onCrash) (*record)->actions_after_crash = actionCrashLibvirt2XenapiEnum(def->onCrash); - if (def->features) { - if (def->features & (1 << VIR_DOMAIN_FEATURE_ACPI)) - allocStringMap(&strings, (char *)"acpi", (char *)"true"); - if (def->features & (1 << VIR_DOMAIN_FEATURE_APIC)) - allocStringMap(&strings, (char *)"apic", (char *)"true"); - if (def->features & (1 << VIR_DOMAIN_FEATURE_PAE)) - allocStringMap(&strings, (char *)"pae", (char *)"true"); - if (def->features & (1 << VIR_DOMAIN_FEATURE_HAP)) - allocStringMap(&strings, (char *)"hap", (char *)"true"); - if (def->features & (1 << VIR_DOMAIN_FEATURE_VIRIDIAN)) - allocStringMap(&strings, (char *)"viridian", (char *)"true"); - } + if (def->features[VIR_DOMAIN_FEATURE_ACPI] == VIR_DOMAIN_FEATURE_STATE_ON) + allocStringMap(&strings, (char *)"acpi", (char *)"true"); + if (def->features[VIR_DOMAIN_FEATURE_APIC] == VIR_DOMAIN_FEATURE_STATE_ON) + allocStringMap(&strings, (char *)"apic", (char *)"true"); + if (def->features[VIR_DOMAIN_FEATURE_PAE] == VIR_DOMAIN_FEATURE_STATE_ON) + allocStringMap(&strings, (char *)"pae", (char *)"true"); + if (def->features[VIR_DOMAIN_FEATURE_HAP] == VIR_DOMAIN_FEATURE_STATE_ON) + allocStringMap(&strings, (char *)"hap", (char *)"true"); + if (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == VIR_DOMAIN_FEATURE_STATE_ON) + allocStringMap(&strings, (char *)"viridian", (char *)"true"); if (strings != NULL) (*record)->platform = strings; diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 3cbe958..d23a3ca 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1202,15 +1202,15 @@ xenParseSxpr(const struct sexpr *root, if (hvm) { if (sexpr_int(root, "domain/image/hvm/acpi")) - def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI); + def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_ON; if (sexpr_int(root, "domain/image/hvm/apic")) - def->features |= (1 << VIR_DOMAIN_FEATURE_APIC); + def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_DOMAIN_FEATURE_STATE_ON; if (sexpr_int(root, "domain/image/hvm/pae")) - def->features |= (1 << VIR_DOMAIN_FEATURE_PAE); + def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_DOMAIN_FEATURE_STATE_ON; if (sexpr_int(root, "domain/image/hvm/hap")) - def->features |= (1 << VIR_DOMAIN_FEATURE_HAP); + def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_DOMAIN_FEATURE_STATE_ON; if (sexpr_int(root, "domain/image/hvm/viridian")) - def->features |= (1 << VIR_DOMAIN_FEATURE_VIRIDIAN); + def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] = VIR_DOMAIN_FEATURE_STATE_ON; } /* 12aaf4a2486b (3.0.3) added a second low-priority 'localtime' setting */ @@ -2333,15 +2333,15 @@ xenFormatSxpr(virConnectPtr conn, } } - if (def->features & (1 << VIR_DOMAIN_FEATURE_ACPI)) + if (def->features[VIR_DOMAIN_FEATURE_ACPI] == VIR_DOMAIN_FEATURE_STATE_ON) virBufferAddLit(&buf, "(acpi 1)"); - if (def->features & (1 << VIR_DOMAIN_FEATURE_APIC)) + if (def->features[VIR_DOMAIN_FEATURE_APIC] == VIR_DOMAIN_FEATURE_STATE_ON) virBufferAddLit(&buf, "(apic 1)"); - if (def->features & (1 << VIR_DOMAIN_FEATURE_PAE)) + if (def->features[VIR_DOMAIN_FEATURE_PAE] == VIR_DOMAIN_FEATURE_STATE_ON) virBufferAddLit(&buf, "(pae 1)"); - if (def->features & (1 << VIR_DOMAIN_FEATURE_HAP)) + if (def->features[VIR_DOMAIN_FEATURE_HAP] == VIR_DOMAIN_FEATURE_STATE_ON) virBufferAddLit(&buf, "(hap 1)"); - if (def->features & (1 << VIR_DOMAIN_FEATURE_VIRIDIAN)) + if (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == VIR_DOMAIN_FEATURE_STATE_ON) virBufferAddLit(&buf, "(viridian 1)"); virBufferAddLit(&buf, "(usb 1)"); diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 9e07f95..88374f4 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -398,23 +398,23 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (xenXMConfigGetBool(conf, "pae", &val, 0) < 0) goto cleanup; else if (val) - def->features |= (1 << VIR_DOMAIN_FEATURE_PAE); + def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_DOMAIN_FEATURE_STATE_ON; if (xenXMConfigGetBool(conf, "acpi", &val, 0) < 0) goto cleanup; else if (val) - def->features |= (1 << VIR_DOMAIN_FEATURE_ACPI); + def->features[VIR_DOMAIN_FEATURE_ACPI] = VIR_DOMAIN_FEATURE_STATE_ON; if (xenXMConfigGetBool(conf, "apic", &val, 0) < 0) goto cleanup; else if (val) - def->features |= (1 << VIR_DOMAIN_FEATURE_APIC); + def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_DOMAIN_FEATURE_STATE_ON; if (xenXMConfigGetBool(conf, "hap", &val, 0) < 0) goto cleanup; else if (val) - def->features |= (1 << VIR_DOMAIN_FEATURE_HAP); + def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_DOMAIN_FEATURE_STATE_ON; if (xenXMConfigGetBool(conf, "viridian", &val, 0) < 0) goto cleanup; else if (val) - def->features |= (1 << VIR_DOMAIN_FEATURE_VIRIDIAN); + def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] = VIR_DOMAIN_FEATURE_STATE_ON; if (xenXMConfigGetBool(conf, "hpet", &val, -1) < 0) goto cleanup; @@ -1571,29 +1571,29 @@ virConfPtr xenFormatXM(virConnectPtr conn, goto cleanup; if (xenXMConfigSetInt(conf, "pae", - (def->features & - (1 << VIR_DOMAIN_FEATURE_PAE)) ? 1 : 0) < 0) + (def->features[VIR_DOMAIN_FEATURE_PAE] == + VIR_DOMAIN_FEATURE_STATE_ON) ? 1 : 0) < 0) goto cleanup; if (xenXMConfigSetInt(conf, "acpi", - (def->features & - (1 << VIR_DOMAIN_FEATURE_ACPI)) ? 1 : 0) < 0) + (def->features[VIR_DOMAIN_FEATURE_ACPI] == + VIR_DOMAIN_FEATURE_STATE_ON) ? 1 : 0) < 0) goto cleanup; if (xenXMConfigSetInt(conf, "apic", - (def->features & - (1 << VIR_DOMAIN_FEATURE_APIC)) ? 1 : 0) < 0) + (def->features[VIR_DOMAIN_FEATURE_APIC] == + VIR_DOMAIN_FEATURE_STATE_ON) ? 1 : 0) < 0) goto cleanup; if (xendConfigVersion >= XEND_CONFIG_VERSION_3_0_4) { if (xenXMConfigSetInt(conf, "hap", - (def->features & - (1 << VIR_DOMAIN_FEATURE_HAP)) ? 1 : 0) < 0) + (def->features[VIR_DOMAIN_FEATURE_HAP] == + VIR_DOMAIN_FEATURE_STATE_ON) ? 1 : 0) < 0) goto cleanup; if (xenXMConfigSetInt(conf, "viridian", - (def->features & - (1 << VIR_DOMAIN_FEATURE_VIRIDIAN)) ? 1 : 0) < 0) + (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] == + VIR_DOMAIN_FEATURE_STATE_ON) ? 1 : 0) < 0) goto cleanup; } -- 1.8.3.2
On Thu, Oct 17, 2013 at 03:10:20PM +0200, Peter Krempa wrote:
Currently we were storing domain feature flags in a bit field as the they were either enabled or disabled. New features such as paravirtual spinlocks however can be tri-state as the default option may depend on hypervisor version.
To allow storing tri-state feature state in the same place instead of having to declare dedicated variables for each feature this patch refactors the bit field to an array. ---
Notes: Version 2: - clarified the intent of this patch in the commit message
src/conf/domain_conf.c | 159 ++++++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 2 +- src/libxl/libxl_conf.c | 9 ++- src/lxc/lxc_container.c | 6 +- src/qemu/qemu_command.c | 15 ++--- src/vbox/vbox_tmpl.c | 45 +++++++------ src/xenapi/xenapi_driver.c | 10 +-- src/xenapi/xenapi_utils.c | 22 +++---- src/xenxs/xen_sxpr.c | 20 +++--- src/xenxs/xen_xm.c | 30 ++++----- 10 files changed, 187 insertions(+), 131 deletions(-)
ACK, would prefer a second reviewer. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
The linux kernel recently added support for paravirtual spinlock handling to avoid performance regressions on overcomitted hosts. This feature needs to be turned in the hypervisor so that the guest OS is notified about the possible support. This patch adds a new feature "paravirt-spinlock" to the XML and supporting code to enable the "kvm_pv_unhalt" pseudo CPU feature in qemu. https://bugzilla.redhat.com/show_bug.cgi?id=1008989 --- Notes: Version 2: - s/paravirt-spinlock/pvspinlock/ docs/formatdomain.html.in | 8 +++++ docs/schemas/domaincommon.rng | 10 +++++- src/conf/domain_conf.c | 36 +++++++++++++++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 13 ++++++++ .../qemuxml2argv-pv-spinlock-disabled.args | 5 +++ .../qemuxml2argv-pv-spinlock-disabled.xml | 26 ++++++++++++++++ .../qemuxml2argv-pv-spinlock-enabled.args | 5 +++ .../qemuxml2argv-pv-spinlock-enabled.xml | 26 ++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmltest.c | 2 ++ 11 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c5a3fa8..14fe84e 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1184,6 +1184,7 @@ <vapic state='on'/> <spinlocks state='on' retries='4096'/> </hyperv> + <pvspinlock/> </features> ...</pre> @@ -1255,6 +1256,13 @@ </tr> </table> </dd> + <dt><code>pvspinlock</code></dt> + <dd>Notify the guest that the host supports paravirtual spinlocks + for example by exposing the pvticketlocks mechanism. This feature + can be explicitly disabled by using <code>state='off'</code> + attribute. + </dd> + </dl> <h3><a name="elementsTime">Time keeping</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 14b6700..cc3096d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3561,7 +3561,7 @@ </define> <!-- A set of optional features: PAE, APIC, ACPI, - HyperV Enlightenment and HAP support + HyperV Enlightenment, paravirtual spinlocks and HAP support --> <define name="features"> <optional> @@ -3607,6 +3607,14 @@ <empty/> </element> </optional> + <optional> + <element name="pvspinlock"> + <optional> + <ref name="featurestate"/> + </optional> + <empty/> + </element> + </optional> </interleave> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6a99e3f..adc27e3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -142,7 +142,8 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST, "hap", "viridian", "privnet", - "hyperv") + "hyperv", + "pvspinlock") VIR_ENUM_IMPL(virDomainFeatureState, VIR_DOMAIN_FEATURE_STATE_LAST, "default", @@ -11435,6 +11436,22 @@ virDomainDefParseXML(xmlDocPtr xml, def->features[val] = VIR_DOMAIN_FEATURE_STATE_ON; break; + case VIR_DOMAIN_FEATURE_PVSPINLOCK: + node = ctxt->node; + ctxt->node = nodes[i]; + if ((tmp = virXPathString("string(./@state)", ctxt))) { + if ((def->features[val] = virDomainFeatureStateTypeFromString(tmp)) == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown state atribute '%s' of feature '%s'"), + tmp, virDomainFeatureTypeToString(val)); + goto error; + } + } else { + def->features[val] = VIR_DOMAIN_FEATURE_STATE_ON; + } + ctxt->node = node; + break; + case VIR_DOMAIN_FEATURE_LAST: break; } @@ -16802,6 +16819,23 @@ virDomainDefFormatInternal(virDomainDefPtr def, break; + case VIR_DOMAIN_FEATURE_PVSPINLOCK: + switch ((enum virDomainFeatureState) def->features[i]) { + case VIR_DOMAIN_FEATURE_STATE_LAST: + case VIR_DOMAIN_FEATURE_STATE_DEFAULT: + break; + + case VIR_DOMAIN_FEATURE_STATE_ON: + virBufferAsprintf(buf, " <%s state='on'/>\n", name); + break; + + case VIR_DOMAIN_FEATURE_STATE_OFF: + virBufferAsprintf(buf, " <%s state='off'/>\n", name); + break; + } + + break; + case VIR_DOMAIN_FEATURE_APIC: if (def->features[i] == VIR_DOMAIN_FEATURE_STATE_ON) { virBufferAddLit(buf, " <apic"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ffd7e9d..05c2a84 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1619,6 +1619,7 @@ enum virDomainFeature { VIR_DOMAIN_FEATURE_VIRIDIAN, VIR_DOMAIN_FEATURE_PRIVNET, VIR_DOMAIN_FEATURE_HYPERV, + VIR_DOMAIN_FEATURE_PVSPINLOCK, VIR_DOMAIN_FEATURE_LAST }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 91ffacc..c1ceed2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6718,6 +6718,19 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, have_cpu = true; } + if (def->features[VIR_DOMAIN_FEATURE_PVSPINLOCK]) { + char sign; + if (def->features[VIR_DOMAIN_FEATURE_PVSPINLOCK] == VIR_DOMAIN_FEATURE_STATE_ON) + sign = '+'; + else + sign = '-'; + + virBufferAsprintf(&buf, "%s,%ckvm_pv_unhalt", + have_cpu ? "" : default_model, + sign); + have_cpu = true; + } + if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_DOMAIN_FEATURE_STATE_ON) { if (!have_cpu) { virBufferAdd(&buf, default_model, -1); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args new file mode 100644 index 0000000..80047f9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc \ +-cpu qemu32,-kvm_pv_unhalt -m 214 -smp 6 -nographic -monitor \ +unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial \ +none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml new file mode 100644 index 0000000..4820476 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-disabled.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>6</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='network'/> + </os> + <features> + <acpi/> + <pae/> + <pvspinlock state='off'/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args new file mode 100644 index 0000000..70db173 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.args @@ -0,0 +1,5 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc \ +-cpu qemu32,+kvm_pv_unhalt -m 214 -smp 6 -nographic -monitor \ +unix:/tmp/test-monitor,server,nowait -boot n -usb -net none -serial \ +none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml new file mode 100644 index 0000000..ac8781b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-pv-spinlock-enabled.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>6</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='network'/> + </os> + <features> + <acpi/> + <pae/> + <pvspinlock state='on'/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 060acf2..d716c58 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -448,6 +448,8 @@ mymain(void) QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_SPICE, QEMU_CAPS_HDA_DUPLEX); DO_TEST("eoi-disabled", NONE); DO_TEST("eoi-enabled", NONE); + DO_TEST("pv-spinlock-disabled", NONE); + DO_TEST("pv-spinlock-enabled", NONE); DO_TEST("kvmclock+eoi-disabled", QEMU_CAPS_ENABLE_KVM); DO_TEST("hyperv", NONE); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 4e308b4..ffff3b5 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -159,6 +159,8 @@ mymain(void) DO_TEST("cpu-eoi-enabled"); DO_TEST("eoi-disabled"); DO_TEST("eoi-enabled"); + DO_TEST("pv-spinlock-disabled"); + DO_TEST("pv-spinlock-enabled"); DO_TEST("hyperv"); DO_TEST("hyperv-off"); -- 1.8.3.2
On Thu, Oct 17, 2013 at 03:10:21PM +0200, Peter Krempa wrote:
The linux kernel recently added support for paravirtual spinlock handling to avoid performance regressions on overcomitted hosts. This feature needs to be turned in the hypervisor so that the guest OS is notified about the possible support.
This patch adds a new feature "paravirt-spinlock" to the XML and supporting code to enable the "kvm_pv_unhalt" pseudo CPU feature in qemu.
https://bugzilla.redhat.com/show_bug.cgi?id=1008989 ---
Notes: Version 2: - s/paravirt-spinlock/pvspinlock/
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
When starting a VM the qemu process may filter out some requested features of a domain as it's not supported either by the host or by qemu. Libvirt didn't check if this happened which might end up in changing of the guest ABI when migrating. The proof of concept implementation adds the check for the recently introduced kvm_pv_unhalt cpuid feature bit. This feature depends on both qemu and host kernel support and thus increase the possibility of guest ABI breakage. --- Notes: Version 2: -- adapted to previous changes src/qemu/qemu_process.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 354e079..ca024a9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -44,6 +44,7 @@ #include "qemu_bridge_filter.h" #include "qemu_migration.h" +#include "cpu/cpu.h" #include "datatypes.h" #include "virlog.h" #include "virerror.h" @@ -3473,6 +3474,54 @@ qemuValidateCpuMax(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return true; } + +static bool +qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, virDomainObjPtr vm) +{ + virDomainDefPtr def = vm->def; + virArch arch = def->os.arch; + virCPUDataPtr guestcpu = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + bool ret = false; + int rc; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorGetGuestCPU(priv->mon, arch, &guestcpu); + qemuDomainObjExitMonitor(driver, vm); + + if (rc < 0) { + if (rc == -2) { + virResetLastError(); + return true; + } + + goto cleanup; + } + + switch (arch) { + case VIR_ARCH_I686: + case VIR_ARCH_X86_64: + if (def->features[VIR_DOMAIN_FEATURE_PVSPINLOCK] == VIR_DOMAIN_FEATURE_STATE_ON) { + if (!cpuHasFeature(guestcpu, VIR_CPU_x86_KVM_PV_UNHALT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("host doesn't support paravirtual spinlocks")); + goto cleanup; + } + } + break; + + default: + break; + } + + ret = true; + +cleanup: + cpuDataFree(guestcpu); + return ret; +} + + int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3940,6 +3989,10 @@ int qemuProcessStart(virConnectPtr conn, priv->agentError = true; } + VIR_DEBUG("Detecting if required emulator features are present"); + if (!qemuProcessVerifyGuestCPU(driver, vm)) + goto cleanup; + VIR_DEBUG("Detecting VCPU PIDs"); if (qemuProcessDetectVcpuPIDs(driver, vm) < 0) goto cleanup; -- 1.8.3.2
On Thu, Oct 17, 2013 at 03:10:22PM +0200, Peter Krempa wrote:
+static bool +qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, virDomainObjPtr vm) +{ + virDomainDefPtr def = vm->def; + virArch arch = def->os.arch; + virCPUDataPtr guestcpu = NULL; + qemuDomainObjPrivatePtr priv = vm->privateData; + bool ret = false; + int rc; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorGetGuestCPU(priv->mon, arch, &guestcpu); + qemuDomainObjExitMonitor(driver, vm); + + if (rc < 0) { + if (rc == -2) { + virResetLastError(); + return true; + } + + goto cleanup; + } + + switch (arch) { + case VIR_ARCH_I686: + case VIR_ARCH_X86_64: + if (def->features[VIR_DOMAIN_FEATURE_PVSPINLOCK] == VIR_DOMAIN_FEATURE_STATE_ON) { + if (!cpuHasFeature(guestcpu, VIR_CPU_x86_KVM_PV_UNHALT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("host doesn't support paravirtual spinlocks")); + goto cleanup; + } + } + break; + + default: + break; + }
If you move the qemuMonitorGetGuestCPU call into this switch() case for x86, then you avoid the need to have the special -2 error case, and avoid resetting an already reported error message (which will have spammed the logs) Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/17/13 14:10, Peter Krempa wrote:
Version 2 fixes stuff pointed out in Dan's review Jiri Denemark (2): cpu: Export few x86-specific APIs qemu: Add monitor APIs to fetch CPUID data from QEMU
Peter Krempa (6): cpu_x86: Refactor storage of CPUID data to add support for KVM features cpu: x86: Parse the CPU feature map only once cpu: x86: Add internal CPUID features support and KVM feature bits conf: Refactor storing and usage of feature flags qemu: Add support for paravirtual spinlocks in the guest qemu: process: Validate specific CPUID flags of a guest
Could somebody please have a look at this series? Thanks Peter
participants (2)
-
Daniel P. Berrange -
Peter Krempa