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

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.

> - 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.

The rest of the patch looks good, but before going further let's get a resolution to the VIR_DOMAIN_OSTYPE_XENPVH vs os.machine approach to modeling PVH. I can do some poking on IRC if there are no responses to this thread.