[Qemu-devel] [PATCH 3/9] Add new domain device: "controller"

[Wolfgang Mauerer]

This augments virDomainDevice with a <controller> element
that is used to represent disk controllers (e.g., scsi
controllers). The XML format is given by

<controller type="scsi" id="<my_id>">
   <bus addr="<Domain>:<Bus>:<Slot>">
</controller>

where type denotes the disk interface (scsi, ide,...), id
is an arbitrary string that identifies the controller for
disk hotadd/remove, and bus addr denotes the controller address
on the PCI bus.

The bus element can be omitted; in this case,
an address will be assigned automatically. Currently,
only hotplugging scsi controllers on a PCI bus
is supported, and this only for qemu guests

Routines for parsing this definition and the associated data
structures are included in this commit.

Signed-off-by: Wolfgang Mauerer <address@hidden>
Signed-off-by: Jan Kiszka <address@hidden>
---
 docs/schemas/domain.rng |  145 ++++++++++++++++++++++++++++++++++-------------
 src/domain_conf.c       |  125 ++++++++++++++++++++++++++++++++++++++++
 src/domain_conf.h       |   24 ++++++++
 3 files changed, 255 insertions(+), 39 deletions(-)

diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
index 70e98a7..dc9b849 100644
--- a/docs/schemas/domain.rng
+++ b/docs/schemas/domain.rng
@@ -364,49 +364,116 @@
   <define name="disk">
     <element name="disk">
       <optional>
-        <attribute name="device">
+       <attribute name="device">
+         <choice>
+           <value>floppy</value>
+           <value>disk</value>
+           <value>cdrom</value>
+         </choice>
+       </attribute>
+      </optional>
+      <optional>
+       <element name="controller">
+         <optional>
+           <attribute name="bus">
+             <ref name="unsignedInt"/>
+           </attribute>
+         </optional>
+         <optional>
+           <attribute name="unit">
+             <ref name="unsignedInt"/>
+           </attribute>
+         </optional>
+         <choice>
+           <attribute name="id">
+             <ref name="genericName"/>
+           </attribute>
+           <attribute name="pciaddr">
+             <!-- Just for testing -->
+             <ref name="deviceName"/>
+           </attribute>
+         </choice>
+       </element>
+      </optional>
+      <choice>
+       <group>
+         <attribute name="type">
+           <value>file</value>
+         </attribute>
+         <interleave>
+           <optional>
+             <element name="source">
+               <attribute name="file">
+                 <ref name="absFilePath"/>
+               </attribute>
+               <empty/>
+             </element>
+           </optional>
+           <ref name="diskspec"/>
+         </interleave>
+       </group>
+       <group>
+         <attribute name="type">
+           <value>block</value>
+         </attribute>
+         <interleave>
+           <optional>
+             <element name="source">
+               <attribute name="dev">
+                 <ref name="deviceName"/>
+               </attribute>
+               <empty/>
+             </element>
+           </optional>
+           <ref name="diskspec"/>
+         </interleave>
+       </group>
+       <ref name="diskspec"/>
+      </choice>
+    </element>
+  </define>
+  <define name="target">
+    <element name="target">
+      <attribute name="dev">
+        <ref name="deviceName"/>
+      </attribute>
+      <optional>
+        <attribute name="bus">
           <choice>
-            <value>floppy</value>
-            <value>disk</value>
-            <value>cdrom</value>
+            <value>ide</value>
+            <value>fdc</value>
+            <value>scsi</value>
+            <value>virtio</value>
+            <value>xen</value>
+            <value>usb</value>
+            <value>uml</value>
           </choice>
         </attribute>
       </optional>
-      <choice>
-        <group>
-          <attribute name="type">
-            <value>file</value>
-          </attribute>
-          <interleave>
-            <optional>
-              <element name="source">
-                <attribute name="file">
-                  <ref name="absFilePath"/>
-                </attribute>
-                <empty/>
-              </element>
-            </optional>
-            <ref name="diskspec"/>
-          </interleave>
-        </group>
-        <group>
-          <attribute name="type">
-            <value>block</value>
-          </attribute>
-          <interleave>
-            <optional>
-              <element name="source">
-                <attribute name="dev">
-                  <ref name="deviceName"/>
-                </attribute>
-                <empty/>
-              </element>
-            </optional>
-            <ref name="diskspec"/>
-          </interleave>
-        </group>
-        <ref name="diskspec"/>
-      </choice>
+    </element>
+  </define>
+
+  <define name="controller">
+    <element name="controller">
+      <interleave>
+       <attribute name="type">
+         <choice>
+           <!-- For now, only SCSI is supported -->
+           <value>scsi</value>
+         </choice>
+       </attribute>
+       <attribute name="id">
+         <ref name="genericName"/>
+       </attribute>
+      </interleave>
+      <optional>
+       <element name="bus">
+         <attribute name="addr">
+           <!-- Just for testing, should be pciaddress -->
+           <ref name="deviceName"/>
+         </attribute>
+       </element>
+      </optional>
     </element>
   </define>
   <define name="target">
