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

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

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
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);
 
-- 
git-series 0.9.1

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

On Thu, Feb 08, 2018 at 11:58:57PM +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.
> 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.
> 
> ---
> 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?
> 
> 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

FYI you need to update lbivirtd_libxl.aug and
test_libvirtd_libxl.aug to handle parsing of this new parameter


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

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

On Mon, Feb 26, 2018 at 03:47:11PM -0700, Jim Fehlig wrote:
> On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:
> > +
> > +# 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>

I like this one :) It means that by just introducing global
"nested_hvm = 0", we can have what I've originally proposed - nested HVM
disabled until explicitly enabled with exactly this config snippet.

This change would require adjusting the comment for the option that it's
only about default value, not really global switch.

-- 
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 02/26/2018 04:10 PM, Marek Marczykowski-Górecki wrote:
> On Mon, Feb 26, 2018 at 03:47:11PM -0700, Jim Fehlig wrote:
>> On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:
>>> +
>>> +# 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>
> 
> I like this one :) It means that by just introducing global
> "nested_hvm = 0", we can have what I've originally proposed - nested HVM
> disabled until explicitly enabled with exactly this config snippet.

Yes. Sorry if we've been going around in circles on some of these topics.

> This change would require adjusting the comment for the option that it's
> only about default value, not really global switch.

Nod.

Regards,
Jim

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

On Mon, Feb 26, 2018 at 04:23:18PM -0700, Jim Fehlig wrote:
> On 02/26/2018 04:10 PM, Marek Marczykowski-Górecki wrote:
> > On Mon, Feb 26, 2018 at 03:47:11PM -0700, Jim Fehlig wrote:
> > > On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:
> > > > +
> > > > +# 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>
> > 
> > I like this one :) It means that by just introducing global
> > "nested_hvm = 0", we can have what I've originally proposed - nested HVM
> > disabled until explicitly enabled with exactly this config snippet.
> 
> Yes. Sorry if we've been going around in circles on some of these topics.

Ok, so before I go with v5 being mainly revert to v3 (+global config),
can you confirm that it is really ok? Will it be consistent enough with
KVM case? Not sure how it's handled there, but I'd guess if _kernel
module_ parameter is set to 0 (is it where the global switch is?), it
will stay disabled regardless of what you specify in libvirt domain XML.

-- 
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 02/26/2018 04:51 PM, Marek Marczykowski-Górecki wrote:
> On Mon, Feb 26, 2018 at 04:23:18PM -0700, Jim Fehlig wrote:
>> On 02/26/2018 04:10 PM, Marek Marczykowski-Górecki wrote:
>>> On Mon, Feb 26, 2018 at 03:47:11PM -0700, Jim Fehlig wrote:
>>>> On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:
>>>>> +
>>>>> +# 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>
>>>
>>> I like this one :) It means that by just introducing global
>>> "nested_hvm = 0", we can have what I've originally proposed - nested HVM
>>> disabled until explicitly enabled with exactly this config snippet.
>>
>> Yes. Sorry if we've been going around in circles on some of these topics.
> 
> Ok, so before I go with v5 being mainly revert to v3 (+global config),
> can you confirm that it is really ok? Will it be consistent enough with
> KVM case? Not sure how it's handled there, but I'd guess if _kernel
> module_ parameter is set to 0 (is it where the global switch is?), it
> will stay disabled regardless of what you specify in libvirt domain XML.

Yes, AFAIK that is the case with KVM. But from a libvirt perspective, it will be 
consistent with many of the other settings in <hypervisor>.conf. If a setting in 
<hypervisor>.conf has a counterpart in domain XML, the latter has precedence.

Regards,
Jim

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