[libvirt] [PATCH 01/25] conf: Generic XMLs for scsi hostdev
Osier Yang
jyang at redhat.com
Tue May 7 10:38:14 UTC 2013
On 07/05/13 00:33, John Ferlan wrote:
> On 05/03/2013 02:07 PM, Osier Yang wrote:
>> From: Han Cheng <hanc.fnst at 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 at cn.fujitsu.com>
>> Signed-off-by: Osier Yang <jyang at 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 @@
>> </devices>
>> ...</pre>
>>
>> + <p>or:</p>
>> +
>> +<pre>
>> + ...
>> + <devices>
>> + <hostdev mode='subsystem' type='scsi'>
>> + <source>
>> + <adapter name='scsi_host0'/>
>> + <address type='scsi' bus='0' target='0' unit='0'/>
>> + </source>
>> + <readonly/>
>> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>> + </hostdev>
>> + </devices>
>> + ...</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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list