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

Wim Ten Have wrote: > From: Wim ten Have <wim.ten.have(a)oracle.com&gt; > > 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(a)oracle.com&gt; > Signed-off-by: Wim ten Have <wim.ten.have(a)oracle.com&gt; > --- > 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?

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