Re: [libvirt] [PATCH 07/10] libxl: add support for PVH

On Mon, Sep 10, 2018 at 03:44:33PM -0600, Jim Fehlig wrote: > On 08/05/2018 03:48 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; >> --- >> docs/formatcaps.html.in | 4 +-- >> docs/schemas/domaincommon.rng | 1 +- >> src/libxl/libxl_capabilities.c | 23 ++++++++++++++++++-- >> src/libxl/libxl_conf.c | 41 ++++++++++++++++++++++++++++++----- >> src/libxl/libxl_driver.c | 6 +++-- >> 5 files changed, 64 insertions(+), 11 deletions(-) >> >> diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in >> index 34a74a9..1f17aa9 100644 >> --- a/docs/formatcaps.html.in >> +++ b/docs/formatcaps.html.in >> @@ -104,8 +104,8 @@ >> <dt><code>machine</code></dt><dd>Machine type, for use in >> <a href="formatdomain.html#attributeOSTypeMachine">machine</a> >> attribute of os/type element in domain XML. For example Xen >> - supports <code>xenfv</code> for HVM or <code>xenpv</code> for >> - PV.</dd> >> + supports <code>xenfv</code> for HVM, <code>xenpv</code> for >> + PV, or <code>xenpvh</code> for PVHv2.</dd> >> <dt><code>domain</code></dt><dd>Supported domain type, named by the >> <code>type</code> attribute.</dd> >> </dl> >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >> index eded1ca..d32b0cb 100644 >> --- a/docs/schemas/domaincommon.rng >> +++ b/docs/schemas/domaincommon.rng >> @@ -341,6 +341,7 @@ >> <choice> >> <value>xenpv</value> >> <value>xenfv</value> >> + <value>xenpvh</value> >> </choice> >> </attribute> >> </optional> >> diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c >> index 18596c7..e7b26f1 100644 >> --- a/src/libxl/libxl_capabilities.c >> +++ b/src/libxl/libxl_capabilities.c >> @@ -57,6 +57,7 @@ struct guest_arch { >> virArch arch; >> int bits; >> int hvm; >> + int pvh; >> int pae; >> int nonpae; >> int ia64_be; >> @@ -491,13 +492,29 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> guest_archs[i].nonpae = nonpae; >> if (ia64_be) >> guest_archs[i].ia64_be = ia64_be; >> + >> + /* On Xen >= 4.9 add PVH for each HVM guest, and do it only once */ > > I'm having problems understanding this. Do you mean add a PVH for each > supported HVM arch, but exclude PAE? E.g. standard xen_caps on x86_64 > contains > > xen-3.0-x86_64 xen-3.0-x86_32p hvm-3.0-x86_32 hvm-3.0-x86_32p hvm-3.0-x86_64 > > Given these caps, should a PVH be added that corresponds to the > hvm-3.0-x86_32 cap and another for the hvm-3.0-x86_64 cap, but the > hvm-3.0-x86_32p cap excluded? Yes, exactly. Setting PAE (or not) is possible only for HVM, but not PVH. It would be much better if Xen would report support for PVH explicitly... >> + if ((ver_info->xen_version_major > 4 || >> + (ver_info->xen_version_major == 4 && >> + ver_info->xen_version_minor >= 9)) && >> + 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].pvh = 1; >> + } > > Without answers to the above questions, I can't really comment on this code. > Regardless, since PVH is not advertised in xen_caps shouldn't it be added to > guest_archs outside of the loop parsing xen_caps? This works on assumption that if you have HVM and new enough Xen, then you have PVH. Just having new Xen isn't enough - for example the hardware may lack VT-x. >> } >> } >> 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].hvm ? "xenfv" : >> + (guest_archs[i].pvh ? "xenpvh" : "xenpv")}; >> virCapsGuestMachinePtr *machines; >> if ((machines = virCapabilitiesAllocMachines(xen_machines, 1)) == NULL) >> @@ -557,7 +574,9 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> 1, >> 0) == NULL) >> return -1; >> + } >> + if (guest_archs[i].hvm || guest_archs[i].pvh) { >> if (virCapabilitiesAddGuestFeature(guest, >> "hap", >> 1, >> @@ -580,7 +599,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 f3da0ed..2df40ec 100644 >> --- a/src/libxl/libxl_conf.c >> +++ b/src/libxl/libxl_conf.c >> @@ -133,8 +133,11 @@ libxlMakeDomCreateInfo(libxl_ctx *ctx, >> libxl_domain_create_info_init(c_info); >> - if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { >> - c_info->type = LIBXL_DOMAIN_TYPE_HVM; >> + if (def->os.type == VIR_DOMAIN_OSTYPE_HVM || >> + (def->os.type == VIR_DOMAIN_OSTYPE_XEN && >> + STREQ(def->os.machine, "xenpvh"))) { >> + c_info->type = def->os.type == VIR_DOMAIN_OSTYPE_HVM ? >> + LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PVH; >> switch ((virTristateSwitch) def->features[VIR_DOMAIN_FEATURE_HAP]) { >> case VIR_TRISTATE_SWITCH_OFF: >> libxl_defbool_set(&c_info->hap, false); >> @@ -276,7 +279,8 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, >> virDomainClockDef clock = def->clock; >> libxl_ctx *ctx = cfg->ctx; >> libxl_domain_build_info *b_info = &d_config->b_info; >> - int hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; >> + bool hvm = def->os.type == VIR_DOMAIN_OSTYPE_HVM; >> + bool pvh = STREQ(def->os.machine, "xenpvh"); >> size_t i; >> size_t nusbdevice = 0; >> @@ -284,6 +288,8 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, >> if (hvm) >> libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM); >> + else if (pvh) >> + libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH); >> else >> libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV); >> @@ -375,7 +381,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, >> def->mem.cur_balloon = VIR_ROUND_UP(def->mem.cur_balloon, 1024); >> b_info->max_memkb = virDomainDefGetMemoryInitial(def); >> b_info->target_memkb = def->mem.cur_balloon; >> - if (hvm) { >> + if (hvm || pvh) { >> if (caps && >> def->cpu && def->cpu->mode == (VIR_CPU_MODE_HOST_PASSTHROUGH)) { >> bool hasHwVirt = false; >> @@ -647,6 +653,31 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, >> return -1; >> } >> #endif >> + } else if (pvh) { >> +#ifdef LIBXL_HAVE_BUILDINFO_KERNEL >> + if (VIR_STRDUP(b_info->cmdline, def->os.cmdline) < 0) >> + return -1; >> + if (VIR_STRDUP(b_info->kernel, def->os.kernel) < 0) >> + return -1; >> + if (VIR_STRDUP(b_info->ramdisk, def->os.initrd) < 0) >> + return -1; >> +#else >> + /* >> + * Shouldn't happen as LIBXL_HAVE_BUILDINFO_KERNEL is there since Xen >> + * 4.5, but PVHv2 since 4.9. >> + */ >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("PVH guest type not supported")); >> +#endif > > I guess this is needed else the build will fail on Xen < 4.5? Yes, exactly. > Maybe it is > time to bump the minimum supported Xen version to 4.6 :-). I say that a bit > jokingly, but I did propose it a few months back. IMO good idea, since Xen < 4.6 is not supported anymore.