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>. <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")); + 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")); + 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"), + 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")); + 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", + 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); -- 1.8.1.4

On 07/05/13 00:33, John Ferlan wrote: I'd like keep the <code>...</code> It should be the "host address". Yeah, I feel a bit confused when seeing them, could be a later patch. Surely I won't want to talk about English with you. :/ 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. will use "scsi hostdev".

From: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; Adding two cap flags for scsi-generic: QEMU_CAPS_SCSI_GENERIC QEMU_CAPS_SCSI_GENERIC_BOOTINDEX Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; Signed-off-by: Osier Yang <hanc.fnst(a)cn.fujitsu.com&gt; --- src/qemu/qemu_capabilities.c | 21 ++++++++++++++++----- src/qemu/qemu_capabilities.h | 2 ++ tests/qemuhelpdata/qemu-1.0-device | 10 ++++++++++ tests/qemuhelpdata/qemu-1.1.0-device | 10 ++++++++++ tests/qemuhelpdata/qemu-1.2.0-device | 5 +++++ tests/qemuhelpdata/qemu-kvm-1.2.0-device | 5 +++++ tests/qemuhelptest.c | 19 ++++++++++++++----- 7 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index a3a8d1f..430e893 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -221,9 +221,12 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "tpm-tis", "nvram", /* 140 */ - "pci-bridge", /* 141 */ - "vfio-pci", /* 142 */ - "vfio-pci.bootindex", /* 143 */ + "pci-bridge", + "vfio-pci", + "vfio-pci.bootindex", + "scsi-generic", + + "scsi-generic.bootindex", /* 145 */ ); struct _virQEMUCaps { @@ -1344,8 +1347,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "VGA", QEMU_CAPS_DEVICE_VGA }, { "cirrus-vga", QEMU_CAPS_DEVICE_CIRRUS_VGA }, { "vmware-svga", QEMU_CAPS_DEVICE_VMWARE_SVGA }, - { "usb-serial", QEMU_CAPS_DEVICE_USB_SERIAL}, - { "usb-net", QEMU_CAPS_DEVICE_USB_NET}, + { "usb-serial", QEMU_CAPS_DEVICE_USB_SERIAL }, + { "usb-net", QEMU_CAPS_DEVICE_USB_NET }, { "virtio-rng-pci", QEMU_CAPS_DEVICE_VIRTIO_RNG }, { "virtio-rng-s390", QEMU_CAPS_DEVICE_VIRTIO_RNG }, { "virtio-rng-ccw", QEMU_CAPS_DEVICE_VIRTIO_RNG }, @@ -1354,6 +1357,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "spapr-nvram", QEMU_CAPS_DEVICE_NVRAM }, { "pci-bridge", QEMU_CAPS_DEVICE_PCI_BRIDGE }, { "vfio-pci", QEMU_CAPS_DEVICE_VFIO_PCI }, + { "scsi-generic", QEMU_CAPS_DEVICE_SCSI_GENERIC }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { @@ -1403,6 +1407,10 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsUsbHost[] = { { "bootindex", QEMU_CAPS_USB_HOST_BOOTINDEX }, }; +static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsScsiGeneric[] = { + { "bootindex", QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX }, +}; + struct virQEMUCapsObjectTypeProps { const char *type; struct virQEMUCapsStringFlags *props; @@ -1438,6 +1446,8 @@ static struct virQEMUCapsObjectTypeProps virQEMUCapsObjectProps[] = { ARRAY_CARDINALITY(virQEMUCapsObjectPropsUsbRedir) }, { "usb-host", virQEMUCapsObjectPropsUsbHost, ARRAY_CARDINALITY(virQEMUCapsObjectPropsUsbHost) }, + { "scsi-generic", virQEMUCapsObjectPropsScsiGeneric, + ARRAY_CARDINALITY(virQEMUCapsObjectPropsScsiGeneric) }, }; @@ -1635,6 +1645,7 @@ virQEMUCapsExtractDeviceStr(const char *qemu, "-device", "usb-redir,?", "-device", "ide-drive,?", "-device", "usb-host,?", + "-device", "scsi-generic,?", NULL); /* qemu -help goes to stdout, but qemu -device ? goes to stderr. */ virCommandSetErrorBuffer(cmd, &output); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 213f63c..a9eea4e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -182,6 +182,8 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_PCI_BRIDGE = 141, /* -device pci-bridge */ QEMU_CAPS_DEVICE_VFIO_PCI = 142, /* -device vfio-pci */ QEMU_CAPS_VFIO_PCI_BOOTINDEX = 143, /* bootindex param for vfio-pci device */ + QEMU_CAPS_DEVICE_SCSI_GENERIC = 144, /* -device scsi-generic */ + QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX = 145, /* -device scsi-generic.bootindex */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/tests/qemuhelpdata/qemu-1.0-device b/tests/qemuhelpdata/qemu-1.0-device index 0bdfbbd..d557f0e 100644 --- a/tests/qemuhelpdata/qemu-1.0-device +++ b/tests/qemuhelpdata/qemu-1.0-device @@ -136,3 +136,13 @@ virtio-net-pci.romfile=string virtio-net-pci.rombar=uint32 virtio-net-pci.multifunction=on/off virtio-net-pci.command_serr_enable=on/off +scsi-generic.drive=drive +scsi-generic.logical_block_size=uint16 +scsi-generic.physical_block_size=uint16 +scsi-generic.min_io_size=uint16 +scsi-generic.opt_io_size=uint32 +scsi-generic.bootindex=int32 +scsi-generic.discard_granularity=uint32 +scsi-generic.channel=uint32 +scsi-generic.scsi-id=uint32 +scsi-generic.lun=uint32 diff --git a/tests/qemuhelpdata/qemu-1.1.0-device b/tests/qemuhelpdata/qemu-1.1.0-device index abbf850..7313a34 100644 --- a/tests/qemuhelpdata/qemu-1.1.0-device +++ b/tests/qemuhelpdata/qemu-1.1.0-device @@ -158,3 +158,13 @@ scsi-disk.dpofua=on/off scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32 +scsi-generic.drive=drive +scsi-generic.logical_block_size=blocksize +scsi-generic.physical_block_size=blocksize +scsi-generic.min_io_size=uint16 +scsi-generic.opt_io_size=uint32 +scsi-generic.bootindex=int32 +scsi-generic.discard_granularity=uint32 +scsi-generic.channel=uint32 +scsi-generic.scsi-id=uint32 +scsi-generic.lun=uint32 diff --git a/tests/qemuhelpdata/qemu-1.2.0-device b/tests/qemuhelpdata/qemu-1.2.0-device index 5613e00..40845e4 100644 --- a/tests/qemuhelpdata/qemu-1.2.0-device +++ b/tests/qemuhelpdata/qemu-1.2.0-device @@ -208,3 +208,8 @@ usb-host.bootindex=int32 usb-host.pipeline=on/off usb-host.port=string usb-host.full-path=on/off +scsi-generic.drive=drive +scsi-generic.bootindex=int32 +scsi-generic.channel=uint32 +scsi-generic.scsi-id=uint32 +scsi-generic.lun=uint32 diff --git a/tests/qemuhelpdata/qemu-kvm-1.2.0-device b/tests/qemuhelpdata/qemu-kvm-1.2.0-device index 879a049..09e3ef7 100644 --- a/tests/qemuhelpdata/qemu-kvm-1.2.0-device +++ b/tests/qemuhelpdata/qemu-kvm-1.2.0-device @@ -220,3 +220,8 @@ usb-host.bootindex=int32 usb-host.pipeline=on/off usb-host.port=string usb-host.full-path=on/off +scsi-generic.drive=drive +scsi-generic.bootindex=int32 +scsi-generic.channel=uint32 +scsi-generic.scsi-id=uint32 +scsi-generic.lun=uint32 diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index fa56406..f7f88d0 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -510,7 +510,8 @@ mymain(void) QEMU_CAPS_DEVICE_VMWARE_SVGA, QEMU_CAPS_DEVICE_USB_SERIAL, QEMU_CAPS_DEVICE_USB_NET, - QEMU_CAPS_DEVICE_PCI_BRIDGE); + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_SCSI_GENERIC); DO_TEST("qemu-kvm-0.12.1.2-rhel61", 12001, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -730,7 +731,9 @@ mymain(void) QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_DEVICE_VMWARE_SVGA, QEMU_CAPS_DEVICE_USB_SERIAL, - QEMU_CAPS_DEVICE_USB_NET); + QEMU_CAPS_DEVICE_USB_NET, + QEMU_CAPS_DEVICE_SCSI_GENERIC, + QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX); DO_TEST("qemu-1.1.0", 1001000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -822,7 +825,9 @@ mymain(void) QEMU_CAPS_DEVICE_USB_NET, QEMU_CAPS_DTB, QEMU_CAPS_IPV6_MIGRATION, - QEMU_CAPS_DEVICE_PCI_BRIDGE); + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_SCSI_GENERIC, + QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX); DO_TEST("qemu-1.2.0", 1002000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -926,7 +931,9 @@ mymain(void) QEMU_CAPS_DTB, QEMU_CAPS_SCSI_MEGASAS, QEMU_CAPS_IPV6_MIGRATION, - QEMU_CAPS_DEVICE_PCI_BRIDGE); + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_SCSI_GENERIC, + QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX); DO_TEST("qemu-kvm-1.2.0", 1002000, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -1035,7 +1042,9 @@ mymain(void) QEMU_CAPS_DTB, QEMU_CAPS_SCSI_MEGASAS, QEMU_CAPS_IPV6_MIGRATION, - QEMU_CAPS_DEVICE_PCI_BRIDGE); + QEMU_CAPS_DEVICE_PCI_BRIDGE, + QEMU_CAPS_DEVICE_SCSI_GENERIC, + QEMU_CAPS_DEVICE_SCSI_GENERIC_BOOTINDEX); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.1.4

