[libvirt] [PATCH v6 0/9] Add setting CPU features (CPUID) with libxenlight driver.

Add support for CPUID setting based on <cpu> element. Since libxl format support only adjusting specific bits over host CPU, only mode='host-passthrough' is supported - other values are rejected (including default 'custom'). This will break some configurations working before (bare <cpu> element with for example NUMA configuration), but libxl driver never supported full 'custom' mode - it was silently ignored, which might lead to some unexpected effects. Since mode='host-passthrough' is now necessary to specify CPU options, do not enable nested HVM feature by mere presence of this element, require also enabling it in libxl.conf. Nested HVM is still in "preview" state, so better be explicit here. v2 of this patch series: https://www.redhat.com/archives/libvir-list/2017-July/msg00050.html v3 of this patch series: https://www.redhat.com/archives/libvir-list/2017-December/msg00314.html v4 of this patch series: https://www.redhat.com/archives/libvir-list/2018-February/msg00504.html v5 of this patch series: https://www.redhat.com/archives/libvir-list/2018-March/msg00796.html Marek Marczykowski-Górecki (9): libxl: fix libxlDriverConfigDispose for partially constructed object libxl: pass driver config to libxlMakeDomBuildInfo libxl: warn about ignored CPU mode=custom libxl: do not enable nested HVM unless global nested_hvm option enabled xenconfig: do not override def->cpu if already set elsewhere libxl: add support for CPUID features policy tests: check CPU features handling in libxl driver xenconfig: add CPUID handling to domXML <-> xl.cfg conversion tests: add test case for CPUID in xenconfig driver src/libxl/libvirtd_libxl.aug | 2 +- src/libxl/libxl.conf | 8 +- src/libxl/libxl_conf.c | 66 +++- src/libxl/libxl_conf.h | 6 +- src/libxl/libxl_domain.c | 2 +- src/libxl/test_libvirtd_libxl.aug.in | 1 +- src/xenconfig/xen_xl.c | 236 ++++++++++++++-- src/xenconfig/xen_xl.h | 2 +- tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 ++++- tests/libxlxml2domconfigdata/fullvirt-cpuid.xml | 37 +++- tests/libxlxml2domconfigtest.c | 27 +- tests/virmocklibxl.c | 25 ++- tests/xlconfigdata/test-fullvirt-cpuid.cfg | 25 ++- tests/xlconfigdata/test-fullvirt-cpuid.xml | 35 ++- tests/xlconfigtest.c | 1 +- 15 files changed, 492 insertions(+), 45 deletions(-) create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml create mode 100644 tests/xlconfigdata/test-fullvirt-cpuid.cfg create mode 100644 tests/xlconfigdata/test-fullvirt-cpuid.xml base-commit: 6ce3acc129bfdbe7fd02bcb8bbe8af6d13903684 -- git-series 0.9.1

libxlDriverConfigNew() use libxlDriverConfigDispose() for cleanup in case of errors. Do not call libxlLoggerFree() on not allocated logger (NULL). Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- Changes since v3: - new patch, mostly unrelated, but found while adjusting tests --- 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); -- git-series 0.9.1

