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@invisiblethingslab.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
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.
+# 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;
@@ -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);
 
-- 
git-series 0.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@invisiblethingslab.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> 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);
>   
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@invisiblethingslab.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > 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.

> 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);
> > 
> 

-- 
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?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 03/21/2018 06:05 PM, Marek Marczykowski-Górecki wrote:
> 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@invisiblethingslab.com>
>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>> 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.

Ok. No need to send the whole series again. Just a followup to this patch will 
do. Thanks!

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Mar 21, 2018 at 06:12:39PM -0600, Jim Fehlig wrote:
> Ok. No need to send the whole series again. Just a followup to this patch
> will do. Thanks!

Just sent, but I've failed to connect it to this thread... It's marked
as v6.1.

-- 
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?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@invisiblethingslab.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
Changes since v6:
 - really allow per-domain override
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.
+# By default it is disabled.
+#nested_hvm = 0
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index dcfdd67..ed8506d 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -360,10 +360,12 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
             bool hasHwVirt = false;
             bool svm = false, vmx = false;
 
+            /* enable nested HVM only if global nested_hvm option enable it and
+             * host support it*/
             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;
+                hasHwVirt = cfg->nested_hvm && (vmx | svm);
             }
 
             if (def->cpu->nfeatures) {
@@ -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);
 
-- 
git-series 0.9.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Mar 22, 2018 at 01:15:31AM +0100, 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@invisiblethingslab.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> Changes since v6:
>  - really allow per-domain override
> 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 03/21/2018 06:15 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.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> Changes since v6:
>   - really allow per-domain override
> 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.
> +# By default it is disabled.
> +#nested_hvm = 0
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index dcfdd67..ed8506d 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -360,10 +360,12 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>               bool hasHwVirt = false;
>               bool svm = false, vmx = false;
>   
> +            /* enable nested HVM only if global nested_hvm option enable it and
> +             * host support it*/
>               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;
> +                hasHwVirt = cfg->nested_hvm && (vmx | svm);
>               }
>   
>               if (def->cpu->nfeatures) {
> @@ -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" }

Fails 'make check' since the default is 0. I've fixed it in my branch.

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

> 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);
>   
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list