From: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; This patch adds util functions for scsi hostdev. Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; Signed-off-by: Osier Yang <jyang(a)redhat.com&gt; --- v3 - v4: * Use strdup instead of virAsprintf if the format string is "%s" v2.5 - v3: * A couple "char *" to "const char *" * s/unsigned int readonly/bool readonly/ * Use STRPREIFX instead of the wrong use of STRSKIP (which can introduce bug) for virSCSIDeviceGetAdapterId. * Improve virSCSIDeviceNew I'm not fan of introducing another list like virPCIDeviceList, which brought much trouble for us, but it doesn't hurt much to do clean up together later, since they are very similar. --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 22 +++ src/util/virscsi.c | 408 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virscsi.h | 84 ++++++++++ 5 files changed, 516 insertions(+) create mode 100644 src/util/virscsi.c create mode 100644 src/util/virscsi.h diff --git a/po/POTFILES.in b/po/POTFILES.in index bf5a864..f3ea4da 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -174,6 +174,7 @@ src/util/virportallocator.c src/util/virprocess.c src/util/virrandom.c src/util/virsexpr.c +src/util/virscsi.c src/util/virsocketaddr.c src/util/virstatslinux.c src/util/virstoragefile.c diff --git a/src/Makefile.am b/src/Makefile.am index 299b8fd..e71975a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -111,6 +111,7 @@ UTIL_SOURCES = \ util/virportallocator.c util/virportallocator.h \ util/virprocess.c util/virprocess.h \ util/virrandom.h util/virrandom.c \ + util/virscsi.c util/virscsi.h \ util/virsexpr.c util/virsexpr.h \ util/virsocketaddr.h util/virsocketaddr.c \ util/virstatslinux.c util/virstatslinux.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 98660dc..64e2816 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1687,6 +1687,28 @@ virRandomGenerateWWN; virRandomInt; +# util/virscsi.h +virSCSIDeviceFileIterate; +virSCSIDeviceFree; +virSCSIDeviceGetAdapter; +virSCSIDeviceGetBus; +virSCSIDeviceGetDevStr; +virSCSIDeviceGetName; +virSCSIDeviceGetReadonly; +virSCSIDeviceGetTarget; +virSCSIDeviceGetUnit; +virSCSIDeviceGetUsedBy; +virSCSIDeviceListAdd; +virSCSIDeviceListCount; +virSCSIDeviceListDel; +virSCSIDeviceListFind; +virSCSIDeviceListGet; +virSCSIDeviceListNew; +virSCSIDeviceListSteal; +virSCSIDeviceNew; +virSCSIDeviceSetUsedBy; + + # util/virsexpr.h sexpr2string; sexpr_append; diff --git a/src/util/virscsi.c b/src/util/virscsi.c new file mode 100644 index 0000000..2d6bd8c --- /dev/null +++ b/src/util/virscsi.c @@ -0,0 +1,408 @@ +/* + * virscsi.c: helper APIs for managing host SCSI devices + * + * Copyright (C) 2013 Fujitsu, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + * + * Authors: + * Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; + */ + +#include <config.h> + +#include <dirent.h> +#include <fcntl.h> +#include <inttypes.h> +#include <limits.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> + +#include "virscsi.h" +#include "virlog.h" +#include "viralloc.h" +#include "virutil.h" +#include "virstring.h" +#include "virerror.h" + +#define SYSFS_SCSI_DEVICES "/sys/bus/scsi/devices" + +/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE + +struct _virSCSIDevice { + unsigned int adapter; + unsigned int bus; + unsigned int target; + unsigned int unit; + + char *name; /* adapter:bus:target:unit */ + char *id; /* model:vendor */ + char *path; + const char *used_by; /* name of the domain using this dev */ + + bool readonly; +}; + +struct _virSCSIDeviceList { + virObjectLockable parent; + unsigned int count; + virSCSIDevicePtr *devs; +}; + +static virClassPtr virSCSIDeviceListClass; + +static void virSCSIDeviceListDispose(void *obj); + +static int +virSCSIOnceInit(void) +{ + if (!(virSCSIDeviceListClass = virClassNew(virClassForObjectLockable(), + "virSCSIDeviceList", + sizeof(virSCSIDeviceList), + virSCSIDeviceListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virSCSI) + +static int +virSCSIDeviceGetAdapterId(const char *adapter, + unsigned int *adapter_id) +{ + if (STRPREFIX(adapter, "scsi_host")) { + if (virStrToLong_ui(adapter + strlen("scsi_host"), + NULL, 0, adapter_id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot parse adapter '%s'"), adapter); + return -1; + } + } + + return 0; +} + +char * +virSCSIDeviceGetDevStr(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit) +{ + DIR *dir = NULL; + struct dirent *entry; + char *path = NULL; + char *sg = NULL; + unsigned int adapter_id; + + if (virSCSIDeviceGetAdapterId(adapter, &adapter_id) < 0) + return NULL; + + if (virAsprintf(&path, + SYSFS_SCSI_DEVICES "/%d:%d:%d:%d/scsi_generic", + adapter_id, bus, target, unit) < 0) { + virReportOOMError(); + return NULL; + } + + if (!(dir = opendir(path))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open %s"), path); + goto cleanup; + } + + while ((entry = readdir(dir))) { + if (entry->d_name[0] == '.') + continue; + + if (!(sg = strdup(entry->d_name))) { + virReportOOMError(); + goto cleanup; + } + } + +cleanup: + closedir(dir); + VIR_FREE(path); + return sg; +} + +virSCSIDevicePtr +virSCSIDeviceNew(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit, + bool readonly) +{ + virSCSIDevicePtr dev, ret = NULL; + char *sg = NULL; + char *vendor_path = NULL; + char *model_path = NULL; + char *vendor = NULL; + char *model = NULL; + + if (VIR_ALLOC(dev) < 0) { + virReportOOMError(); + return NULL; + } + + dev->bus = bus; + dev->target = target; + dev->unit = unit; + dev->readonly = readonly; + + if (!(sg = virSCSIDeviceGetDevStr(adapter, bus, target, unit))) + goto cleanup; + + if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) + goto cleanup; + + if (virAsprintf(&dev->name, "%d:%d:%d:%d", dev->adapter, + dev->bus, dev->bus, dev->unit) < 0 || + virAsprintf(&dev->path, "/dev/%s", sg) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (access(dev->path, F_OK) != 0) { + virReportSystemError(errno, + _("Device %s not found: could not access %s"), + dev->name, dev->path); + goto cleanup; + } + + if (virAsprintf(&vendor_path, + SYSFS_SCSI_DEVICES "/%s/vendor", dev->name) < 0 || + virAsprintf(&model_path, + SYSFS_SCSI_DEVICES "/%s/model", dev->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virFileReadAll(vendor_path, 1024, &vendor) < 0) + goto cleanup; + + if (virFileReadAll(model_path, 1024, &model) < 0) + goto cleanup; + + virTrimSpaces(vendor, NULL); + virTrimSpaces(model, NULL); + + if (virAsprintf(&dev->id, "%s:%s", vendor, model) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret = dev; +cleanup: + VIR_FREE(vendor); + VIR_FREE(model); + VIR_FREE(vendor_path); + VIR_FREE(model_path); + if (!ret) + virSCSIDeviceFree(dev); + return ret; +} + +void +virSCSIDeviceFree(virSCSIDevicePtr dev) +{ + if (!dev) + return; + + VIR_FREE(dev->id); + VIR_FREE(dev->name); + VIR_FREE(dev->path); + VIR_FREE(dev); +} + +void +virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, + const char *name) +{ + dev->used_by = name; +} + +const char * +virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev) +{ + return dev->used_by; +} + +const char * +virSCSIDeviceGetName(virSCSIDevicePtr dev) +{ + return dev->name; +} + +unsigned int +virSCSIDeviceGetAdapter(virSCSIDevicePtr dev) +{ + return dev->adapter; +} + +unsigned int +virSCSIDeviceGetBus(virSCSIDevicePtr dev) +{ + return dev->bus; +} + +unsigned int +virSCSIDeviceGetTarget(virSCSIDevicePtr dev) +{ + return dev->target; +} + +unsigned int +virSCSIDeviceGetUnit(virSCSIDevicePtr dev) +{ + return dev->unit; +} + +bool +virSCSIDeviceGetReadonly(virSCSIDevicePtr dev) +{ + return dev->readonly; +} + +int +virSCSIDeviceFileIterate(virSCSIDevicePtr dev, + virSCSIDeviceFileActor actor, + void *opaque) +{ + return (actor)(dev, dev->path, opaque); +} + +virSCSIDeviceListPtr +virSCSIDeviceListNew(void) +{ + virSCSIDeviceListPtr list; + + if (virSCSIInitialize() < 0) + return NULL; + + if (!(list = virObjectLockableNew(virSCSIDeviceListClass))) + return NULL; + + return list; +} + +static void +virSCSIDeviceListDispose(void *obj) +{ + virSCSIDeviceListPtr list = obj; + int i; + + for (i = 0; i < list->count; i++) + virSCSIDeviceFree(list->devs[i]); + + VIR_FREE(list->devs); +} + +int +virSCSIDeviceListAdd(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + if (virSCSIDeviceListFind(list, dev)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device %s already exists"), + dev->name); + return -1; + } + + if (VIR_REALLOC_N(list->devs, list->count + 1) < 0) { + virReportOOMError(); + return -1; + } + + list->devs[list->count++] = dev; + + return 0; +} + +virSCSIDevicePtr +virSCSIDeviceListGet(virSCSIDeviceListPtr list, int idx) +{ + if (idx >= list->count || idx < 0) + return NULL; + + return list->devs[idx]; +} + +int +virSCSIDeviceListCount(virSCSIDeviceListPtr list) +{ + return list->count; +} + +virSCSIDevicePtr +virSCSIDeviceListSteal(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + virSCSIDevicePtr ret = NULL; + int i; + + for (i = 0; i < list->count; i++) { + if (list->devs[i]->adapter != dev->adapter || + list->devs[i]->bus != dev->bus || + list->devs[i]->target != dev->target || + list->devs[i]->unit != dev->unit) + continue; + + ret = list->devs[i]; + + if (i != list->count--) + memmove(&list->devs[i], + &list->devs[i+1], + sizeof(*list->devs) * (list->count - i)); + + if (VIR_REALLOC_N(list->devs, list->count) < 0) { + ; /* not fatal */ + } + + break; + } + + return ret; +} + +void +virSCSIDeviceListDel(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev); + virSCSIDeviceFree(ret); +} + +virSCSIDevicePtr +virSCSIDeviceListFind(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev) +{ + int i; + + for (i = 0; i < list->count; i++) { + if (list->devs[i]->adapter == dev->adapter && + list->devs[i]->bus == dev->bus && + list->devs[i]->target == dev->target && + list->devs[i]->unit == dev->unit) + return list->devs[i]; + } + + return NULL; +} diff --git a/src/util/virscsi.h b/src/util/virscsi.h new file mode 100644 index 0000000..9d63acc --- /dev/null +++ b/src/util/virscsi.h @@ -0,0 +1,84 @@ +/* + * virscsi.h: helper APIs for managing host SCSI devices + * + * Copyright (C) 2013 Fujitsu, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + * + * Authors: + * Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; + */ + +#ifndef __VIR_SCSI_H__ +# define __VIR_SCSI_H__ + +# include "internal.h" +# include "virobject.h" + +typedef struct _virSCSIDevice virSCSIDevice; +typedef virSCSIDevice *virSCSIDevicePtr; + +typedef struct _virSCSIDeviceList virSCSIDeviceList; +typedef virSCSIDeviceList *virSCSIDeviceListPtr; + +char *virSCSIDeviceGetDevStr(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit); + +virSCSIDevicePtr virSCSIDeviceNew(const char *adapter, + unsigned int bus, + unsigned int target, + unsigned int unit, + bool readonly); + +void virSCSIDeviceFree(virSCSIDevicePtr dev); +void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name); +const char *virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev); +const char *virSCSIDeviceGetName(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev); +unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev); +bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev); + +/* + * Callback that will be invoked once for each file + * associated with / used for SCSI host device access. + * + * Should return 0 if successfully processed, or + * -1 to indicate error and abort iteration + */ +typedef int (*virSCSIDeviceFileActor)(virSCSIDevicePtr dev, + const char *path, void *opaque); + +int virSCSIDeviceFileIterate(virSCSIDevicePtr dev, + virSCSIDeviceFileActor actor, + void *opaque); + +virSCSIDeviceListPtr virSCSIDeviceListNew(void); +int virSCSIDeviceListAdd(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev); +virSCSIDevicePtr virSCSIDeviceListGet(virSCSIDeviceListPtr list, + int idx); +int virSCSIDeviceListCount(virSCSIDeviceListPtr list); +virSCSIDevicePtr virSCSIDeviceListSteal(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev); +void virSCSIDeviceListDel(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev); +virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list, + virSCSIDevicePtr dev); + +#endif /* __VIR_SCSI_H__ */ -- 1.8.1.4