Preparation for global nestedhvm configuration - libxlMakeDomBuildInfo needs access to libxlDriverConfig. No functional change. Adjusting tests require slightly more mockup functions, because of libxlDriverConfigNew() call. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 25 +++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 17 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..e7727a1 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; @@ -2287,17 +2288,17 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + libxlDriverConfigPtr cfg, libxl_domain_config *d_config) { + virCapsPtr caps = cfg->caps; + 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 @@ -2329,7 +2330,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, #endif #ifdef LIBXL_HAVE_DEVICE_CHANNEL - if (libxlMakeChannelList(channelDir, def, d_config) < 0) + if (libxlMakeChannelList(cfg->channelDir, def, d_config) < 0) return -1; #endif diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..ce9db26 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -215,9 +215,7 @@ libxlCreateXMLConf(void); int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + 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 395c8a9..8879481 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, &d_config) < 0) goto cleanup_dom; if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..cfbc37c 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,18 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig); + if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + + cfg->caps = caps; + 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 +93,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, cfg, &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 +126,11 @@ 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); + cfg->caps = NULL; + 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, -- git-series 0.9.1

On Wed, Mar 21, 2018 at 05:32:26PM +0100, 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.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 25 +++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 17 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 03/21/2018 10:32 AM, 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.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 25 +++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 17 deletions(-)
I had to rebase this patch to account for commits 7ebc4f2a4c, 4c9c7a5ba2, and c391e07eb0.
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..e7727a1 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; @@ -2287,17 +2288,17 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + libxlDriverConfigPtr cfg, libxl_domain_config *d_config) { + virCapsPtr caps = cfg->caps; + 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 @@ -2329,7 +2330,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, #endif
#ifdef LIBXL_HAVE_DEVICE_CHANNEL - if (libxlMakeChannelList(channelDir, def, d_config) < 0) + if (libxlMakeChannelList(cfg->channelDir, def, d_config) < 0) return -1; #endif
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..ce9db26 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -215,9 +215,7 @@ libxlCreateXMLConf(void); int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + 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 395c8a9..8879481 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, &d_config) < 0) goto cleanup_dom;
if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..cfbc37c 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,18 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig);
+ if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + + cfg->caps = caps; +
Before pushing this series I decided to build test it on several Xen releases I had readily available: 4.10, 4.9, 4.7, and 4.5. It works fine on all of those except 4.5. libxlxml2domconfigtest segfaults here on the first test Program received signal SIGSEGV, Segmentation fault. 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 (gdb) bt #0 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 #1 0x00007ffff75250c6 in xs_talkv (h=h@entry=0x1, t=t@entry=0, type=type@entry=XS_READ, iovec=iovec@entry=0x7fffffffce50, num_vecs=num_vecs@entry=1, len=len@entry=0x0) at xs.c:491 #2 0x00007ffff752544a in xs_single (h=0x1, t=t@entry=0, type=type@entry=XS_READ, string=string@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:551 #3 0x00007ffff75257e4 in xs_read (h=<optimized out>, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:597 #4 0x00007ffff7978ca9 in libxl__xs_read (gc=gc@entry=0x7fffffffcf40, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack") at libxl_xshelp.c:123 #5 0x00007ffff79622ab in libxl__get_free_memory_slack ( gc=gc@entry=0x7fffffffcf40, free_mem_slack=free_mem_slack@entry=0x7fffffffcf3c) at libxl.c:5122 #6 0x00007ffff796238e in libxl_get_free_memory (ctx=<optimized out>, memkb=0x7fffffffd014) at libxl.c:5394 #7 0x000000000041313b in libxlDriverConfigNew () at libxl/libxl_conf.c:1690 #8 0x000000000040aff0 in testCompareXMLToDomConfig ( xmlfile=0x63bd20 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.xml", jsonfile=0x63be10 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.json") at libxlxml2domconfigtest.c:71 #9 0x000000000040b4b1 in testCompareXMLToDomConfigHelper ( data=0x637c50 <info>) at libxlxml2domconfigtest.c:164 #10 0x000000000040bcd4 in virTestRun ( title=0x42dcc0 "LibXL XML-2-JSON basic-pv", body=0x40b3cb <testCompareXMLToDomConfigHelper>, data=0x637c50 <info>) at testutils.c:180 I wonder how much we care about Xen 4.5? According to the Xen wiki [0] it is no longer supported upstream. 4.6 gets security support until Oct 2018, so IMO it should be the minimum version supported by upstream libvirt. Or maybe 4.7 since that is the earliest version still getting general upstream support? Regardless, I'll wait to push the series until we get this hashed out. Regards, Jim [0] https://wiki.xenproject.org/wiki/Xen_Project_Release_Features
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 +93,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, cfg, &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 +126,11 @@ 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); + cfg->caps = NULL; + 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 03/22/2018 04:44 PM, Jim Fehlig wrote:
On 03/21/2018 10:32 AM, 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.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 25 +++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 17 deletions(-)
I had to rebase this patch to account for commits 7ebc4f2a4c, 4c9c7a5ba2, and c391e07eb0.
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..e7727a1 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; @@ -2287,17 +2288,17 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + libxlDriverConfigPtr cfg, libxl_domain_config *d_config) { + virCapsPtr caps = cfg->caps; + 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 @@ -2329,7 +2330,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, #endif #ifdef LIBXL_HAVE_DEVICE_CHANNEL - if (libxlMakeChannelList(channelDir, def, d_config) < 0) + if (libxlMakeChannelList(cfg->channelDir, def, d_config) < 0) return -1; #endif diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..ce9db26 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -215,9 +215,7 @@ libxlCreateXMLConf(void); int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + 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 395c8a9..8879481 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, &d_config) < 0) goto cleanup_dom; if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..cfbc37c 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,18 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig); + if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + + cfg->caps = caps; +
Before pushing this series I decided to build test it on several Xen releases I had readily available: 4.10, 4.9, 4.7, and 4.5. It works fine on all of those except 4.5. libxlxml2domconfigtest segfaults here on the first test
Program received signal SIGSEGV, Segmentation fault. 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 (gdb) bt #0 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 #1 0x00007ffff75250c6 in xs_talkv (h=h@entry=0x1, t=t@entry=0, type=type@entry=XS_READ, iovec=iovec@entry=0x7fffffffce50, num_vecs=num_vecs@entry=1, len=len@entry=0x0) at xs.c:491 #2 0x00007ffff752544a in xs_single (h=0x1, t=t@entry=0, type=type@entry=XS_READ, string=string@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:551 #3 0x00007ffff75257e4 in xs_read (h=<optimized out>, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:597 #4 0x00007ffff7978ca9 in libxl__xs_read (gc=gc@entry=0x7fffffffcf40, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack") at libxl_xshelp.c:123 #5 0x00007ffff79622ab in libxl__get_free_memory_slack ( gc=gc@entry=0x7fffffffcf40, free_mem_slack=free_mem_slack@entry=0x7fffffffcf3c) at libxl.c:5122 #6 0x00007ffff796238e in libxl_get_free_memory (ctx=<optimized out>, memkb=0x7fffffffd014) at libxl.c:5394 #7 0x000000000041313b in libxlDriverConfigNew () at libxl/libxl_conf.c:1690 #8 0x000000000040aff0 in testCompareXMLToDomConfig ( xmlfile=0x63bd20 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.xml", jsonfile=0x63be10 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.json") at libxlxml2domconfigtest.c:71 #9 0x000000000040b4b1 in testCompareXMLToDomConfigHelper ( data=0x637c50 <info>) at libxlxml2domconfigtest.c:164 #10 0x000000000040bcd4 in virTestRun ( title=0x42dcc0 "LibXL XML-2-JSON basic-pv", body=0x40b3cb <testCompareXMLToDomConfigHelper>, data=0x637c50 <info>) at testutils.c:180
I wonder how much we care about Xen 4.5? According to the Xen wiki [0] it is no longer supported upstream. 4.6 gets security support until Oct 2018, so IMO it should be the minimum version supported by upstream libvirt. Or maybe 4.7 since that is the earliest version still getting general upstream support?
BTW, I forgot to mention that we currently claim support for Xen >= 4.4. See m4/virt-driver-libxl.m4. Regards, Jim

On Thu, Mar 22, 2018 at 04:48:48PM -0600, Jim Fehlig wrote:
On 03/22/2018 04:44 PM, Jim Fehlig wrote:
On 03/21/2018 10:32 AM, 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.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 25 +++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 17 deletions(-)
I had to rebase this patch to account for commits 7ebc4f2a4c, 4c9c7a5ba2, and c391e07eb0.
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..e7727a1 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; @@ -2287,17 +2288,17 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + libxlDriverConfigPtr cfg, libxl_domain_config *d_config) { + virCapsPtr caps = cfg->caps; + 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 @@ -2329,7 +2330,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, #endif #ifdef LIBXL_HAVE_DEVICE_CHANNEL - if (libxlMakeChannelList(channelDir, def, d_config) < 0) + if (libxlMakeChannelList(cfg->channelDir, def, d_config) < 0) return -1; #endif diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..ce9db26 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -215,9 +215,7 @@ libxlCreateXMLConf(void); int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + 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 395c8a9..8879481 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, &d_config) < 0) goto cleanup_dom; if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..cfbc37c 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,18 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig); + if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + + cfg->caps = caps; +
Before pushing this series I decided to build test it on several Xen releases I had readily available: 4.10, 4.9, 4.7, and 4.5. It works fine on all of those except 4.5. libxlxml2domconfigtest segfaults here on the first test
Program received signal SIGSEGV, Segmentation fault. 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 (gdb) bt #0 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 #1 0x00007ffff75250c6 in xs_talkv (h=h@entry=0x1, t=t@entry=0, type=type@entry=XS_READ, iovec=iovec@entry=0x7fffffffce50, num_vecs=num_vecs@entry=1, len=len@entry=0x0) at xs.c:491 #2 0x00007ffff752544a in xs_single (h=0x1, t=t@entry=0, type=type@entry=XS_READ, string=string@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:551 #3 0x00007ffff75257e4 in xs_read (h=<optimized out>, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:597 #4 0x00007ffff7978ca9 in libxl__xs_read (gc=gc@entry=0x7fffffffcf40, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack") at libxl_xshelp.c:123 #5 0x00007ffff79622ab in libxl__get_free_memory_slack ( gc=gc@entry=0x7fffffffcf40, free_mem_slack=free_mem_slack@entry=0x7fffffffcf3c) at libxl.c:5122 #6 0x00007ffff796238e in libxl_get_free_memory (ctx=<optimized out>, memkb=0x7fffffffd014) at libxl.c:5394 #7 0x000000000041313b in libxlDriverConfigNew () at libxl/libxl_conf.c:1690 #8 0x000000000040aff0 in testCompareXMLToDomConfig ( xmlfile=0x63bd20 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.xml", jsonfile=0x63be10 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.json") at libxlxml2domconfigtest.c:71 #9 0x000000000040b4b1 in testCompareXMLToDomConfigHelper ( data=0x637c50 <info>) at libxlxml2domconfigtest.c:164 #10 0x000000000040bcd4 in virTestRun ( title=0x42dcc0 "LibXL XML-2-JSON basic-pv", body=0x40b3cb <testCompareXMLToDomConfigHelper>, data=0x637c50 <info>) at testutils.c:180
I wonder how much we care about Xen 4.5? According to the Xen wiki [0] it is no longer supported upstream. 4.6 gets security support until Oct 2018, so IMO it should be the minimum version supported by upstream libvirt. Or maybe 4.7 since that is the earliest version still getting general upstream support?
BTW, I forgot to mention that we currently claim support for Xen >= 4.4. See m4/virt-driver-libxl.m4.
Does it work before applying this series? -- 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?

On 03/22/2018 04:59 PM, Marek Marczykowski-Górecki wrote:
On Thu, Mar 22, 2018 at 04:48:48PM -0600, Jim Fehlig wrote:
On 03/22/2018 04:44 PM, Jim Fehlig wrote:
On 03/21/2018 10:32 AM, 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.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 25 +++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 17 deletions(-)
I had to rebase this patch to account for commits 7ebc4f2a4c, 4c9c7a5ba2, and c391e07eb0.
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..e7727a1 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; @@ -2287,17 +2288,17 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + libxlDriverConfigPtr cfg, libxl_domain_config *d_config) { + virCapsPtr caps = cfg->caps; + 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 @@ -2329,7 +2330,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, #endif #ifdef LIBXL_HAVE_DEVICE_CHANNEL - if (libxlMakeChannelList(channelDir, def, d_config) < 0) + if (libxlMakeChannelList(cfg->channelDir, def, d_config) < 0) return -1; #endif diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..ce9db26 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -215,9 +215,7 @@ libxlCreateXMLConf(void); int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + 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 395c8a9..8879481 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, &d_config) < 0) goto cleanup_dom; if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..cfbc37c 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,18 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig); + if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + + cfg->caps = caps; +
Before pushing this series I decided to build test it on several Xen releases I had readily available: 4.10, 4.9, 4.7, and 4.5. It works fine on all of those except 4.5. libxlxml2domconfigtest segfaults here on the first test
Program received signal SIGSEGV, Segmentation fault. 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 (gdb) bt #0 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 #1 0x00007ffff75250c6 in xs_talkv (h=h@entry=0x1, t=t@entry=0, type=type@entry=XS_READ, iovec=iovec@entry=0x7fffffffce50, num_vecs=num_vecs@entry=1, len=len@entry=0x0) at xs.c:491 #2 0x00007ffff752544a in xs_single (h=0x1, t=t@entry=0, type=type@entry=XS_READ, string=string@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:551 #3 0x00007ffff75257e4 in xs_read (h=<optimized out>, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:597 #4 0x00007ffff7978ca9 in libxl__xs_read (gc=gc@entry=0x7fffffffcf40, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack") at libxl_xshelp.c:123 #5 0x00007ffff79622ab in libxl__get_free_memory_slack ( gc=gc@entry=0x7fffffffcf40, free_mem_slack=free_mem_slack@entry=0x7fffffffcf3c) at libxl.c:5122 #6 0x00007ffff796238e in libxl_get_free_memory (ctx=<optimized out>, memkb=0x7fffffffd014) at libxl.c:5394 #7 0x000000000041313b in libxlDriverConfigNew () at libxl/libxl_conf.c:1690 #8 0x000000000040aff0 in testCompareXMLToDomConfig ( xmlfile=0x63bd20 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.xml", jsonfile=0x63be10 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.json") at libxlxml2domconfigtest.c:71 #9 0x000000000040b4b1 in testCompareXMLToDomConfigHelper ( data=0x637c50 <info>) at libxlxml2domconfigtest.c:164 #10 0x000000000040bcd4 in virTestRun ( title=0x42dcc0 "LibXL XML-2-JSON basic-pv", body=0x40b3cb <testCompareXMLToDomConfigHelper>, data=0x637c50 <info>) at testutils.c:180
I wonder how much we care about Xen 4.5? According to the Xen wiki [0] it is no longer supported upstream. 4.6 gets security support until Oct 2018, so IMO it should be the minimum version supported by upstream libvirt. Or maybe 4.7 since that is the earliest version still getting general upstream support?
BTW, I forgot to mention that we currently claim support for Xen >= 4.4. See m4/virt-driver-libxl.m4.
Does it work before applying this series?
Sorry, forgot to mention that too. Yes, it works before applying the series. Regards, Jim

On 03/22/2018 07:59 PM, Jim Fehlig wrote:
On 03/22/2018 04:59 PM, Marek Marczykowski-Górecki wrote:
On Thu, Mar 22, 2018 at 04:48:48PM -0600, Jim Fehlig wrote:
On 03/22/2018 04:44 PM, Jim Fehlig wrote:
On 03/21/2018 10:32 AM, 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.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 25 +++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 17 deletions(-)
I had to rebase this patch to account for commits 7ebc4f2a4c, 4c9c7a5ba2, and c391e07eb0.
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..e7727a1 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; @@ -2287,17 +2288,17 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + libxlDriverConfigPtr cfg, libxl_domain_config *d_config) { + virCapsPtr caps = cfg->caps; + 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 @@ -2329,7 +2330,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, #endif #ifdef LIBXL_HAVE_DEVICE_CHANNEL - if (libxlMakeChannelList(channelDir, def, d_config) < 0) + if (libxlMakeChannelList(cfg->channelDir, def, d_config) < 0) return -1; #endif diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..ce9db26 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -215,9 +215,7 @@ libxlCreateXMLConf(void); int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + 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 395c8a9..8879481 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, &d_config) < 0) goto cleanup_dom; if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..cfbc37c 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,18 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig); + if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + + cfg->caps = caps; +
Before pushing this series I decided to build test it on several Xen releases I had readily available: 4.10, 4.9, 4.7, and 4.5. It works fine on all of those except 4.5. libxlxml2domconfigtest segfaults here on the first test
Program received signal SIGSEGV, Segmentation fault. 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 (gdb) bt #0 0x00007ffff3d60244 in pthread_mutex_lock () from /lib64/libpthread.so.0 #1 0x00007ffff75250c6 in xs_talkv (h=h@entry=0x1, t=t@entry=0, type=type@entry=XS_READ, iovec=iovec@entry=0x7fffffffce50, num_vecs=num_vecs@entry=1, len=len@entry=0x0) at xs.c:491 #2 0x00007ffff752544a in xs_single (h=0x1, t=t@entry=0, type=type@entry=XS_READ, string=string@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:551 #3 0x00007ffff75257e4 in xs_read (h=<optimized out>, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack", len=len@entry=0x0) at xs.c:597 #4 0x00007ffff7978ca9 in libxl__xs_read (gc=gc@entry=0x7fffffffcf40, t=t@entry=0, path=path@entry=0x7ffff79a4b70 "/local/domain/0/memory/freemem-slack") at libxl_xshelp.c:123 #5 0x00007ffff79622ab in libxl__get_free_memory_slack ( gc=gc@entry=0x7fffffffcf40, free_mem_slack=free_mem_slack@entry=0x7fffffffcf3c) at libxl.c:5122 #6 0x00007ffff796238e in libxl_get_free_memory (ctx=<optimized out>, memkb=0x7fffffffd014) at libxl.c:5394 #7 0x000000000041313b in libxlDriverConfigNew () at libxl/libxl_conf.c:1690 #8 0x000000000040aff0 in testCompareXMLToDomConfig ( xmlfile=0x63bd20 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.xml",
jsonfile=0x63be10 "/home/jfehlig/virt/upstream/libvirt/tests/libxlxml2domconfigdata/basic-pv.json")
at libxlxml2domconfigtest.c:71 #9 0x000000000040b4b1 in testCompareXMLToDomConfigHelper ( data=0x637c50 <info>) at libxlxml2domconfigtest.c:164 #10 0x000000000040bcd4 in virTestRun ( title=0x42dcc0 "LibXL XML-2-JSON basic-pv", body=0x40b3cb <testCompareXMLToDomConfigHelper>, data=0x637c50 <info>) at testutils.c:180
I wonder how much we care about Xen 4.5? According to the Xen wiki [0] it is no longer supported upstream. 4.6 gets security support until Oct 2018, so IMO it should be the minimum version supported by upstream libvirt. Or maybe 4.7 since that is the earliest version still getting general upstream support?
BTW, I forgot to mention that we currently claim support for Xen >= 4.4. See m4/virt-driver-libxl.m4.
Does it work before applying this series?
Sorry, forgot to mention that too. Yes, it works before applying the series.
Did you have an opportunity to look at this problem on Xen 4.5? Alternatively we could increase the minimum supported Xen version to 4.6 as I've done in this patchset https://www.redhat.com/archives/libvir-list/2018-March/msg01704.html Regards, Jim

On Tue, Mar 27, 2018 at 04:34:27PM -0600, Jim Fehlig wrote:
Did you have an opportunity to look at this problem on Xen 4.5?
Actually I'm on it right now. Getting Xen 4.5 built using modern gcc is kind of a challenge, thanks to -Werror...
Alternatively we could increase the minimum supported Xen version to 4.6 as I've done in this patchset
https://www.redhat.com/archives/libvir-list/2018-March/msg01704.html
-- 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?

On Wed, Mar 28, 2018 at 12:38:22AM +0200, Marek Marczykowski-Górecki wrote:
On Tue, Mar 27, 2018 at 04:34:27PM -0600, Jim Fehlig wrote:
Did you have an opportunity to look at this problem on Xen 4.5?
Actually I'm on it right now. Getting Xen 4.5 built using modern gcc is kind of a challenge, thanks to -Werror...
Ok, found it - libxlDriverConfigNew calls libxl_get_free_memory, which tries to access xenstore. But in tests we have mocks for xs_daemon_open, which returns handle (void*)0x1. Using this handle obviously leads to SEGV. Previously it worked because libxlDriverConfigNew wasn't needed during tests. Solution: add mock for libxl_get_free_memory (its return value isn't even used - its purpose is to initialize xenstore entries): VIR_MOCK_STUB_RET_ARGS(libxl_get_free_memory, int, 0, libxl_ctx *, ctx, uint32_t *, memkb); Do you want new version of this patch, or the whole series? -- 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?

Preparation for global nestedhvm configuration - libxlMakeDomBuildInfo needs access to libxlDriverConfig. No functional change. Adjusting tests require slightly more mockup functions, because of libxlDriverConfigNew() call. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v6: - tests: add libxl_get_free_memory mock needed on Xen 4.5 Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 30 ++++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 17 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..e7727a1 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; @@ -2287,17 +2288,17 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + libxlDriverConfigPtr cfg, libxl_domain_config *d_config) { + virCapsPtr caps = cfg->caps; + 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 @@ -2329,7 +2330,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, #endif #ifdef LIBXL_HAVE_DEVICE_CHANNEL - if (libxlMakeChannelList(channelDir, def, d_config) < 0) + if (libxlMakeChannelList(cfg->channelDir, def, d_config) < 0) return -1; #endif diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..ce9db26 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -215,9 +215,7 @@ libxlCreateXMLConf(void); int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + 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 395c8a9..8879481 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, &d_config) < 0) goto cleanup_dom; if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..cfbc37c 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,18 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig); + if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + + cfg->caps = caps; + 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 +93,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, cfg, &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 +126,11 @@ 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); + cfg->caps = NULL; + virObjectUnref(cfg); return ret; } diff --git a/tests/virmocklibxl.c b/tests/virmocklibxl.c index 747f9f8..28281b6 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,24 @@ 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(libxl_get_free_memory, + int, 0, + libxl_ctx *, ctx, + uint32_t *, memkb); + VIR_MOCK_STUB_RET_ARGS(xc_interface_close, int, 0, xc_interface *, handle) @@ -68,6 +87,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, -- git-series 0.9.1

On 03/27/2018 05:55 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.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v6: - tests: add libxl_get_free_memory mock needed on Xen 4.5 Changes since v4: - drop now unneeded parameters Changes since v3: - new patch, preparation --- src/libxl/libxl_conf.c | 13 +++++++------ src/libxl/libxl_conf.h | 4 +--- src/libxl/libxl_domain.c | 2 +- tests/libxlxml2domconfigtest.c | 23 ++++++++++++++++------- tests/virmocklibxl.c | 30 ++++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 2d2a707..e7727a1 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; @@ -2287,17 +2288,17 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + libxlDriverConfigPtr cfg, libxl_domain_config *d_config) { + virCapsPtr caps = cfg->caps; + 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 @@ -2329,7 +2330,7 @@ libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, #endif
#ifdef LIBXL_HAVE_DEVICE_CHANNEL - if (libxlMakeChannelList(channelDir, def, d_config) < 0) + if (libxlMakeChannelList(cfg->channelDir, def, d_config) < 0) return -1; #endif
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..ce9db26 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -215,9 +215,7 @@ libxlCreateXMLConf(void); int libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, virDomainDefPtr def, - const char *channelDir LIBXL_ATTR_UNUSED, - libxl_ctx *ctx, - virCapsPtr caps, + 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 395c8a9..8879481 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, &d_config) < 0) goto cleanup_dom;
if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0) diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..cfbc37c 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,18 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config_init(&actualconfig); libxl_domain_config_init(&expectconfig);
+ if (!(cfg = libxlDriverConfigNew())) + goto cleanup; + + cfg->caps = caps; + 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 +93,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, cfg, &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 +126,11 @@ 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); + cfg->caps = NULL; + virObjectUnref(cfg); return ret; }
diff --git a/tests/virmocklibxl.c b/tests/virmocklibxl.c index 747f9f8..28281b6 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,24 @@ 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(libxl_get_free_memory, + int, 0, + libxl_ctx *, ctx, + uint32_t *, memkb); +
This doesn't compile with Xen >= 4.8 In file included from virmocklibxl.c:26:0: virmocklibxl.c:66:24: error: conflicting types for 'libxl_get_free_memory' VIR_MOCK_STUB_RET_ARGS(libxl_get_free_memory, ^ virmock.h:182:13: note: in definition of macro 'VIR_MOCK_STUB_RET_ARGS' rettype name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__)) \ ^~~~ In file included from virmocklibxl.c:29:0: /usr/include/libxl.h:1570:5: note: previous declaration of 'libxl_get_free_memory' was here int libxl_get_free_memory(libxl_ctx *ctx, uint64_t *memkb); ^~~~~~~~~~~~~~~~~~~~~ Using the uint32_t variant works in the libxl driver since we have -DLIBXL_API_VERSION=0x040400 in LIBXL_CFLAGS. I worked around the compilation failure with LIBXL_HAVE_MEMKB_64BITS, but then libxlxml2domconfigtest crashed. BTW, since we are on the heels of the 4.2.0 release, and since this thread is getting lengthy, can you resend the series after fixing this, and include all the R-B in the patches? Thanks, and sorry for yet another version :-(. On the bright side, we should be able to commit this soon after the release! Regards, Jim
VIR_MOCK_STUB_RET_ARGS(xc_interface_close, int, 0, xc_interface *, handle) @@ -68,6 +87,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 Wed, Mar 28, 2018 at 01:42:47PM -0600, Jim Fehlig wrote:
On 03/27/2018 05:55 PM, Marek Marczykowski-Górecki wrote:
diff --git a/tests/virmocklibxl.c b/tests/virmocklibxl.c index 747f9f8..28281b6 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,24 @@ 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(libxl_get_free_memory, + int, 0, + libxl_ctx *, ctx, + uint32_t *, memkb); +
This doesn't compile with Xen >= 4.8
In file included from virmocklibxl.c:26:0: virmocklibxl.c:66:24: error: conflicting types for 'libxl_get_free_memory' VIR_MOCK_STUB_RET_ARGS(libxl_get_free_memory, ^ virmock.h:182:13: note: in definition of macro 'VIR_MOCK_STUB_RET_ARGS' rettype name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__)) \ ^~~~ In file included from virmocklibxl.c:29:0: /usr/include/libxl.h:1570:5: note: previous declaration of 'libxl_get_free_memory' was here int libxl_get_free_memory(libxl_ctx *ctx, uint64_t *memkb); ^~~~~~~~~~~~~~~~~~~~~
Using the uint32_t variant works in the libxl driver since we have -DLIBXL_API_VERSION=0x040400 in LIBXL_CFLAGS. I worked around the compilation failure with LIBXL_HAVE_MEMKB_64BITS,
I can't reproduce this problem, either with 4.8 or 4.10. Even more, if I add alternative mock with uint64_t, under #if LIBXL_HAVE_MEMKB_64BITS, I get compile failure, because of conflicting types (with libxl_get_free_memory_0x040700)... Can you confirm it's really a problem, not some mismatching header versions on your side?
but then libxlxml2domconfigtest crashed.
I've got test failure, apparently because something have changed with VGA model in json format. I'll remove it from the test, as it is unrelated to CPUID. -- 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?

On 04/06/2018 06:54 PM, Marek Marczykowski-Górecki wrote:
On Wed, Mar 28, 2018 at 01:42:47PM -0600, Jim Fehlig wrote:
On 03/27/2018 05:55 PM, Marek Marczykowski-Górecki wrote:
diff --git a/tests/virmocklibxl.c b/tests/virmocklibxl.c index 747f9f8..28281b6 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,24 @@ 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(libxl_get_free_memory, + int, 0, + libxl_ctx *, ctx, + uint32_t *, memkb); +
This doesn't compile with Xen >= 4.8
In file included from virmocklibxl.c:26:0: virmocklibxl.c:66:24: error: conflicting types for 'libxl_get_free_memory' VIR_MOCK_STUB_RET_ARGS(libxl_get_free_memory, ^ virmock.h:182:13: note: in definition of macro 'VIR_MOCK_STUB_RET_ARGS' rettype name(VIR_MOCK_ARGTYPENAMES_UNUSED(__VA_ARGS__)) \ ^~~~ In file included from virmocklibxl.c:29:0: /usr/include/libxl.h:1570:5: note: previous declaration of 'libxl_get_free_memory' was here int libxl_get_free_memory(libxl_ctx *ctx, uint64_t *memkb); ^~~~~~~~~~~~~~~~~~~~~
Using the uint32_t variant works in the libxl driver since we have -DLIBXL_API_VERSION=0x040400 in LIBXL_CFLAGS. I worked around the compilation failure with LIBXL_HAVE_MEMKB_64BITS,
I can't reproduce this problem, either with 4.8 or 4.10. Even more, if I add alternative mock with uint64_t, under #if LIBXL_HAVE_MEMKB_64BITS, I get compile failure, because of conflicting types (with libxl_get_free_memory_0x040700)...
Can you confirm it's really a problem, not some mismatching header versions on your side?
Perhaps I've also made a mistake rebasing some of these patches. Can you pretty please rebase against current master and repost a V7 (adding all the R-B)? If you say it passes 'make check' on 4.5 and 4.10, I'll chase down any problems on my side. Thanks! Regards, Jim

When support for mode=custom will be added in the future, semantics of current config will change. Reduce the surprise by emitting a warning. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v5: - new patch, instead of "libxl: error out on not supported CPU mode, instead of silently ignoring" --- src/libxl/libxl_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index e7727a1..dcfdd67 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -389,6 +389,12 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_defbool_set(&b_info->u.hvm.nested_hvm, hasHwVirt); } + if (def->cpu && def->cpu->mode == VIR_CPU_MODE_CUSTOM) { + VIR_WARN("Ignoring CPU with mode=custom, update your config to " + "mode=host-passthrough to avoid risk of changed guest " + "semantics when mode=custom is supported in the future"); + } + if (def->nsounds > 0) { /* * Use first sound device. man xl.cfg(5) describes soundhw as -- git-series 0.9.1

On Wed, Mar 21, 2018 at 05:32:27PM +0100, Marek Marczykowski-Górecki wrote:
When support for mode=custom will be added in the future, semantics of current config will change. Reduce the surprise by emitting a warning.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v5: - new patch, instead of "libxl: error out on not supported CPU mode, instead of silently ignoring" --- src/libxl/libxl_conf.c | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

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@invisiblethingslab.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- 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. +# 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; @@ -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); -- git-series 0.9.1

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@invisiblethingslab.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- 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. 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);

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@invisiblethingslab.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- 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?

On 03/21/2018 06:05 PM, Marek Marczykowski-Górecki wrote:
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@invisiblethingslab.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- 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.
Ok. No need to send the whole series again. Just a followup to this patch will do. Thanks! Regards, Jim

On Wed, Mar 21, 2018 at 06:12:39PM -0600, Jim Fehlig wrote:
Ok. No need to send the whole series again. Just a followup to this patch will do. Thanks!
Just sent, but I've failed to connect it to this thread... It's marked as v6.1. -- 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?

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@invisiblethingslab.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- Changes since v6: - really allow per-domain override 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. +# By default it is disabled. +#nested_hvm = 0 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index dcfdd67..ed8506d 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -360,10 +360,12 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, bool hasHwVirt = false; bool svm = false, vmx = false; + /* enable nested HVM only if global nested_hvm option enable it and + * host support it*/ 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"); - hasHwVirt = vmx | svm; + hasHwVirt = cfg->nested_hvm && (vmx | svm); } if (def->cpu->nfeatures) { @@ -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); -- git-series 0.9.1

On Thu, Mar 22, 2018 at 01:15:31AM +0100, 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@invisiblethingslab.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- Changes since v6: - really allow per-domain override 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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 03/21/2018 06:15 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.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- Changes since v6: - really allow per-domain override 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. +# By default it is disabled. +#nested_hvm = 0 diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index dcfdd67..ed8506d 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -360,10 +360,12 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, bool hasHwVirt = false; bool svm = false, vmx = false;
+ /* enable nested HVM only if global nested_hvm option enable it and + * host support it*/ 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"); - hasHwVirt = vmx | svm; + hasHwVirt = cfg->nested_hvm && (vmx | svm); }
if (def->cpu->nfeatures) { @@ -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" }
Fails 'make check' since the default is 0. I've fixed it in my branch. Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
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);

This will help with adding cpuid support. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- Changes since v4: - patch separated from "libxl: do not enable nested HVM unless global nested_hvm option enabled" --- src/xenconfig/xen_xl.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 2ef80eb..93eba51 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) -- git-series 0.9.1

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> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- Changes since v4: - added spec-ctrl/ibrsb 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 | 36 +++++++++++++++++++++++++++++++++--- src/xenconfig/xen_xl.c | 35 +++++++++++++++++++++++++++++++++++ src/xenconfig/xen_xl.h | 2 ++ 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 3b9e828..235b982 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 @@ -359,6 +360,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { bool hasHwVirt = false; bool svm = false, vmx = false; + char xlCPU[32]; /* enable nested HVM only if global nested_hvm option enable it and * host support it*/ @@ -376,17 +378,45 @@ 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"))) + (svm && STREQ(def->cpu->features[i].name, "svm"))) { hasHwVirt = true; + 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 93eba51..94d91cd 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -218,6 +218,41 @@ 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" }, + { "spec-ctrl", "ibrsb" }, + }; + 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__ */ -- git-series 0.9.1

Test enabling/disabling individual CPU features and also setting nested HVM support, which is also controlled by CPU features node. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- Changes since v3: - adjust for modified nested HVM handling Changes since v1: - rewritten to Jim's test suite for libxl_domain_config generator --- tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 +++++++++++++++++- tests/libxlxml2domconfigdata/fullvirt-cpuid.xml | 37 ++++++++++- tests/libxlxml2domconfigtest.c | 1 +- 3 files changed, 102 insertions(+) create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json new file mode 100644 index 0000000..28037be --- /dev/null +++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json @@ -0,0 +1,64 @@ +{ + "c_info": { + "type": "hvm", + "name": "XenGuest2", + "uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809" + }, + "b_info": { + "max_vcpus": 1, + "avail_vcpus": [ + 0 + ], + "max_memkb": 592896, + "target_memkb": 403456, + "video_memkb": 8192, + "shadow_memkb": 5656, + "cpuid": [ + { + "leaf": 1, + "ecx": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx0", + "edx": "xxxxxxxxxxxxxxxxxxxxxxxxxxx1xxxx" + } + ], + "sched_params": { + "weight": 1000 + }, + "type.hvm": { + "pae": "True", + "apic": "True", + "acpi": "True", + "vga": { + + }, + "nested_hvm": "False", + "vnc": { + "enable": "False" + }, + "sdl": { + "enable": "False" + }, + "spice": { + + }, + "boot": "c", + "rdm": { + + } + }, + "arch_arm": { + + } + }, + "disks": [ + { + "pdev_path": "/dev/HostVG/XenGuest2", + "vdev": "hda", + "backend": "phy", + "format": "raw", + "removable": 1, + "readwrite": 1 + } + ], + "on_reboot": "restart", + "on_crash": "restart" +} diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml b/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml new file mode 100644 index 0000000..e9eba2e --- /dev/null +++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.xml @@ -0,0 +1,37 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='xenfv'>hvm</type> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <cpu mode='host-passthrough'> + <feature policy='forbid' name='pni'/> + <feature policy='forbid' name='vmx'/> + <feature policy='require' name='tsc'/> + </cpu> + <clock offset='variable' adjustment='0' basis='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <disk type='block' device='disk'> + <driver name='phy' type='raw'/> + <source dev='/dev/HostVG/XenGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <video> + <model type='cirrus' vram='8192' heads='1' primary='yes'/> + </video> + </devices> +</domain> diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index 8819032..10e1bb2 100644 --- a/tests/libxlxml2domconfigtest.c +++ b/tests/libxlxml2domconfigtest.c @@ -203,6 +203,7 @@ mymain(void) DO_TEST("moredevs-hvm"); DO_TEST("vnuma-hvm"); DO_TEST("multiple-ip"); + DO_TEST("fullvirt-cpuid"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- git-series 0.9.1

On Wed, Mar 21, 2018 at 05:32:31PM +0100, Marek Marczykowski-Górecki wrote:
Test enabling/disabling individual CPU features and also setting nested HVM support, which is also controlled by CPU features node.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- Changes since v3: - adjust for modified nested HVM handling Changes since v1: - rewritten to Jim's test suite for libxl_domain_config generator --- tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 +++++++++++++++++- tests/libxlxml2domconfigdata/fullvirt-cpuid.xml | 37 ++++++++++- tests/libxlxml2domconfigtest.c | 1 +- 3 files changed, 102 insertions(+) create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json new file mode 100644 index 0000000..28037be --- /dev/null +++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json @@ -0,0 +1,64 @@ +{ + "c_info": { + "type": "hvm", + "name": "XenGuest2", + "uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809" + }, + "b_info": { + "max_vcpus": 1, + "avail_vcpus": [ + 0 + ], + "max_memkb": 592896, + "target_memkb": 403456, + "video_memkb": 8192, + "shadow_memkb": 5656, + "cpuid": [ + { + "leaf": 1, + "ecx": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx0", + "edx": "xxxxxxxxxxxxxxxxxxxxxxxxxxx1xxxx"
IIUC, this means we have 1 bit marked as forbidden and 1 bit marked as required but....
+ <cpu mode='host-passthrough'> + <feature policy='forbid' name='pni'/> + <feature policy='forbid' name='vmx'/>
2 features marked forbidden
+ <feature policy='require' name='tsc'/>
and 1 marked required. IOW, shouldn't we have seen two 0 bits in the cpuid config above not just one ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Thu, Mar 22, 2018 at 10:03:50AM +0000, Daniel P. Berrangé wrote:
On Wed, Mar 21, 2018 at 05:32:31PM +0100, Marek Marczykowski-Górecki wrote:
Test enabling/disabling individual CPU features and also setting nested HVM support, which is also controlled by CPU features node.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- Changes since v3: - adjust for modified nested HVM handling Changes since v1: - rewritten to Jim's test suite for libxl_domain_config generator --- tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 +++++++++++++++++- tests/libxlxml2domconfigdata/fullvirt-cpuid.xml | 37 ++++++++++- tests/libxlxml2domconfigtest.c | 1 +- 3 files changed, 102 insertions(+) create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json new file mode 100644 index 0000000..28037be --- /dev/null +++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json @@ -0,0 +1,64 @@ +{ + "c_info": { + "type": "hvm", + "name": "XenGuest2", + "uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809" + }, + "b_info": { + "max_vcpus": 1, + "avail_vcpus": [ + 0 + ], + "max_memkb": 592896, + "target_memkb": 403456, + "video_memkb": 8192, + "shadow_memkb": 5656, + "cpuid": [ + { + "leaf": 1, + "ecx": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx0", + "edx": "xxxxxxxxxxxxxxxxxxxxxxxxxxx1xxxx"
IIUC, this means we have 1 bit marked as forbidden and 1 bit marked as required but....
+ <cpu mode='host-passthrough'> + <feature policy='forbid' name='pni'/> + <feature policy='forbid' name='vmx'/>
2 features marked forbidden
vmx is translated to nested_hvm setting.
+ <feature policy='require' name='tsc'/>
and 1 marked required.
IOW, shouldn't we have seen two 0 bits in the cpuid config above not just one ?
Regards, Daniel
-- 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?

