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

On Wed, Mar 21, 2018 at 05:55:28PM -0600, Jim Fehlig wrote: > 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. Ough, conflict resolution went wrong after changing 3/9 :/ Fixed patch will follow.