diff --git a/src/domain_conf.c b/src/domain_conf.c
index a6120c8..bbaf944 100644
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -79,6 +79,7 @@ VIR_ENUM_IMPL(virDomainLifecycle, VIR_DOMAIN_LIFECYCLE_LAST,
 
 VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST,
               "disk",
+              "controller",
               "filesystem",
               "interface",
               "input",
@@ -294,6 +295,18 @@ void virDomainDiskDefFree(virDomainDiskDefPtr def)
     VIR_FREE(def);
 }
 
+void virDomainControllerDefFree(virDomainControllerDefPtr def)
+{
+    if (!def)
+        return;
+
+    VIR_FREE(def->type);
+    VIR_FREE(def->id);
+    VIR_FREE(def->pci_addr);
+
+    VIR_FREE(def);
+}
+
 void virDomainFSDefFree(virDomainFSDefPtr def)
 {
     if (!def)
@@ -651,6 +664,7 @@ virDomainDiskDefParseXML(virConnectPtr conn,
     char *source = NULL;
     char *target = NULL;
     char *controller = NULL;
+    char *controller_pci_addr = NULL;
     char *bus_id = NULL;
     char *unit_id = NULL;
     char *controller_id = NULL;
@@ -709,6 +723,7 @@ virDomainDiskDefParseXML(virConnectPtr conn,
             } else if ((controller == NULL) &&
                        (xmlStrEqual(cur->name, BAD_CAST "controller"))) {
              controller_id = virXMLPropString(cur, "id");
+             controller_pci_addr =  virXMLPropString(cur, "pci_addr");
              bus_id = virXMLPropString(cur, "bus");
              unit_id = virXMLPropString(cur, "unit");
             } else if ((driverName == NULL) &&
@@ -827,6 +842,19 @@ virDomainDiskDefParseXML(virConnectPtr conn,
                         _("Cannot parse <controller> 'unit' attribute"));
            goto error;
        } 
+
+       /* TODO: Should we re-use devaddr for this purpose,
+           or is an extra field justified? */
+       if (controller_pci_addr &&
+           sscanf(controller_pci_addr, "%x:%x:%x",
+                  &def->controller_pci_addr.domain,
+                  &def->controller_pci_addr.bus,
+                  &def->controller_pci_addr.slot) < 3) {
+           virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                 _("Unable to parse <controller> 'pci_addr' parameter '%s'"),
+                                controller_pci_addr);
+           goto error;
+       }
     }
 
     if (def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY &&
@@ -895,6 +923,77 @@ cleanup:
 }
 
 
+/* Parse the XML definition for a controller
+ * @param node XML nodeset to parse for controller definition
+ */
+static virDomainControllerDefPtr
+virDomainControllerDefParseXML(virConnectPtr conn,
+                              xmlNodePtr node,
+                              int flags ATTRIBUTE_UNUSED) {
+    virDomainControllerDefPtr def;
+    xmlNodePtr cur;
+    char *type = NULL;
+    char *bus_addr = NULL;
+
+    if (VIR_ALLOC(def) < 0) {
+        virReportOOMError(conn);
+        return NULL;
+    }
+
+    type = virXMLPropString(node, "type");
+    def->type = VIR_DOMAIN_DISK_BUS_SCSI;
+    if (type) {
+        if ((def->type = virDomainDiskBusTypeFromString(type)) < 0) {
+            virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                 _("unknown disk bus type '%s'"), type);
+            goto error;
+        }
+    } 
+
+    def->id = virXMLPropString(node, "id");
+
+    cur = node->children;
+    while (cur != NULL) {
+        if (cur->type == XML_ELEMENT_NODE &&
+            bus_addr == NULL &&
+            xmlStrEqual(cur->name, BAD_CAST "bus")) {
+        
+            bus_addr = virXMLPropString(cur, "addr");
+            
+            def->pci_addr.domain = -1;
+            def->pci_addr.bus = -1;
+            def->pci_addr.slot = -1;
+            if (bus_addr &&
+                sscanf(bus_addr, "%x:%x:%x",
+                       &def->pci_addr.domain,
+                       &def->pci_addr.bus,
+                       &def->pci_addr.slot) < 3) {
+                virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                                     "Unable to parse <bus> 'addr' parameter 
'%s'",
+                                     bus_addr);
+                goto error;
+            }
+            
+            VIR_DEBUG("Parse PCI address of controller as %d:%d:%d\n",
+                      def->pci_addr.domain, def->pci_addr.bus, 
+                      def->pci_addr.slot);
+        } 
+        cur = cur->next;
+    }
+
+
+cleanup:
+    VIR_FREE(type);
+    VIR_FREE(bus_addr);
+
+    return def;
+
+ error:
+    virDomainControllerDefFree(def);
+    def = NULL;
+    goto cleanup;
+}
+
 /* Parse the XML definition for a disk
  * @param node XML nodeset to parse for disk definition
  */
