[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 03/04/13 16:29, Osier Yang wrote:
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?

Ignore this, just typed too quickly. Besides, the tests for the new XMLs should be merged into this patch. It's also for you to find out if the XML works well, other than
later.


+{
+    /* 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]