[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 01/25] conf: Generic XMLs for scsi hostdev



On 07/05/13 00:33, John Ferlan wrote:
On 05/03/2013 02:07 PM, Osier Yang wrote:
From: Han Cheng <hanc fnst cn fujitsu com>

An example of the scsi hostdev XML:

     <hostdev mode='subsystem' type='scsi'>
       <source>
         <adapter name='scsi_host0'/>
         <address bus='0' target='0' unit='0'/>
       </source>
       <address type='drive' controller='0' bus='0' target='4' unit='8'/>
     </hostdev>

Controller is implicitly added for scsi hostdev, though the scsi
controller's model defaults to "lsilogic", which might be not what
the user wants (same problem exists for virtio-scsi disk). It's
the existing problem, will be addressed later.

The device address must be specified manually. Later patch will let
libvirt generate it automatically.

This only introduces the generic XMLs for scsi hostdev, later patches
will add other elements, e.g. <readonly>, <shareable>.

Signed-off-by: Han Cheng <hanc fnst cn fujitsu com>
Signed-off-by: Osier Yang <jyang redhat com>

---
v3 - v4:
   * Split 1/10 in v3 into two patches, <readonly> will be in a later
     patch
   * Error messages are improved.

v2.5 - v3:
   * Remove the rigid algrithom to generate the address, the address of
     scsi host device must be specified manually now, later patch will
     add the generator.
   * Merge the xml2xml test from 10/10 into this patch
   * s/1.0.5/1.0.6/
   * Improve the XML parsing, fixes on virReportError statements, typos
   * hostdev->readonly is changed from bit field into boolean
---
  docs/formatdomain.html.in                          |  34 ++++-
  docs/schemas/domaincommon.rng                      |  26 ++++
  src/conf/domain_audit.c                            |  10 ++
  src/conf/domain_conf.c                             | 161 ++++++++++++++++++++-
  src/conf/domain_conf.h                             |   7 +
  .../qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml |  35 +++++
  tests/qemuxml2xmltest.c                            |   2 +
  7 files changed, 266 insertions(+), 9 deletions(-)
  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 8d4edfb..488673f 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2249,13 +2249,13 @@
<h4><a name="elementsHostDev">Host device assignment</a></h4> - <h5><a href="elementsHostDevSubsys">USB / PCI devices</a></h5>
+    <h5><a href="elementsHostDevSubsys">USB / PCI / SCSI devices</a></h5>
<p>
-      USB and PCI devices attached to the host can be passed through
+      USB, PCI and SCSI devices attached to the host can be passed through
        to the guest using the <code>hostdev</code> element.
-      <span class="since">since after 0.4.4 for USB and 0.6.0 for PCI
-        (KVM only)</span>:
+      <span class="since">since after 0.4.4 for USB, 0.6.0 for PCI(KVM only)
+        and 1.0.6 for SCSI(KVM only)</span>:
      </p>
<pre>
@@ -2286,12 +2286,29 @@
    &lt;/devices&gt;
    ...</pre>
+ <p>or:</p>
+
+<pre>
+  ...
+  &lt;devices&gt;
+    &lt;hostdev mode='subsystem' type='scsi'&gt;
+      &lt;source&gt;
+        &lt;adapter name='scsi_host0'/&gt;
+        &lt;address type='scsi' bus='0' target='0' unit='0'/&gt;
+      &lt;/source&gt;
+      &lt;readonly/&gt;
+      &lt;address type='drive' controller='0' bus='0' target='0' unit='0'/&gt;
+    &lt;/hostdev&gt;
+  &lt;/devices&gt;
+  ...</pre>
+
      <dl>
        <dt><code>hostdev</code></dt>
        <dd>The <code>hostdev</code> element is the main container for describing
          host devices. For usb device passthrough <code>mode</code> is always
-        "subsystem" and <code>type</code> is "usb" for a USB device and "pci"
-        for a PCI device. When <code>managed</code> is "yes" for a PCI
+        "subsystem" and <code>type</code> is "usb" for a USB device, "pci"
+        for a PCI device and "scsi" for a SCSI device. When
+        <code>managed</code> is "yes" for a PCI
          device, it is detached from the host before being passed on to
          the guest, and reattached to the host after the guest exits.
          If <code>managed</code> is omitted or "no", and for USB
@@ -2301,13 +2318,16 @@
          hot-plugging the device,
          and <code>virNodeDeviceReAttach</code> (or <code>virsh
          nodedev-reattach</code>) after hot-unplug or stopping the