@@ -2375,6 +2474,11 @@ virDomainDeviceDefPtr 
virDomainDeviceDefParse(virConnectPtr conn,
         dev->type = VIR_DOMAIN_DEVICE_DISK;
         if (!(dev->data.disk = virDomainDiskDefParseXML(conn, node, flags)))
             goto error;
+    } else if (xmlStrEqual(node->name, BAD_CAST "controller")) {
+        dev->type = VIR_DOMAIN_DEVICE_CONTROLLER;
+        if (!(dev->data.controller = 
+             virDomainControllerDefParseXML(conn, node, flags)))
+            goto error;
     } else if (xmlStrEqual(node->name, BAD_CAST "filesystem")) {
         dev->type = VIR_DOMAIN_DEVICE_FS;
         if (!(dev->data.fs = virDomainFSDefParseXML(conn, node, flags)))
@@ -2783,6 +2887,27 @@ static virDomainDefPtr 
virDomainDefParseXML(virConnectPtr conn,
     }
     VIR_FREE(nodes);
 
+    /* analysis of the disk controllers */
+    if ((n = virXPathNodeSet(conn, "./devices/controller", ctxt, &nodes)) < 0) 
{
+        virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
+                             "%s", _("cannot extract controller devices"));
+        goto error;
+    }
+    if (n && VIR_ALLOC_N(def->controllers, n) < 0)
+        goto no_memory;
+    for (i = 0 ; i < n ; i++) {
+        virDomainControllerDefPtr controller = 
+           virDomainControllerDefParseXML(conn, nodes[i], flags);
+        if (!controller)
+            goto error;
+
+        def->controllers[def->ncontrollers++] = controller;
+    }
+    /*    qsort(def->controllers, def->ncontrollers, sizeof(*def->controllers),
+          virDomainControllerQSort); */
+    VIR_FREE(nodes);
+
+
     /* analysis of the filesystems */
     if ((n = virXPathNodeSet(conn, "./devices/filesystem", ctxt, &nodes)) < 0) 
{
         virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
diff --git a/src/domain_conf.h b/src/domain_conf.h
index 898f6c9..6b3cb09 100644
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -111,6 +111,11 @@ struct _virDomainDiskDef {
     char *src;
     char *dst;
     char *controller_id;
+    struct {
+        unsigned domain;
+        unsigned bus;
+        unsigned slot;
+    } controller_pci_addr;
     char *driverName;
     char *driverType;
     char *serial;
@@ -125,6 +130,19 @@ struct _virDomainDiskDef {
     virStorageEncryptionPtr encryption;
 };
 
+/* Stores the virtual disk controller configuration */
+typedef struct _virDomainControllerDef virDomainControllerDef;
+typedef virDomainControllerDef *virDomainControllerDefPtr;
+struct _virDomainControllerDef {
+    int type;
+    char *id;
+    struct {
+        unsigned domain;
+        unsigned bus;
+        unsigned slot;
+    } pci_addr;
+};
+
 static inline int
 virDiskHasValidPciAddr(virDomainDiskDefPtr def)
 {
@@ -441,6 +459,7 @@ enum virDomainDeviceType {
     VIR_DOMAIN_DEVICE_SOUND,
     VIR_DOMAIN_DEVICE_VIDEO,
     VIR_DOMAIN_DEVICE_HOSTDEV,
+    VIR_DOMAIN_DEVICE_CONTROLLER,
 
     VIR_DOMAIN_DEVICE_LAST,
 };
@@ -451,6 +470,7 @@ struct _virDomainDeviceDef {
     int type;
     union {
         virDomainDiskDefPtr disk;
+        virDomainControllerDefPtr controller;
         virDomainFSDefPtr fs;
         virDomainNetDefPtr net;
         virDomainInputDefPtr input;
@@ -561,6 +581,9 @@ struct _virDomainDef {
     int ndisks;
     virDomainDiskDefPtr *disks;
 
+    int ncontrollers;
+    virDomainControllerDefPtr *controllers;
+
     int nfss;
     virDomainFSDefPtr *fss;
 
@@ -637,6 +660,7 @@ virDomainObjPtr virDomainFindByName(const 
virDomainObjListPtr doms,
 void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def);
 void virDomainInputDefFree(virDomainInputDefPtr def);
 void virDomainDiskDefFree(virDomainDiskDefPtr def);
+void virDomainControllerDefFree(virDomainControllerDefPtr def);
 void virDomainFSDefFree(virDomainFSDefPtr def);
 void virDomainNetDefFree(virDomainNetDefPtr def);
 void virDomainChrDefFree(virDomainChrDefPtr def);
-- 
1.6.4