[libvirt] [PATCH 0/3] Add setting CPU features (CPUID) with libxenlight driver.
Marek Marczykowski-Górecki (3): cpu: define sub-leaf 0 for leaf 7 in cpu_map.xml libxl: add support for CPUID features policy tests: check domain XML to libxl structures conversion src/cpu/cpu_map.xml | 58 +++++------ src/libxl/libxl_conf.c | 139 ++++++++++++++++++++----- src/libxl/libxl_conf.h | 1 +- tests/Makefile.am | 8 +- tests/libxlxmltest.c | 232 ++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 383 insertions(+), 55 deletions(-) create mode 100644 tests/libxlxmltest.c base-commit: f914b3f2d24d05615095e5ab5b0c91560c4dc903 -- git-series 0.9.1
CPUID leaf 7 is sub-leaf aware. Add missing attribute. --- src/cpu/cpu_map.xml | 58 +++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 29b5b59..037a057 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -195,93 +195,93 @@ <!-- cpuid function 0x7 ecx 0x0 features --> <feature name='fsgsbase'> - <cpuid eax_in='0x07' ebx='0x00000001'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000001'/> </feature> <feature name='tsc_adjust'> <!-- tsc-adjust --> - <cpuid eax_in='0x07' ebx='0x00000002'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000002'/> </feature> <feature name='bmi1'> - <cpuid eax_in='0x07' ebx='0x00000008'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000008'/> </feature> <feature name='hle'> - <cpuid eax_in='0x07' ebx='0x00000010'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000010'/> </feature> <feature name='avx2'> - <cpuid eax_in='0x07' ebx='0x00000020'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000020'/> </feature> <feature name='smep'> - <cpuid eax_in='0x07' ebx='0x00000080'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000080'/> </feature> <feature name='bmi2'> - <cpuid eax_in='0x07' ebx='0x00000100'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000100'/> </feature> <feature name='erms'> - <cpuid eax_in='0x07' ebx='0x00000200'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000200'/> </feature> <feature name='invpcid'> - <cpuid eax_in='0x07' ebx='0x00000400'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000400'/> </feature> <feature name='rtm'> - <cpuid eax_in='0x07' ebx='0x00000800'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000800'/> </feature> <feature name='cmt'> - <cpuid eax_in='0x07' ebx='0x00001000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00001000'/> </feature> <feature name='mpx'> - <cpuid eax_in='0x07' ebx='0x00004000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00004000'/> </feature> <feature name='avx512f'> - <cpuid eax_in='0x07' ebx='0x00010000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00010000'/> </feature> <feature name='avx512dq'> - <cpuid eax_in='0x07' ebx='0x00020000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00020000'/> </feature> <feature name='rdseed'> - <cpuid eax_in='0x07' ebx='0x00040000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00040000'/> </feature> <feature name='adx'> - <cpuid eax_in='0x07' ebx='0x00080000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00080000'/> </feature> <feature name='smap'> - <cpuid eax_in='0x07' ebx='0x00100000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00100000'/> </feature> <feature name='avx512ifma'> - <cpuid eax_in='0x07' ebx='0x00200000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00200000'/> </feature> <feature name='clflushopt'> - <cpuid eax_in='0x07' ebx='0x00800000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00800000'/> </feature> <feature name='avx512pf'> - <cpuid eax_in='0x07' ebx='0x04000000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x04000000'/> </feature> <feature name='avx512er'> - <cpuid eax_in='0x07' ebx='0x08000000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x08000000'/> </feature> <feature name='avx512cd'> - <cpuid eax_in='0x07' ebx='0x10000000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x10000000'/> </feature> <feature name='avx512bw'> - <cpuid eax_in='0x07' ebx='0x40000000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x40000000'/> </feature> <feature name='avx512vl'> - <cpuid eax_in='0x07' ebx='0x80000000'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x80000000'/> </feature> <feature name='avx512vbmi'> - <cpuid eax_in='0x07' ecx='0x00000002'/> + <cpuid eax_in='0x07' ecx_in='0' ecx='0x00000002'/> </feature> <feature name='pku'> - <cpuid eax_in='0x07' ecx='0x00000008'/> + <cpuid eax_in='0x07' ecx_in='0' ecx='0x00000008'/> </feature> <feature name='ospke'> - <cpuid eax_in='0x07' ecx='0x00000010'/> + <cpuid eax_in='0x07' ecx_in='0' ecx='0x00000010'/> </feature> <feature name='avx512-4vnniw'> - <cpuid eax_in='0x07' edx='0x00000004'/> + <cpuid eax_in='0x07' ecx_in='0' edx='0x00000004'/> </feature> <feature name='avx512-4fmaps'> - <cpuid eax_in='0x07' edx='0x00000008'/> + <cpuid eax_in='0x07' ecx_in='0' edx='0x00000008'/> </feature> <!-- Processor Extended State Enumeration sub leaf 1 --> -- git-series 0.9.1
On Thu, Jun 29, 2017 at 03:11:41 +0200, Marek Marczykowski-Górecki wrote:
CPUID leaf 7 is sub-leaf aware. Add missing attribute. --- src/cpu/cpu_map.xml | 58 +++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 29b5b59..037a057 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -195,93 +195,93 @@
<!-- cpuid function 0x7 ecx 0x0 features --> <feature name='fsgsbase'> - <cpuid eax_in='0x07' ebx='0x00000001'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000001'/>
Why? Zero is the default value for ecx_in, I don't see a strong reason for mentioning it explicitly. If we do this, we should set the value to 0x00 though. Jirka
On Thu, Jun 29, 2017 at 09:51:58AM +0200, Jiri Denemark wrote:
On Thu, Jun 29, 2017 at 03:11:41 +0200, Marek Marczykowski-Górecki wrote:
CPUID leaf 7 is sub-leaf aware. Add missing attribute. --- src/cpu/cpu_map.xml | 58 +++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 29b5b59..037a057 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -195,93 +195,93 @@
<!-- cpuid function 0x7 ecx 0x0 features --> <feature name='fsgsbase'> - <cpuid eax_in='0x07' ebx='0x00000001'/> + <cpuid eax_in='0x07' ecx_in='0' ebx='0x00000001'/>
Why? Zero is the default value for ecx_in, I don't see a strong reason for mentioning it explicitly. If we do this, we should set the value to 0x00 though.
There is a difference (at CPUID instruction level) between ignoring ECX value (leaf 1 for example) and requiring it to be set to 0. Looks like virCPUx86CPUID structure does not support this distinction though. Anyway I think it's better to include it there, will change to 0x00. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
Set CPU features in appropriate libxl structure. Use old "xend" syntax, because it allow control over any bit and libvirt already have API for translating features to appropriate cpuid bits. And also features naming in libxl do not match the one of libvirt in multiple cases. Side effect is that all the bits derived from CPU <model> (which is required anyway) are also set as "required". Libxl do not have distinction between "force" and "required" policy (there is only "force") and also between "forbid" and "disable" (there is only "disable"). So, merge them appropriately. If anything, "require" and "forbid" should be enforced outside of specific driver. --- src/libxl/libxl_conf.c | 139 ++++++++++++++++++++++++++++++++++-------- src/libxl/libxl_conf.h | 1 +- 2 files changed, 116 insertions(+), 24 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 938e09d..4c400f4 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -291,6 +291,93 @@ libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf) return 0; } +static int libxlMakeCPUID(virArch arch, + const virCPUDef *cpu, + libxl_cpuid_policy_list *cpuid) +{ + virCPUDataPtr cpu_policy[4]; + /* forced, required, disabled, forbidden */ + char libxl_policy_char[4] = { '1', '1', '0', '0' }; + int leaf, subleaf, reg; + size_t i, j; + char leaf_text[48]; + char *reg_text; + int policy_index = 0; + uint32_t reg_value; + int ret = -1; + int cpuid_bit; + /* supported values, based on libxl_cpuid.c */ + int leafs[] = { 1, 6, 7, 0x80000001 }; + int subleafs[] = { -1, -1, 0, -1 }; + + if (cpuEncode(arch, + cpu, + &cpu_policy[0], + &cpu_policy[1], + NULL, + &cpu_policy[2], + &cpu_policy[3], + NULL)) + return -1; + + for (i = 0; i < sizeof(leafs)/sizeof(*leafs); i++) { + leaf = leafs[i]; + subleaf = subleafs[i]; + + for (reg = 0; reg < 4; reg++) { + if (subleaf != -1) + snprintf(leaf_text, sizeof(leaf_text), + "%#x,%d:e%cx=" LIBXL_DEFAULT_CPUID_REG_CONFIG, + leaf, subleaf, 'a'+reg); + else + snprintf(leaf_text, sizeof(leaf_text), + "%#x:e%cx=" LIBXL_DEFAULT_CPUID_REG_CONFIG, + leaf, 'a'+reg); + + reg_text = strstr(leaf_text, "=") + 1; + + for (policy_index = 0; policy_index < 4; policy_index++) { + /* search for this leaf in CPUData structure */ + for (j = 0; j < cpu_policy[policy_index]->data.x86.len; j++) { + virCPUx86CPUID *cpuid_policy = + &cpu_policy[policy_index]->data.x86.data[j]; + + if (cpuid_policy->eax_in == leaf && + (subleaf == -1 || cpuid_policy->ecx_in == subleaf)) { + + switch (reg) { + case 0: reg_value = cpuid_policy->eax; break; + case 1: reg_value = cpuid_policy->ebx; break; + case 2: reg_value = cpuid_policy->ecx; break; + case 3: reg_value = cpuid_policy->edx; break; + } + + for (cpuid_bit = 0; cpuid_bit < 32; cpuid_bit++) { + if (reg_value & (1<<cpuid_bit)) + reg_text[31-cpuid_bit] = libxl_policy_char[policy_index]; + } + } + } + } + + /* set it in domain config only if any bit was modified */ + if (STRNEQ(reg_text, LIBXL_DEFAULT_CPUID_REG_CONFIG)) { + if (libxl_cpuid_parse_config_xend(cpuid, leaf_text)) + goto cleanup; + } + } + } + + ret = 0; + + cleanup: + virCPUDataFree(cpu_policy[0]); + virCPUDataFree(cpu_policy[1]); + virCPUDataFree(cpu_policy[2]); + virCPUDataFree(cpu_policy[3]); + return ret; +} + static int libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_ctx *ctx, @@ -376,38 +463,42 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, def->features[VIR_DOMAIN_FEATURE_ACPI] == VIR_TRISTATE_SWITCH_ON); - if (caps && - def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { - bool hasHwVirt = false; - bool svm = false, vmx = false; + if (caps && def->cpu) { + if (def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { + bool hasHwVirt = false; + bool svm = false, vmx = false; - if (ARCH_IS_X86(def->os.arch)) { - vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx"); - svm = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "svm"); - hasHwVirt = vmx | svm; - } + if (ARCH_IS_X86(def->os.arch)) { + vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx"); + svm = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "svm"); + hasHwVirt = vmx | svm; + } - if (def->cpu->nfeatures) { - for (i = 0; i < def->cpu->nfeatures; i++) { + if (def->cpu->nfeatures) { + for (i = 0; i < def->cpu->nfeatures; i++) { - switch (def->cpu->features[i].policy) { + switch (def->cpu->features[i].policy) { - case VIR_CPU_FEATURE_DISABLE: - case VIR_CPU_FEATURE_FORBID: - if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) || - (svm && STREQ(def->cpu->features[i].name, "svm"))) - hasHwVirt = false; - break; + case VIR_CPU_FEATURE_DISABLE: + case VIR_CPU_FEATURE_FORBID: + if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) || + (svm && STREQ(def->cpu->features[i].name, "svm"))) + hasHwVirt = false; + break; - case VIR_CPU_FEATURE_FORCE: - case VIR_CPU_FEATURE_REQUIRE: - case VIR_CPU_FEATURE_OPTIONAL: - case VIR_CPU_FEATURE_LAST: - break; + case VIR_CPU_FEATURE_FORCE: + case VIR_CPU_FEATURE_REQUIRE: + case VIR_CPU_FEATURE_OPTIONAL: + case VIR_CPU_FEATURE_LAST: + break; + } } } + libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); } - libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); + + if (libxlMakeCPUID(def->os.arch, def->cpu, &b_info->cpuid)) + return -1; } if (def->nsounds > 0) { diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..8d89ccd 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -60,6 +60,7 @@ # define LIBXL_DUMP_DIR LIBXL_LIB_DIR "/dump" # define LIBXL_CHANNEL_DIR LIBXL_LIB_DIR "/channel/target" # define LIBXL_BOOTLOADER_PATH "pygrub" +# define LIBXL_DEFAULT_CPUID_REG_CONFIG "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" typedef struct _libxlDriverPrivate libxlDriverPrivate; -- git-series 0.9.1
On Thu, Jun 29, 2017 at 03:11:42 +0200, Marek Marczykowski-Górecki wrote:
Set CPU features in appropriate libxl structure. Use old "xend" syntax, because it allow control over any bit and libvirt already have API for translating features to appropriate cpuid bits. And also features naming in libxl do not match the one of libvirt in multiple cases.
How does the new syntax look like? And would it be actually better? The problem with setting individual CPUID bits is very fragile. That's mostly what we naively started to do in qemu driver and we're fighting with it since then. It's way too easy to create a virtual CPU which a guest OS will crash on. Jirka
On Thu, Jun 29, 2017 at 10:04:42AM +0200, Jiri Denemark wrote:
On Thu, Jun 29, 2017 at 03:11:42 +0200, Marek Marczykowski-Górecki wrote:
Set CPU features in appropriate libxl structure. Use old "xend" syntax, because it allow control over any bit and libvirt already have API for translating features to appropriate cpuid bits. And also features naming in libxl do not match the one of libvirt in multiple cases.
How does the new syntax look like?
"host,tm=0,sse3=0"
And would it be actually better?
Maybe? But the new syntax use different names for many features (like pni vs sse3, or sep vs sysenter), so it would require some ugly translation table... What I would really like here, is to get list of bits specified with <feature ...>, _without_ those defined by <model>. And maybe even without requiring <model> to be set at all. Another misleading thing about interpreting <model> here is, it doesn't mask out features not really present in that CPU. So, you get a CPU based on <model> but with many additional features.
The problem with setting individual CPUID bits is very fragile. That's mostly what we naively started to do in qemu driver and we're fighting with it since then. It's way too easy to create a virtual CPU which a guest OS will crash on.
Well, IMHO if you modify things like CPUID you should really know what you are doing... -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
On Thu, Jun 29, 2017 at 10:39:26AM +0200, Marek Marczykowski-Górecki wrote:
On Thu, Jun 29, 2017 at 10:04:42AM +0200, Jiri Denemark wrote:
On Thu, Jun 29, 2017 at 03:11:42 +0200, Marek Marczykowski-Górecki wrote:
Set CPU features in appropriate libxl structure. Use old "xend" syntax, because it allow control over any bit and libvirt already have API for translating features to appropriate cpuid bits. And also features naming in libxl do not match the one of libvirt in multiple cases.
How does the new syntax look like?
"host,tm=0,sse3=0"
And would it be actually better?
Maybe? But the new syntax use different names for many features (like pni vs sse3, or sep vs sysenter), so it would require some ugly translation table...
That's not particularly difficult or ugly. Currently virCPUFeatureDEf stores the feature as a string, but we could easily define an enum for the feature names, and then store them as an int. We already have support for doing trivial enum -> string conversions.
What I would really like here, is to get list of bits specified with <feature ...>, _without_ those defined by <model>. And maybe even without requiring <model> to be set at all.
That would take operation of the Xen driver in a very different direction to the QEMU driver, which I feels is not desirable for applications using libvirt.
Another misleading thing about interpreting <model> here is, it doesn't mask out features not really present in that CPU. So, you get a CPU based on <model> but with many additional features.
The problem with setting individual CPUID bits is very fragile. That's mostly what we naively started to do in qemu driver and we're fighting with it since then. It's way too easy to create a virtual CPU which a guest OS will crash on.
Well, IMHO if you modify things like CPUID you should really know what you are doing...
The problem is that we've seen real world evidence that apps & users don't know what they're doing here. Sooo many bug reports where guests have been given arbitrary features, causing the guest OS and/or apps to randomly crash due to wierdly unexpected combinations of features. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jun 29, 2017 at 10:39:26 +0200, Marek Marczykowski-Górecki wrote:
On Thu, Jun 29, 2017 at 10:04:42AM +0200, Jiri Denemark wrote:
On Thu, Jun 29, 2017 at 03:11:42 +0200, Marek Marczykowski-Górecki wrote:
Set CPU features in appropriate libxl structure. Use old "xend" syntax, because it allow control over any bit and libvirt already have API for translating features to appropriate cpuid bits. And also features naming in libxl do not match the one of libvirt in multiple cases.
How does the new syntax look like?
"host,tm=0,sse3=0"
Is "host" fixed there or are other CPU models supported too? If only "host" is allowed, libxl driver should support host-passthrough mode only and it would be pretty easy to map it to the libxl's syntax. The following CPU XML <cpu mode='host-passthrough'> <feature name='avx' policy='disable'/> <feature name='aes' policy='require'/> </cpu> could be directly translated into "host,avx=0,aes=1".
And would it be actually better?
Maybe? But the new syntax use different names for many features (like pni vs sse3, or sep vs sysenter), so it would require some ugly translation table...
I don't a translation table would be ugly.
What I would really like here, is to get list of bits specified with <feature ...>, _without_ those defined by <model>. And maybe even without requiring <model> to be set at all.
That's what host-passthrough mode is for. Jirka
On Thu, Jun 29, 2017 at 11:00:35AM +0200, Jiri Denemark wrote:
On Thu, Jun 29, 2017 at 10:39:26 +0200, Marek Marczykowski-Górecki wrote:
On Thu, Jun 29, 2017 at 10:04:42AM +0200, Jiri Denemark wrote:
On Thu, Jun 29, 2017 at 03:11:42 +0200, Marek Marczykowski-Górecki wrote:
Set CPU features in appropriate libxl structure. Use old "xend" syntax, because it allow control over any bit and libvirt already have API for translating features to appropriate cpuid bits. And also features naming in libxl do not match the one of libvirt in multiple cases.
How does the new syntax look like?
"host,tm=0,sse3=0"
Is "host" fixed there or are other CPU models supported too?
Only "host".
If only "host" is allowed, libxl driver should support host-passthrough mode only and it would be pretty easy to map it to the libxl's syntax. The following CPU XML
<cpu mode='host-passthrough'> <feature name='avx' policy='disable'/> <feature name='aes' policy='require'/> </cpu>
could be directly translated into "host,avx=0,aes=1".
That would make sense, indeed. One practical issue is that I'm doing the whole thing to disable "smap" bit. And this bit isn't supported in the new libxl syntax. At least not yet - I've already submitted a patch to add it :)
And would it be actually better?
Maybe? But the new syntax use different names for many features (like pni vs sse3, or sep vs sysenter), so it would require some ugly translation table...
I don't a translation table would be ugly.
Ok.
What I would really like here, is to get list of bits specified with <feature ...>, _without_ those defined by <model>. And maybe even without requiring <model> to be set at all.
That's what host-passthrough mode is for.
Ok. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
On 06/29/2017 09:39 AM, Marek Marczykowski-Górecki wrote:
On Thu, Jun 29, 2017 at 10:04:42AM +0200, Jiri Denemark wrote:
On Thu, Jun 29, 2017 at 03:11:42 +0200, Marek Marczykowski-Górecki wrote:
Set CPU features in appropriate libxl structure. Use old "xend" syntax, because it allow control over any bit and libvirt already have API for translating features to appropriate cpuid bits. And also features naming in libxl do not match the one of libvirt in multiple cases.
How does the new syntax look like?
"host,tm=0,sse3=0"
And would it be actually better?
Maybe? But the new syntax use different names for many features (like pni vs sse3, or sep vs sysenter), so it would require some ugly translation table... What I would really like here, is to get list of bits specified with <feature ...>, _without_ those defined by <model>. And maybe even without requiring <model> to be set at all. Another misleading thing about interpreting <model> here is, it doesn't mask out features not really present in that CPU. So, you get a CPU based on <model> but with many additional features.
Two suggestions (the first one to hopefully complement current discussion): 1) Maybe there could be a driver config item to allow the Xend syntax (since it's a rather fragile facility that allows all bits controlled)? Or else prefer first libxl format through the translation table of features and use xend format for this driver config such that non-supported features in libxl format fallback to xend format instead? Probably this allows for best of both worlds (better safety with libxl-format) yet higher flexibility/controllability with xend format. Also I am not sure we should support models either - since our CPUID view is restricted to host-{passthrough,model} only. 2) perhaps part of the code in this patch could be better placed at xenconfig/xen_{xl,xm}.c since it's really two different formats of config? This would also allow supporting Xen <-> Xml config file conversions. Joao
On Thu, Jun 29, 2017 at 10:21:11AM +0100, Joao Martins wrote:
On 06/29/2017 09:39 AM, Marek Marczykowski-Górecki wrote:
On Thu, Jun 29, 2017 at 10:04:42AM +0200, Jiri Denemark wrote:
On Thu, Jun 29, 2017 at 03:11:42 +0200, Marek Marczykowski-Górecki wrote:
Set CPU features in appropriate libxl structure. Use old "xend" syntax, because it allow control over any bit and libvirt already have API for translating features to appropriate cpuid bits. And also features naming in libxl do not match the one of libvirt in multiple cases.
How does the new syntax look like?
"host,tm=0,sse3=0"
And would it be actually better?
Maybe? But the new syntax use different names for many features (like pni vs sse3, or sep vs sysenter), so it would require some ugly translation table... What I would really like here, is to get list of bits specified with <feature ...>, _without_ those defined by <model>. And maybe even without requiring <model> to be set at all. Another misleading thing about interpreting <model> here is, it doesn't mask out features not really present in that CPU. So, you get a CPU based on <model> but with many additional features.
Two suggestions (the first one to hopefully complement current discussion):
1) Maybe there could be a driver config item to allow the Xend syntax (since it's a rather fragile facility that allows all bits controlled)? Or else prefer first libxl format through the translation table of features and use xend format for this driver config such that non-supported features in libxl format fallback to xend format instead? Probably this allows for best of both worlds (better safety with libxl-format) yet higher flexibility/controllability with xend format. Also I am not sure we should support models either - since our CPUID view is restricted to host-{passthrough,model} only.
IIUC setting CPU model is required to get CPUID bits from libvirt cpu API (cpuEncode function). If there is some other way, that would be great. But if that would require changing cpu libvirt internal API, I think just a fallback doesn't worth it.
2) perhaps part of the code in this patch could be better placed at xenconfig/xen_{xl,xm}.c since it's really two different formats of config? This would also allow supporting Xen <-> Xml config file conversions.
Ok, I'll look into it. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
libxl contains a method to dump libxl_domain_config in json format, which is really convenient for tests. Unfortunately it require libxl_ctx, which can be only created when running on Xen (wither in dom0 or domU). Because of this, skip the test in other cases. For now, have just two tests, including just-introduced CPUID handling, but this commit introduces a framework for further tests of src/libxl/libxl_conf.c file. --- tests/Makefile.am | 8 +- tests/libxlxmltest.c | 232 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 238 insertions(+), 2 deletions(-) create mode 100644 tests/libxlxmltest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 19986dc..06c5c73 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -274,7 +274,7 @@ test_programs += xml2sexprtest sexpr2xmltest \ endif WITH_XEN if WITH_LIBXL -test_programs += xlconfigtest +test_programs += xlconfigtest libxlxmltest endif WITH_LIBXL if WITH_QEMU @@ -528,9 +528,13 @@ libxl_LDADDS += $(LDADDS) xlconfigtest_SOURCES = \ xlconfigtest.c testutilsxen.c testutilsxen.h \ testutils.c testutils.h +libxlxmltest_SOURCES = \ + libxlxmltest.c testutilsxen.c testutilsxen.h \ + testutils.c testutils.h xlconfigtest_LDADD =$(libxl_LDADDS) +libxlxmltest_LDADD =$(libxl_LDADDS) else ! WITH_LIBXL -EXTRA_DIST += xlconfigtest.c +EXTRA_DIST += xlconfigtest.c libxlxmltest.c endif ! WITH_LIBXL QEMUMONITORTESTUTILS_SOURCES = \ diff --git a/tests/libxlxmltest.c b/tests/libxlxmltest.c new file mode 100644 index 0000000..7e11af4 --- /dev/null +++ b/tests/libxlxmltest.c @@ -0,0 +1,232 @@ +/* + * libxlxmltest.c: Test xl.cfg(5) <-> libxl_domain_config conversion + * + * Copyright (C) 2007, 2010-2011, 2014 Red Hat, Inc. + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + * Author: Kiarie Kahurani <davidkiarie4@gmail.com> + * Author: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> + * + */ + +#include <config.h> + +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#ifdef WITH_YAJL2 +# define HAVE_YAJL_V2 1 +#endif +#include <libxl.h> +#include <libxl_json.h> + +#include "internal.h" +#include "datatypes.h" +#include "viralloc.h" +#include "virstring.h" +#include "testutils.h" +#include "testutilsxen.h" +#include "libxl/libxl_conf.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +static virCapsPtr caps; +static virDomainXMLOptionPtr xmlopt; +xentoollog_logger_stdiostream *logger; +libxl_ctx *ctx; + + +/* + * This function provides a mechanism to replace variables in test + * data files whose values are discovered at built time. + */ +static char * +testReplaceVarsXML(const char *xml) +{ + char *xmlcfgData; + char *replacedXML; + + if (virTestLoadFile(xml, &xmlcfgData) < 0) + return NULL; + + replacedXML = virStringReplace(xmlcfgData, "/LIBXL_FIRMWARE_DIR", + LIBXL_FIRMWARE_DIR); + + /* libxl driver checks for emulator existence */ + replacedXML = virStringReplace(xmlcfgData, "/usr/lib/xen/bin/qemu-system-i386", + "/bin/true"); + + VIR_FREE(xmlcfgData); + return replacedXML; +} + +/* + * Parses domXML to virDomainDef object, which is then converted to json + * config and compared with expected config. + */ +static int +testCompareParseXML(const char *json, const char *xml, bool replaceVars) +{ + char *gotjsonData = NULL; + virConfPtr conf = NULL; + virConnectPtr conn = NULL; + size_t wrote = 0; + int ret = -1; + virDomainDefPtr def = NULL; + char *replacedXML = NULL; + virPortAllocatorPtr reservedGraphicsPorts = NULL; + libxl_domain_config d_config; + yajl_gen gen = NULL; + + conn = virGetConnect(); + if (!conn) goto fail; + + if (replaceVars) { + if (!(replacedXML = testReplaceVarsXML(xml))) + goto fail; + if (!(def = virDomainDefParseString(replacedXML, caps, xmlopt, + NULL, VIR_DOMAIN_XML_INACTIVE))) + goto fail; + } else { + if (!(def = virDomainDefParseFile(xml, caps, xmlopt, + NULL, VIR_DOMAIN_XML_INACTIVE))) + goto fail; + } + + if (!(reservedGraphicsPorts = virPortAllocatorNew("VNC", + LIBXL_VNC_PORT_MIN, + LIBXL_VNC_PORT_MAX, + 0))) + goto fail; + + if (libxlBuildDomainConfig(reservedGraphicsPorts, + def, + NULL, /* channelDir, unused in current version */ + ctx, + caps, + &d_config)) + goto fail; + + if (!(gen = libxl_yajl_gen_alloc(NULL))) + goto fail; + + if (libxl_domain_config_gen_json(gen, &d_config) != yajl_gen_status_ok) + goto fail; + + if (yajl_gen_get_buf(gen, + (const unsigned char **)&gotjsonData, + &wrote) != yajl_gen_status_ok) + goto fail; + + if (virTestCompareToFile(gotjsonData, json) < 0) + goto fail; + + ret = 0; + + fail: + VIR_FREE(replacedXML); + // yajl_gen_free handle also gotjsonData + if (gen) + yajl_gen_free(gen); + if (conf) + virConfFree(conf); + virObjectUnref(reservedGraphicsPorts); + virDomainDefFree(def); + virObjectUnref(conn); + + return ret; +} + +struct testInfo { + const char *name; + bool replaceVars; +}; + +static int +testCompareHelper(const void *data) +{ + int result = -1; + const struct testInfo *info = data; + char *xml = NULL; + char *json = NULL; + + if (virAsprintf(&xml, "%s/xlconfigdata/test-%s.xml", + abs_srcdir, info->name) < 0 || + virAsprintf(&json, "%s/xlconfigdata/test-%s.json", + abs_srcdir, info->name) < 0) + goto cleanup; + + result = testCompareParseXML(json, xml, info->replaceVars); + + cleanup: + VIR_FREE(xml); + VIR_FREE(json); + + return result; +} + + +static int +mymain(void) +{ + int ret = 0; + + logger = xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0); + if (!logger) + return EXIT_FAILURE; + + if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, (xentoollog_logger*)logger)) + return EXIT_AM_SKIP; + + if (!(caps = testXLInitCaps())) + return EXIT_FAILURE; + + if (!(xmlopt = libxlCreateXMLConf())) + return EXIT_FAILURE; + +#define DO_TEST_PARSE(name, replace) \ + do { \ + struct testInfo info0 = { name, replace }; \ + if (virTestRun("Xen XML-2-json Parse " name, \ + testCompareHelper, &info0) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST(name) \ + do { \ + DO_TEST_PARSE(name, false); \ + } while (0) + +#define DO_TEST_REPLACE_VARS(name) \ + do { \ + DO_TEST_PARSE(name, true); \ + } while (0) + + DO_TEST_REPLACE_VARS("fullvirt-ovmf"); + + DO_TEST("fullvirt-cpuid"); + + virObjectUnref(caps); + virObjectUnref(xmlopt); + libxl_ctx_free(ctx); + xtl_logger_destroy((xentoollog_logger*)logger); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) -- git-series 0.9.1
On 06/29/2017 02:11 AM, Marek Marczykowski-Górecki wrote:
libxl contains a method to dump libxl_domain_config in json format, which is really convenient for tests. Unfortunately it require libxl_ctx, which can be only created when running on Xen (wither in dom0 or domU). Because of this, skip the test in other cases.
For now, have just two tests, including just-introduced CPUID handling, but this commit introduces a framework for further tests of src/libxl/libxl_conf.c file.
There's related work about this. You might wanna look at the recent Jim's series (CC'in him) from a few months ago (see below). He ressurected danpb work on these xml <-> libxl_domain_config conversion, and had a whole test suite for that where your cpuid test would be better inserted? Patch series is here: https://www.redhat.com/archives/libvir-list/2017-February/msg01477.html
--- tests/Makefile.am | 8 +- tests/libxlxmltest.c | 232 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 238 insertions(+), 2 deletions(-) create mode 100644 tests/libxlxmltest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am index 19986dc..06c5c73 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -274,7 +274,7 @@ test_programs += xml2sexprtest sexpr2xmltest \ endif WITH_XEN
if WITH_LIBXL -test_programs += xlconfigtest +test_programs += xlconfigtest libxlxmltest endif WITH_LIBXL
if WITH_QEMU @@ -528,9 +528,13 @@ libxl_LDADDS += $(LDADDS) xlconfigtest_SOURCES = \ xlconfigtest.c testutilsxen.c testutilsxen.h \ testutils.c testutils.h +libxlxmltest_SOURCES = \ + libxlxmltest.c testutilsxen.c testutilsxen.h \ + testutils.c testutils.h xlconfigtest_LDADD =$(libxl_LDADDS) +libxlxmltest_LDADD =$(libxl_LDADDS) else ! WITH_LIBXL -EXTRA_DIST += xlconfigtest.c +EXTRA_DIST += xlconfigtest.c libxlxmltest.c endif ! WITH_LIBXL
QEMUMONITORTESTUTILS_SOURCES = \ diff --git a/tests/libxlxmltest.c b/tests/libxlxmltest.c new file mode 100644 index 0000000..7e11af4 --- /dev/null +++ b/tests/libxlxmltest.c @@ -0,0 +1,232 @@ +/* + * libxlxmltest.c: Test xl.cfg(5) <-> libxl_domain_config conversion + * + * Copyright (C) 2007, 2010-2011, 2014 Red Hat, Inc. + * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + * Author: Kiarie Kahurani <davidkiarie4@gmail.com> + * Author: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> + * + */ + +#include <config.h> + +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#ifdef WITH_YAJL2 +# define HAVE_YAJL_V2 1 +#endif +#include <libxl.h> +#include <libxl_json.h> + +#include "internal.h" +#include "datatypes.h" +#include "viralloc.h" +#include "virstring.h" +#include "testutils.h" +#include "testutilsxen.h" +#include "libxl/libxl_conf.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +static virCapsPtr caps; +static virDomainXMLOptionPtr xmlopt; +xentoollog_logger_stdiostream *logger; +libxl_ctx *ctx; + + +/* + * This function provides a mechanism to replace variables in test + * data files whose values are discovered at built time. + */ +static char * +testReplaceVarsXML(const char *xml) +{ + char *xmlcfgData; + char *replacedXML; + + if (virTestLoadFile(xml, &xmlcfgData) < 0) + return NULL; + + replacedXML = virStringReplace(xmlcfgData, "/LIBXL_FIRMWARE_DIR", + LIBXL_FIRMWARE_DIR); + + /* libxl driver checks for emulator existence */ + replacedXML = virStringReplace(xmlcfgData, "/usr/lib/xen/bin/qemu-system-i386", + "/bin/true"); + + VIR_FREE(xmlcfgData); + return replacedXML; +} + +/* + * Parses domXML to virDomainDef object, which is then converted to json + * config and compared with expected config. + */ +static int +testCompareParseXML(const char *json, const char *xml, bool replaceVars) +{ + char *gotjsonData = NULL; + virConfPtr conf = NULL; + virConnectPtr conn = NULL; + size_t wrote = 0; + int ret = -1; + virDomainDefPtr def = NULL; + char *replacedXML = NULL; + virPortAllocatorPtr reservedGraphicsPorts = NULL; + libxl_domain_config d_config; + yajl_gen gen = NULL; + + conn = virGetConnect(); + if (!conn) goto fail; + + if (replaceVars) { + if (!(replacedXML = testReplaceVarsXML(xml))) + goto fail; + if (!(def = virDomainDefParseString(replacedXML, caps, xmlopt, + NULL, VIR_DOMAIN_XML_INACTIVE))) + goto fail; + } else { + if (!(def = virDomainDefParseFile(xml, caps, xmlopt, + NULL, VIR_DOMAIN_XML_INACTIVE))) + goto fail; + } + + if (!(reservedGraphicsPorts = virPortAllocatorNew("VNC", + LIBXL_VNC_PORT_MIN, + LIBXL_VNC_PORT_MAX, + 0))) + goto fail; + + if (libxlBuildDomainConfig(reservedGraphicsPorts, + def, + NULL, /* channelDir, unused in current version */ + ctx, + caps, + &d_config)) + goto fail; + + if (!(gen = libxl_yajl_gen_alloc(NULL))) + goto fail; + + if (libxl_domain_config_gen_json(gen, &d_config) != yajl_gen_status_ok) + goto fail; + + if (yajl_gen_get_buf(gen, + (const unsigned char **)&gotjsonData, + &wrote) != yajl_gen_status_ok) + goto fail; + + if (virTestCompareToFile(gotjsonData, json) < 0) + goto fail; + + ret = 0; + + fail: + VIR_FREE(replacedXML); + // yajl_gen_free handle also gotjsonData + if (gen) + yajl_gen_free(gen); + if (conf) + virConfFree(conf); + virObjectUnref(reservedGraphicsPorts); + virDomainDefFree(def); + virObjectUnref(conn); + + return ret; +} + +struct testInfo { + const char *name; + bool replaceVars; +}; + +static int +testCompareHelper(const void *data) +{ + int result = -1; + const struct testInfo *info = data; + char *xml = NULL; + char *json = NULL; + + if (virAsprintf(&xml, "%s/xlconfigdata/test-%s.xml", + abs_srcdir, info->name) < 0 || + virAsprintf(&json, "%s/xlconfigdata/test-%s.json", + abs_srcdir, info->name) < 0) + goto cleanup; + + result = testCompareParseXML(json, xml, info->replaceVars); + + cleanup: + VIR_FREE(xml); + VIR_FREE(json); + + return result; +} + + +static int +mymain(void) +{ + int ret = 0; + + logger = xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0); + if (!logger) + return EXIT_FAILURE; + + if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, (xentoollog_logger*)logger)) + return EXIT_AM_SKIP; + + if (!(caps = testXLInitCaps())) + return EXIT_FAILURE; + + if (!(xmlopt = libxlCreateXMLConf())) + return EXIT_FAILURE; + +#define DO_TEST_PARSE(name, replace) \ + do { \ + struct testInfo info0 = { name, replace }; \ + if (virTestRun("Xen XML-2-json Parse " name, \ + testCompareHelper, &info0) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST(name) \ + do { \ + DO_TEST_PARSE(name, false); \ + } while (0) + +#define DO_TEST_REPLACE_VARS(name) \ + do { \ + DO_TEST_PARSE(name, true); \ + } while (0) + + DO_TEST_REPLACE_VARS("fullvirt-ovmf"); + + DO_TEST("fullvirt-cpuid"); + + virObjectUnref(caps); + virObjectUnref(xmlopt); + libxl_ctx_free(ctx); + xtl_logger_destroy((xentoollog_logger*)logger); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain)
On Thu, Jun 29, 2017 at 10:06:39AM +0100, Joao Martins wrote:
On 06/29/2017 02:11 AM, Marek Marczykowski-Górecki wrote:
libxl contains a method to dump libxl_domain_config in json format, which is really convenient for tests. Unfortunately it require libxl_ctx, which can be only created when running on Xen (wither in dom0 or domU). Because of this, skip the test in other cases.
For now, have just two tests, including just-introduced CPUID handling, but this commit introduces a framework for further tests of src/libxl/libxl_conf.c file.
There's related work about this. You might wanna look at the recent Jim's series (CC'in him) from a few months ago (see below). He ressurected danpb work on these xml <-> libxl_domain_config conversion, and had a whole test suite for that where your cpuid test would be better inserted? Patch series is here:
https://www.redhat.com/archives/libvir-list/2017-February/msg01477.html
This looks like a much more complete solution. Indeed it would make sense to put this cpuid test there. Is there any reason why it isn't merged yet and there is no response in that thread? -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
On 06/29/2017 11:08 PM, Marek Marczykowski-Górecki wrote:
On Thu, Jun 29, 2017 at 10:06:39AM +0100, Joao Martins wrote:
On 06/29/2017 02:11 AM, Marek Marczykowski-Górecki wrote:
libxl contains a method to dump libxl_domain_config in json format, which is really convenient for tests. Unfortunately it require libxl_ctx, which can be only created when running on Xen (wither in dom0 or domU). Because of this, skip the test in other cases.
For now, have just two tests, including just-introduced CPUID handling, but this commit introduces a framework for further tests of src/libxl/libxl_conf.c file.
There's related work about this. You might wanna look at the recent Jim's series (CC'in him) from a few months ago (see below). He ressurected danpb work on these xml <-> libxl_domain_config conversion, and had a whole test suite for that where your cpuid test would be better inserted? Patch series is here:
https://www.redhat.com/archives/libvir-list/2017-February/msg01477.html
This looks like a much more complete solution. Indeed it would make sense to put this cpuid test there. Is there any reason why it isn't merged yet and there is no response in that thread?
I guess it wasn't reviewed by folks. I was short on time with other deliverables so that series slipped me and couldn't help much. Perhaps a resubmission to restart discussion - It's really nice work and would greatly improve testing :) Cheers, Joao
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
Joao Martins -
Marek Marczykowski-Górecki