-        guest.</dd>
+        guest. For SCSI device, user is responsible to make sure the device
+        is not used by host.</dd>
        <dt><code>source</code></dt>
        <dd>The source element describes the device as seen from the host.
        The USB device can either be addressed by vendor / product id using the
        <code>vendor</code> and <code>product</code> elements or by the device's
        address on the hosts using the <code>address</code> element. PCI devices
        on the other hand can only be described by their <code>address</code>.
+      SCSI devices can be described by <code>adapter</code> and
+      <code>address</code>.
s/can be/are/
s/by/by both the/
s/./ elements./

E.G. "SCSI devices are described by both the adapter and address elements."

I'd like keep the <code>...</code>


I would say that a couple of changes could be made...  Your choice
whether to make them or not.  The paragraph just reads funny as if
someone has only "added" to it without regard to how the existing
elements are described.

"USB devices can either be described by vendor/product id using the
vendor or product elements or by the hosts' USB device address using the
address element."

"PCI devices are described by their address element."  ?Is this the
"hosts' PCI address?  or just one fabricated for the guest?

It should be the "host address". Yeah, I feel a bit confused when seeing
them, could be a later patch.



<span class="since">Since 1.0.0</span>, the <code>source</code> element
        of USB devices may contain <code>startupPolicy</code> attribute which can
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 10596dc..6a4b77a 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3094,6 +3094,7 @@
      <choice>
        <ref name="hostdevsubsyspci"/>
        <ref name="hostdevsubsysusb"/>
+      <ref name="hostdevsubsysscsi"/>
      </choice>
    </define>
@@ -3162,6 +3163,20 @@
      </element>
    </define>
+ <define name="hostdevsubsysscsi">
+    <attribute name="type">
+      <value>scsi</value>
+    </attribute>
+    <element name="source">
+      <interleave>
+        <ref name="sourceinfoadapter"/>
+        <element name="address">
+          <ref name="scsiaddress"/>
+        </element>
+      </interleave>
+    </element>
+  </define>
+
    <define name="hostdevcapsstorage">
      <attribute name="type">
        <value>storage</value>
@@ -3217,6 +3232,17 @@
        </attribute>
      </element>
    </define>
+  <define name="scsiaddress">
+    <attribute name="bus">
+      <ref name="driveBus"/>
+    </attribute>
+    <attribute name="target">
+      <ref name="driveTarget"/>
+    </attribute>
+    <attribute name="unit">
+      <ref name="driveUnit"/>
+    </attribute>
+  </define>
    <define name="usbportaddress">
      <attribute name="bus">
        <ref name="usbAddr"/>
diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c
index 40910d6..a6cefb2 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -399,6 +399,16 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev,
                  goto cleanup;
              }
              break;
+        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
+            if (virAsprintf(&address, "%s:%d:%d:%d",
+                            hostdev->source.subsys.u.scsi.adapter,
+                            hostdev->source.subsys.u.scsi.bus,
+                            hostdev->source.subsys.u.scsi.target,
+                            hostdev->source.subsys.u.scsi.unit) < 0) {
+                VIR_WARN("OOM while encoding audit message");
+                goto cleanup;
+            }
+            break;
          default:
              VIR_WARN("Unexpected hostdev type while encoding audit message: %d",
                       hostdev->source.subsys.type);
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fe97c02..9e6b65b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -585,7 +585,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST,
VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST,
                "usb",
-              "pci")
+              "pci",
+              "scsi")
VIR_ENUM_IMPL(virDomainHostdevSubsysPciBackend,
                VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST,
@@ -1634,7 +1635,8 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
      if (def->parent.type == VIR_DOMAIN_DEVICE_NONE)
          virDomainDeviceInfoFree(def->info);
- if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) {
+    switch (def->mode) {
+    case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES:
          switch (def->source.caps.type) {
          case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE:
              VIR_FREE(def->source.caps.u.storage.block);
@@ -1646,6 +1648,11 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
              VIR_FREE(def->source.caps.u.net.iface);
              break;
          }
+        break;
+    case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
+        if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
+            VIR_FREE(def->source.subsys.u.scsi.adapter);
+        break;
      }
  }
@@ -3681,6 +3688,94 @@ out:
  }
