On 10/2/18 5:02 PM, Marek Marczykowski-Górecki wrote:
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 <marma...@invisiblethingslab.com>
---
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.

Given the ifdef trickery needed with the extra variable, I think the better idea is your original idea :-)

i == nr_guest_archs - 1

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to