On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:

libxlDriverConfigNew() use libxlDriverConfigDispose() for cleanup in case of errors. Do not call libxlLoggerFree() on not allocated logger (NULL).

--- Changes since v3: - new patch, mostly unrelated, but found while adjusting tests

Trivial. I'd push it, but you need to add a S-O-B to patches now. See commit d64d5ccb06. Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim

--- src/libxl/libxl_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 970cff2..2d2a707 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -80,7 +80,8 @@ libxlDriverConfigDispose(void *obj)

virObjectUnref(cfg->caps); libxl_ctx_free(cfg->ctx); - libxlLoggerFree(cfg->logger); + if (cfg->logger) + libxlLoggerFree(cfg->logger);

VIR_FREE(cfg->configDir); VIR_FREE(cfg->autostartDir);

On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:

Preparation for global nestedhvm configuration - libxlMakeDomBuildInfo needs access to libxlDriverConfig. No functional change.

Adjusting tests require slightly more mockup functions, because of libxlDriverConfigNew() call.

--- Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 8 +++++--- src/libxl/libxl_conf.h | 2 +- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 20 +++++++++++++------- tests/virmocklibxl.c | 25 +++++++++++++++++++++++++ 5 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..8cced29 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -271,10 +271,11 @@ libxlMakeChrdevStr(virDomainChrDefPtr def, char **buf)

static int libxlMakeDomBuildInfo(virDomainDefPtr def, - libxl_ctx *ctx, + libxlDriverConfigPtr cfg, virCapsPtr caps, libxl_domain_config *d_config) { + libxl_ctx *ctx = cfg->ctx; libxl_domain_build_info *b_info = &d_config->b_info; int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; size_t i; @@ -2288,16 +2289,17 @@ int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, + libxlDriverConfigPtr cfg,

I can't recall if the only reason we were avoiding passing a libxlDriverConfigPtr is the extra mocking. If so, that's not reason enough to avoid it.

virCapsPtr caps, libxl_domain_config *d_config) { + libxl_ctx *ctx = cfg->ctx; libxl_domain_config_init(d_config);

if (libxlMakeDomCreateInfo(ctx, def, &d_config->c_info) < 0) return -1;

- if (libxlMakeDomBuildInfo(def, ctx, caps, d_config) < 0) + if (libxlMakeDomBuildInfo(def, cfg, caps, d_config) < 0) return -1;

#ifdef LIBXL_HAVE_VNUMA diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..8eefe06 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, virCapsPtr caps, libxl_domain_config *d_config);

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 395c8a9..0a60444 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1253,7 +1253,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, goto cleanup_dom;

if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def, - cfg->channelDir, cfg->ctx, cfg->caps, &d_config) < 0) + cfg->channelDir, cfg, cfg->caps, &d_config) < 0)

If we are going to pass the entire libxlDriverConfigPtr to libxlBuildDomainConfig(), the channelDir and caps parameters can be dropped. Regards, Jim

goto cleanup_dom;

if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..0105550 100644 --- a/tests/libxlxml2domconfigtest.c +++ b/tests/libxlxml2domconfigtest.c @@ -56,8 +56,8 @@ testCompareXMLToDomConfig(const char *xmlfile, int ret = -1; libxl_domain_config actualconfig; libxl_domain_config expectconfig; + libxlDriverConfigPtr cfg; xentoollog_logger *log = NULL; - libxl_ctx *ctx = NULL; virPortAllocatorPtr gports = NULL; virDomainXMLOptionPtr xmlopt = NULL; virDomainDefPtr vmdef = NULL; @@ -68,10 +68,16 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig);

+ if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + if (!(log = (xentoollog_logger *)xtl_createlogger_stdiostream(stderr, XTL_DEBUG, 0))) goto cleanup;

- if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, log) < 0) + /* replace logger with stderr one */ + libxl_ctx_free(cfg->ctx); + + if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, log) < 0) goto cleanup;