static int
+virDomainHostdevSubsysScsiDefParseXML(const xmlNodePtr node,
+                                      virDomainHostdevDefPtr def)
+{
+    int ret = -1;
+    bool got_address = false, got_adapter = false;
+    xmlNodePtr cur;
+    char *bus = NULL, *target = NULL, *unit = NULL;
+
+    cur = node->children;
+    while (cur != NULL) {
+        if (cur->type == XML_ELEMENT_NODE) {
+            if (xmlStrEqual(cur->name, BAD_CAST "address")) {
+                if (got_address) {
+                    virReportError(VIR_ERR_XML_ERROR, "%s",
+                                   _("more than one source addresses are "
+                                     "specified for scsi hostdev"));
"more than one source address is"

Surely I won't want to talk about English with you. :/


Should scsi be SCSI when referencing the architecture rather than the
attribute name?  Or should scsi be single quoted, eg 'scsi'?  Not sure
if there is a "norm", but it should be consistent with other uses (usb
and pci).

It should be "scsi", which is consistent with what we use in the XML.
And here it's "scsi hostdev", both are the terms we use in XML.



+                    goto cleanup;
+                }
+
+                if (!(bus = virXMLPropString(cur, "bus")) ||
+                    !(target = virXMLPropString(cur, "target")) ||
+                    !(unit = virXMLPropString(cur, "unit"))) {
+                    virReportError(VIR_ERR_XML_ERROR, "%s",
+                                   _("'bus', 'target', and 'unit' must be specified "
+                                     "for scsi hostdev source address"));
+                    goto cleanup;
+                }
+
+                if (virStrToLong_ui(bus, NULL, 0, &def->source.subsys.u.scsi.bus) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot parse bus '%s'"), bus);
+                    goto cleanup;
+                }
+
+                if (virStrToLong_ui(target, NULL, 0, &def->source.subsys.u.scsi.target) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot parse target '%s'"), target);
+                    goto cleanup;
+                }
+
+                if (virStrToLong_ui(unit, NULL, 0, &def->source.subsys.u.scsi.unit) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot parse unit '%s'"), unit);
+                    goto cleanup;
+                }
+
+                got_address = true;
+            } else if (xmlStrEqual(cur->name, BAD_CAST "adapter")) {
+                if (got_adapter) {
+                    virReportError(VIR_ERR_XML_ERROR, "%s",
+                                   _("more than one adapters are specified "
+                                     "for scsi hostdev source"));
"more than one adapter is specified"

+                    goto cleanup;
+                }
+                if (!(def->source.subsys.u.scsi.adapter =
+                      virXMLPropString(cur, "name"))) {
+                    virReportError(VIR_ERR_XML_ERROR, "%s",
+                                   _("'adapter' must be specified for scsi hostdev source"));
+                    goto cleanup;
+                }
+
+                got_adapter = true;
+            } else {
+                virReportError(VIR_ERR_XML_ERROR,
+                               _("unsuported element '%s' of scsi hostdev source"),
s/unsuported/unsupported

+                               cur->name);
+                goto cleanup;
+            }
+        }
+        cur = cur->next;
+    }
+
+    if (!got_address || !got_adapter) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("'adapter' and 'address' must be specified for scsi "
+                         "hostdev source"));
+        goto cleanup;
+    }
+
+    ret = 0;
+cleanup:
+    VIR_FREE(bus);
+    VIR_FREE(target);
+    VIR_FREE(unit);
+    return ret;
+}
+
+static int
  virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
                                    xmlXPathContextPtr ctxt,
                                    const char *type,
@@ -3761,6 +3856,12 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
          if (virDomainHostdevSubsysUsbDefParseXML(sourcenode, def) < 0)
              goto error;
          break;
+
+    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
+        if (virDomainHostdevSubsysScsiDefParseXML(sourcenode, def) < 0)
+            goto error;
+        break;
+
      default:
          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                         _("address type='%s' not supported in hostdev interfaces"),
@@ -8617,6 +8718,13 @@ virDomainHostdevDefParseXML(const xmlNodePtr node,
                  goto error;
              }
              break;
+        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
+            if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+                virReportError(VIR_ERR_XML_ERROR, "%s",
+                               _("SCSI host devices must have address specified"));
See here it's capitalized - SCSI...  Just be consistent in use.

will use "scsi hostdev".


+                goto error;
+            }
+            break;
          }
      }
@@ -9144,6 +9252,17 @@ virDomainHostdevMatchSubsysPCI(virDomainHostdevDefPtr a,
      return 0;
  }