On 07/05/13 01:20, John Ferlan wrote: No, forcely checking it when parsing is not a good idea, just for QEMU/KVM driver, we want it has a prefix "scsi_host", it might be different for other drivers, though currently we only support qemu driver. Even checking it here is not that good, but given that this helper is only used by qemu driver now, I think it's fine, it can be changed if need in future. Agreed As said in commit log, I'm thinking about having a common enough class for the list, or may be in another way, but it will be later patch, cleaning up vir{pci,usb}.[ch] together. Osier

On 13/05/13 18:42, Osier Yang wrote: With 15/25 merged.

On 05/03/2013 02:07 PM, Osier Yang wrote: Might be nice to include this mapping in the code comments especially for those looking for this type of information some day that may not think to look in a git commit log... VIR_FREE(sg); No comments on remainder. I assume the test config is good - at least it's there! ACK John

On 07/05/13 01:37, John Ferlan wrote: Fixed, and pushed.

On 05/03/2013 02:07 PM, Osier Yang wrote: I just realized your example in 1/25 has the 'readonly' element listed in the new "<pre> ... </pre> section, but it's not described. I think you probably need to split that line out of there and then put it into this patch. Does anything need to be said here that the underlying QEMU needs to support the readonly too? That is some version or capability? Since there is a check later on to actually pass/add readonly to the command line, it'd surely raise a question some day if someone defined it as readonly, but then come to find out the domain didn't honor that... See comment above. If this is defined as readonly, but the underlying system doesn't support it, then should we fail? It'd be bad news to have a domain expect something readonly, but still load and potentially write on something it wasn't supposed to. If there's a precedent already that allows a defined readonly element to be used read/write, then I guess it's a weak ACK, but otherwise, I think it might be best to fail. John

On 07/05/13 18:58, Osier Yang wrote: Pushed with the nits fixed.

From: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; This adds the scsi-generic device into the device controller's whitelist, so that it's allowed to used by the qemu process. Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; Signed-off-by: Osier Yang <jyang(a)redhat.com&gt; --- v2.5 - v3: * Splitted from 8/10 of v2.5 * Don't forget the other backends (DAC, and apparmor) --- src/qemu/qemu_cgroup.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 4819cfe..9dd3104 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -31,6 +31,7 @@ #include "viralloc.h" #include "virerror.h" #include "domain_audit.h" +#include "virscsi.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -213,6 +214,33 @@ qemuSetupHostUsbDeviceCgroup(virUSBDevicePtr dev ATTRIBUTE_UNUSED, return 0; } +static int +qemuSetupHostScsiDeviceCgroup(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, + const char *path, + void *opaque) +{ + virDomainObjPtr vm = opaque; + qemuDomainObjPrivatePtr priv = vm->privateData; + int rc; + + VIR_DEBUG("Process path '%s' for SCSI device", path); + + rc = virCgroupAllowDevicePath(priv->cgroup, path, + virSCSIDeviceGetReadonly(dev) ? + VIR_CGROUP_DEVICE_READ : + VIR_CGROUP_DEVICE_RW); + + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, + virSCSIDeviceGetReadonly(dev) ? "r" : "rw", rc); + if (rc < 0) { + virReportSystemError(-rc, + _("Unable to allow device %s"), + path); + return -1; + } + + return 0; +} int qemuSetupHostdevCGroup(virDomainObjPtr vm, @@ -222,6 +250,7 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; virPCIDevicePtr pci = NULL; virUSBDevicePtr usb = NULL; + virSCSIDevicePtr scsi = NULL; char *path = NULL; /* currently this only does something for PCI devices using vfio @@ -286,6 +315,20 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, goto cleanup; } break; + + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if ((scsi = virSCSIDeviceNew(dev->source.subsys.u.scsi.adapter, + dev->source.subsys.u.scsi.bus, + dev->source.subsys.u.scsi.target, + dev->source.subsys.u.scsi.unit, + dev->readonly)) == NULL) + goto cleanup; + + if (virSCSIDeviceFileIterate(scsi, + qemuSetupHostScsiDeviceCgroup, + vm) < 0) + goto cleanup; + default: break; } @@ -295,12 +338,11 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm, cleanup: virPCIDeviceFree(pci); virUSBDeviceFree(usb); + virSCSIDeviceFree(scsi); VIR_FREE(path); return ret; } - - int qemuTeardownHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) -- 1.8.1.4

