[libvirt] [PATCH v4 4/8] libxl: do not enable nested HVM unless global nested_hvm option enabled
Jim Fehlig
jfehlig at suse.com
Mon Feb 26 22:47:11 UTC 2018
On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:
> Introduce global libxl option for enabling nested HVM feature, similar
> to kvm module parameter. This will prevent enabling experimental feature
> by mere presence of <cpu mode='host-passthrough'> element in domain
> config, unless explicitly enabled. <cpu mode='host-passthrough'> element
> may be used to configure other features, like NUMA, or CPUID.
I don't think we should mix this change with the next two.
> Also, adjust xenconfig driver to appropriately translate to/from
> nestedhvm=1.
>
> While at it, adjust xenconfig driver to not override def->cpu if already
> set elsewhere. This will help with adding cpuid support.
These two look fine, but are actually separate bug fixes IMO. (This series keeps
uncovering all sorts of goodies.)
> ---
> As for xenconfig part, I'm not sure if missing "nestedhvm" option in xl
> config shouldn't produce <cpu> element too, to disable nested HVM (as it
> would be on plain xl). Any preference?
Given the global option to disable nesting, I don't think we need to worry about
missing "nestedhvm". Even if xl.cfg contains "vnuma" and/or "cpuid", and hence
we have a def->cpu, the global setting would ensure nestedhvm is disabled. If
one were paranoid, <feature policy='disable' name='vmx|svm'/> could be added to
an existing def->cpu.
> Changes since v3:
> - use config option nested_hvm, instead of requiring explicit <feature
> ...> entries
> - title changed from "libxl: do not enable nested HVM by mere presence
> of <cpu> element"
> - xenconfig: don't add <feature policy='force' name='vmx'/> since it is
> implied by presence of <cpu> element
> - xenconfig: produce <cpu> element even when converting on host not
> supporting vmx/svm, to not lose setting value
> Changes since v2:
> - new patch
> ---
> src/libxl/libxl.conf | 6 ++++++-
> src/libxl/libxl_conf.c | 7 ++++++-
> src/libxl/libxl_conf.h | 2 ++-
> src/xenconfig/xen_xl.c | 37 +++++++++++------------------------
> tests/libxlxml2domconfigtest.c | 3 +++-
> 5 files changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/src/libxl/libxl.conf b/src/libxl/libxl.conf
> index 264af7c..0e842c9 100644
> --- a/src/libxl/libxl.conf
> +++ b/src/libxl/libxl.conf
> @@ -41,3 +41,9 @@
> #
> #keepalive_interval = 5
> #keepalive_count = 5
> +
> +# Nested HVM global control. In order to use nested HVM feature, this option
> +# needs to be enabled, in addition to specifying <cpu mode='host-passthrough'>
> +# in domain configuration.
> +# By default it is disabled.
> +#nested_hvm = 0
I think per-domain settings should override this one. Users would find it odd
that they don't have vmx in their hvm guest with
<cpu mode='host-passthrough'>
<feature policy='require' name='vmx'/>
</cpu>
Regards,
Jim
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 66956a7..417ce7c 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -366,7 +366,9 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> return -1;
> }
>
> - if (ARCH_IS_X86(def->os.arch)) {
> + /* consider host support for nested HVM only if global nested_hvm
> + * option enable it */
> + if (cfg->nested_hvm && 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;
> @@ -1699,6 +1701,9 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
> if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0)
> goto cleanup;
>
> + if (virConfGetValueBool(conf, "nested_hvm", &cfg->nested_hvm) < 0)
> + goto cleanup;
> +
> ret = 0;
>
> cleanup:
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 8eefe06..e32a5e7 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -88,6 +88,8 @@ struct _libxlDriverConfig {
> int keepAliveInterval;
> unsigned int keepAliveCount;
>
> + bool nested_hvm;
> +
> /* Once created, caps are immutable */
> virCapsPtr caps;
>
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 6cda305..394cc0d 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -170,17 +170,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0)
> return -1;
>
> - if (val == 1) {
> - virCPUDefPtr cpu;
> -
> - if (VIR_ALLOC(cpu) < 0)
> - return -1;
> -
> - cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
> - cpu->type = VIR_CPU_TYPE_GUEST;
> - def->cpu = cpu;
> - } else if (val == 0) {
> - const char *vtfeature = NULL;
> + if (val != -1) {
> + const char *vtfeature = "vmx";
>
> if (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch)) {
> if (virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx"))
> @@ -189,28 +180,24 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
> vtfeature = "svm";
> }
>
> - if (vtfeature) {
> + if (!def->cpu) {
> virCPUDefPtr cpu;
> -
> if (VIR_ALLOC(cpu) < 0)
> return -1;
>
> - if (VIR_ALLOC(cpu->features) < 0) {
> - VIR_FREE(cpu);
> - return -1;
> - }
> -
> - if (VIR_STRDUP(cpu->features->name, vtfeature) < 0) {
> - VIR_FREE(cpu->features);
> - VIR_FREE(cpu);
> - return -1;
> - }
> - cpu->features->policy = VIR_CPU_FEATURE_DISABLE;
> - cpu->nfeatures = cpu->nfeatures_max = 1;
> cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
> cpu->type = VIR_CPU_TYPE_GUEST;
> + cpu->nfeatures = 0;
> + cpu->nfeatures_max = 0;
> def->cpu = cpu;
> }
> +
> + if (val == 0) {
> + if (virCPUDefAddFeature(def->cpu,
> + vtfeature,
> + VIR_CPU_FEATURE_DISABLE) < 0)
> + return -1;
> + }
> }
> } else {
> if (xenConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
> diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
> index 0105550..f2af286 100644
> --- a/tests/libxlxml2domconfigtest.c
> +++ b/tests/libxlxml2domconfigtest.c
> @@ -74,6 +74,9 @@ testCompareXMLToDomConfig(const char *xmlfile,
> if (!(log = (xentoollog_logger *)xtl_createlogger_stdiostream(stderr, XTL_DEBUG, 0)))
> goto cleanup;
>
> + /* for testing nested HVM */
> + cfg->nested_hvm = true;
> +
> /* replace logger with stderr one */
> libxl_ctx_free(cfg->ctx);
>
>
More information about the libvir-list
mailing list