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

Re: [libvirt] [PATCH v2 03/10] conf: Introduce scsi hostdev



On 01/04/13 20:00, Han Cheng wrote:
Adding scsi hostdev, it should like:

s/Adding/Add, s/should/looks/,


     <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>

Signed-off-by: Han Cheng <hanc fnst cn fujitsu com>
---
  docs/formatdomain.html.in     |   34 ++++++--
  docs/schemas/domaincommon.rng |   24 +++++
  src/conf/domain_audit.c       |   10 ++
  src/conf/domain_conf.c        |  192 ++++++++++++++++++++++++++++++++++++++++-
  src/conf/domain_conf.h        |    7 ++
  5 files changed, 258 insertions(+), 9 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index a6bacfa..be3630e 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2172,13 +2172,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 0.13.0 for SCSI(KVM only)</span>:

s/0.13.0/1.0.5/, confused with qemu's version?

      </p>
<pre>
@@ -2209,12 +2209,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='scsi' 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
@@ -2224,13 +2241,15 @@
          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 do not
+        use this device on host</dd>

s/do not use this device on host/the device is not used by either host or any guest./

        <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 is described by <code>adapter<code> and <code>address<code>.
<span class="since">Since 1.0.0</span>, the <code>source</code> element
        of USB devices may contain <code>startupPolicy</code> attribute which can
@@ -2268,6 +2287,7 @@
        <a href="#elementsOSBIOS">BIOS bootloader</a> section.
        <span class="since">Since 0.8.8</span> for PCI devices,
        <span class="since">Since 1.0.1</span> for USB devices.
+      <span class="since">Since 1.0.0</span> for SCSI devices.

s/1.0.0/1.0.5/

        <dt><code>rom</code></dt>
        <dd>The <code>rom</code> element is used to change how a PCI
          device's ROM is presented to the guest. The optional <code>bar</code>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 364abf1..209d4f5 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2945,6 +2945,7 @@
      <choice>
        <ref name="hostdevsubsyspci"/>
        <ref name="hostdevsubsysusb"/>
+      <ref name="hostdevsubsysscsi"/>
      </choice>
    </define>
@@ -2997,6 +2998,18 @@
      </element>
    </define>
+ <define name="hostdevsubsysscsi">
+    <attribute name="type">
+      <value>scsi</value>
+    </attribute>
+    <element name="source">
+      <ref name="sourceinfoadapter"/>
+      <element name="address">
+        <ref name="scsiaddress"/>
+      </element>
+    </element>
+  </define>

I was confused. The attribute "readonly" should be in "hostdevsubsysscsi",
as it's only about the "source".

+
    <define name="hostdevcapsstorage">
      <attribute name="type">
        <value>storage</value>
@@ -3041,6 +3054,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 a776058..2fb5989 100644
--- a/src/conf/domain_audit.c
+++ b/src/conf/domain_audit.c
@@ -398,6 +398,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 d9d6b9f..c98ca48 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -573,7 +573,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(virDomainHostdevCaps, VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST,
                "storage",
@@ -1558,7 +1559,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);
@@ -1567,6 +1569,11 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
              VIR_FREE(def->source.caps.u.misc.chardev);
              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;
      }
  }
@@ -3080,6 +3087,104 @@ virDomainParseLegacyDeviceAddress(char *devaddr,
  }
static int
+virDomainHostdevSubsysScsiDefParseXML(const xmlNodePtr node,
+                                      virDomainHostdevDefPtr def)
+{
+    int ret = -1, got_address = 0, got_adapter = 0;

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,
+                                   _("more than one addresses are specified"));
+                    goto out;
+                }
+                if (!(bus = virXMLPropString(cur, "bus"))) {
+                    virReportError(VIR_ERR_XML_ERROR,
+                                   _("bus must be specified for scsi hostdev address"));
+                    goto out;
+                }
+                if (virStrToLong_ui(bus, NULL, 0, &def->source.subsys.u.scsi.bus) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot parse bus %s"), bus);
+                    goto out;
+                }
+                if (!(target = virXMLPropString(cur, "target"))) {
+                    virReportError(VIR_ERR_XML_ERROR,
+                                   _("target must be specified for scsi hostdev address"));
+                    goto out;
+                }
+                if (virStrToLong_ui(target, NULL, 0, &def->source.subsys.u.scsi.target) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot parse target %s"), target);
+                    goto out;
+                }
+                if (!(unit = virXMLPropString(cur, "unit"))) {
+                    virReportError(VIR_ERR_XML_ERROR,
+                                   _("unit must be specified for scsi hostdev address"));
+                    goto out;
+                }
+                if (virStrToLong_ui(unit, NULL, 0, &def->source.subsys.u.scsi.unit) < 0) {
+                    virReportError(VIR_ERR_INTERNAL_ERROR,
+                                   _("cannot parse unit %s"), unit);
+                    goto out;
+                }
+                VIR_FREE(bus);
+                VIR_FREE(target);
+                VIR_FREE(unit);

No need to free these 3 strings here. They are free'ed in "out:" anyway. And since you have "git_address". They can be only parsed once. So no worry of the leaking
here.

+                got_address = 1;

And

got_adress = true;