From: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; Although virtio-scsi supports SCSI PR, the device on host may do not support it. To avoid losing data, we only allow one scsi hostdev to be passthroughed to one live guest, Just like what we do for PCI and USB devices. Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; --- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_driver.c | 3 + src/qemu/qemu_hostdev.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hostdev.h | 10 +++ src/qemu/qemu_process.c | 3 + 5 files changed, 231 insertions(+) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 77d3d2f..30a2a22 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -36,6 +36,7 @@ # include "security/security_manager.h" # include "virpci.h" # include "virusb.h" +# include "virscsi.h" # include "cpu_conf.h" # include "driver.h" # include "virportallocator.h" @@ -203,6 +204,7 @@ struct _virQEMUDriver { virPCIDeviceListPtr activePciHostdevs; virPCIDeviceListPtr inactivePciHostdevs; virUSBDeviceListPtr activeUsbHostdevs; + virSCSIDeviceListPtr activeScsiHostdevs; /* Immutable pointer. Unsafe APIs. XXX */ virHashTablePtr sharedDisks; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d5d7de3..b631a1f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -675,6 +675,9 @@ qemuStateInitialize(bool privileged, if ((qemu_driver->inactivePciHostdevs = virPCIDeviceListNew()) == NULL) goto error; + if (!(qemu_driver->activeScsiHostdevs = virSCSIDeviceListNew())) + goto error; + if (!(qemu_driver->sharedDisks = virHashCreate(30, qemuSharedDiskEntryFree))) goto error; diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index e4af036..d5f94d5 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -29,6 +29,7 @@ #include "viralloc.h" #include "virpci.h" #include "virusb.h" +#include "virscsi.h" #include "virnetdev.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -226,6 +227,47 @@ cleanup: return ret; } +int +qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver, + virDomainDefPtr def) +{ + virDomainHostdevDefPtr hostdev = NULL; + int i; + int ret = -1; + + if (!def->nhostdevs) + return 0; + + virObjectLock(driver->activeScsiHostdevs); + for (i = 0; i < def->nhostdevs; i++) { + virSCSIDevicePtr scsi = NULL; + hostdev = def->hostdevs[i]; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + continue; + + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly))) + goto cleanup; + + virSCSIDeviceSetUsedBy(scsi, def->name); + + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) { + virSCSIDeviceFree(scsi); + goto cleanup; + } + } + ret = 0; + +cleanup: + virObjectUnlock(driver->activeScsiHostdevs); + return ret; +} + static int qemuDomainHostdevPciSysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) { @@ -827,6 +869,107 @@ cleanup: return ret; } +int +qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + int i, j, count; + virSCSIDeviceListPtr list; + virSCSIDevicePtr tmp; + + /* To prevent situation where SCSI device is assigned to two domains + * we need to keep a list of currently assigned SCSI devices. + * This is done in several loops which cannot be joined into one big + * loop. See qemuPrepareHostdevPCIDevices() + */ + if (!(list = virSCSIDeviceListNew())) + goto cleanup; + + /* Loop 1: build temporary list */ + for (i = 0 ; i < nhostdevs ; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + virSCSIDevicePtr scsi; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + continue; + + if (hostdev->managed) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("SCSI host device doesn't support managed mode")); + goto cleanup; + } + + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly))) + goto cleanup; + + if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) { + virSCSIDeviceFree(scsi); + goto cleanup; + } + } + + /* Loop 2: Mark devices in temporary list as used by @name + * and add them to driver list. However, if something goes + * wrong, perform rollback. + */ + virObjectLock(driver->activeScsiHostdevs); + count = virSCSIDeviceListCount(list); + + for (i = 0; i < count; i++) { + virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i); + if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) { + const char *other_name = virSCSIDeviceGetUsedBy(tmp); + + if (other_name) + virReportError(VIR_ERR_OPERATION_INVALID, + _("SCSI device %s is in use by domain %s"), + virSCSIDeviceGetName(tmp), other_name); + else + virReportError(VIR_ERR_OPERATION_INVALID, + _("SCSI device %s is already in use"), + virSCSIDeviceGetName(tmp)); + goto error; + } + + virSCSIDeviceSetUsedBy(scsi, name); + VIR_DEBUG("Adding %s to activeScsiHostdevs", virSCSIDeviceGetName(scsi)); + + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) + goto error; + } + + virObjectUnlock(driver->activeScsiHostdevs); + + /* Loop 3: Temporary list was successfully merged with + * driver list, so steal all items to avoid freeing them + * when freeing temporary list. + */ + while (virSCSIDeviceListCount(list) > 0) { + tmp = virSCSIDeviceListGet(list, 0); + virSCSIDeviceListSteal(list, tmp); + } + + virObjectUnref(list); + return 0; + +error: + for (j = 0; j < i; j++) { + tmp = virSCSIDeviceListGet(list, i); + virSCSIDeviceListSteal(driver->activeScsiHostdevs, tmp); + } + virObjectUnlock(driver->activeScsiHostdevs); +cleanup: + virObjectUnref(list); + return -1; +} + int qemuPrepareHostDevices(virQEMUDriverPtr driver, virDomainDefPtr def, bool coldBoot) @@ -841,6 +984,10 @@ int qemuPrepareHostDevices(virQEMUDriverPtr driver, if (qemuPrepareHostUSBDevices(driver, def, coldBoot) < 0) return -1; + if (qemuPrepareHostdevSCSIDevices(driver, def->name, + def->hostdevs, def->nhostdevs) < 0) + return -1; + return 0; } @@ -1025,6 +1172,69 @@ qemuDomainReAttachHostUsbDevices(virQEMUDriverPtr driver, virObjectUnlock(driver->activeUsbHostdevs); } +void +qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + int i; + + virObjectLock(driver->activeScsiHostdevs); + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + virSCSIDevicePtr scsi, tmp; + const char *used_by = NULL; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + continue; + + if (!(scsi = virSCSIDeviceNew(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + hostdev->readonly))) { + VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain %s", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + name); + continue; + } + + /* Only delete the devices which are marked as being used by @name, + * because qemuProcessStart could fail on the half way. */ + + tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi); + virSCSIDeviceFree(scsi); + + if (!tmp) { + VIR_WARN("Unable to find device %s:%d:%d:%d " + "in list of active SCSI devices", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit); + continue; + } + + used_by = virSCSIDeviceGetUsedBy(tmp); + if (STREQ_NULLABLE(used_by, name)) { + VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs", + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit, + name); + + virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp); + } + } + virObjectUnlock(driver->activeScsiHostdevs); +} + void qemuDomainReAttachHostDevices(virQEMUDriverPtr driver, virDomainDefPtr def) { @@ -1036,4 +1246,7 @@ void qemuDomainReAttachHostDevices(virQEMUDriverPtr driver, qemuDomainReAttachHostUsbDevices(driver, def->name, def->hostdevs, def->nhostdevs); + + qemuDomainReAttachHostScsiDevices(driver, def->name, def->hostdevs, + def->nhostdevs); } diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index a1b8b9e..327d4d5 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -31,6 +31,8 @@ int qemuUpdateActivePciHostdevs(virQEMUDriverPtr driver, virDomainDefPtr def); int qemuUpdateActiveUsbHostdevs(virQEMUDriverPtr driver, virDomainDefPtr def); +int qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver, + virDomainDefPtr def); int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver, const char *name, const unsigned char *uuid, @@ -42,9 +44,17 @@ int qemuFindHostdevUSBDevice(virDomainHostdevDefPtr hostdev, int qemuPrepareHostdevUSBDevices(virQEMUDriverPtr driver, const char *name, virUSBDeviceListPtr list); +int qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs); int qemuPrepareHostDevices(virQEMUDriverPtr driver, virDomainDefPtr def, bool coldBoot); +void qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs); void qemuReattachPciDevice(virPCIDevicePtr dev, virQEMUDriverPtr driver); void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver, const char *name, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3dd178c..ff5fc89 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2972,6 +2972,9 @@ qemuProcessReconnect(void *opaque) if (qemuUpdateActiveUsbHostdevs(driver, obj->def) < 0) goto error; + if (qemuUpdateActiveScsiHostdevs(driver, obj->def) < 0) + goto error; + if (qemuInitCgroup(driver, obj, false) < 0) goto error; -- 1.8.1.4

