Re: [libvirt] [PATCH v2 1/3] libxl: set nestedhvm for mode host-passthrough

On 03/24/2017 03:02 PM, Wim Ten Have wrote: > From: Wim ten Have <wim.ten.have(a)oracle.com&gt; > > Xen feature nestedhvm is the option on Xen 4.4+ which enables > nested virtualization when mode host-passthrough is applied. > > Virtualization on target domain can be disabled by specifying > such under feature policy rule on target name; > > [On Intel (VT-x) architecture] > <feature policy='disable' name='vmx'/> > > or: > > [On AMD (AMD-V) architecture] > <feature policy='disable' name='svm'/> I think we should also give an example of enabling nested HVM. E.g. <cpu mode='host-passthrough'/>

> 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; > --- > src/libxl/libxl_conf.c | 47 ++++++++++++++++++++++++++++++++++++++++++----- > src/libxl/libxl_conf.h | 2 +- > src/libxl/libxl_domain.c | 2 +- > 3 files changed, 44 insertions(+), 7 deletions(-) > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index f5b788b..ede7c8a 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -47,6 +47,7 @@ > #include "libxl_utils.h" > #include "virstoragefile.h" > #include "secret_util.h" > +#include "cpu/cpu.h" > > > #define VIR_FROM_THIS VIR_FROM_LIBXL > @@ -292,7 +293,7 @@ libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf) > > static int > libxlMakeDomBuildInfo(virDomainDefPtr def, > - libxl_ctx *ctx, > + libxlDriverConfigPtr cfg, > libxl_domain_config *d_config) > { > libxl_domain_build_info *b_info = &d_config->b_info; > @@ -308,7 +309,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV); > > b_info->max_vcpus = virDomainDefGetVcpusMax(def); > - if (libxl_cpu_bitmap_alloc(ctx, &b_info->avail_vcpus, b_info->max_vcpus)) > + if (libxl_cpu_bitmap_alloc(cfg->ctx, &b_info->avail_vcpus, b_info->max_vcpus)) > return -1; > libxl_bitmap_set_none(&b_info->avail_vcpus); > for (i = 0; i < virDomainDefGetVcpus(def); i++) > @@ -374,6 +375,42 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, > def->features[VIR_DOMAIN_FEATURE_ACPI] == > VIR_TRISTATE_SWITCH_ON); > > + if (cfg && def->cpu && > + def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { > + bool hasHwVirt = false; > + bool svm = false, vmx = false; > + virCapsPtr caps = cfg->caps; > + > + if (caps && ARCH_IS_X86(def->os.arch)) { > + virCPUDefPtr cpuDef = caps->host.cpu; I don't see much value in having this local variable.

> + > + vmx = virCPUCheckFeature( > + cfg->caps->host.arch, cpuDef, "vmx"); > + svm = virCPUCheckFeature( > + cfg->caps->host.arch, cpuDef, "svm"); > + hasHwVirt = (vmx | svm); > + } And you already have a local 'caps' for cfg->caps. So this could be shortened a bit to vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx"); svm = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "svm"); hasHwVirt = vmx | svm;

> + > + if (def->cpu->nfeatures) { > + for (i = 0; i < def->cpu->nfeatures; i++) { > + > + switch (def->cpu->features[i].policy) { > + > + case VIR_CPU_FEATURE_DISABLE: > + case VIR_CPU_FEATURE_FORBID: > + if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) || > + (svm && STREQ(def->cpu->features[i].name, "svm"))) > + hasHwVirt = false; > + break; > + > + default: > + break; Generally libvirt prefers to explicitly list all enum values in switch statements, e.g. case VIR_CPU_FEATURE_FORCE: case VIR_CPU_FEATURE_REQUIRE: case VIR_CPU_FEATURE_OPTIONAL: case VIR_CPU_FEATURE_LAST: break;

> + } > + } > + } > + libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); > + } > + > if (def->nsounds > 0) { > /* > * Use first sound device. man xl.cfg(5) describes soundhw as > @@ -2087,15 +2124,15 @@ int > libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, > virDomainDefPtr def, > const char *channelDir LIBXL_ATTR_UNUSED, > - libxl_ctx *ctx, > + libxlDriverConfigPtr cfg, > libxl_domain_config *d_config) Long ago, in commits 5da28f24 and a6abdbf6, we changed this function signature to make it easier to unit test. Unfortunately, the subsequent unit tests were never ACKed and committed, but I haven't given up on that effort. Latest attempt was sent to the list in February https://www.redhat.com/archives/libvir-list/2017-February/msg01477.html Looks like we just need cfg->caps in the call chain. Can we pass the virCapsPtr to libxlBuildDomainConfig instead of the libxlDriverConfig object?

> { > libxl_domain_config_init(d_config); > > - if (libxlMakeDomCreateInfo(ctx, def, &d_config->c_info) < 0) > + if (libxlMakeDomCreateInfo(cfg->ctx, def, &d_config->c_info) < 0) > return -1; > > - if (libxlMakeDomBuildInfo(def, ctx, d_config) < 0) > + if (libxlMakeDomBuildInfo(def, cfg, d_config) < 0) > return -1; > > if (libxlMakeDiskList(def, d_config) < 0) > diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h > index c653c9f..7a83669 100644 > --- a/src/libxl/libxl_conf.h > +++ b/src/libxl/libxl_conf.h > @@ -216,7 +216,7 @@ int > libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, > virDomainDefPtr def, > const char *channelDir LIBXL_ATTR_UNUSED, > - libxl_ctx *ctx, > + libxlDriverConfigPtr cfg, > libxl_domain_config *d_config); > > static inline void > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c > index 57ec661..562bc67 100644 > --- a/src/libxl/libxl_domain.c > +++ b/src/libxl/libxl_domain.c > @@ -1256,7 +1256,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, > goto cleanup_dom; > > if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, > - cfg->channelDir, cfg->ctx, &d_config) < 0) > + cfg->channelDir, cfg, &d_config) < 0) > goto cleanup_dom; > > if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) >