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(a)cn.fujitsu.com&gt; >> >> 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(a)cn.fujitsu.com&gt; >> Signed-off-by: Osier Yang <jyang(a)redhat.com&gt; >> >> --- >> 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/