if (!(gports = virPortAllocatorNew("vnc", 5900, 6000, @@ -85,22 +91,22 @@ testCompareXMLToDomConfig(const char *xmlfile, NULL, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;

- if (libxlBuildDomainConfig(gports, vmdef, NULL, ctx, caps, &actualconfig) < 0) + if (libxlBuildDomainConfig(gports, vmdef, NULL, cfg, caps, &actualconfig) < 0) goto cleanup;

- if (!(actualjson = libxl_domain_config_to_json(ctx, &actualconfig))) { + if (!(actualjson = libxl_domain_config_to_json(cfg->ctx, &actualconfig))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Failed to retrieve JSON doc for libxl_domain_config"); goto cleanup; }

virTestLoadFile(jsonfile, &tempjson); - if (libxl_domain_config_from_json(ctx, &expectconfig, tempjson) != 0) { + if (libxl_domain_config_from_json(cfg->ctx, &expectconfig, tempjson) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Failed to create libxl_domain_config from JSON doc"); goto cleanup; } - if (!(expectjson = libxl_domain_config_to_json(ctx, &expectconfig))) { + if (!(expectjson = libxl_domain_config_to_json(cfg->ctx, &expectconfig))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "Failed to retrieve JSON doc for libxl_domain_config"); goto cleanup; @@ -118,10 +124,10 @@ testCompareXMLToDomConfig(const char *xmlfile, virDomainDefFree(vmdef); virObjectUnref(gports); virObjectUnref(xmlopt); - libxl_ctx_free(ctx); libxl_domain_config_dispose(&actualconfig); libxl_domain_config_dispose(&expectconfig); xtl_logger_destroy(log); + virObjectUnref(cfg); return ret; }

diff --git a/tests/virmocklibxl.c b/tests/virmocklibxl.c index 747f9f8..133064b 100644 --- a/tests/virmocklibxl.c +++ b/tests/virmocklibxl.c @@ -27,6 +27,7 @@ # include <sys/stat.h> # include <unistd.h> # include <libxl.h> +# include <util/virfile.h> # include <xenstore.h> # include <xenctrl.h>

@@ -48,6 +49,19 @@ VIR_MOCK_IMPL_RET_ARGS(xc_interface_open, }

+VIR_MOCK_IMPL_RET_ARGS(libxl_get_version_info, + const libxl_version_info*, + libxl_ctx *, ctx) +{ + static libxl_version_info info; + + memset(&info, 0, sizeof(info)); + + return &info; + /* silence gcc warning */ + return real_libxl_get_version_info(ctx); +} + VIR_MOCK_STUB_RET_ARGS(xc_interface_close, int, 0, xc_interface *, handle) @@ -68,6 +82,17 @@ VIR_MOCK_STUB_RET_ARGS(xc_sharing_used_frames, VIR_MOCK_STUB_VOID_ARGS(xs_daemon_close, struct xs_handle *, handle)

+VIR_MOCK_IMPL_RET_ARGS(virFileMakePath, int, + const char *, path) +{ + /* replace log path with a writable directory */ + if (strstr(path, "/log/")) { + snprintf((char*)path, strlen(path), "."); + return 0; + } + return real_virFileMakePath(path); +} + VIR_MOCK_IMPL_RET_ARGS(__xstat, int, int, ver, const char *, path,

On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:

This change make libvirt XML with plain <cpu> element invalid for libxl, which affect not only upcoming CPUID support, but also NUMA. In fact, default mode 'custom' does not match what the driver actually does, so it was a bug. Adjust xenconfig driver accordingly.

But nevertheless this commit break some configurations that were working before.

--- Changes since v3: - fix vnuma tests (nested HVM implicitly enabled there) Changes since v2: - change separated from 'libxl: add support for CPUID features policy' --- src/libxl/libxl_conf.c | 10 ++++++++-- src/xenconfig/xen_xl.c | 1 +- tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg | 1 +- tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml | 2 +- tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg | 1 +- tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml | 2 +- tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg | 1 +- tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml | 2 +- tests/xlconfigdata/test-fullvirt-vnuma.cfg | 1 +- tests/xlconfigdata/test-fullvirt-vnuma.xml | 2 +- 10 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8cced29..66956a7 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -355,11 +355,17 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, def->features[VIR_DOMAIN_FEATURE_ACPI] == VIR_TRISTATE_SWITCH_ON);

- if (caps && - def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { + if (caps && def->cpu) { bool hasHwVirt = false; bool svm = false, vmx = false;

+ if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported cpu mode '%s'"), + virCPUModeTypeToString(def->cpu->mode)); + return -1; + }

It looks like we never answered my question from V3, i.e. should we change the default mode in the post-parse callback if NUMA or CPU features are defined within <cpu>? It sure feels like such configuration tweaks and error checks should be handled in the parsing phase, similar to qemuDomainDefVcpusPostParse() and qemuDomainDefCPUPostParse() in the qemu driver. I think danpb is vaguely familiar with this series and can help answer that question. Daniel, do you have an opinion? Or a general comment about guidelines for checking/tweaking config in parse phase vs domain start phase? Regards, Jim

+ if (ARCH_IS_X86(def->os.arch)) { vmx = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx"); svm = virCPUCheckFeature(caps->host.arch, caps->host.cpu, "svm"); diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 2ef80eb..6cda305 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -494,6 +494,7 @@ xenParseXLVnuma(virConfPtr conf, goto cleanup; }

+ cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH; cpu->type = VIR_CPU_TYPE_GUEST; def->cpu = cpu;

diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg index edba69a..2c9de44 100644 --- a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg +++ b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg @@ -22,5 +22,6 @@ parallel = "none" serial = "none" builder = "hvm" boot = "d" +nestedhvm = 1 vnuma = [ [ "pnode=0", "size=2048", "vcpus=0,11", "vdistances=10,21,31,41,51,61" ], [ "pnode=1", "size=2048", "vcpus=1,10", "vdistances=21,10,21,31,41,51" ], [ "pnode=2", "size=2048", "vcpus=2,9", "vdistances=31,21,10,21,31,41" ], [ "pnode=3", "size=2048", "vcpus=3,8", "vdistances=41,31,21,10,21,31" ], [ "pnode=4", "size=2048", "vcpus=4,7", "vdistances=51,41,31,21,10,21" ], [ "pnode=5", "size=2048", "vcpus=5-6", "vdistances=61,51,41,31,21,10" ] ] disk = [ "format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ] diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml index e3639eb..3c486ad 100644 --- a/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml +++ b/tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml @@ -14,7 +14,7 @@ <apic/> <pae/> </features> - <cpu> + <cpu mode='host-passthrough'> <numa> <cell id='0' cpus='0,11' memory='2097152' unit='KiB'> <distances> diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg index 8186edf..ca8e5b0 100644 --- a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg +++ b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg @@ -22,5 +22,6 @@ parallel = "none" serial = "none" builder = "hvm" boot = "d" +nestedhvm = 1 vnuma = [ [ "pnode=0", "size=2048", "vcpus=0-1", "vdistances=10,20,20,20" ], [ "pnode=1", "size=2048", "vcpus=2-3", "vdistances=20,10,20,20" ], [ "pnode=2", "size=2048", "vcpus=4-5", "vdistances=20,20,10,20" ], [ "pnode=3", "size=2048", "vcpus=6-7", "vdistances=20,20,20,10" ] ] disk = [ "format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ] diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml index 9cab3ca..17c9ca5 100644 --- a/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml +++ b/tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml @@ -14,7 +14,7 @@ <apic/> <pae/> </features> - <cpu> + <cpu mode='host-passthrough'> <numa> <cell id='0' cpus='0-1' memory='2097152' unit='KiB'/> <cell id='1' cpus='2-3' memory='2097152' unit='KiB'/> diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg b/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg index 861a50e..46d5f90 100644 --- a/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg +++ b/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg @@ -22,5 +22,6 @@ parallel = "none" serial = "none" builder = "hvm" boot = "d" +nestedhvm = 1 vnuma = [ [ "pnode=0", "size=2048", "vcpus=0-1", "vdistances=10,21,31,41" ], [ "pnode=1", "size=2048", "vcpus=2-3", "vdistances=21,10,20,20" ], [ "pnode=2", "size=2048", "vcpus=4-5", "vdistances=31,20,10,20" ], [ "pnode=3", "size=2048", "vcpus=6-7", "vdistances=41,20,20,10" ] ] disk = [ "format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ] diff --git a/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml b/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml index 084b889..291fc37 100644 --- a/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml +++ b/tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml @@ -14,7 +14,7 @@ <apic/> <pae/> </features> - <cpu> + <cpu mode='host-passthrough'> <numa> <cell id='0' cpus='0-1' memory='2097152' unit='KiB'> <distances> diff --git a/tests/xlconfigdata/test-fullvirt-vnuma.cfg b/tests/xlconfigdata/test-fullvirt-vnuma.cfg index 91e233a..813ad7f 100644 --- a/tests/xlconfigdata/test-fullvirt-vnuma.cfg +++ b/tests/xlconfigdata/test-fullvirt-vnuma.cfg @@ -22,5 +22,6 @@ parallel = "none" serial = "none" builder = "hvm" boot = "d" +nestedhvm = 1 vnuma = [ [ "pnode=0", "size=2048", "vcpus=0-1", "vdistances=10,21,31,41" ], [ "pnode=1", "size=2048", "vcpus=2-3", "vdistances=21,10,21,31" ], [ "pnode=2", "size=2048", "vcpus=4-5", "vdistances=31,21,10,21" ], [ "pnode=3", "size=2048", "vcpus=6-7", "vdistances=41,31,21,10" ] ] disk = [ "format=raw,vdev=hda,access=rw,backendtype=phy,target=/dev/HostVG/XenGuest2" ] diff --git a/tests/xlconfigdata/test-fullvirt-vnuma.xml b/tests/xlconfigdata/test-fullvirt-vnuma.xml index 5368b0d..9a9f495 100644 --- a/tests/xlconfigdata/test-fullvirt-vnuma.xml +++ b/tests/xlconfigdata/test-fullvirt-vnuma.xml @@ -14,7 +14,7 @@ <apic/> <pae/> </features> - <cpu> + <cpu mode='host-passthrough'> <numa> <cell id='0' cpus='0-1' memory='2097152' unit='KiB'> <distances>

On 02/15/2018 02:47 PM, Marek Marczykowski-Górecki wrote:

On Tue, Feb 13, 2018 at 09:02:35AM -0700, Jim Fehlig wrote:

...

On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:

...

This change make libvirt XML with plain <cpu> element invalid for libxl, which affect not only upcoming CPUID support, but also NUMA. In fact, default mode 'custom' does not match what the driver actually does, so it was a bug. Adjust xenconfig driver accordingly.

But nevertheless this commit break some configurations that were working before.

--- Changes since v3: - fix vnuma tests (nested HVM implicitly enabled there) Changes since v2: - change separated from 'libxl: add support for CPUID features policy' --- src/libxl/libxl_conf.c | 10 ++++++++-- src/xenconfig/xen_xl.c | 1 +- tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.cfg | 1 +- tests/xlconfigdata/test-fullvirt-vnuma-autocomplete.xml | 2 +- tests/xlconfigdata/test-fullvirt-vnuma-nodistances.cfg | 1 +- tests/xlconfigdata/test-fullvirt-vnuma-nodistances.xml | 2 +- tests/xlconfigdata/test-fullvirt-vnuma-partialdist.cfg | 1 +- tests/xlconfigdata/test-fullvirt-vnuma-partialdist.xml | 2 +- tests/xlconfigdata/test-fullvirt-vnuma.cfg | 1 +- tests/xlconfigdata/test-fullvirt-vnuma.xml | 2 +- 10 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8cced29..66956a7 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -355,11 +355,17 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, def->features[VIR_DOMAIN_FEATURE_ACPI] == VIR_TRISTATE_SWITCH_ON); - if (caps && - def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { + if (caps && def->cpu) { bool hasHwVirt = false; bool svm = false, vmx = false; + if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported cpu mode '%s'"), + virCPUModeTypeToString(def->cpu->mode)); + return -1; + }

It looks like we never answered my question from V3, i.e. should we change the default mode in the post-parse callback if NUMA or CPU features are defined within <cpu>?

Hmm, but this means changing the config behind user's back, no?

Well, sort of. But it is really just making the implicit explicit.

You have disabled nested HVM, upgrade, now you have enabled. Global switch is some consolation here, but is it enough?

I _think_ so. With a global default that disables nesting, are there any scenarios you envision where a previously disabled nesting becomes enabled after upgrade? BTW, I'm fine with this patch, just playing devil's advocate with handling such things post-parse vs domain start. It's not the only place where default xen config is not reflected in libvirt :-/. Regards, Jim

...

It sure feels like such configuration tweaks and error checks should be handled in the parsing phase, similar to qemuDomainDefVcpusPostParse() and qemuDomainDefCPUPostParse() in the qemu driver. I think danpb is vaguely familiar with this series and can help answer that question. Daniel, do you have an opinion? Or a general comment about guidelines for checking/tweaking config in parse phase vs domain start phase?

On 02/08/2018 03:58 PM, 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.

I don't think we should mix this change with the next two.

Also, adjust xenconfig driver to appropriately translate to/from nestedhvm=1.

While at it, adjust xenconfig driver to not override def->cpu if already set elsewhere. This will help with adding cpuid support.

These two look fine, but are actually separate bug fixes IMO. (This series keeps uncovering all sorts of goodies.)

--- As for xenconfig part, I'm not sure if missing "nestedhvm" option in xl config shouldn't produce <cpu> element too, to disable nested HVM (as it would be on plain xl). Any preference?

Given the global option to disable nesting, I don't think we need to worry about missing "nestedhvm". Even if xl.cfg contains "vnuma" and/or "cpuid", and hence we have a def->cpu, the global setting would ensure nestedhvm is disabled. If one were paranoid, <feature policy='disable' name='vmx|svm'/> could be added to an existing def->cpu.

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/libxl.conf | 6 ++++++- src/libxl/libxl_conf.c | 7 ++++++- src/libxl/libxl_conf.h | 2 ++- src/xenconfig/xen_xl.c | 37 +++++++++++------------------------ tests/libxlxml2domconfigtest.c | 3 +++- 5 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/src/libxl/libxl.conf b/src/libxl/libxl.conf index 264af7c..0e842c9 100644 --- a/src/libxl/libxl.conf +++ b/src/libxl/libxl.conf @@ -41,3 +41,9 @@ # #keepalive_interval = 5 #keepalive_count = 5 + +# Nested HVM global 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. +# By default it is disabled. +#nested_hvm = 0

I think per-domain settings should override this one. Users would find it odd that they don't have vmx in their hvm guest with <cpu mode='host-passthrough'> <feature policy='require' name='vmx'/> </cpu> Regards, Jim

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 66956a7..417ce7c 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -366,7 +366,9 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, return -1; }

- if (ARCH_IS_X86(def->os.arch)) { + /* consider host support for nested HVM only if global nested_hvm + * option enable 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; @@ -1699,6 +1701,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 8eefe06..e32a5e7 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/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 6cda305..394cc0d 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -170,17 +170,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (xenConfigGetBool(conf, "nestedhvm", &val, -1) < 0) return -1;

- if (val == 1) { - virCPUDefPtr cpu; - - if (VIR_ALLOC(cpu) < 0) - return -1; - - 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 (val != -1) { + const char *vtfeature = "vmx";

if (caps && caps->host.cpu && ARCH_IS_X86(def->os.arch)) { if (virCPUCheckFeature(caps->host.arch, caps->host.cpu, "vmx")) @@ -189,28 +180,24 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) vtfeature = "svm"; }

- if (vtfeature) { + if (!def->cpu) { virCPUDefPtr cpu; - if (VIR_ALLOC(cpu) < 0) return -1;

- if (VIR_ALLOC(cpu->features) < 0) { - VIR_FREE(cpu); - return -1; - } - - 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; + cpu->nfeatures = 0; + cpu->nfeatures_max = 0; def->cpu = cpu; } + + if (val == 0) { + if (virCPUDefAddFeature(def->cpu, + vtfeature, + VIR_CPU_FEATURE_DISABLE) < 0) + return -1; + } } } else { if (xenConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index 0105550..f2af286 100644 --- a/tests/libxlxml2domconfigtest.c +++ b/tests/libxlxml2domconfigtest.c @@ -74,6 +74,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);

On 02/08/2018 03:58 PM, Marek Marczykowski-Górecki wrote:

Convert CPU features policy into libxl cpuid policy settings. Use new ("libxl") syntax, which allow to enable/disable specific bits, using host CPU as a base. For this reason, only "host-passthrough" mode is accepted. Libxl do not have distinction between "force" and "required" policy (there is only "force") and also between "forbid" and "disable" (there is only "disable"). So, merge them appropriately. If anything, "require" and "forbid" should be enforced outside of specific driver. Nested HVM (vmx and svm features) is handled separately, so exclude it from translation.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v2: - drop spurious changes - move libxlTranslateCPUFeature function to xen_xl.c, to be reused by xenconfig driver Changes since v1: - use new ("libxl") syntax to set only bits explicitly mentioned in domain XML --- src/libxl/libxl_conf.c | 35 ++++++++++++++++++++++++++++++++++- src/xenconfig/xen_xl.c | 34 ++++++++++++++++++++++++++++++++++ src/xenconfig/xen_xl.h | 2 ++ 3 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 417ce7c..d0afc1f 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -50,6 +50,7 @@ #include "secret_util.h" #include "cpu/cpu.h" #include "xen_common.h" +#include "xen_xl.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL @@ -358,6 +359,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, if (caps && def->cpu) { bool hasHwVirt = false; bool svm = false, vmx = false; + char xlCPU[32];

if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -382,12 +384,43 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, 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"))) + (svm && STREQ(def->cpu->features[i].name, "svm"))) { hasHwVirt = false; + continue; + } + + snprintf(xlCPU, + sizeof(xlCPU), + "%s=0", + xenTranslateCPUFeature( + def->cpu->features[i].name, + false)); + if (libxl_cpuid_parse_config(&b_info->cpuid, xlCPU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported cpu feature '%s'"), + def->cpu->features[i].name); + return -1; + } break;

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"))) + continue; + + snprintf(xlCPU, + sizeof(xlCPU), + "%s=1", + xenTranslateCPUFeature( + def->cpu->features[i].name, false)); + if (libxl_cpuid_parse_config(&b_info->cpuid, xlCPU)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported cpu feature '%s'"), + def->cpu->features[i].name); + return -1; + } + break; case VIR_CPU_FEATURE_OPTIONAL: case VIR_CPU_FEATURE_LAST: break; diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 394cc0d..d5dff59 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -218,6 +218,40 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) return 0; }

+/* + * Translate CPU feature name from libvirt to libxl (from_libxl=false) or from + * libxl to libvirt (from_libxl=true). + */ +const char * +xenTranslateCPUFeature(const char *feature_name, bool from_libxl) +{ + static const char *translation_table[][2] = { + /* libvirt name, libxl name */ + { "cx16", "cmpxchg16" }, + { "cid", "cntxid" }, + { "ds_cpl", "dscpl" }, + { "pclmuldq", "pclmulqdq" }, + { "pni", "sse3" }, + { "ht", "htt" }, + { "pn", "psn" }, + { "clflush", "clfsh" }, + { "sep", "sysenter" }, + { "cx8", "cmpxchg8" }, + { "nodeid_msr", "nodeid" }, + { "cr8legacy", "altmovcr8" }, + { "lahf_lm", "lahfsahf" }, + { "cmp_legacy", "cmplegacy" }, + { "fxsr_opt", "ffxsr" }, + { "pdpe1gb", "page1gb" }, + };

In the case of ibrsb and stibp added in xen commit 0d703a701cc and spec-ctrl added in libvirt commit 8b605530e80, have we already encountered the drawback of this approach discussed in V3? Regards, Jim

+ size_t i; + + for (i = 0; i < ARRAY_CARDINALITY(translation_table); i++) + if (STREQ(translation_table[i][from_libxl], feature_name)) + return translation_table[i][!from_libxl]; + return feature_name; +} +

static int xenParseXLSpice(virConfPtr conf, virDomainDefPtr def) diff --git a/src/xenconfig/xen_xl.h b/src/xenconfig/xen_xl.h index dd96326..68f332a 100644 --- a/src/xenconfig/xen_xl.h +++ b/src/xenconfig/xen_xl.h @@ -33,4 +33,6 @@ virDomainDefPtr xenParseXL(virConfPtr conn,

virConfPtr xenFormatXL(virDomainDefPtr def, virConnectPtr);

+const char *xenTranslateCPUFeature(const char *feature_name, bool from_libxl); + #endif /* __VIR_XEN_XL_H__ */