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

On 10/10/2018 10:45 AM, Daniel P. Berrangé wrote: > On Tue, Oct 09, 2018 at 04:45:01PM -0600, Jim Fehlig wrote: >> On 10/7/18 9:39 AM, 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 >>> - use #ifdef LIBXL_DOMAIN_TYPE_PVH instead of hypervisor version to >>> check for PVH support >>> - compile fix for Xen < 4.9 >>> >>> Changes in v4: >>> - revert to "i == nr_guest_archs-1" check >>> - let configure check for LIBXL_DOMAIN_TYPE_PVH and use #ifdef >>> HAVE_XEN_PVH then >>> - fix comment about Xen version >>> --- >>> docs/formatcaps.html.in | 4 +-- >>> docs/schemas/domaincommon.rng | 1 +- >>> m4/virt-driver-libxl.m4 | 3 ++- >>> src/conf/domain_conf.c | 6 ++-- >>> src/libxl/libxl_capabilities.c | 38 ++++++++++++++++++++++----- >>> src/libxl/libxl_conf.c | 50 ++++++++++++++++++++++++++++++----- >>> src/libxl/libxl_driver.c | 6 ++-- >>> 7 files changed, 90 insertions(+), 18 deletions(-) >>> >>> diff --git a/docs/formatcaps.html.in b/docs/formatcaps.html.in >>> index 0d9c53d..fe113bc 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 PVH.</dd> >>> <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/m4/virt-driver-libxl.m4 b/m4/virt-driver-libxl.m4 >>> index 479d911..2cd97cc 100644 >>> --- a/m4/virt-driver-libxl.m4 >>> +++ b/m4/virt-driver-libxl.m4 >>> @@ -75,6 +75,9 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_LIBXL], [ >>> ]) >>> fi >>> + dnl Check if Xen has support for PVH >>> + AC_CHECK_DECL(LIBXL_DOMAIN_TYPE_PVH, [AC_DEFINE([HAVE_XEN_PVH], [1], [Define to 1 if Xen has PVH support.])], [], [#include <libxl.h>]) >>> + >>> AC_SUBST([LIBXL_CFLAGS]) >>> AC_SUBST([LIBXL_LIBS]) >>> ]) >>> 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 >> >> I'd still like to hear what others think of extending the hack. mprivozn? >> You often have an interesting opinion :-). > > IIUC, this means that we get this behaviour: > > Input Output > > type=linux, machine=NULL type=linux, machine=NULL > type=linux, machine=xenpv type=linux, machine=xenpvh I think we would get type=xen, machine=xenpv, wouldn't we?

> type=xen, machine=NULL type=linux, machine=NULL > type=xen, machine=xenpv type=linux, machine=xenpv > type=xen, machine=xenpvh type=xen, machine=xenpvh > > And one combo not allowed > > type=linux, machine=xenpvh > > I can understand the motivation behind this change, but I wonder if it is > worth it or not to have the difference in behaviour. I'm quite torn. I think we can do this. xenpvh falls into xen world and not Linux. IIRC the question in previous versions was if it is safe to make decisions based on machine type in post parse callbacks. Short answer is yes. The longer answer is yes, but in driver specific callbacks. I view code under src/conf/ to be hypervisor agnostic (at least it should be) and thus any hypervisor specific decisions/changes to user XML should be made from src/hypervisor/.