On 07/05/13 18:57, John Ferlan wrote: Yes, I should make it more clear. Okay. Personally I like "!", but I don't have a good reason to keep it, and keep consistence sounds good. Another personal habit I think, though I prefer to put them together too. The VIR_WARN is useless, as virSCSIDeviceNew has errors. It's not *taken* from list, I.E. list still keep the pointers, so they are just stealed (let's say detached) from activeList, and virObjectUnref will takes care of freeing them. That's why I don't like the vir{pci,usb}.[ch], it causes a lot of mess. virObjectUnref will do it, virSCSIDeviceListDispose is involked. Osier

On 05/03/2013 02:07 PM, Osier Yang wrote: Seems this is a bug fix that potentially could be separated from this patch stream... Right? How is cleanup affected if the next call fails? That is we have something on 'list', but not on the hostdev, so wouldn't the device need to be taken from there... Perhaps it's just Unref of list takes care of that - something I'm not yet clear on. Moving the above hunk causes no "ordering" issues with the following two calls, right? Since you're placing USB devices on the hostdev list in the hunk and that won't happen until afterwards now thus the CGroup and HostdevLabel adjustments below will possibly have different results, right? John

From: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; This adds both attachment and detachment support for scsi host device. Signed-off-by: Han Cheng <hanc.fnst(a)cn.fujitsu.com&gt; Signed-off-by: Osier Yang <jyang@redhat> --- src/qemu/qemu_hotplug.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 136 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 422d336..e6bc3a2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1194,6 +1194,74 @@ cleanup: return ret; } +static int +qemuDomainAttachHostScsiDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + char *devstr = NULL; + char *drvstr = NULL; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE) || + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) || + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SCSI passthrough is not supported by this version of qemu")); + return -1; + } + + if (qemuPrepareHostdevSCSIDevices(driver, vm->def->name, + &hostdev, 1)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to prepare scsi hostdev: %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); + return -1; + } + + if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, 0) < 0) + goto cleanup; + + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) + goto cleanup; + + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, hostdev, priv->qemuCaps))) + goto cleanup; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + qemuDomainObjEnterMonitor(driver, vm); + if ((ret = qemuMonitorAddDrive(priv->mon, drvstr)) == 0) { + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) { + VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", + drvstr, devstr); + qemuMonitorDriveDel(priv->mon, drvstr); + } + } + qemuDomainObjExitMonitor(driver, vm); + + virDomainAuditHostdev(vm, hostdev, "attach", ret == 0); + if (ret < 0) + goto cleanup; + + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; + + ret = 0; +cleanup: + if (ret < 0) + qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &hostdev, 1); + VIR_FREE(drvstr); + VIR_FREE(devstr); + return ret; +} + int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) @@ -1225,6 +1293,12 @@ int qemuDomainAttachHostDevice(virQEMUDriverPtr driver, goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + if (qemuDomainAttachHostScsiDevice(driver, vm, + hostdev) < 0) + goto error; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -2441,11 +2515,59 @@ qemuDomainDetachHostUsbDevice(virQEMUDriverPtr driver, return ret; } -static -int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainHostdevDefPtr detach, - int idx) +static int +qemuDomainDetachHostScsiDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *drvstr = NULL; + char *devstr = NULL; + int ret = -1; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached with this QEMU version")); + return -1; + } + + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(detach, priv->qemuCaps))) + goto cleanup; + if (!(devstr = qemuBuildSCSIHostdevDevStr(vm->def, detach, priv->qemuCaps))) + goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + if ((ret = qemuMonitorDelDevice(priv->mon, detach->info->alias)) == 0) { + if ((ret = qemuMonitorDriveDel(priv->mon, drvstr)) < 0) { + VIR_WARN("qemuMonitorDriveDel failed on %s (%s)", + detach->info->alias, drvstr); + qemuMonitorAddDevice(priv->mon, devstr); + } + } + qemuDomainObjExitMonitor(driver, vm); + + virDomainAuditHostdev(vm, detach, "detach", ret == 0); + + if (ret == 0) + qemuDomainReAttachHostScsiDevices(driver, vm->def->name, &detach, 1); + +cleanup: + VIR_FREE(drvstr); + VIR_FREE(devstr); + return ret; +} + +static int +qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach, + int idx) { int ret = -1; @@ -2472,6 +2594,9 @@ int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: ret = qemuDomainDetachHostUsbDevice(driver, vm, detach); break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + ret = qemuDomainDetachHostScsiDevice(driver, vm, detach); + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -2531,6 +2656,12 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, subsys->u.usb.vendor, subsys->u.usb.product); } break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + virReportError(VIR_ERR_OPERATION_FAILED, + _("host scsi device %s:%d:%d.%d not found"), + subsys->u.scsi.adapter, subsys->u.scsi.bus, + subsys->u.scsi.target, subsys->u.scsi.unit); + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected hostdev type %d"), subsys->type); -- 1.8.1.4

On 07/05/13 20:42, John Ferlan wrote: I think you meant keeping the original error? here is the diff:

On 14/05/13 00:08, John Ferlan wrote: Thanks, pushed with the diff.

On 05/03/2013 02:07 PM, Osier Yang wrote: s/algrithom/algorithm s/it's another story/that's a different issue Implicit creation I would expect 16 and 7 to be constants The 'max_unit' check is irrelevant if max_unit=7, then we're not getting here anyway... This ends up being Scsi specific right? E.G. AssignSCSIAddress instead of generic AssignAddress (or wherever the "best" place is to put SCSI) s/;/ = 0; Guess I would have figured these would be constants somewhere... Not that I expect the values to change... Units are numbered 0 -> 15 and 0 -> 6 then? If we're "implicitly" creating a new controller, then is it's number based on the total number of controllers or the next scsi controller? To be fair, I'm not familiar (yet) with how things are laid out, but it would seem to me that this might not be returning what you want/expect. I'm missing a nuance here - the 'controller' field is generally based off the def->controllers[i]->idx value, except when we're "implicitly creating" one. In this case it's just a 'random' value based on some count of existing nscsi_controllers. If there are zero existing controllers, then we start at 1 If there is one existing/full controller, then we start at 2 How does the 'idx' value get populated? Is it possible that this setting conflicts with a different and non scsi controller? Is there a max controller number we shouldn't go past? John

