Re: [libvirt] [PATCH v6 4/9] libxl: do not enable nested HVM unless global nested_hvm option enabled

On 03/21/2018 10:32 AM, 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. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek(a)invisiblethingslab.com&gt; > Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > Changes since v4: > - add nested_hvm option to test_libvirtd_libxl.aug.in and libvirtd_libxl.aug > - make it possible to override nested_hvm=0 with explicit <feature > policy='require' name='vmx'/> > - split xenconfig changes into separate commits > 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/libvirtd_libxl.aug | 2 ++ > src/libxl/libxl.conf | 8 ++++++++ > src/libxl/libxl_conf.c | 12 +++++++++++- > src/libxl/libxl_conf.h | 2 ++ > src/libxl/test_libvirtd_libxl.aug.in | 1 + > tests/libxlxml2domconfigtest.c | 3 +++ > 6 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/src/libxl/libvirtd_libxl.aug b/src/libxl/libvirtd_libxl.aug > index b31cc07..58b9af3 100644 > --- a/src/libxl/libvirtd_libxl.aug > +++ b/src/libxl/libvirtd_libxl.aug > @@ -28,12 +28,14 @@ module Libvirtd_libxl = > let lock_entry = str_entry "lock_manager" > let keepalive_interval_entry = int_entry "keepalive_interval" > let keepalive_count_entry = int_entry "keepalive_count" > + let nested_hvm_entry = bool_entry "nested_hvm" > (* Each entry in the config is one of the following ... *) > let entry = autoballoon_entry > | lock_entry > | keepalive_interval_entry > | keepalive_count_entry > + | nested_hvm_entry > let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] > let empty = [ label "#empty" . eol ] > diff --git a/src/libxl/libxl.conf b/src/libxl/libxl.conf > index 264af7c..72825a7 100644 > --- a/src/libxl/libxl.conf > +++ b/src/libxl/libxl.conf > @@ -41,3 +41,11 @@ > # > #keepalive_interval = 5 > #keepalive_count = 5 > + > +# Nested HVM default 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. This can be overridden in domain configuration by > +# explicitly setting <feature policy='require' name='vmx'/> inside <cpu/> > +# element. Cool, the setting can be overridden by per-domain config. > +# By default it is disabled. > +#nested_hvm = 0 > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index dcfdd67..3b9e828 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -360,7 +360,9 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > bool hasHwVirt = false; > bool svm = false, vmx = false; > - if (ARCH_IS_X86(def->os.arch)) { > + /* enable nested HVM only if global nested_hvm option enable it and > + * host support 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; But IIUC this change will not allow per-domain config to override the global setting. If cfg->nested_hvm is false, svm and vmx are both false and FEATURE_REQUIRE is not honored.

Regards, Jim > @@ -380,6 +382,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > case VIR_CPU_FEATURE_FORCE: > case VIR_CPU_FEATURE_REQUIRE: > + if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) || > + (svm && STREQ(def->cpu->features[i].name, "svm"))) > + hasHwVirt = true; > + break; > + > case VIR_CPU_FEATURE_OPTIONAL: > case VIR_CPU_FEATURE_LAST: > break; > @@ -1699,6 +1706,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 ce9db26..27cfc1a 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/libxl/test_libvirtd_libxl.aug.in b/src/libxl/test_libvirtd_libxl.aug.in > index 63558e5..9106abe 100644 > --- a/src/libxl/test_libvirtd_libxl.aug.in > +++ b/src/libxl/test_libvirtd_libxl.aug.in > @@ -6,3 +6,4 @@ module Test_libvirtd_libxl = > { "lock_manager" = "lockd" } > { "keepalive_interval" = "5" } > { "keepalive_count" = "5" } > +{ "nested_hvm" = "1" } > diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c > index cfbc37c..8819032 100644 > --- a/tests/libxlxml2domconfigtest.c > +++ b/tests/libxlxml2domconfigtest.c > @@ -76,6 +76,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); >