+static int
+virDomainHostdevMatchSubsysSCSI(virDomainHostdevDefPtr a,
+                                virDomainHostdevDefPtr b)
+{
+    if (STREQ(a->source.subsys.u.scsi.adapter, b->source.subsys.u.scsi.adapter) &&
+        a->source.subsys.u.scsi.bus == b->source.subsys.u.scsi.bus &&
+        a->source.subsys.u.scsi.target == b->source.subsys.u.scsi.target &&
+        a->source.subsys.u.scsi.unit == b->source.subsys.u.scsi.unit)
+        return 1;
+    return 0;
+}
static int
  virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
@@ -9157,6 +9276,8 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
          return virDomainHostdevMatchSubsysPCI(a, b);
      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
          return virDomainHostdevMatchSubsysUSB(a, b);
+    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
+        return virDomainHostdevMatchSubsysSCSI(a, b);
      }
      return 0;
  }
@@ -12971,6 +13092,30 @@ virDomainDefMaybeAddSmartcardController(virDomainDefPtr def)
      return 0;
  }
+static int
+virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
+{
+    /* Look for any hostdev scsi dev */
+    int i;
+    int maxController = -1;
+    virDomainHostdevDefPtr hostdev;
+
+    for (i = 0; i < def->nhostdevs; i++) {
+        hostdev = def->hostdevs[i];
+        if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+            hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
+            (int)hostdev->info->addr.drive.controller > maxController) {
+            maxController = hostdev->info->addr.drive.controller;
+        }
+    }
+
+    for (i = 0; i <= maxController; i++) {
+        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0)
+            return -1;
+    }
+
+    return 0;
+}
/*
   * Based on the declared <address/> info for any devices,
@@ -13007,6 +13152,9 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def)
      if (virDomainDefMaybeAddSmartcardController(def) < 0)
          return -1;
+ if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0)
+        return -1;
+
      return 0;
  }
@@ -13912,6 +14060,15 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
              virBufferAddLit(buf, "</origstates>\n");
          }
          break;
+    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
+        virBufferAsprintf(buf, "<adapter name='%s'/>\n",
+                          def->source.subsys.u.scsi.adapter);
+        virBufferAsprintf(buf, "<address %sbus='%d' target='%d' unit='%d'/>\n",
s/sbus/bus/

+                          includeTypeInAddr ? "type='scsi' " : "",
+                          def->source.subsys.u.scsi.bus,
+                          def->source.subsys.u.scsi.target,
+                          def->source.subsys.u.scsi.unit);
+        break;
      default:
          virReportError(VIR_ERR_INTERNAL_ERROR,
                         _("unexpected hostdev type %d"),
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 21f7ce2..1efae69 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -384,6 +384,7 @@ enum virDomainHostdevMode {
  enum virDomainHostdevSubsysType {
      VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB,
      VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI,
+    VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI,
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST
  };
@@ -417,6 +418,12 @@ struct _virDomainHostdevSubsys {
              virDevicePCIAddress addr; /* host address */
              int backend; /* enum virDomainHostdevSubsysPciBackendType */
          } pci;
+        struct {
+            char *adapter;
+            unsigned bus;
+            unsigned target;
+            unsigned unit;
+        } scsi;
      } u;
  };
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml
new file mode 100644
index 0000000..5a263e7
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi.xml
@@ -0,0 +1,35 @@
+<domain type='qemu'>
+  <name>QEMUGuest2</name>
+  <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>219100</memory>
+  <currentMemory unit='KiB'>219100</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='i686' machine='pc'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu</emulator>
+    <disk type='block' device='disk'>
+      <source dev='/dev/HostVG/QEMUGuest2'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <controller type='scsi' index='0' model='virtio-scsi'/>
+    <controller type='usb' index='0'/>
+    <controller type='ide' index='0'/>
+    <controller type='pci' index='0' model='pci-root'/>
+    <hostdev mode='subsystem' type='scsi' managed='yes'>
+      <source>
+        <adapter name='scsi_host0'/>
+        <address bus='0' target='0' unit='0'/>
+      </source>
+      <address type='drive' controller='0' bus='0' target='4' unit='8'/>
+    </hostdev>
+    <memballoon model='virtio'/>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 64c1c83..1ca1f7e 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -285,6 +285,8 @@ mymain(void)
      DO_TEST_DIFFERENT("pci-autoadd-addr");
      DO_TEST_DIFFERENT("pci-autoadd-idx");
+ DO_TEST("hostdev-scsi");
+
      virObjectUnref(driver.caps);
      virObjectUnref(driver.xmlopt);
ACK w/ nits.

John

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]