[libvirt] [PATCH v3 2/3] xenconfig: add conversions for xen-xl

[Jim Fehlig]

Wim Ten Have wrote:
> From: Wim ten Have <wim.ten.have at oracle.com>
> 
> Per xen-xl conversions from and to native under host-passthrough
> mode we take care for Xen (nestedhvm = mode) applied and inherited
> settings generating or processing correct feature policy:
> 
> [On Intel (VT-x) architectures]
> <feature policy='disable' name='vmx'/>
> 
> or
> 
> [On AMD (AMD-V) architectures]
> <feature policy='disable' name='svm'/>
> 
> It will then generate (or parse) for nestedhvm=1 in/from xl format.
> 
> Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
> Signed-off-by: Wim ten Have <wim.ten.have at oracle.com>
> ---
>  cfg.mk                 |  2 +-
>  src/xenconfig/xen_xl.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 69e3f3a..32c3725 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -777,7 +777,7 @@ sc_prohibit_cross_inclusion:
>  	    locking/) safe="($$dir|util|conf|rpc)";;			\
>  	    cpu/| network/| node_device/| rpc/| security/| storage/)	\
>  	      safe="($$dir|util|conf|storage)";;			\
> -	    xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen)";;	\
> +	    xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";;	\

It would be nice to get another libvirt dev opinion on this change. As I said in
the V2 thread, it seems we could remove xenconfig from this check.

>  	    *) safe="($$dir|$(mid_dirs)|util)";;			\

E.g. let it be handled in this case.

>  	  esac;								\
>  	  in_vc_files="^src/$$dir"					\
> diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
> index 74f68b3..62af8b8 100644
> --- a/src/xenconfig/xen_xl.c
> +++ b/src/xenconfig/xen_xl.c
> @@ -34,6 +34,7 @@
>  #include "virstoragefile.h"
>  #include "xen_xl.h"
>  #include "libxl_capabilities.h"
> +#include "cpu/cpu.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_XENXL
>  
> @@ -106,6 +107,7 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
>      if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
>          const char *bios;
>          const char *boot;
> +        int val = 0;
>  
>          if (xenConfigGetString(conf, "bios", &bios, NULL) < 0)
>              return -1;
> @@ -164,6 +166,40 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps)
>              }
>              def->os.nBootDevs++;
>          }
> +
> +        if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0)
> +            return -1;
> +
> +        if (val != -1) {
> +            virCPUDefPtr cpu = NULL;
> +
> +            if (VIR_ALLOC(cpu) < 0)
> +                return -1;
> +
> +            if (val == 0) {
> +                bool isVTx = true;
> +
> +                if (VIR_ALLOC(cpu->features) < 0) {
> +                    VIR_FREE(cpu);
> +                    return -1;
> +                }
> +
> +                if (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch))
> +                    isVTx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx");
> +
> +                if (VIR_STRDUP(cpu->features->name, isVTx ? "vmx" : "svm") < 0) {
> +                    VIR_FREE(cpu->features);
> +                    VIR_FREE(cpu);
> +                    return -1;

So if I understand this correctly, the feature would have the name "vmx" if arch
!= x86. If arch == x86 but feature "vmx" is not found, then the feature name
would be "svm".

IMO, it would be better to ignore <cpu> altogether if we can't find the name of
the virt technology feature to disable. Without a <cpu> def, you'd get the libxl
default, which is nestedhvm=disabled (and also the current behavior of
libvirt+libxl). E.g. what do you think of the below diff to your patch?

Regards,
Jim

diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index c536e57a0..4f24d457c 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -170,36 +170,48 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def,
virCapsPtr caps)
         if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0)
             return -1;

-        if (val != -1) {
-            virCPUDefPtr cpu = NULL;
+        if (val == 1) {
+            virCPUDefPtr cpu;

             if (VIR_ALLOC(cpu) < 0)
                 return -1;

-            if (val == 0) {
-                bool isVTx = true;
+            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 (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch)) {
+                if (virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx"))
+                    vtfeature = "vmx";
+                else if (virCPUCheckFeature(caps->host.arch, caps->host.cpu,
"svm"))
+                    vtfeature = "svm";
+            }
+
+            if (vtfeature) {
+                virCPUDefPtr cpu;
+
+                if (VIR_ALLOC(cpu) < 0)
+                    return -1;

                 if (VIR_ALLOC(cpu->features) < 0) {
                     VIR_FREE(cpu);
                     return -1;
                 }

-                if (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch))
-                    isVTx = virCPUCheckFeature(caps->host.arch, caps->host.cpu,
"vmx");
-
-                if (VIR_STRDUP(cpu->features->name, isVTx ? "vmx" : "svm") < 0) {
+                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;
+                def->cpu = cpu;
             }
-            cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
-            cpu->type = VIR_CPU_TYPE_GUEST;
-            def->cpu = cpu;
         }
-