xen: explicitly set hostdev driver.name at runtime, not in postparse

Xen only supports a single type of PCI hostdev assignment, so it is
superfluous to have <driver name='xen'/> peppered throughout the
config. It *is* necessary to have the driver type explicitly set in
the hostdev object before calling into the hypervisor-agnostic "hostdev
manager" though (otherwise the hostdev manager doesn't know whether it
should do Xen-specific setup, or VFIO-specific setup).

Historically, the Xen driver has checked for "default" driver name
(i.e. not set in the XML), and set it to "xen', during the XML
postparse, thus guaranteeing that it will be set by the time the
object is sent to the hostdev manager at runtime, but also setting it
so early that a simple round-trip of parse-format results in the XML
always containing an explicit <driver name='xen'/>, even if that
wasn't specified in the original XML.

The QEMU driver *doesn't* set driver.name during postparse though;
instead, it waits until domain startup time (or device attach time for
hotplug), and sets the driver.name then. The result is that a
parse-format round trip of the XML in the QEMU driver *doesn't* add in
the <driver name='vfio'/>.

This patch modifies the Xen driver to behave similarly to the QEMU
driver - the PostParse just checks for a driver.name that isn't
supported by the Xen driver, and any explicit setting to "xen" is
deferred until domain runtime rather than during the postparse, thus
Xen domain XML also doesn't get extraneous <driver name='xen'/>.

This delayed setting of driver.name of course results in slightly
different xml2xml parse-format results, so the unit test data is
modified accordingly.

Signed-off-by: Laine Stump <laine@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 1a7de00704375ed320d900d39ec57d48e3b448e4..670fd2881eb43deecb39dfaf3e3a6a91a668d41c 100644 (file)
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -160,8 +160,15 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev,
 
         if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
             hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
-            pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT)
-            pcisrc->driver.name = VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN;
+            pcisrc->driver.name != VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT &&
+            pcisrc->driver.name != VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN) {
+
+            /* Xen only supports "Xen" style of hostdev */
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("XEN does not support device assignment mode '%1$s'"),
+                           virDeviceHostdevPCIDriverNameTypeToString(pcisrc->driver.name));
+            return -1;
+        }
     }
 
     if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) {
@@ -986,18 +993,9 @@ libxlNetworkPrepareDevices(virDomainDef *def)
         if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV &&
             net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
             /* Each type='hostdev' network device must also have a
-             * corresponding entry in the hostdevs array. For netdevs
-             * that are hardcoded as type='hostdev', this is already
-             * done by the parser, but for those allocated from a
-             * network / determined at runtime, we need to do it
-             * separately.
+             * corresponding entry in the hostdevs array.
              */
             virDomainHostdevDef *hostdev = virDomainNetGetActualHostdev(net);
-            virDomainHostdevSubsysPCI *pcisrc = &hostdev->source.subsys.u.pci;
-
-            if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-                hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
-                pcisrc->driver.name = VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN;
 
             if (virDomainHostdevInsert(def, hostdev) < 0)
                 return -1;
@@ -1007,6 +1005,54 @@ libxlNetworkPrepareDevices(virDomainDef *def)
     return 0;
 }
 
+
+static int
+libxlHostdevPrepareDevices(virDomainDef *def)
+{
+    size_t i;
+
+    for (i = 0; i < def->nhostdevs; i++) {
+        virDomainHostdevDef *hostdev = def->hostdevs[i];
+        virDomainHostdevSubsysPCI *pcisrc = &hostdev->source.subsys.u.pci;
+
+        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+            hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI &&
+            pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT) {
+
+
+            pcisrc->driver.name = VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN;
+
+            /* Q: Why do we set an explicit driver.name here even
+             * though Xen (currently) only supports one driver.name for
+             * PCI hostdev assignment?
+             *
+             * A: Setting the driver name *in the domain XML config*
+             * is optional (and actually pointless at the time of
+             * writing, since each hypervisor only supports a single
+             * type of PCI hostdev device assignment). But the
+             * hypervisor-agnostic virHostdevPrepareDomainDevices(),
+             * which is called immediately after this function in
+             * order to do any driver-related setup (e.g. bind the
+             * host device to a driver kernel driver), requires that
+             * the driver.name be explicitly set (otherwise it wouldn't
+             * know whether to bind to the Xen driver or VFIO driver,
+             * for example).
+             *
+             * NB: If there are ever multiple types of device
+             * assignment supported by Xen, DO NOT CHANGE the value of
+             * driver.name set above when the config is "default" to
+             * anything other than "xen", unless the guest ABI of the
+             * new type is compatible with that of current "xen"
+             * device assignment.
+             *
+             */
+        }
+    }
+
+    return 0;
+}
+
+
 static void
 libxlConsoleCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
 {
@@ -1174,6 +1220,9 @@ libxlDomainStartPrepare(libxlDriverPrivate *driver,
     if (libxlNetworkPrepareDevices(vm->def) < 0)
         goto error;
 
+    if (libxlHostdevPrepareDevices(vm->def) < 0)
+        goto error;
+
     if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_INTERNAL_NAME,
                                        vm->def, hostdev_flags) < 0)
         goto error;

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 29fbc823ddab6832ca7a0cd6bce46b77cc18f409..6c843b9054e25422313fccdc8b1067fc6bab694e 100644 (file)
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3087,6 +3087,22 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivate *driver,
 
     VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1);
 