Unlike disk device, the scsi-generic always writethrough the data, so no need to introduce a "cache" tag, and set "cache=off". --- docs/formatdomain.html.in | 6 ++++ docs/schemas/domaincommon.rng | 5 +++ src/conf/domain_conf.c | 4 +++ src/conf/domain_conf.h | 1 + .../qemuxml2argv-hostdev-scsi-shareable.xml | 36 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 53 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-shareable.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9cd79e5..6437d6d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2410,6 +2410,12 @@ <dd>Indicates that the device is readonly, only supported by SCSI host device now. <span class="since">Since 1.0.6</span> </dd> + <dt><code>shareable</code></dt> + <dd>If present, this indicates the device is expected to be shared + between domains (assuming the hypervisor and OS support this). + Only supported by SCSI host device now. + <span class="since">Since 1.0.6</span> + </dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4fdacab..ca79e67 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3080,6 +3080,11 @@ <empty/> </element> </optional> + <optional> + <element name="shareable"> + <empty/> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cff2b46..e7b096b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8860,6 +8860,8 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, if (virXPathBoolean("boolean(./readonly)", ctxt)) def->readonly = true; + if (virXPathBoolean("boolean(./shareable)", ctxt)) + def->shareable = true; break; } } @@ -15482,6 +15484,8 @@ virDomainHostdevDefFormat(virBufferPtr buf, if (def->readonly) virBufferAddLit(buf, "<readonly/>\n"); + if (def->shareable) + virBufferAddLit(buf, "<shareable/>\n"); virBufferAdjustIndent(buf, -6); if (virDomainDeviceInfoFormat(buf, def->info, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5471cd3..7073967 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -462,6 +462,7 @@ struct _virDomainHostdevDef { bool managed; bool missing; bool readonly; + bool shareable; union { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-shareable.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-shareable.xml new file mode 100644 index 0000000..f9ce8af --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-shareable.xml @@ -0,0 +1,36 @@ +<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> + <shareable/> + <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 492ac60..d37a657 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -288,6 +288,7 @@ mymain(void) DO_TEST("hostdev-scsi-lsi"); DO_TEST("hostdev-scsi-virtio-scsi"); DO_TEST("hostdev-scsi-readonly"); + DO_TEST("hostdev-scsi-shareable"); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); -- 1.8.1.4

On 07/05/13 21:45, John Ferlan wrote: Thanks, pushed with the change.

"Shared disk" is not only the thing we should care about after "scsi hostdev" is introduced. A same scsi device can be used as "disk" for one domain, and as "scsi hostdev" for another domain at the same time. That's why this patch renames qemu_driver->sharedDisks. Related functions and structs are also renamed. --- src/qemu/qemu_conf.c | 76 ++++++++++++++++++++++++------------------------- src/qemu/qemu_conf.h | 16 +++++------ src/qemu/qemu_driver.c | 4 +-- src/qemu/qemu_process.c | 2 +- 4 files changed, 49 insertions(+), 49 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index c16b90d..244795d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -915,23 +915,23 @@ virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks, VIR_FREE(list); } -struct _qemuSharedDiskEntry { +struct _qemuSharedDeviceEntry { size_t ref; char **domains; /* array of domain names */ }; -/* Construct the hash key for sharedDisks as "major:minor" */ +/* Construct the hash key for sharedDevices as "major:minor" */ char * -qemuGetSharedDiskKey(const char *disk_path) +qemuGetSharedDeviceKey(const char *device_path) { int maj, min; char *key = NULL; int rc; - if ((rc = virGetDeviceID(disk_path, &maj, &min)) < 0) { + if ((rc = virGetDeviceID(device_path, &maj, &min)) < 0) { virReportSystemError(-rc, _("Unable to get minor number of device '%s'"), - disk_path); + device_path); return NULL; } @@ -951,7 +951,7 @@ qemuGetSharedDiskKey(const char *disk_path) * Returns 0 if no conflicts, otherwise returns -1. */ static int -qemuCheckSharedDisk(virHashTablePtr sharedDisks, +qemuCheckSharedDisk(virHashTablePtr sharedDevices, virDomainDiskDefPtr disk) { char *sysfs_path = NULL; @@ -976,7 +976,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDisks, if (!virFileExists(sysfs_path)) goto cleanup; - if (!(key = qemuGetSharedDiskKey(disk->src))) { + if (!(key = qemuGetSharedDeviceKey(disk->src))) { ret = -1; goto cleanup; } @@ -984,7 +984,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDisks, /* It can't be conflict if no other domain is * is sharing it. */ - if (!(virHashLookup(sharedDisks, key))) + if (!(virHashLookup(sharedDevices, key))) goto cleanup; if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) { @@ -1020,9 +1020,9 @@ cleanup: } bool -qemuSharedDiskEntryDomainExists(qemuSharedDiskEntryPtr entry, - const char *name, - int *idx) +qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry, + const char *name, + int *idx) { size_t i; @@ -1038,9 +1038,9 @@ qemuSharedDiskEntryDomainExists(qemuSharedDiskEntryPtr entry, } void -qemuSharedDiskEntryFree(void *payload, const void *name ATTRIBUTE_UNUSED) +qemuSharedDeviceEntryFree(void *payload, const void *name ATTRIBUTE_UNUSED) { - qemuSharedDiskEntryPtr entry = payload; + qemuSharedDeviceEntryPtr entry = payload; size_t i; if (!entry) @@ -1053,10 +1053,10 @@ qemuSharedDiskEntryFree(void *payload, const void *name ATTRIBUTE_UNUSED) VIR_FREE(entry); } -static qemuSharedDiskEntryPtr -qemuSharedDiskEntryCopy(const qemuSharedDiskEntryPtr entry) +static qemuSharedDeviceEntryPtr +qemuSharedDeviceEntryCopy(const qemuSharedDeviceEntryPtr entry) { - qemuSharedDiskEntryPtr ret = NULL; + qemuSharedDeviceEntryPtr ret = NULL; size_t i; if (VIR_ALLOC(ret) < 0) { @@ -1080,7 +1080,7 @@ qemuSharedDiskEntryCopy(const qemuSharedDiskEntryPtr entry) return ret; cleanup: - qemuSharedDiskEntryFree(ret, NULL); + qemuSharedDeviceEntryFree(ret, NULL); return NULL; } @@ -1098,8 +1098,8 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, const char *name) { - qemuSharedDiskEntry *entry = NULL; - qemuSharedDiskEntry *new_entry = NULL; + qemuSharedDeviceEntry *entry = NULL; + qemuSharedDeviceEntry *new_entry = NULL; char *key = NULL; int ret = -1; @@ -1116,47 +1116,47 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, return 0; qemuDriverLock(driver); - if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0) + if (qemuCheckSharedDisk(driver->sharedDevices, disk) < 0) goto cleanup; - if (!(key = qemuGetSharedDiskKey(disk->src))) + if (!(key = qemuGetSharedDeviceKey(disk->src))) goto cleanup; - if ((entry = virHashLookup(driver->sharedDisks, key))) { + if ((entry = virHashLookup(driver->sharedDevices, key))) { /* Nothing to do if the shared disk is already recorded * in the table. */ - if (qemuSharedDiskEntryDomainExists(entry, name, NULL)) { + if (qemuSharedDeviceEntryDomainExists(entry, name, NULL)) { ret = 0; goto cleanup; } - if (!(new_entry = qemuSharedDiskEntryCopy(entry))) + if (!(new_entry = qemuSharedDeviceEntryCopy(entry))) goto cleanup; if ((VIR_EXPAND_N(new_entry->domains, new_entry->ref, 1) < 0) || !(new_entry->domains[new_entry->ref - 1] = strdup(name))) { - qemuSharedDiskEntryFree(new_entry, NULL); + qemuSharedDeviceEntryFree(new_entry, NULL); virReportOOMError(); goto cleanup; } - if (virHashUpdateEntry(driver->sharedDisks, key, new_entry) < 0) { - qemuSharedDiskEntryFree(new_entry, NULL); + if (virHashUpdateEntry(driver->sharedDevices, key, new_entry) < 0) { + qemuSharedDeviceEntryFree(new_entry, NULL); goto cleanup; } } else { if ((VIR_ALLOC(entry) < 0) || (VIR_ALLOC_N(entry->domains, 1) < 0) || !(entry->domains[0] = strdup(name))) { - qemuSharedDiskEntryFree(entry, NULL); + qemuSharedDeviceEntryFree(entry, NULL); virReportOOMError(); goto cleanup; } entry->ref = 1; - if (virHashAddEntry(driver->sharedDisks, key, entry)) + if (virHashAddEntry(driver->sharedDevices, key, entry)) goto cleanup; } @@ -1181,8 +1181,8 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, const char *name) { - qemuSharedDiskEntryPtr entry = NULL; - qemuSharedDiskEntryPtr new_entry = NULL; + qemuSharedDeviceEntryPtr entry = NULL; + qemuSharedDeviceEntryPtr new_entry = NULL; char *key = NULL; int ret = -1; int idx; @@ -1196,22 +1196,22 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver, return 0; qemuDriverLock(driver); - if (!(key = qemuGetSharedDiskKey(disk->src))) + if (!(key = qemuGetSharedDeviceKey(disk->src))) goto cleanup; - if (!(entry = virHashLookup(driver->sharedDisks, key))) + if (!(entry = virHashLookup(driver->sharedDevices, key))) goto cleanup; /* Nothing to do if the shared disk is not recored in * the table. */ - if (!qemuSharedDiskEntryDomainExists(entry, name, &idx)) { + if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx)) { ret = 0; goto cleanup; } if (entry->ref != 1) { - if (!(new_entry = qemuSharedDiskEntryCopy(entry))) + if (!(new_entry = qemuSharedDeviceEntryCopy(entry))) goto cleanup; if (idx != new_entry->ref - 1) @@ -1221,12 +1221,12 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver, VIR_SHRINK_N(new_entry->domains, new_entry->ref, 1); - if (virHashUpdateEntry(driver->sharedDisks, key, new_entry) < 0){ - qemuSharedDiskEntryFree(new_entry, NULL); + if (virHashUpdateEntry(driver->sharedDevices, key, new_entry) < 0){ + qemuSharedDeviceEntryFree(new_entry, NULL); goto cleanup; } } else { - if (virHashRemoveEntry(driver->sharedDisks, key) < 0) + if (virHashRemoveEntry(driver->sharedDevices, key) < 0) goto cleanup; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 30a2a22..d4a54a0 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -207,7 +207,7 @@ struct _virQEMUDriver { virSCSIDeviceListPtr activeScsiHostdevs; /* Immutable pointer. Unsafe APIs. XXX */ - virHashTablePtr sharedDisks; + virHashTablePtr sharedDevices; /* Immutable pointer, self-locking APIs */ virPortAllocatorPtr remotePorts; @@ -275,12 +275,12 @@ void virQEMUCloseCallbacksRun(virQEMUCloseCallbacksPtr closeCallbacks, virConnectPtr conn, virQEMUDriverPtr driver); -typedef struct _qemuSharedDiskEntry qemuSharedDiskEntry; -typedef qemuSharedDiskEntry *qemuSharedDiskEntryPtr; +typedef struct _qemuSharedDeviceEntry qemuSharedDeviceEntry; +typedef qemuSharedDeviceEntry *qemuSharedDeviceEntryPtr; -bool qemuSharedDiskEntryDomainExists(qemuSharedDiskEntryPtr entry, - const char *name, - int *index) +bool qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry, + const char *name, + int *index) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuAddSharedDisk(virQEMUDriverPtr driver, @@ -293,10 +293,10 @@ int qemuRemoveSharedDisk(virQEMUDriverPtr driver, const char *name) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); -char * qemuGetSharedDiskKey(const char *disk_path) +char * qemuGetSharedDeviceKey(const char *disk_path) ATTRIBUTE_NONNULL(1); -void qemuSharedDiskEntryFree(void *payload, const void *name) +void qemuSharedDeviceEntryFree(void *payload, const void *name) ATTRIBUTE_NONNULL(1); int qemuDriverAllocateID(virQEMUDriverPtr driver); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b631a1f..4c2ab7b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -678,7 +678,7 @@ qemuStateInitialize(bool privileged, if (!(qemu_driver->activeScsiHostdevs = virSCSIDeviceListNew())) goto error; - if (!(qemu_driver->sharedDisks = virHashCreate(30, qemuSharedDiskEntryFree))) + if (!(qemu_driver->sharedDevices = virHashCreate(30, qemuSharedDeviceEntryFree))) goto error; if (privileged) { @@ -949,7 +949,7 @@ qemuStateCleanup(void) { virObjectUnref(qemu_driver->activePciHostdevs); virObjectUnref(qemu_driver->inactivePciHostdevs); virObjectUnref(qemu_driver->activeUsbHostdevs); - virHashFree(qemu_driver->sharedDisks); + virHashFree(qemu_driver->sharedDevices); virObjectUnref(qemu_driver->caps); virQEMUCapsCacheFree(qemu_driver->qemuCapsCache); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ff5fc89..70eadb6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2979,7 +2979,7 @@ qemuProcessReconnect(void *opaque) goto error; /* XXX: Need to change as long as lock is introduced for - * qemu_driver->sharedDisks. + * qemu_driver->sharedDevices. */ for (i = 0; i < obj->def->ndisks; i++) { if (qemuTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) -- 1.8.1.4

On 05/03/2013 02:07 PM, Osier Yang wrote: If 'name' then what - we go back to the top of the loop and if there's more entries then we'll strdup() another name.... Although I have to wonder if there can be more than non hidden entry in the list... Makes me wonder about my earlier reviews which didn't think about the loop exit (that is virSCSIDeviceGetSgName()) ACK with the proper exit strategy. John

On 07/05/13 22:51, Osier Yang wrote: Actually adding the break looks a bit strange, and I think we won't care it tries one more condition checking.. So I pushed this as-is.

On 05/03/2013 02:07 PM, Osier Yang wrote: And this is the controller that's being OR'd so I think this is right, but making sure :-) Over time HPVM was asked to allow a whole controller to be shareable because the guest would control/manage the luns... Same in reverse... ACK on the remainder John

This adds the shared device entry when starting domain (more exactly, when preparing host devices), and remove the entry when destroying domain (when reattaching host devices). --- src/qemu/qemu_hostdev.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index d5f94d5..f099a27 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -879,6 +879,19 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, virSCSIDeviceListPtr list; virSCSIDevicePtr tmp; + /* Loop 1: Add the shared scsi host device to shared device + * table. + */ + for (i = 0; i < nhostdevs; i++) { + virDomainDeviceDef dev; + + dev.type = VIR_DOMAIN_DEVICE_HOSTDEV; + dev.data.hostdev = hostdevs[i]; + + if (qemuAddSharedDevice(driver, &dev, name) < 0) + return -1; + } + /* To prevent situation where SCSI device is assigned to two domains * we need to keep a list of currently assigned SCSI devices. * This is done in several loops which cannot be joined into one big @@ -887,7 +900,7 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, if (!(list = virSCSIDeviceListNew())) goto cleanup; - /* Loop 1: build temporary list */ + /* Loop 2: build temporary list */ for (i = 0 ; i < nhostdevs ; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; virSCSIDevicePtr scsi; @@ -915,7 +928,7 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, } } - /* Loop 2: Mark devices in temporary list as used by @name + /* Loop 3: Mark devices in temporary list as used by @name * and add them to driver list. However, if something goes * wrong, perform rollback. */ @@ -947,7 +960,7 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver, virObjectUnlock(driver->activeScsiHostdevs); - /* Loop 3: Temporary list was successfully merged with + /* Loop 4: Temporary list was successfully merged with * driver list, so steal all items to avoid freeing them * when freeing temporary list. */ @@ -1185,6 +1198,12 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev = hostdevs[i]; virSCSIDevicePtr scsi, tmp; const char *used_by = NULL; + virDomainDeviceDef dev; + + dev.type = VIR_DOMAIN_DEVICE_HOSTDEV; + dev.data.hostdev = hostdev; + + ignore_value(qemuRemoveSharedDevice(driver, &dev, name)); if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) -- 1.8.1.4

"sgio" is only valid for scsi host device. --- docs/formatdomain.html.in | 7 ++++- docs/schemas/domaincommon.rng | 8 +++++ src/conf/domain_conf.c | 34 ++++++++++++++++++--- src/conf/domain_conf.h | 1 + .../qemuxml2argv-hostdev-scsi-sgio.xml | 35 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 6 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-sgio.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6437d6d..89d11e7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2319,7 +2319,12 @@ and <code>virNodeDeviceReAttach</code> (or <code>virsh nodedev-reattach</code>) after hot-unplug or stopping the guest. For SCSI device, user is responsible to make sure the device - is not used by host.</dd> + is not used by host. + The optional <code>sgio</code> (<span class="since">since 1.0.6</span>) + attribute indicates whether the kernel will filter unprivileged + SG_IO commands for the disk, valid settings are "filtered" or + "unfiltered". Defaults to "filtered". + </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 diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ca79e67..0a59c7a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3179,6 +3179,14 @@ <attribute name="type"> <value>scsi</value> </attribute> + <optional> + <attribute name="sgio"> + <choice> + <value>filtered</value> + <value>unfiltered</value> + </choice> + </attribute> + </optional> <element name="source"> <interleave> <ref name="sourceinfoadapter"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9823b9c..d604e5b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3784,6 +3784,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, { xmlNodePtr sourcenode; char *managed = NULL; + char *sgio = NULL; char *backendStr = NULL; int backend; int ret = -1; @@ -3798,6 +3799,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, def->managed = true; } + sgio = virXMLPropString(node, "sgio"); + /* @type is passed in from the caller rather than read from the * xml document, because it is specified in different places for * different kinds of defs - it is an attribute of @@ -3834,6 +3837,22 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, goto error; } + if (sgio) { + if (def->source.subsys.type != + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("sgio is only supported for scsi host device")); + goto error; + } + + if ((def->source.subsys.u.scsi.sgio = + virDomainDeviceSGIOTypeFromString(sgio)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown sgio mode '%s'"), sgio); + goto error; + } + } + switch (def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (virDomainHostdevSubsysPciDefParseXML(sourcenode, def, flags) < 0) @@ -3872,6 +3891,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, ret = 0; error: VIR_FREE(managed); + VIR_FREE(sgio); VIR_FREE(backendStr); return ret; } @@ -15464,11 +15484,17 @@ virDomainHostdevDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <hostdev mode='%s' type='%s'", mode, type); - if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) - virBufferAsprintf(buf, " managed='%s'>\n", + if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + virBufferAsprintf(buf, " managed='%s'", def->managed ? "yes" : "no"); - else - virBufferAddLit(buf, ">\n"); + + if (def->source.subsys.type == + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + def->source.subsys.u.scsi.sgio) + virBufferAsprintf(buf, " sgio='%s'", + virDomainDeviceSGIOTypeToString(def->source.subsys.u.scsi.sgio)); + } + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 6); switch (def->mode) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 59cf6a7..8d67e67 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -423,6 +423,7 @@ struct _virDomainHostdevSubsys { unsigned bus; unsigned target; unsigned unit; + int sgio; /* enum virDomainDeviceSGIO */ } scsi; } u; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-sgio.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-sgio.xml new file mode 100644 index 0000000..21404ee --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-sgio.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' sgio='unfiltered'> + <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 d37a657..04be8dd 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -289,6 +289,7 @@ mymain(void) DO_TEST("hostdev-scsi-virtio-scsi"); DO_TEST("hostdev-scsi-readonly"); DO_TEST("hostdev-scsi-shareable"); + DO_TEST("hostdev-scsi-sgio"); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); -- 1.8.1.4

On 08/05/13 03:17, John Ferlan wrote: I'd like keep it, one can also define "filtered" mode in the same XML for a different device. Pushed.

Just like what previous patches do, it refactors qemuSetUnprivSGIO to take the virDomainDeviceDefPtr as argument instead. --- src/qemu/qemu_conf.c | 67 ++++++++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_process.c | 2 +- 4 files changed, 58 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b7dea57..cf1c7ee 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1309,39 +1309,82 @@ cleanup: } int -qemuSetUnprivSGIO(virDomainDiskDefPtr disk) +qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) { + virDomainDiskDefPtr disk = NULL; + virDomainHostdevDefPtr hostdev = NULL; char *sysfs_path = NULL; + char *path = NULL; + char *hostdev_name = NULL; + char *hostdev_path = NULL; int val = -1; int ret = 0; /* "sgio" is only valid for block disk; cdrom * and floopy disk can have empty source. */ - if (!disk->src || - disk->device != VIR_DOMAIN_DISK_DEVICE_LUN || - (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && - disk->srcpool && - disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + disk = dev->data.disk; + + if (!disk->src || + disk->device != VIR_DOMAIN_DISK_DEVICE_LUN || + (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + !(disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME && + disk->srcpool && + disk->srcpool->voltype == VIR_STORAGE_VOL_BLOCK))) + return 0; + + path = disk->src; + } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { + hostdev = dev->data.hostdev; + + if (!hostdev->shareable || + (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)) + return 0; + + if (!(hostdev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit))) + goto cleanup; + + if (virAsprintf(&hostdev_path, "/dev/%s", hostdev_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + path = hostdev_path; + } else { return 0; + } - sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL); - if (sysfs_path == NULL) - return -1; + sysfs_path = virGetUnprivSGIOSysfsPath(path, NULL); + if (sysfs_path == NULL) { + ret = -1; + goto cleanup; + } /* By default, filter the SG_IO commands, i.e. set unpriv_sgio to 0. */ - val = (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED); + + if (dev->type == VIR_DOMAIN_DEVICE_DISK) + val = (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED); + else + val = (hostdev->source.subsys.u.scsi.sgio == + VIR_DOMAIN_DEVICE_SGIO_UNFILTERED); /* Do not do anything if unpriv_sgio is not supported by the kernel and the * whitelist is enabled. But if requesting unfiltered access, always call * virSetDeviceUnprivSGIO, to report an error for unsupported unpriv_sgio. */ if ((virFileExists(sysfs_path) || val == 1) && - virSetDeviceUnprivSGIO(disk->src, NULL, val) < 0) + virSetDeviceUnprivSGIO(path, NULL, val) < 0) ret = -1; +cleanup: VIR_FREE(sysfs_path); + VIR_FREE(hostdev_name); + VIR_FREE(hostdev_path); return ret; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 5e76739..2483fd6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -299,7 +299,7 @@ int qemuRemoveSharedDevice(virQEMUDriverPtr driver, const char *name) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); -int qemuSetUnprivSGIO(virDomainDiskDefPtr disk); +int qemuSetUnprivSGIO(virDomainDeviceDefPtr dev); int qemuDriverAllocateID(virQEMUDriverPtr driver); virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 991b68b..f523f03 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5651,7 +5651,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) goto end; - if (qemuSetUnprivSGIO(disk) < 0) + if (qemuSetUnprivSGIO(dev) < 0) goto end; if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index eb42df1..a6d58cb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3645,7 +3645,7 @@ int qemuProcessStart(virConnectPtr conn, if (qemuAddSharedDevice(driver, &dev, vm->def->name) < 0) goto cleanup; - if (qemuSetUnprivSGIO(disk) < 0) + if (qemuSetUnprivSGIO(&dev) < 0) goto cleanup; } -- 1.8.1.4

Just like previous patches, this changes qemuCheckSharedDisk into qemuCheckSharedDevice, which takes a virDomainDeviceDefPtr argument instead. --- src/qemu/qemu_conf.c | 86 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 61 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index cf1c7ee..f8264f6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -943,29 +943,55 @@ qemuGetSharedDeviceKey(const char *device_path) return key; } -/* Check if a shared disk's setting conflicts with the conf +/* Check if a shared device's setting conflicts with the conf * used by other domain(s). Currently only checks the sgio * setting. Note that this should only be called for disk with - * block source. + * block source if the device type is disk. * * Returns 0 if no conflicts, otherwise returns -1. */ static int -qemuCheckSharedDisk(virHashTablePtr sharedDevices, - virDomainDiskDefPtr disk) +qemuCheckSharedDevice(virHashTablePtr sharedDevices, + virDomainDeviceDefPtr dev) { + virDomainDiskDefPtr disk = NULL; + virDomainHostdevDefPtr hostdev = NULL; char *sysfs_path = NULL; char *key = NULL; + char *hostdev_name = NULL; + char *hostdev_path = NULL; + char *device_path = NULL; int val; int ret = 0; - /* The only conflicts between shared disk we care about now - * is sgio setting, which is only valid for device='lun'. - */ - if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) - return 0; + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + disk = dev->data.disk; + + /* The only conflicts between shared disk we care about now + * is sgio setting, which is only valid for device='lun'. + */ + if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) + return 0; + + device_path = disk->src; + } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { + hostdev = dev->data.hostdev; + + if (!(hostdev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit))) + goto cleanup; + + if (virAsprintf(&hostdev_path, "/dev/%s", hostdev_name) < 0) { + virReportOOMError(); + goto cleanup; + } + + device_path = hostdev_path; + } - if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL))) { + if (!(sysfs_path = virGetUnprivSGIOSysfsPath(device_path, NULL))) { ret = -1; goto cleanup; } @@ -976,7 +1002,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices, if (!virFileExists(sysfs_path)) goto cleanup; - if (!(key = qemuGetSharedDeviceKey(disk->src))) { + if (!(key = qemuGetSharedDeviceKey(device_path))) { ret = -1; goto cleanup; } @@ -987,7 +1013,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices, if (!(virHashLookup(sharedDevices, key))) goto cleanup; - if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) { + if (virGetDeviceUnprivSGIO(device_path, NULL, &val) < 0) { ret = -1; goto cleanup; } @@ -999,26 +1025,36 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices, disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED)) goto cleanup; - if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("sgio of shared disk 'pool=%s' 'volume=%s' conflicts " - "with other active domains"), - disk->srcpool->pool, - disk->srcpool->volume); + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("sgio of shared disk 'pool=%s' 'volume=%s' conflicts " + "with other active domains"), + disk->srcpool->pool, + disk->srcpool->volume); + } else { + virReportError(VIR_ERR_OPERATION_INVALID, + _("sgio of shared disk '%s' conflicts with other " + "active domains"), disk->src); + } } else { virReportError(VIR_ERR_OPERATION_INVALID, - _("sgio of shared disk '%s' conflicts with other " - "active domains"), disk->src); + _("sgio of shared scsi host device '%s-%d-%d-%d' conflicts " + "with other active domains"), + hostdev->source.subsys.u.scsi.adapter, + hostdev->source.subsys.u.scsi.bus, + hostdev->source.subsys.u.scsi.target, + hostdev->source.subsys.u.scsi.unit); } ret = -1; - cleanup: + VIR_FREE(hostdev_name); + VIR_FREE(hostdev_path); VIR_FREE(sysfs_path); VIR_FREE(key); return ret; } - bool qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry, const char *name, @@ -1133,10 +1169,10 @@ qemuAddSharedDevice(virQEMUDriverPtr driver, } qemuDriverLock(driver); - if (dev->type == VIR_DOMAIN_DEVICE_DISK) { - if (qemuCheckSharedDisk(driver->sharedDevices, disk) < 0) - goto cleanup; + if (qemuCheckSharedDevice(driver->sharedDevices, dev) < 0) + goto cleanup; + if (dev->type == VIR_DOMAIN_DEVICE_DISK) { if (!(key = qemuGetSharedDeviceKey(disk->src))) goto cleanup; } else { -- 1.8.1.4

On 08/05/13 08:05, John Ferlan wrote: To not introduce coverity errors anymore, I setup the coverity env on my local box, and the errors in this patch are disapeared after add the else branch: } else { return 0; } So pushed. Thanks. Osier