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

On Tue, Oct 02, 2018 at 04:50:02PM -0600, Jim Fehlig wrote: > On 9/30/18 8:15 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; >> --- >> Changes in v2 proposed by Jim: >> - use new_arch_added var instead of i == nr_guest_archs for clarity >> - improve comment >> - adjust for now required Xen >= 4.6 (remove part for Xen < 4.5) >> >> Changes in v3: >> - limit VIR_DOMAIN_OSTYPE_XEN -> VIR_DOMAIN_OSTYPE_LINUX conversion to >> Xen PV only >> - do not accept VIR_DOMAIN_OSTYPE_LINUX for PVH >> - fix reported capabilities for PVH - remove hostdev passthrough and >> video/graphics > > No video, graphics or hostdev passthrough - bummer. Begs the question: what > to do with PVH XML config containing these devices? Reject it? Silently > ignore? I'll also need to remember to enable these as PVH gains support for > the devices. This is based on assumption that by design there is no QEMU, so there is nothing that could emulate VGA. But actually I'm not so sure about it right now - because from libxl side it is the same as for PV, and actually I see some code to start qemu for PV domains. I'm not sure if it's capable of emulating VGA, but probably can do vfb+vkb backend and expose it as VNC. Hostdev passthrough will be there one day... >> - use #ifdef LIBXL_DOMAIN_TYPE_PVH instead of hypervisor version to >> check for PVH support > > This is a much better approach than the version check. I should have thought > of that earlier, sorry. > >> - compile fix for Xen < 4.9 >> --- >> docs/formatcaps.html.in | 4 +-- >> docs/schemas/domaincommon.rng | 1 +- >> src/conf/domain_conf.c | 6 ++-- >> src/libxl/libxl_capabilities.c | 47 ++++++++++++++++++++++++++++----- >> src/libxl/libxl_conf.c | 50 ++++++++++++++++++++++++++++++----- >> src/libxl/libxl_driver.c | 6 ++-- >> 6 files changed, 95 insertions(+), 19 deletions(-) >> >> diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in >> index 0d9c53d..b8102fd 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> > > s/PVHv2/PVH/ > >> <dt><code>domain</code></dt><dd>The <code>type</code> attribute of >> this element specifies the type of hypervisor required to run the >> domain. Use in <a href="formatdomain.html#attributeDomainType">type</a> >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >> index 099a949..820a7c6 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/conf/domain_conf.c b/src/conf/domain_conf.c >> index 9911d56..a945ad4 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -19094,7 +19094,8 @@ virDomainDefParseCaps(virDomainDefPtr def, >> * be 'xen'. So we accept the former and convert >> */ >> if (def->os.type == VIR_DOMAIN_OSTYPE_LINUX && >> - def->virtType == VIR_DOMAIN_VIRT_XEN) { >> + def->virtType == VIR_DOMAIN_VIRT_XEN && >> + (!def->os.machine || STREQ(def->os.machine, "xenpv"))) { >> def->os.type = VIR_DOMAIN_OSTYPE_XEN; >> } >> @@ -27705,7 +27706,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, >> * be 'xen'. So we convert to the former for backcompat >> */ >> if (def->virtType == VIR_DOMAIN_VIRT_XEN && >> - def->os.type == VIR_DOMAIN_OSTYPE_XEN) >> + def->os.type == VIR_DOMAIN_OSTYPE_XEN && >> + (!def->os.machine || STREQ(def->os.machine, "xenpv"))) >> virBufferAsprintf(buf, ">%s</type>\n", >> virDomainOSTypeToString(VIR_DOMAIN_OSTYPE_LINUX)); >> else >> diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c >> index 18596c7..be51a4d 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; >> @@ -439,6 +440,9 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> int hvm = STRPREFIX(&token[subs[1].rm_so], "hvm"); >> virArch arch; >> int pae = 0, nonpae = 0, ia64_be = 0; >> +#ifdef LIBXL_DOMAIN_TYPE_PVH >> + bool new_arch_added = false; >> +#endif >> if (STRPREFIX(&token[subs[2].rm_so], "x86_32")) { >> arch = VIR_ARCH_I686; >> @@ -475,8 +479,12 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) >> if (i >= ARRAY_CARDINALITY(guest_archs)) >> continue; >> /* Didn't find a match, so create a new one */ >> - if (i == nr_guest_archs) >> + if (i == nr_guest_archs) { >> nr_guest_archs++; >> +#ifdef LIBXL_DOMAIN_TYPE_PVH >> + new_arch_added = true; >> +#endif >> + } > > My idea to add this variable for clarity now seems diluted by the need for > the extra ifdef's :-/. Your original approach would avoid them. This #ifdef is only to mute compiler warning (variable set but unused). If you have a better idea for that, I'd very much like to drop #ifdef here.