+            } else if (xmlStrEqual(cur->name, BAD_CAST "adapter")) {
+                if (got_adapter) {
+                    virReportError(VIR_ERR_XML_ERROR,
+                                   _("more than one adapters are specified"));
+                    goto out;
+                }
+                if (!(def->source.subsys.u.scsi.adapter =
+                            virXMLPropString(cur, "name"))) {
+                    virReportError(VIR_ERR_XML_ERROR,
+                                   _("adapter must be specified for scsi hostdev address"));
+                    goto out;
+                }
+                got_adapter = 1;

got_adapter = true;

+            } else {
+                virReportError(VIR_ERR_XML_ERROR,
+                               _("unknown scsi source type '%s'"),

Improper error.

+                               cur->name);
+                goto out;
+            }
+        }
+        cur = cur->next;
+    }
+
+    if (!got_address && !got_adapter) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       "%s", _("missing adapter and address"));
+        goto out;
+    }
+    if (!got_address && got_adapter) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       "%s", _("missing address"));
+        goto out;
+    }
+    if (got_address && !got_adapter) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       "%s", _("missing adapter"));
+        goto out;
+    }

Above checking can be simplied as:

if (!got_address || !got_dapter) {
      virReportError(VIR_ERR_XML_ERROR, "%s",
_("'adapter' and 'address' must be specified"));
      goto out;
}

+
+    ret = 0;
+out:

s/out/cleanup/, this applies across the function. Some old code still
use the "out", but new code should be consistent as much as possible.

+    VIR_FREE(bus);
+    VIR_FREE(target);
+    VIR_FREE(unit);
+    return ret;
+}
+
+static int
  virDomainHostdevSubsysUsbDefParseXML(const xmlNodePtr node,
                                       virDomainHostdevDefPtr def)
  {
@@ -3374,6 +3479,10 @@ 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"),
@@ -8036,6 +8145,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node,
      }
if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
+        xmlNodePtr cur;
+        unsigned int readonly = 0;
          switch (def->source.subsys.type) {
          case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
              if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
@@ -8044,6 +8155,20 @@
(const xmlNodePtr node,
                                 _("PCI host devices must use 'pci' address type"));
                  goto error;
              }
+
+            break;
+        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
+            cur = node->children;
+            while (cur != NULL) {
+                if (cur->type == XML_ELEMENT_NODE) {
+                    if (readonly == 0 &&
+                        xmlStrEqual(cur->name, BAD_CAST "readonly"))
+                        readonly = 1;
+                }
+                cur = cur->next;
+            }
+            def->readonly = readonly;

Agreed with Hu Tao, that "readonly" should be of boolean type.

+
              break;
          }
      }
@@ -8567,6 +8692,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,
@@ -8580,6 +8716,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;
  }
@@ -10773,6 +10911,16 @@ virDomainDefParseXML(virCapsPtr caps,
              goto error;
          }
+ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
+            hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+            /* reverse first 16 unit for disk usage */
+            hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+            hostdev->info->addr.drive.controller = 0;
+            hostdev->info->addr.drive.bus = 0;
+            hostdev->info->addr.drive.target = 0;

Why this defdaults to 0? Can you explain it either in the commit log or
by comments?

+            hostdev->info->addr.drive.unit = 16 + i;

And why the "16".

+        }
+
          def->hostdevs[def->nhostdevs++] = hostdev;
      }
      VIR_FREE(nodes);
@@ -12340,6 +12488,30 @@ virDomainDefMaybeAddSmartcardController(virDomainDefPtr def)
      return 0;
  }
+static int
+virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)

Why do we need this?

+{
+    /* Look for any hostdev scsi dev */
+    int i;
+    int maxController = -1;
+    virDomainHostdevDefPtr hostdev;
+
+    for (i = 0; i < def->nhostdevs; i++) {
+        hostdev = *(def->hostdevs + i);

Generally we use:
                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) < 0)
+            return -1;
+    }
+
+    return 0;
+}
/*
   * Based on the declared <address/> info for any devices,
@@ -12376,6 +12548,9 @@ virDomainDefAddImplicitControllers(virDomainDefPtr def)
      if (virDomainDefMaybeAddSmartcardController(def) < 0)
          return -1;
+ if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0)
+        return -1;
+
      return 0;
  }
@@ -13194,6 +13369,15 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
      virBufferAdjustIndent(buf, 2);
      switch (def->source.subsys.type)
      {
+    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",
+                           ? "type='scsi' " : "",
+                          def->source.subsys.u.scsi.bus,
+                          def->source.subsys.u.scsi.target,
+                          def->source.subsys.u.scsi.unit);
+        break;
      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
          if (def->source.subsys.u.usb.vendor) {
              virBufferAsprintf(buf, "<vendor id='0x%.4x'/>\n",
@@ -14446,6 +14630,9 @@ virDomainHostdevDefFormat(virBufferPtr buf,
      case VIR_DOMAIN_HOSTDEV_MODE_SUBSYS:
          if (virDomainHostdevDefFormatSubsys(buf, def, flags, false) < 0)
              return -1;
+        if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI &&
+            def->readonly)
+            virBufferAsprintf(buf, "<readonly/>\n");
          break;
      case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES:
          if (virDomainHostdevDefFormatCaps(buf, def) < 0)
@@ -14454,6 +14641,7 @@ virDomainHostdevDefFormat(virBufferPtr buf,
      }
      virBufferAdjustIndent(buf, -6);
+
      if (virDomainDeviceInfoFormat(buf, def->info,
                                    flags | VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT
                                    | VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM) < 0)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f8e3973..2d238eb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -380,6 +380,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
  };
@@ -399,6 +400,12 @@ struct _virDomainHostdevSubsys {
              unsigned vendor;
              unsigned product;
          } usb;
+        struct {
+            char *adapter;
+            unsigned bus;
+            unsigned target;
+            unsigned unit;
+        } scsi;
          virDevicePCIAddress pci; /* host address */
      } u;
  };


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