On Mon, 17 Apr 2017 12:04:53 -0600
Jim Fehlig <jfehlig@xxxxxxxx> wrote:
> On 03/24/2017 03:02 PM, Wim Ten Have wrote:
> > From: Wim ten Have <wim.ten.have@xxxxxxxxxx>
> >
> > 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'/>
Agree, will add.
> > Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> > Signed-off-by: Wim ten Have <wim.ten.have@xxxxxxxxxx>
> > ---
> > 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.
Indeed redundant (will remove).
> > +
> > + 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;
Agree.
> > +
> > + 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;
Ah, i was not aware but see it around and it makes sense. I'll apply.
> > + }
> > + }
> > + }
> > + 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?
Perhaps I am missing what you try to tell me here. We need cfg->ctx for
libxl allocation requirements. Eliminating by solely switching to virCapsPtr
won't work.
Is it good to skip this for now? There's more coming forth soon so i'll keep
this in mind and try to give it a good approach near future if possible.
Thanks for your review comments.
Regards,
- Wim.
> > {
> > 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)
> >
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
