Re: [PATCH v6 4/9] libxl: do not enable nested HVM unless global nested_hvm option enabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 21, 2018 at 05:55:28PM -0600, Jim Fehlig wrote:
> On 03/21/2018 10:32 AM, 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.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > ---
> > Changes since v4:
> >   - add nested_hvm option to test_libvirtd_libxl.aug.in and libvirtd_libxl.aug
> >   - make it possible to override nested_hvm=0 with explicit <feature
> >     policy='require' name='vmx'/>
> >   - split xenconfig changes into separate commits
> > 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/libvirtd_libxl.aug         |  2 ++
> >   src/libxl/libxl.conf                 |  8 ++++++++
> >   src/libxl/libxl_conf.c               | 12 +++++++++++-
> >   src/libxl/libxl_conf.h               |  2 ++
> >   src/libxl/test_libvirtd_libxl.aug.in |  1 +
> >   tests/libxlxml2domconfigtest.c       |  3 +++
> >   6 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libxl/libvirtd_libxl.aug b/src/libxl/libvirtd_libxl.aug
> > index b31cc07..58b9af3 100644
> > --- a/src/libxl/libvirtd_libxl.aug
> > +++ b/src/libxl/libvirtd_libxl.aug
> > @@ -28,12 +28,14 @@ module Libvirtd_libxl =
> >      let lock_entry = str_entry "lock_manager"
> >      let keepalive_interval_entry = int_entry "keepalive_interval"
> >      let keepalive_count_entry = int_entry "keepalive_count"
> > +   let nested_hvm_entry = bool_entry "nested_hvm"
> >      (* Each entry in the config is one of the following ... *)
> >      let entry = autoballoon_entry
> >                | lock_entry
> >                | keepalive_interval_entry
> >                | keepalive_count_entry
> > +             | nested_hvm_entry
> >      let comment = [ label "#comment" . del /#[ \t]*/ "# " .  store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]
> >      let empty = [ label "#empty" . eol ]
> > diff --git a/src/libxl/libxl.conf b/src/libxl/libxl.conf
> > index 264af7c..72825a7 100644
> > --- a/src/libxl/libxl.conf
> > +++ b/src/libxl/libxl.conf
> > @@ -41,3 +41,11 @@
> >   #
> >   #keepalive_interval = 5
> >   #keepalive_count = 5
> > +
> > +# Nested HVM default 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. This can be overridden in domain configuration by
> > +# explicitly setting <feature policy='require' name='vmx'/> inside <cpu/>
> > +# element.
> 
> Cool, the setting can be overridden by per-domain config.
> 
> > +# By default it is disabled.
> > +#nested_hvm = 0
> > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> > index dcfdd67..3b9e828 100644
> > --- a/src/libxl/libxl_conf.c
> > +++ b/src/libxl/libxl_conf.c
> > @@ -360,7 +360,9 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> >               bool hasHwVirt = false;
> >               bool svm = false, vmx = false;
> > -            if (ARCH_IS_X86(def->os.arch)) {
> > +            /* enable nested HVM only if global nested_hvm option enable it and
> > +             * host support 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;
> 
> But IIUC this change will not allow per-domain config to override the global
> setting. If cfg->nested_hvm is false, svm and vmx are both false and
> FEATURE_REQUIRE is not honored.

Ough, conflict resolution went wrong after changing 3/9 :/
Fixed patch will follow.

> Regards,
> Jim
> 
> > @@ -380,6 +382,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
> >                           case VIR_CPU_FEATURE_FORCE:
> >                           case VIR_CPU_FEATURE_REQUIRE:
> > +                            if ((vmx && STREQ(def->cpu->features[i].name, "vmx")) ||
> > +                                (svm && STREQ(def->cpu->features[i].name, "svm")))
> > +                                hasHwVirt = true;
> > +                            break;
> > +
> >                           case VIR_CPU_FEATURE_OPTIONAL:
> >                           case VIR_CPU_FEATURE_LAST:
> >                               break;
> > @@ -1699,6 +1706,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 ce9db26..27cfc1a 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/libxl/test_libvirtd_libxl.aug.in b/src/libxl/test_libvirtd_libxl.aug.in
> > index 63558e5..9106abe 100644
> > --- a/src/libxl/test_libvirtd_libxl.aug.in
> > +++ b/src/libxl/test_libvirtd_libxl.aug.in
> > @@ -6,3 +6,4 @@ module Test_libvirtd_libxl =
> >   { "lock_manager" = "lockd" }
> >   { "keepalive_interval" = "5" }
> >   { "keepalive_count" = "5" }
> > +{ "nested_hvm" = "1" }
> > diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
> > index cfbc37c..8819032 100644
> > --- a/tests/libxlxml2domconfigtest.c
> > +++ b/tests/libxlxml2domconfigtest.c
> > @@ -76,6 +76,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);
> > 
> 

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

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux