Re: [libvirt] [PATCH 1/2] libxl: add support for PVH

On Fri, Sep 16, 2016 at 04:39:23PM -0600, Jim Fehlig wrote: > On 08/05/2016 12:05 PM, Marek Marczykowski-Górecki wrote: >> Since this is something between PV and HVM, it makes sense to put the >> setting in place where domain type is specified. >> To enable it, use <os><type machine="xenpvh">...</type></os>. It is >> also included in capabilities.xml, for every supported HVM guest type - it >> doesn't seems to be any other requirement (besides new enough Xen). >> >> Signed-off-by: Marek Marczykowski-Górecki <marmarek(a)invisiblethingslab.com&gt; >> --- >> src/libxl/libxl_capabilities.c | 40 +++++++++++++++++++++++++++++++--------- >> src/libxl/libxl_conf.c | 2 ++ >> src/libxl/libxl_driver.c | 6 ++++-- >> 3 files changed, 37 insertions(+), 11 deletions(-) > I didn't investigate, but this patch did not apply cleanly. > > Does 'xenpvh' need to be added to docs/schema/domaincommon.rng? The schema looks > dated anyhow since it currently contains 'xenpv' and 'xenner'. And perhaps this > value should be added to docs/formatdomain.html.in, along with a sentence about > the possible values for Xen machines. After further evaluation[1], PVHv1 is not the thing I wanted here. And PVHv2 is going to be significantly different. While this patch do work for me, I'm not going to spend more time on PVHv1.

>> diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c >> index 0145116..c443353 100644 >> --- a/src/libxl/libxl_capabilities.c >> +++ b/src/libxl/libxl_capabilities.c >> @@ -45,11 +45,16 @@ VIR_LOG_INIT("libxl.libxl_capabilities"); >> /* see xen-unstable.hg/xen/include/asm-x86/cpufeature.h */ >> #define LIBXL_X86_FEATURE_PAE_MASK 0x40 >> >> +enum machine_type { >> + machine_hvm, >> + machine_pvh, >> + machine_pv, >> +}; >> >> struct guest_arch { >> virArch arch; >> int bits; >> - int hvm; >> + enum machine_type machine; >> int pae; >> int nonpae; >> int ia64_be; >> @@ -296,7 +301,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> /* Search for existing matching (model,hvm) tuple */ >> for (i = 0; i < nr_guest_archs; i++) { >> if ((guest_archs[i].arch == arch) && >> - guest_archs[i].hvm == hvm) >> + guest_archs[i].machine == (hvm ? machine_hvm : machine_pv)) >> break; >> } >> >> @@ -308,7 +313,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> nr_guest_archs++; >> >> guest_archs[i].arch = arch; >> - guest_archs[i].hvm = hvm; >> + guest_archs[i].machine = hvm ? machine_hvm : machine_pv; >> >> /* Careful not to overwrite a previous positive >> setting with a negative one here - some archs >> @@ -320,23 +325,40 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> guest_archs[i].nonpae = nonpae; >> if (ia64_be) >> guest_archs[i].ia64_be = ia64_be; >> + >> + /* On Xen >= 4.4 add PVH for each HVM guest, and do it only once */ >> + if ((ver_info->xen_version_major > 4 || >> + (ver_info->xen_version_major == 4 && >> + ver_info->xen_version_minor >= 4)) && >> + hvm && i == nr_guest_archs-1) { >> + i = nr_guest_archs; >> + /* Too many arch flavours - highly unlikely ! */ >> + if (i >= ARRAY_CARDINALITY(guest_archs)) >> + continue; >> + nr_guest_archs++; >> + guest_archs[i].arch = arch; >> + guest_archs[i].machine = machine_pvh; >> + } >> } >> } >> regfree(&regex); >> >> for (i = 0; i < nr_guest_archs; ++i) { >> virCapsGuestPtr guest; >> - char const *const xen_machines[] = {guest_archs[i].hvm ? "xenfv" : "xenpv"}; >> + char const *const xen_machines[] = { >> + guest_archs[i].machine == machine_hvm ? "xenfv" : >> + (guest_archs[i].machine == machine_pvh ? "xenpvh" : "xenpv")}; >> virCapsGuestMachinePtr *machines; >> >> if ((machines = virCapabilitiesAllocMachines(xen_machines, 1)) == NULL) >> return -1; >> >> if ((guest = virCapabilitiesAddGuest(caps, >> - guest_archs[i].hvm ? VIR_DOMAIN_OSTYPE_HVM : VIR_DOMAIN_OSTYPE_XEN, >> + guest_archs[i].machine == machine_hvm ? >> + VIR_DOMAIN_OSTYPE_HVM : VIR_DOMAIN_OSTYPE_XEN, > Is a new VIR_DOMAIN_OSTYPE_XENPVH needed? Not sure about this. Wouldn't that require adding `os.type == VIR_DOMAIN_OSTYPE_XEN || os.type == VIR_DOMAIN_OSTYPE_XENPVH` in a lot of places? If actual settings are mostly the same, I don't see any reason for introducing such value.

