On Wed, Oct 10, 2018 at 11:06:38AM +0200, Michal Privoznik wrote:
> 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@xxxxxxxxxxxxxxxxxxxxxx>
> >>> ---
> >>> 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?
Sigh, yes, of course.
>
> > 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/.
So we should move the hack to the post-parse callbacks in the xen
driver - but we don't have an equivalent pre-format callback for
the reverse hack, do we ?
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