On Thu, Mar 22, 2018 at 04:47:21PM +0100, Marek Marczykowski-Górecki wrote:
On Thu, Mar 22, 2018 at 10:03:50AM +0000, Daniel P. Berrangé wrote:
On Wed, Mar 21, 2018 at 05:32:31PM +0100, Marek Marczykowski-Górecki wrote:
Test enabling/disabling individual CPU features and also setting nested HVM support, which is also controlled by CPU features node.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- Changes since v3: - adjust for modified nested HVM handling Changes since v1: - rewritten to Jim's test suite for libxl_domain_config generator --- tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 +++++++++++++++++- tests/libxlxml2domconfigdata/fullvirt-cpuid.xml | 37 ++++++++++- tests/libxlxml2domconfigtest.c | 1 +- 3 files changed, 102 insertions(+) create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json new file mode 100644 index 0000000..28037be --- /dev/null +++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json @@ -0,0 +1,64 @@ +{ + "c_info": { + "type": "hvm", + "name": "XenGuest2", + "uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809" + }, + "b_info": { + "max_vcpus": 1, + "avail_vcpus": [ + 0 + ], + "max_memkb": 592896, + "target_memkb": 403456, + "video_memkb": 8192, + "shadow_memkb": 5656, + "cpuid": [ + { + "leaf": 1, + "ecx": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx0", + "edx": "xxxxxxxxxxxxxxxxxxxxxxxxxxx1xxxx"
IIUC, this means we have 1 bit marked as forbidden and 1 bit marked as required but....
+ <cpu mode='host-passthrough'> + <feature policy='forbid' name='pni'/> + <feature policy='forbid' name='vmx'/>
2 features marked forbidden
vmx is translated to nested_hvm setting.
Ok then, Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 03/21/2018 10:32 AM, Marek Marczykowski-Górecki wrote:
Test enabling/disabling individual CPU features and also setting nested HVM support, which is also controlled by CPU features node.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- Changes since v3: - adjust for modified nested HVM handling Changes since v1: - rewritten to Jim's test suite for libxl_domain_config generator --- tests/libxlxml2domconfigdata/fullvirt-cpuid.json | 64 +++++++++++++++++- tests/libxlxml2domconfigdata/fullvirt-cpuid.xml | 37 ++++++++++- tests/libxlxml2domconfigtest.c | 1 +- 3 files changed, 102 insertions(+) create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.json create mode 100644 tests/libxlxml2domconfigdata/fullvirt-cpuid.xml
diff --git a/tests/libxlxml2domconfigdata/fullvirt-cpuid.json b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json new file mode 100644 index 0000000..28037be --- /dev/null +++ b/tests/libxlxml2domconfigdata/fullvirt-cpuid.json @@ -0,0 +1,64 @@ +{ + "c_info": { + "type": "hvm", + "name": "XenGuest2", + "uuid": "c7a5fdb2-cdaf-9455-926a-d65c16db1809" + }, + "b_info": { + "max_vcpus": 1, + "avail_vcpus": [ + 0 + ], + "max_memkb": 592896, + "target_memkb": 403456, + "video_memkb": 8192, + "shadow_memkb": 5656, + "cpuid": [ + { + "leaf": 1, + "ecx": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx0", + "edx": "xxxxxxxxxxxxxxxxxxxxxxxxxxx1xxxx" + } + ], + "sched_params": { + "weight": 1000 + },
This needed fixed up after commit 83edaf44.
+ "type.hvm": { + "pae": "True", + "apic": "True", + "acpi": "True", + "vga": { + + },
The XML file has <model type='cirrus' .../>, so we need "kind": "cirrus" here. I've fixed these nits in my branch. Regards, Jim

Only "libxl" format supported for now. Special care needed around vmx/svm, because those two are translated into "nestedhvm" setting. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v5: - adjust for ignoring mode=custom instead of rejecting it Changes since v3: - improve error reporting (VIR_ERR_CONF_SYNTAX) - ignore empty cpuid option - same as libxl - fix cleanup on error Changes since v2: - new patch --- src/xenconfig/xen_xl.c | 164 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 164 insertions(+) diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 94d91cd..52ec7e4 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -253,6 +253,91 @@ xenTranslateCPUFeature(const char *feature_name, bool from_libxl) return feature_name; } +static int +xenParseXLCPUID(virConfPtr conf, virDomainDefPtr def) +{ + const char *cpuid_str = NULL; + char **cpuid_pairs = NULL; + char **name_and_value = NULL; + size_t i; + int ret = -1; + int policy; + + if (xenConfigGetString(conf, "cpuid", &cpuid_str, NULL) < 0) + return -1; + + if (!cpuid_str) + return 0; + + if (!def->cpu) { + if (VIR_ALLOC(def->cpu) < 0) + goto cleanup; + def->cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH; + def->cpu->type = VIR_CPU_TYPE_GUEST; + def->cpu->nfeatures = 0; + def->cpu->nfeatures_max = 0; + } + + cpuid_pairs = virStringSplit(cpuid_str, ",", 0); + if (!cpuid_pairs) + goto cleanup; + + if (!cpuid_pairs[0]) { + ret = 0; + goto cleanup; + } + + if (STRNEQ(cpuid_pairs[0], "host")) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("cpuid starting with %s is not supported, only libxl format is"), + cpuid_pairs[0]); + goto cleanup; + } + + for (i = 1; cpuid_pairs[i]; i++) { + name_and_value = virStringSplit(cpuid_pairs[i], "=", 2); + if (!name_and_value) + goto cleanup; + if (!name_and_value[0] || !name_and_value[1]) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Invalid libxl cpuid key=value element: %s"), + cpuid_pairs[i]); + goto cleanup; + } + if (STREQ(name_and_value[1], "1")) { + policy = VIR_CPU_FEATURE_FORCE; + } else if (STREQ(name_and_value[1], "0")) { + policy = VIR_CPU_FEATURE_DISABLE; + } else if (STREQ(name_and_value[1], "x")) { + policy = VIR_CPU_FEATURE_OPTIONAL; + } else if (STREQ(name_and_value[1], "k")) { + policy = VIR_CPU_FEATURE_OPTIONAL; + } else if (STREQ(name_and_value[1], "s")) { + policy = VIR_CPU_FEATURE_OPTIONAL; + } else { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Invalid libxl cpuid value: %s"), + cpuid_pairs[i]); + goto cleanup; + } + + if (virCPUDefAddFeature(def->cpu, + xenTranslateCPUFeature(name_and_value[0], true), + policy) < 0) + goto cleanup; + + virStringListFree(name_and_value); + name_and_value = NULL; + } + + ret = 0; + + cleanup: + virStringListFree(name_and_value); + virStringListFree(cpuid_pairs); + return ret; +} + static int xenParseXLSpice(virConfPtr conf, virDomainDefPtr def) @@ -1089,6 +1174,9 @@ xenParseXL(virConfPtr conf, goto cleanup; #endif + if (xenParseXLCPUID(conf, def) < 0) + goto cleanup; + if (xenParseXLDisk(conf, def) < 0) goto cleanup; @@ -1231,6 +1319,79 @@ xenFormatXLOS(virConfPtr conf, virDomainDefPtr def) return 0; } +static int +xenFormatXLCPUID(virConfPtr conf, virDomainDefPtr def) +{ + char **cpuid_pairs = NULL; + char *cpuid_string = NULL; + size_t i, j; + int ret = -1; + + if (!def->cpu) + return 0; + + if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { + VIR_WARN("ignoring CPU mode '%s', only host-passthrough mode " + "is supported", virCPUModeTypeToString(def->cpu->mode)); + return 0; + } + + /* "host" + all features + NULL */ + if (VIR_ALLOC_N(cpuid_pairs, def->cpu->nfeatures + 2) < 0) + return -1; + + if (VIR_STRDUP(cpuid_pairs[0], "host") < 0) + goto cleanup; + + j = 1; + for (i = 0; i < def->cpu->nfeatures; i++) { + const char *feature_name = xenTranslateCPUFeature( + def->cpu->features[i].name, + false); + const char *policy = NULL; + + if (STREQ(feature_name, "vmx") || STREQ(feature_name, "svm")) + /* ignore vmx/svm in cpuid option, translated into nestedhvm + * elsewhere */ + continue; + + switch (def->cpu->features[i].policy) { + case VIR_CPU_FEATURE_FORCE: + case VIR_CPU_FEATURE_REQUIRE: + policy = "1"; + break; + case VIR_CPU_FEATURE_OPTIONAL: + policy = "x"; + break; + case VIR_CPU_FEATURE_DISABLE: + case VIR_CPU_FEATURE_FORBID: + policy = "0"; + break; + } + if (virAsprintf(&cpuid_pairs[j++], "%s=%s", + feature_name, + policy) < 0) + goto cleanup; + } + cpuid_pairs[j] = NULL; + + if (j > 1) { + cpuid_string = virStringListJoin((const char **)cpuid_pairs, ","); + if (!cpuid_string) + goto cleanup; + + if (xenConfigSetString(conf, "cpuid", cpuid_string) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + virStringListFree(cpuid_pairs); + VIR_FREE(cpuid_string); + return ret; +} + #ifdef LIBXL_HAVE_VNUMA static int xenFormatXLVnode(virConfValuePtr list, @@ -1996,6 +2157,9 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn) if (xenFormatXLOS(conf, def) < 0) goto cleanup; + if (xenFormatXLCPUID(conf, def) < 0) + goto cleanup; + #ifdef LIBXL_HAVE_VNUMA if (xenFormatXLDomainVnuma(conf, def) < 0) goto cleanup; -- git-series 0.9.1

On Wed, Mar 21, 2018 at 05:32:32PM +0100, Marek Marczykowski-Górecki wrote:
Only "libxl" format supported for now. Special care needed around vmx/svm, because those two are translated into "nestedhvm" setting.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes since v5: - adjust for ignoring mode=custom instead of rejecting it Changes since v3: - improve error reporting (VIR_ERR_CONF_SYNTAX) - ignore empty cpuid option - same as libxl - fix cleanup on error Changes since v2: - new patch --- src/xenconfig/xen_xl.c | 164 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 164 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Check conversion of "cpuid" setting, check all supported policy settings ("1", "0", "x"). Also, check interaction with "nestedhvm" - should not be included as "vmx=1" in "cpuid" setting. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- Changes since v3: - adjust for nested HVM enabled by just <cpu> element Changes since v2: - new patch --- tests/xlconfigdata/test-fullvirt-cpuid.cfg | 25 ++++++++++++++++- tests/xlconfigdata/test-fullvirt-cpuid.xml | 35 +++++++++++++++++++++++- tests/xlconfigtest.c | 1 +- 3 files changed, 61 insertions(+) create mode 100644 tests/xlconfigdata/test-fullvirt-cpuid.cfg create mode 100644 tests/xlconfigdata/test-fullvirt-cpuid.xml diff --git a/tests/xlconfigdata/test-fullvirt-cpuid.cfg b/tests/xlconfigdata/test-fullvirt-cpuid.cfg new file mode 100644 index 0000000..bb7b9c7 --- /dev/null +++ b/tests/xlconfigdata/test-fullvirt-cpuid.cfg @@ -0,0 +1,25 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +pae = 1 +acpi = 0 +apic = 0 +viridian = 0 +rtc_timeoffset = 0 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-system-i386" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +parallel = "none" +serial = "none" +builder = "hvm" +boot = "d" +nestedhvm = 1 +cpuid = "host,tm=0,sse3=1,page1gb=x" diff --git a/tests/xlconfigdata/test-fullvirt-cpuid.xml b/tests/xlconfigdata/test-fullvirt-cpuid.xml new file mode 100644 index 0000000..adc55d2 --- /dev/null +++ b/tests/xlconfigdata/test-fullvirt-cpuid.xml @@ -0,0 +1,35 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='xenfv'>hvm</type> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <pae/> + </features> + <cpu mode='host-passthrough'> + <feature policy='disable' name='tm'/> + <feature policy='force' name='pni'/> + <feature policy='optional' name='pdpe1gb'/> + </cpu> + <clock offset='variable' adjustment='0' basis='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-system-i386</emulator> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1'> + <listen type='address' address='127.0.0.1'/> + </graphics> + <video> + <model type='cirrus' vram='8192' heads='1' primary='yes'/> + </video> + </devices> +</domain> diff --git a/tests/xlconfigtest.c b/tests/xlconfigtest.c index 57a0a67..39f51e2 100644 --- a/tests/xlconfigtest.c +++ b/tests/xlconfigtest.c @@ -270,6 +270,7 @@ mymain(void) DO_TEST("fullvirt-multi-timer"); DO_TEST("fullvirt-nestedhvm"); DO_TEST("fullvirt-nestedhvm-disabled"); + DO_TEST("fullvirt-cpuid"); #ifdef LIBXL_HAVE_VNUMA DO_TEST("fullvirt-vnuma"); DO_TEST_PARSE("fullvirt-vnuma-autocomplete", false); -- git-series 0.9.1

On Wed, Mar 21, 2018 at 05:32:33PM +0100, Marek Marczykowski-Górecki wrote:
Check conversion of "cpuid" setting, check all supported policy settings ("1", "0", "x"). Also, check interaction with "nestedhvm" - should not be included as "vmx=1" in "cpuid" setting.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- Changes since v3: - adjust for nested HVM enabled by just <cpu> element Changes since v2: - new patch --- tests/xlconfigdata/test-fullvirt-cpuid.cfg | 25 ++++++++++++++++- tests/xlconfigdata/test-fullvirt-cpuid.xml | 35 +++++++++++++++++++++++- tests/xlconfigtest.c | 1 +- 3 files changed, 61 insertions(+) create mode 100644 tests/xlconfigdata/test-fullvirt-cpuid.cfg create mode 100644 tests/xlconfigdata/test-fullvirt-cpuid.xml
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé
-
Jim Fehlig
-
Marek Marczykowski-Górecki