>> guest_archs[i].arch, >> LIBXL_EXECBIN_DIR "/qemu-system-i386", >> - (guest_archs[i].hvm ? >> + (guest_archs[i].machine == machine_hvm ? >> LIBXL_FIRMWARE_DIR "/hvmloader" : >> NULL), >> 1, >> @@ -375,7 +397,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> 0) == NULL) >> return -1; >> >> - if (guest_archs[i].hvm) { >> + if (guest_archs[i].machine != machine_pv) { >> if (virCapabilitiesAddGuestFeature(guest, >> "acpi", >> 1, >> @@ -390,7 +412,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> if (virCapabilitiesAddGuestFeature(guest, >> "hap", >> 1, >> - 1) == NULL) >> + guest_archs[i].machine == machine_hvm) == NULL) >> return -1; >> } >> } >> @@ -409,7 +431,7 @@ libxlMakeDomainOSCaps(const char *machine, >> >> os->supported = true; >> >> - if (STREQ(machine, "xenpv")) >> + if (STREQ(machine, "xenpv") || STREQ(machine, "xenpvh")) >> return 0; >> >> capsLoader->supported = true; >> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c >> index 5202ca1..aa06586 100644 >> --- a/src/libxl/libxl_conf.c >> +++ b/src/libxl/libxl_conf.c >> @@ -173,6 +173,8 @@ libxlMakeDomCreateInfo(libxl_ctx *ctx, >> } >> } else { >> c_info->type = LIBXL_DOMAIN_TYPE_PV; >> + if (STREQ(def->os.machine, "xenpvh")) >> + libxl_defbool_set(&c_info->pvh, true); > I assume this won't change with HVMlite, aka pvh2? It will, unfortunately. HVMlite is enabled by setting device model to none. >> } >> >> if (VIR_STRDUP(c_info->name, def->name) < 0) >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c >> index 4957072..fa58346 100644 >> --- a/src/libxl/libxl_driver.c >> +++ b/src/libxl/libxl_driver.c >> @@ -6321,9 +6321,11 @@ libxlConnectGetDomainCapabilities(virConnectPtr conn, >> emulatorbin = "/usr/bin/qemu-system-x86_64"; >> >> if (machine) { >> - if (STRNEQ(machine, "xenpv") && STRNEQ(machine, "xenfv")) { >> + if (STRNEQ(machine, "xenpv") && >> + STRNEQ(machine, "xenpvh") && >> + STRNEQ(machine, "xenfv")) { >> virReportError(VIR_ERR_INVALID_ARG, "%s", >> - _("Xen only supports 'xenpv' and 'xenfv' machines")); >> + _("Xen only supports 'xenpv', 'xenpvh' and 'xenfv' machines")); >> goto cleanup; >> } >> } else { > WRT domain capabilities, should pvh be treated like pv? I.e. do they both have > the same max vcpus, etc? Yes, PVH behave like PV. But PVHv2 like HVM.

> Also, supporting a new knob in the XML usually means supporting conversion of > that knob to xl.cfg. Can you add domXML <-> xl.cfg conversion for pvh? And a > test case for the conversion too please? I'll add this for PVHv2... [1] http://markmail.org/message/c7o7qsc3chkigdzv