+    /* The only supported driverType for Xen is
+     * VIR_DOMAIN_HOSTDEV_PCI_DRIVER_TYPE_XEN, which normally isn't
+     * set in the config (because it doesn't need to be), but it does
+     * need to be set for the impending call to
+     * virHostdevPreparePCIDevices()
+     */
+    if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT)
+        pcisrc->driver.name = VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN;
+
+    if (pcisrc->driver.name != VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("XEN does not support device assignment mode '%1$s'"),
+                       virDeviceHostdevPCIDriverNameTypeToString(pcisrc->driver.name));
+        goto cleanup;
+    }
+
     if (virHostdevPreparePCIDevices(hostdev_mgr, LIBXL_DRIVER_INTERNAL_NAME,
                                     vm->def->name, vm->def->uuid,
                                     &hostdev, 1, 0) < 0)
@@ -3395,15 +3411,6 @@ libxlDomainAttachNetDevice(libxlDriverPrivate *driver,
 
     if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
         virDomainHostdevDef *hostdev = virDomainNetGetActualHostdev(net);
-        virDomainHostdevSubsysPCI *pcisrc = &hostdev->source.subsys.u.pci;
-
-        /* For those just allocated from a network pool whose driver type is
-         * still VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_DEFAULT, we need to set
-         * driver name correctly.
-         */
-        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
-            hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
-            pcisrc->driver.name = VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_XEN;
 
         /* This is really a "smart hostdev", so it should be attached
          * as a hostdev (the hostdev code will reach over into the

diff --git a/tests/libxlxml2domconfigdata/moredevs-hvm.xml b/tests/libxlxml2domconfigdata/moredevs-hvm.xml
index 89ad80631d9fb4572f527d028156375150da72bb..64eeeff889e506dceb9787b3bf4f958d669f2408 100644 (file)
--- a/tests/libxlxml2domconfigdata/moredevs-hvm.xml
+++ b/tests/libxlxml2domconfigdata/moredevs-hvm.xml
@@ -43,7 +43,6 @@
     </interface>
     <interface type='hostdev' managed='yes'>
       <mac address='00:16:3e:2e:e7:fc'/>
-      <driver name='xen'/>
       <source>
         <address type='pci' domain='0x0000' bus='0x0a' slot='0x10' function='0x0'/>
       </source>

diff --git a/tests/xlconfigdata/test-fullvirt-pci.xml b/tests/xlconfigdata/test-fullvirt-pci.xml
index 75aa6ac25bf0b70e2a3eaee9bc52d26290c58980..6826d14b9e1178188c22464ab1510441d6ffd09d 100644 (file)
--- a/tests/xlconfigdata/test-fullvirt-pci.xml
+++ b/tests/xlconfigdata/test-fullvirt-pci.xml
@@ -37,13 +37,11 @@
       <model type='cirrus' vram='8192' heads='1' primary='yes'/>
     </video>
     <hostdev mode='subsystem' type='pci' managed='no'>
-      <driver name='xen'/>
       <source>
         <address domain='0x0000' bus='0x01' slot='0x1a' function='0x1'/>
       </source>
     </hostdev>
     <hostdev mode='subsystem' type='pci' managed='no'>
-      <driver name='xen'/>
       <source writeFiltering='no'>
         <address domain='0x0000' bus='0x02' slot='0x00' function='0x0'/>
       </source>

diff --git a/tests/xmconfigdata/test-pci-dev-syntax.xml b/tests/xmconfigdata/test-pci-dev-syntax.xml
index 5d5d29c61c119c9f563fb4dab60dda7ddfedd5ca..1d5d8570721d07bb21aff29b3d5f04297a2c2833 100644 (file)
--- a/tests/xmconfigdata/test-pci-dev-syntax.xml
+++ b/tests/xmconfigdata/test-pci-dev-syntax.xml
@@ -55,13 +55,11 @@
       <model type='cirrus' vram='8192' heads='1' primary='yes'/>
     </video>
     <hostdev mode='subsystem' type='pci' managed='no'>
-      <driver name='xen'/>
       <source>
         <address domain='0x0001' bus='0x0c' slot='0x1b' function='0x2'/>
       </source>
     </hostdev>
     <hostdev mode='subsystem' type='pci' managed='no'>
-      <driver name='xen'/>
       <source>
         <address domain='0x0000' bus='0x01' slot='0x13' function='0x0'/>
       </source>

diff --git a/tests/xmconfigdata/test-pci-devs.xml b/tests/xmconfigdata/test-pci-devs.xml
index 5d5d29c61c119c9f563fb4dab60dda7ddfedd5ca..1d5d8570721d07bb21aff29b3d5f04297a2c2833 100644 (file)
--- a/tests/xmconfigdata/test-pci-devs.xml
+++ b/tests/xmconfigdata/test-pci-devs.xml
@@ -55,13 +55,11 @@
       <model type='cirrus' vram='8192' heads='1' primary='yes'/>
     </video>
     <hostdev mode='subsystem' type='pci' managed='no'>
-      <driver name='xen'/>
       <source>
         <address domain='0x0001' bus='0x0c' slot='0x1b' function='0x2'/>
       </source>
     </hostdev>
     <hostdev mode='subsystem' type='pci' managed='no'>
-      <driver name='xen'/>
       <source>
         <address domain='0x0000' bus='0x01' slot='0x13' function='0x0'/>
       </source>