As was suggested in an earlier review comment[1], we can catch some additional code points by cleaning up how we use the hostdev subsystem type in some switch statements. [1] End of https://www.redhat.com/archives/libvir-list/2016-September/msg00399.html Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; --- src/conf/domain_conf.c | 11 +++++++++-- src/qemu/qemu_cgroup.c | 11 +++++++---- src/security/security_apparmor.c | 4 ++-- src/security/security_selinux.c | 8 ++++---- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a233c0c..043f0e2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12992,7 +12992,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, } if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { - switch (def->source.subsys.type) { + switch ((virDomainHostdevSubsysType) def->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && def->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { @@ -13014,6 +13014,11 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, if (virXPathBoolean("boolean(./shareable)", ctxt)) def->shareable = true; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + goto error;

+ break; } } @@ -13880,7 +13885,7 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, if (a->source.subsys.type != b->source.subsys.type) return 0; - switch (a->source.subsys.type) { + switch ((virDomainHostdevSubsysType) a->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: return virDomainHostdevMatchSubsysPCI(a, b); case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: @@ -13894,6 +13899,8 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, return virDomainHostdevMatchSubsysSCSIiSCSI(a, b); else return virDomainHostdevMatchSubsysSCSIHost(a, b); + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: + return 0; } return 0; } diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 4489c64..1443f7e 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -301,7 +301,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { - switch (dev->source.subsys.type) { + switch ((virDomainHostdevSubsysType) dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { int rv; @@ -376,7 +376,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, break; } - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: break; } } @@ -410,7 +410,7 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { - switch (dev->source.subsys.type) { + switch ((virDomainHostdevSubsysType) dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: if (pcisrc->backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) { int rv; @@ -437,7 +437,10 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: /* nothing to tear down for USB */ break; - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: + /* nothing to tear down for SCSI */ + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: break; } } diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index af2b639..beddf6d 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -856,7 +856,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr, ptr->mgr = mgr; ptr->def = def; - switch (dev->source.subsys.type) { + switch ((virDomainHostdevSubsysType) dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { virUSBDevicePtr usb = virUSBDeviceNew(usbsrc->bus, usbsrc->device, vroot); @@ -909,7 +909,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr, break; } - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: ret = 0; break; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 5dad22c..89c93dc 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1436,7 +1436,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr, scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) return 0; - switch (dev->source.subsys.type) { + switch ((virDomainHostdevSubsysType) dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { virUSBDevicePtr usb; @@ -1498,7 +1498,7 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr, break; } - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: ret = 0; break; } @@ -1640,7 +1640,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) return 0; - switch (dev->source.subsys.type) { + switch ((virDomainHostdevSubsysType) dev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: { virUSBDevicePtr usb; @@ -1700,7 +1700,7 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, break; } - default: + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: ret = 0; break; }

s/$SUBJ/Introduce framework for hostdev SCSI_Host subsys type On 11/08/2016 01:26 PM, Eric Farman wrote: > We already have a "scsi" hostdev type, which refers to a single LUN > that is passed through to a guest. But what of things where multiple > LUNs are passed through via a single SCSI HBA, such as with the > vhost-scsi target? Create a new hostdev type that will carry this. s/hostdev type/hostdev subsys type/ The XML will eventually be: <hostdev mode='subsystem' type='scsi_host' [managed='no']> <source protocol='vhost' wwpn='naa.XXXXXXXXXXXXXXXX'/> </hostdev> NB: The "hostdev" RNG definition does list an optional "<address>" element which it doesn't seem you save in the guest config during any of these patches. I do see a remnant of CCW for the running guest, but nothing for PCI (which in the end is what's used - vhost-scsi-pci or vhost-scsi-ccw). In any case, having "predictable" or "saved" addresses is something we've found through history (USB and more recently Memory dimms) to be a good idea. The point being - I think you need to make sure that if not supplied, then an address is generated (whether it's for PCI or CCW). While not in this patch per se, it is something you'll need to handle in patch 7.

> Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; > --- > src/conf/domain_conf.c | 12 +++++++++++- > src/conf/domain_conf.h | 18 ++++++++++++++++++ > src/qemu/qemu_cgroup.c | 7 +++++++ > src/qemu/qemu_hotplug.c | 2 ++ > src/security/security_apparmor.c | 4 ++++ > src/security/security_dac.c | 8 ++++++++ > src/security/security_selinux.c | 8 ++++++++ > tests/domaincapsschemadata/full.xml | 1 + > 8 files changed, 59 insertions(+), 1 deletion(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 043f0e2..b8a3366 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -647,7 +647,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST, > VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST, > "usb", > "pci", > - "scsi") > + "scsi", > + "scsi_host") > > VIR_ENUM_IMPL(virDomainHostdevSubsysPCIBackend, > VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST, > @@ -661,6 +662,11 @@ VIR_ENUM_IMPL(virDomainHostdevSubsysSCSIProtocol, > "adapter", > "iscsi") > > +VIR_ENUM_IMPL(virDomainHostdevSubsysHostProtocol, In order to "follow" convention for structure names already s/SubsysHostProtocol/SubsysSCSIHostProtocol/ Of course this has ramifications beyond this patch...

> + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_LAST, > + "none", > + "vhost") [1] because of this... > + > VIR_ENUM_IMPL(virDomainHostdevCaps, VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST, > "storage", > "misc", > @@ -13016,6 +13022,8 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, > break; > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: > break; > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: > + break; > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > goto error; > break; > @@ -13899,6 +13907,8 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, > return virDomainHostdevMatchSubsysSCSIiSCSI(a, b); > else > return virDomainHostdevMatchSubsysSCSIHost(a, b); > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: > + /* Fall through for now */ > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > return 0; > } > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 541b600..8b03561 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -294,6 +294,7 @@ typedef enum { > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB, > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI, > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, > + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST, [1] - I think this should use SCSI_HOST as well. s/HOST/SCSI_HOST There's lots of impact from hereonin... > > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST > } virDomainHostdevSubsysType; > @@ -368,6 +369,22 @@ struct _virDomainHostdevSubsysSCSI { > } u; > }; > > +typedef enum { > + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_NONE, > + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST, > + > + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_LAST, These would all need s/HOST/SCSI_HOST/ > +} virDomainHostdevSubsysHostProtocolType; > + > +VIR_ENUM_DECL(virDomainHostdevSubsysHostProtocol) And these would have s/Host/SCSIHost/ > + > +typedef struct _virDomainHostdevSubsysHost virDomainHostdevSubsysHost; > +typedef virDomainHostdevSubsysHost *virDomainHostdevSubsysHostPtr; > +struct _virDomainHostdevSubsysHost { s/Host/SCSIHost/ > + int protocol; We've been trying to add the enum in a comment e.g. int protocol; /* enum virDomainHostdevSubsysSCSIHostProtocolType */

> + char *wwpn; > +}; > + > typedef struct _virDomainHostdevSubsys virDomainHostdevSubsys; > typedef virDomainHostdevSubsys *virDomainHostdevSubsysPtr; > struct _virDomainHostdevSubsys { > @@ -376,6 +393,7 @@ struct _virDomainHostdevSubsys { > virDomainHostdevSubsysUSB usb; > virDomainHostdevSubsysPCI pci; > virDomainHostdevSubsysSCSI scsi; > + virDomainHostdevSubsysHost host; s/SubsysHost/SubsysSCSIHost/ and s/ host;/ scsi_host;/ I think the compiler will find the rest ;-) John > } u; > }; > > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index 1443f7e..ee31d14 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -376,6 +376,10 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, > break; > } > > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { > + break; > + } > + > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > break; > } > @@ -440,6 +444,9 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: > /* nothing to tear down for SCSI */ > break; > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: > + /* nothing to tear down for scsi_host */ > + break; > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > break; > } > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index e06862c..2d6b086 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -3626,6 +3626,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: > qemuDomainRemoveSCSIHostDevice(driver, vm, hostdev); > break; > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: > + break; > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > break; > } > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c > index beddf6d..e7e3c8c 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -909,6 +909,10 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr, > break; > } > > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { > + /* Fall through for now */ > + } > + > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > ret = 0; > break; > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index fd74e8b..eba2a87 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -676,6 +676,10 @@ virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr, > break; > } > > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { > + /* Fall through for now */ > + } > + > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > ret = 0; > break; > @@ -805,6 +809,10 @@ virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr, > break; > } > > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { > + /* Fall through for now */ > + } > + > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > ret = 0; > break; > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c > index 89c93dc..a94bba3 100644 > --- a/src/security/security_selinux.c > +++ b/src/security/security_selinux.c > @@ -1498,6 +1498,10 @@ virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr, > break; > } > > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { > + /* Fall through for now */ > + } > + > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > ret = 0; > break; > @@ -1700,6 +1704,10 @@ virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, > break; > } > > + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { > + /* Fall through for now */ > + } > + > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: > ret = 0; > break; > diff --git a/tests/domaincapsschemadata/full.xml b/tests/domaincapsschemadata/full.xml > index eaf6eb6..6abd499 100644 > --- a/tests/domaincapsschemadata/full.xml > +++ b/tests/domaincapsschemadata/full.xml > @@ -87,6 +87,7 @@ > <value>usb</value> > <value>pci</value> > <value>scsi</value> > + <value>scsi_host</value> > </enum> > <enum name='capsType'> > <value>storage</value> >

On 11/14/2016 08:31 AM, Eric Farman wrote: > > On 11/11/2016 04:41 PM, John Ferlan wrote: >> s/$SUBJ/Introduce framework for hostdev SCSI_Host subsys type >> >> On 11/08/2016 01:26 PM, Eric Farman wrote: >>> We already have a "scsi" hostdev type, which refers to a single LUN >>> that is passed through to a guest. But what of things where multiple >>> LUNs are passed through via a single SCSI HBA, such as with the >>> vhost-scsi target? Create a new hostdev type that will carry this. >> s/hostdev type/hostdev subsys type/ >> >> The XML will eventually be: >> >> <hostdev mode='subsystem' type='scsi_host' [managed='no']> >> <source protocol='vhost' wwpn='naa.XXXXXXXXXXXXXXXX'/> >> </hostdev> >> >> NB: The "hostdev" RNG definition does list an optional "<address>" >> element which it doesn't seem you save in the guest config during any of >> these patches. I do see a remnant of CCW for the running guest, but >> nothing for PCI (which in the end is what's used - vhost-scsi-pci or >> vhost-scsi-ccw). In any case, having "predictable" or "saved" addresses >> is something we've found through history (USB and more recently Memory >> dimms) to be a good idea. >> >> The point being - I think you need to make sure that if not supplied, >> then an address is generated (whether it's for PCI or CCW). While not in >> this patch per se, it is something you'll need to handle in patch 7. > Okay. > >> >>> Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; >>> --- >>> src/conf/domain_conf.c | 12 +++++++++++- >>> src/conf/domain_conf.h | 18 ++++++++++++++++++ >>> src/qemu/qemu_cgroup.c | 7 +++++++ >>> src/qemu/qemu_hotplug.c | 2 ++ >>> src/security/security_apparmor.c | 4 ++++ >>> src/security/security_dac.c | 8 ++++++++ >>> src/security/security_selinux.c | 8 ++++++++ >>> tests/domaincapsschemadata/full.xml | 1 + >>> 8 files changed, 59 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>> index 043f0e2..b8a3366 100644 >>> --- a/src/conf/domain_conf.c >>> +++ b/src/conf/domain_conf.c >>> @@ -647,7 +647,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, >>> VIR_DOMAIN_HOSTDEV_MODE_LAST, >>> VIR_ENUM_IMPL(virDomainHostdevSubsys, >>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST, >>> "usb", >>> "pci", >>> - "scsi") >>> + "scsi", >>> + "scsi_host") >>> VIR_ENUM_IMPL(virDomainHostdevSubsysPCIBackend, >>> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST, >>> @@ -661,6 +662,11 @@ VIR_ENUM_IMPL(virDomainHostdevSubsysSCSIProtocol, >>> "adapter", >>> "iscsi") >>> +VIR_ENUM_IMPL(virDomainHostdevSubsysHostProtocol, >> In order to "follow" convention for structure names already >> >> s/SubsysHostProtocol/SubsysSCSIHostProtocol/ >> >> Of course this has ramifications beyond this patch... > I avoided this (and many of the subsequent comments below) because the > existing struct virDomainHostdevSubsysSCSI has a union for both iSCSI > and it's version of host, and the latter is of course named > virDomainHostdevSubsysSCSIHost(Ptr). If that's not much of a concern, > then I'll go shake out the variations of host->scsihost you describe below. > Out of order, but didn't want to lose this... So there's: typedef struct _virDomainHostdevSubsysSCSI virDomainHostdevSubsysSCSI; typedef virDomainHostdevSubsysSCSI *virDomainHostdevSubsysSCSIPtr; struct _virDomainHostdevSubsysSCSI { int protocol; /* enum virDomainHostdevSCSIProtocolType */ int sgio; /* enum virDomainDeviceSGIO */ int rawio; /* enum virTristateBool */ union { virDomainHostdevSubsysSCSIHost host; virDomainHostdevSubsysSCSIiSCSI iscsi; } u; }; and when I first went through your changes I thought - why not just alter the virDomainHostdevSCSIProtocolType protocol here to add vHost and then add a SubsysSCSIvHost vhost structure at this point. But I think I had one of those *pfft* moments where the brain just explodes thinking about the details. The SCSIHost one is for XML: <devices> <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'> <source> <adapter name='scsi_host0'/> <address bus='0' target='0' unit='0'/> </source> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </hostdev> </devices> while the SCSIiSCSI is for XML: <devices> <hostdev mode='subsystem' type='scsi'> <source protocol='iscsi' name='iqn.2014-08.com.example:iscsi-nopool/1'> <host name='example.com' port='3260'/> <auth username='myuser'> <secret type='iscsi' usage='libvirtiscsi'/> </auth> </source> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </hostdev> </devices> whereas your proposed XML is <hostdev mode='subsystem' type='scsi_host'> <source protocol='vhost' wwpn='naa.5001405df3e54061'/> <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x1000'/> </hostdev> So yes, I suppose it could fit if you change the 'wwpn' to be 'name', then go through the process of altering existing code to be able handle the 'iscsi' protocol and the 'vhost' protocol. But the big difference I saw which changed my mind was the "hostdev ... type='scsi_host'", so that's why I left it alone since it's a different abstraction.

If we have conflicts with names, then we need to address them. If current code naming is incorrect, then we should fix that first. If you point it out, either of us can make the patch and the other can review it...

John > - Eric > >>> + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_LAST, >>> + "none", >>> + "vhost") >> [1] because of this... >> >>> + >>> VIR_ENUM_IMPL(virDomainHostdevCaps, VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST, >>> "storage", >>> "misc", >>> @@ -13016,6 +13022,8 @@ >>> virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, >>> break; >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: >>> break; >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: >>> + break; >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> goto error; >>> break; >>> @@ -13899,6 +13907,8 @@ >>> virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, >>> return virDomainHostdevMatchSubsysSCSIiSCSI(a, b); >>> else >>> return virDomainHostdevMatchSubsysSCSIHost(a, b); >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: >>> + /* Fall through for now */ >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> return 0; >>> } >>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h >>> index 541b600..8b03561 100644 >>> --- a/src/conf/domain_conf.h >>> +++ b/src/conf/domain_conf.h >>> @@ -294,6 +294,7 @@ typedef enum { >>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB, >>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI, >>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, >>> + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST, >> [1] - I think this should use SCSI_HOST as well. >> >> s/HOST/SCSI_HOST >> >> There's lots of impact from hereonin... >> >>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST >>> } virDomainHostdevSubsysType; >>> @@ -368,6 +369,22 @@ struct _virDomainHostdevSubsysSCSI { >>> } u; >>> }; >>> +typedef enum { >>> + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_NONE, >>> + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST, >>> + >>> + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_LAST, >> These would all need s/HOST/SCSI_HOST/ >> >>> +} virDomainHostdevSubsysHostProtocolType; >>> + >>> +VIR_ENUM_DECL(virDomainHostdevSubsysHostProtocol) >> And these would have s/Host/SCSIHost/ >>> + >>> +typedef struct _virDomainHostdevSubsysHost virDomainHostdevSubsysHost; >>> +typedef virDomainHostdevSubsysHost *virDomainHostdevSubsysHostPtr; >>> +struct _virDomainHostdevSubsysHost { >> s/Host/SCSIHost/ >> >>> + int protocol; >> We've been trying to add the enum in a comment e.g. >> >> int protocol; /* enum virDomainHostdevSubsysSCSIHostProtocolType */ > Hrm, I thought I'd included that. Maybe I dreamt it. :) > >>> + char *wwpn; >>> +}; >>> + >>> typedef struct _virDomainHostdevSubsys virDomainHostdevSubsys; >>> typedef virDomainHostdevSubsys *virDomainHostdevSubsysPtr; >>> struct _virDomainHostdevSubsys { >>> @@ -376,6 +393,7 @@ struct _virDomainHostdevSubsys { >>> virDomainHostdevSubsysUSB usb; >>> virDomainHostdevSubsysPCI pci; >>> virDomainHostdevSubsysSCSI scsi; >>> + virDomainHostdevSubsysHost host; >> s/SubsysHost/SubsysSCSIHost/ >> >> and >> >> s/ host;/ scsi_host;/ >> >> >> I think the compiler will find the rest ;-) >> >> John >> >>> } u; >>> }; >>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c >>> index 1443f7e..ee31d14 100644 >>> --- a/src/qemu/qemu_cgroup.c >>> +++ b/src/qemu/qemu_cgroup.c >>> @@ -376,6 +376,10 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, >>> break; >>> } >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { >>> + break; >>> + } >>> + >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> break; >>> } >>> @@ -440,6 +444,9 @@ qemuTeardownHostdevCgroup(virDomainObjPtr vm, >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: >>> /* nothing to tear down for SCSI */ >>> break; >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: >>> + /* nothing to tear down for scsi_host */ >>> + break; >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> break; >>> } >>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >>> index e06862c..2d6b086 100644 >>> --- a/src/qemu/qemu_hotplug.c >>> +++ b/src/qemu/qemu_hotplug.c >>> @@ -3626,6 +3626,8 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr >>> driver, >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: >>> qemuDomainRemoveSCSIHostDevice(driver, vm, hostdev); >>> break; >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: >>> + break; >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> break; >>> } >>> diff --git a/src/security/security_apparmor.c >>> b/src/security/security_apparmor.c >>> index beddf6d..e7e3c8c 100644 >>> --- a/src/security/security_apparmor.c >>> +++ b/src/security/security_apparmor.c >>> @@ -909,6 +909,10 @@ >>> AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr, >>> break; >>> } >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { >>> + /* Fall through for now */ >>> + } >>> + >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> ret = 0; >>> break; >>> diff --git a/src/security/security_dac.c b/src/security/security_dac.c >>> index fd74e8b..eba2a87 100644 >>> --- a/src/security/security_dac.c >>> +++ b/src/security/security_dac.c >>> @@ -676,6 +676,10 @@ >>> virSecurityDACSetHostdevLabel(virSecurityManagerPtr mgr, >>> break; >>> } >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { >>> + /* Fall through for now */ >>> + } >>> + >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> ret = 0; >>> break; >>> @@ -805,6 +809,10 @@ >>> virSecurityDACRestoreHostdevLabel(virSecurityManagerPtr mgr, >>> break; >>> } >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { >>> + /* Fall through for now */ >>> + } >>> + >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> ret = 0; >>> break; >>> diff --git a/src/security/security_selinux.c >>> b/src/security/security_selinux.c >>> index 89c93dc..a94bba3 100644 >>> --- a/src/security/security_selinux.c >>> +++ b/src/security/security_selinux.c >>> @@ -1498,6 +1498,10 @@ >>> virSecuritySELinuxSetHostdevSubsysLabel(virSecurityManagerPtr mgr, >>> break; >>> } >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { >>> + /* Fall through for now */ >>> + } >>> + >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> ret = 0; >>> break; >>> @@ -1700,6 +1704,10 @@ >>> virSecuritySELinuxRestoreHostdevSubsysLabel(virSecurityManagerPtr mgr, >>> break; >>> } >>> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { >>> + /* Fall through for now */ >>> + } >>> + >>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >>> ret = 0; >>> break; >>> diff --git a/tests/domaincapsschemadata/full.xml >>> b/tests/domaincapsschemadata/full.xml >>> index eaf6eb6..6abd499 100644 >>> --- a/tests/domaincapsschemadata/full.xml >>> +++ b/tests/domaincapsschemadata/full.xml >>> @@ -87,6 +87,7 @@ >>> <value>usb</value> >>> <value>pci</value> >>> <value>scsi</value> >>> + <value>scsi_host</value> >>> </enum> >>> <enum name='capsType'> >>> <value>storage</value> >>>

Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 19 +++ src/util/virhost.c | 299 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virhost.h | 72 ++++++++++++ src/util/virhostdev.c | 155 ++++++++++++++++++++++++ src/util/virhostdev.h | 16 +++ tests/qemuxml2argvmock.c | 9 ++ 8 files changed, 572 insertions(+) create mode 100644 src/util/virhost.c create mode 100644 src/util/virhost.h

diff --git a/po/POTFILES.in b/po/POTFILES.in index 1469240..a7cc542 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -199,6 +199,7 @@ src/util/virfirewall.c src/util/virfirmware.c src/util/virhash.c src/util/virhook.c +src/util/virhost.c src/util/virhostcpu.c src/util/virhostdev.c src/util/virhostmem.c diff --git a/src/Makefile.am b/src/Makefile.am index d417b6e..404c64e 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -122,6 +122,7 @@ UTIL_SOURCES = \ util/virhash.c util/virhash.h \ util/virhashcode.c util/virhashcode.h \ util/virhook.c util/virhook.h \ + util/virhost.c util/virhost.h \ util/virhostcpu.c util/virhostcpu.h util/virhostcpupriv.h \ util/virhostdev.c util/virhostdev.h \ util/virhostmem.c util/virhostmem.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 74dd527..ff535f9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1675,6 +1675,23 @@ virHookInitialize; virHookPresent; +# util/virhost.h +virHostDeviceFileIterate; +virHostDeviceFree; +virHostDeviceGetName; +virHostDeviceListAdd; +virHostDeviceListCount; +virHostDeviceListDel; +virHostDeviceListFind; +virHostDeviceListFindIndex;

+virHostDeviceListGet; +virHostDeviceListNew; +virHostDeviceListSteal; +virHostDeviceNew; +virHostDeviceSetUsedBy; +virHostOpenVhostSCSI; + + # util/virhostdev.h virHostdevFindUSBDevice; virHostdevManagerGetDefault; @@ -1682,10 +1699,12 @@ virHostdevPCINodeDeviceDetach; virHostdevPCINodeDeviceReAttach; virHostdevPCINodeDeviceReset; virHostdevPrepareDomainDevices; +virHostdevPrepareHostDevices; virHostdevPreparePCIDevices; virHostdevPrepareSCSIDevices; virHostdevPrepareUSBDevices; virHostdevReAttachDomainDevices; +virHostdevReAttachHostDevices; virHostdevReAttachPCIDevices; virHostdevReAttachSCSIDevices; virHostdevReAttachUSBDevices; diff --git a/src/util/virhost.c b/src/util/virhost.c new file mode 100644 index 0000000..9b5f524 --- /dev/null +++ b/src/util/virhost.c @@ -0,0 +1,299 @@ +/* + * virhost.c: helper APIs for managing scsi_host devices + * + * Copyright (C) 2016 IBM Corporation + * + * 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: + * Eric Farman <farman(a)linux.vnet.ibm.com&gt; + */ + +#include <config.h> +#include <fcntl.h> + +#include "virhost.h" +#include "virlog.h" +#include "viralloc.h" +#include "virerror.h" +#include "virfile.h" +#include "virstring.h" + +VIR_LOG_INIT("util.host"); + +#define SYSFS_VHOST_SCSI_DEVICES "/sys/kernel/config/target/vhost/" +#define VHOST_SCSI_DEVICE "/dev/vhost-scsi" + +struct _virUsedByInfo { + char *drvname; /* which driver */ + char *domname; /* which domain */ +}; +typedef struct _virUsedByInfo *virUsedByInfoPtr;

+ +struct _virHostDevice {

+ char *name; /* naa.<wwn> */ + char *path; + virUsedByInfoPtr *used_by; /* driver:domain(s) using this dev */ + size_t n_used_by; /* how many domains are using this dev */

+}; + +struct _virHostDeviceList {

+ virObjectLockable parent; + size_t count; + virHostDevicePtr *devs; +}; + +static virClassPtr virHostDeviceListClass;

+ +static void +virHostDeviceListDispose(void *obj) +{ + virHostDeviceListPtr list = obj; + size_t i; + + for (i = 0; i < list->count; i++) + virHostDeviceFree(list->devs[i]); + + VIR_FREE(list->devs); +} + +

+static int +virHostOnceInit(void) +{ + if (!(virHostDeviceListClass = virClassNew(virClassForObjectLockable(), + "virHostDeviceList", + sizeof(virHostDeviceList), + virHostDeviceListDispose))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virHost) + +/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE + +int +virHostOpenVhostSCSI(int *vhostfd) +{ + if (!virFileExists(VHOST_SCSI_DEVICE)) + goto error; + + *vhostfd = open(VHOST_SCSI_DEVICE, O_RDWR); + + if (*vhostfd < 0) { + virReportSystemError(errno, _("Failed to open %s"), VHOST_SCSI_DEVICE); + goto error; + } + + return 0; + + error: + VIR_FORCE_CLOSE(*vhostfd); + + return -1; +} + +static void +virHostDeviceUsedByInfoFree(virUsedByInfoPtr used_by) +{

+ VIR_FREE(used_by->drvname); + VIR_FREE(used_by->domname); + VIR_FREE(used_by); +} + +void +virHostDeviceListDel(virHostDeviceListPtr list, + virHostDevicePtr dev, + const char *drvname, + const char *domname) +{ + virHostDevicePtr tmp = NULL; + size_t i; +

+ for (i = 0; i < dev->n_used_by; i++) { + if (STREQ_NULLABLE(dev->used_by[i]->drvname, drvname) && + STREQ_NULLABLE(dev->used_by[i]->domname, domname)) { + if (dev->n_used_by > 1) { + virHostDeviceUsedByInfoFree(dev->used_by[i]); + VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by); + } else { + tmp = virHostDeviceListSteal(list, dev); + virHostDeviceFree(tmp); + } + break; + } + } +} + +int

+virHostDeviceListFindIndex(virHostDeviceListPtr list, virHostDevicePtr dev) +{ + size_t i; + + for (i = 0; i < list->count; i++) { + virHostDevicePtr other = list->devs[i]; + if (STREQ_NULLABLE(other->name, dev->name)) + return i; + } + return -1; +} + +virHostDevicePtr +virHostDeviceListGet(virHostDeviceListPtr list, int idx) +{ + if (idx >= list->count || idx < 0) + return NULL; + + return list->devs[idx]; +} + +size_t +virHostDeviceListCount(virHostDeviceListPtr list) +{ + return list->count; +} + +virHostDevicePtr +virHostDeviceListSteal(virHostDeviceListPtr list, + virHostDevicePtr dev) +{ + virHostDevicePtr ret = NULL; + size_t i; + + for (i = 0; i < list->count; i++) { + if (STREQ_NULLABLE(list->devs[i]->name, dev->name)) { + ret = list->devs[i]; + VIR_DELETE_ELEMENT(list->devs, i, list->count); + break; + } + } + + return ret; +} + +virHostDevicePtr +virHostDeviceListFind(virHostDeviceListPtr list, virHostDevicePtr dev) +{ + int idx; + + if ((idx = virHostDeviceListFindIndex(list, dev)) >= 0) + return list->devs[idx]; + else + return NULL; +} + +int +virHostDeviceListAdd(virHostDeviceListPtr list, + virHostDevicePtr dev) +{ + if (virHostDeviceListFind(list, dev)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Device %s is already in use"), dev->name); + return -1; + } + return VIR_APPEND_ELEMENT(list->devs, list->count, dev); +} + +virHostDeviceListPtr +virHostDeviceListNew(void) +{ + virHostDeviceListPtr list; + + if (virHostInitialize() < 0) + return NULL; + + if (!(list = virObjectLockableNew(virHostDeviceListClass))) + return NULL; + + return list;

+} + +int +virHostDeviceSetUsedBy(virHostDevicePtr dev, + const char *drvname, + const char *domname) +{

+ virUsedByInfoPtr copy; + if (VIR_ALLOC(copy) < 0) + return -1; + if (VIR_STRDUP(copy->drvname, drvname) < 0 || + VIR_STRDUP(copy->domname, domname) < 0) + goto cleanup; + + if (VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy) < 0) + goto cleanup; + + return 0; + + cleanup: + virHostDeviceUsedByInfoFree(copy); + return -1; +} + +int +virHostDeviceFileIterate(virHostDevicePtr dev, + virHostDeviceFileActor actor, + void *opaque) +{ + return (actor)(dev, dev->path, opaque); +} + +const char * +virHostDeviceGetName(virHostDevicePtr dev) +{ + return dev->name; +} + +virHostDevicePtr +virHostDeviceNew(const char *name) +{ + virHostDevicePtr dev; + + if (VIR_ALLOC(dev) < 0) + return NULL; + + if (VIR_STRDUP(dev->name, name) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("dev->name buffer overflow: %s"), + name); + goto error; + } + + if (virAsprintf(&dev->path, "%s/%s", + SYSFS_VHOST_SCSI_DEVICES, name) < 0) + goto cleanup; + + VIR_DEBUG("%s: initialized", dev->name); + + cleanup: + return dev; + + error: + virHostDeviceFree(dev); + dev = NULL; + goto cleanup; +} + +void +virHostDeviceFree(virHostDevicePtr dev) +{

+ if (!dev) + return; + VIR_DEBUG("%s: freeing", dev->name);

+ VIR_FREE(dev); +} diff --git a/src/util/virhost.h b/src/util/virhost.h new file mode 100644 index 0000000..6d7b790 --- /dev/null +++ b/src/util/virhost.h @@ -0,0 +1,72 @@ +/* + * virhost.h: helper APIs for managing host scsi_host devices + * + * Copyright (C) 2016 IBM Corporation + * + * 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: + * Eric Farman <farman(a)linux.vnet.ibm.com&gt; + */ + +#ifndef __VIR_HOST_H__ +# define __VIR_HOST_H__ + +# include "internal.h" +# include "virobject.h" +# include "virutil.h" + +typedef struct _virHostDevice virHostDevice; +typedef virHostDevice *virHostDevicePtr; +typedef struct _virHostDeviceAddress virHostDeviceAddress; +typedef virHostDeviceAddress *virHostDeviceAddressPtr; +typedef struct _virHostDeviceList virHostDeviceList; +typedef virHostDeviceList *virHostDeviceListPtr; + +struct _virHostDeviceAddress { + char *wwpn; +};

+ +typedef int (*virHostDeviceFileActor)(virHostDevicePtr dev, + const char *name, void *opaque); + +int virHostDeviceFileIterate(virHostDevicePtr dev, + virHostDeviceFileActor actor, + void *opaque); +const char *virHostDeviceGetName(virHostDevicePtr dev); +virHostDevicePtr virHostDeviceListGet(virHostDeviceListPtr list, + int idx); +size_t virHostDeviceListCount(virHostDeviceListPtr list); +virHostDevicePtr virHostDeviceListSteal(virHostDeviceListPtr list, + virHostDevicePtr dev); +int virHostDeviceListFindIndex(virHostDeviceListPtr list, + virHostDevicePtr dev);

+virHostDevicePtr virHostDeviceListFind(virHostDeviceListPtr list, + virHostDevicePtr dev); +int virHostDeviceListAdd(virHostDeviceListPtr list, + virHostDevicePtr dev); +void virHostDeviceListDel(virHostDeviceListPtr list, + virHostDevicePtr dev, + const char *drvname, + const char *domname); +virHostDeviceListPtr virHostDeviceListNew(void); +virHostDevicePtr virHostDeviceNew(const char *name); +int virHostDeviceSetUsedBy(virHostDevicePtr dev, + const char *drvname, + const char *domname); +void virHostDeviceFree(virHostDevicePtr dev); +int virHostOpenVhostSCSI(int *vhostfd); + +#endif /* __VIR_HOST_H__ */ diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 9c2262e..b92e246 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -146,6 +146,7 @@ virHostdevManagerDispose(void *obj) virObjectUnref(hostdevMgr->inactivePCIHostdevs); virObjectUnref(hostdevMgr->activeUSBHostdevs); virObjectUnref(hostdevMgr->activeSCSIHostdevs); + virObjectUnref(hostdevMgr->activeHostHostdevs); VIR_FREE(hostdevMgr->stateDir); } @@ -170,6 +171,9 @@ virHostdevManagerNew(void) if (!(hostdevMgr->activeSCSIHostdevs = virSCSIDeviceListNew())) goto error; + if (!(hostdevMgr->activeHostHostdevs = virHostDeviceListNew())) + goto error; + if (privileged) { if (VIR_STRDUP(hostdevMgr->stateDir, HOSTDEV_STATE_DIR) < 0) goto error; @@ -1472,6 +1476,102 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr, return -1; } +int +virHostdevPrepareHostDevices(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + size_t i, j; + int count; + virHostDeviceListPtr list; + virHostDevicePtr tmp; + + if (!nhostdevs) + return 0; + + /* To prevent situation where scsi_host device is assigned to two domains + * we need to keep a list of currently assigned scsi_host devices. + * This is done in several loops which cannot be joined into one big + * loop. See virHostdevPreparePCIDevices() + */ + if (!(list = virHostDeviceListNew())) + goto cleanup; + + /* Loop 1: build temporary list */ + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + virDomainHostdevSubsysHostPtr hostsrc = &hostdev->source.subsys.u.host; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) + continue; + + if (hostsrc->protocol != VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) { + continue; /* Not supported */ + } else {

+ virHostDevicePtr host; + if (!(host = virHostDeviceNew(hostsrc->wwpn))) + goto cleanup; + + if (virHostDeviceListAdd(list, host) < 0) { + virHostDeviceFree(host); + 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(mgr->activeHostHostdevs); + count = virHostDeviceListCount(list); + + for (i = 0; i < count; i++) { + virHostDevicePtr host = virHostDeviceListGet(list, i);

+ if ((tmp = virHostDeviceListFind(mgr->activeHostHostdevs, host))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("SCSI_host device %s is already in use by " + "another domain"), + virHostDeviceGetName(tmp)); + goto error; + } else { + if (virHostDeviceSetUsedBy(host, drv_name, dom_name) < 0) + goto error; + + VIR_DEBUG("Adding %s to activeHostHostdevs", virHostDeviceGetName(host)); + + if (virHostDeviceListAdd(mgr->activeHostHostdevs, host) < 0) + goto error; + } + } + + virObjectUnlock(mgr->activeHostHostdevs); + + /* Loop 3: Temporary list was successfully merged with + * driver list, so steal all items to avoid freeing them + * when freeing temporary list. + */ + while (virHostDeviceListCount(list) > 0) { + tmp = virHostDeviceListGet(list, 0); + virHostDeviceListSteal(list, tmp); + } + + virObjectUnref(list); + return 0; + error: + for (j = 0; j < i; j++) { + tmp = virHostDeviceListGet(list, i); + virHostDeviceListSteal(mgr->activeHostHostdevs, tmp); + } + virObjectUnlock(mgr->activeHostHostdevs); + cleanup: + virObjectUnref(list); + return -1; +} + void virHostdevReAttachUSBDevices(virHostdevManagerPtr mgr, const char *drv_name, @@ -1604,6 +1704,61 @@ virHostdevReAttachSCSIDevices(virHostdevManagerPtr mgr, virObjectUnlock(mgr->activeSCSIHostdevs); } +void +virHostdevReAttachHostDevices(virHostdevManagerPtr mgr, + const char *drv_name, + const char *dom_name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + size_t i; + virHostDevicePtr host, tmp; + + + if (!nhostdevs) + return; + + virObjectLock(mgr->activeHostHostdevs); + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + virDomainHostdevSubsysHostPtr hostsrc = &hostdev->source.subsys.u.host; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || + hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) + continue; + + if (hostsrc->protocol != VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) + continue; /* Not supported */ + + if (!(host = virHostDeviceNew(hostsrc->wwpn))) { + VIR_WARN("Unable to reattach SCSI_host device %s on domain %s", + hostsrc->wwpn, dom_name); + virObjectUnlock(mgr->activeHostHostdevs); + return; + } +

+ /* Only delete the devices which are marked as being used by @name, + * because qemuProcessStart could fail half way through. */ + + if (!(tmp = virHostDeviceListFind(mgr->activeHostHostdevs, host))) { + VIR_WARN("Unable to find device %s " + "in list of active SCSI_host devices", + hostsrc->wwpn); + virHostDeviceFree(host); + virObjectUnlock(mgr->activeHostHostdevs); + return; + } + + VIR_DEBUG("Removing %s dom=%s from activeHostHostdevs", + hostsrc->wwpn, dom_name); + + virHostDeviceListDel(mgr->activeHostHostdevs, tmp, + drv_name, dom_name); + virHostDeviceFree(host); + } + virObjectUnlock(mgr->activeHostHostdevs); +} + int virHostdevPCINodeDeviceDetach(virHostdevManagerPtr mgr, virPCIDevicePtr pci) diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index f2f51bd..19cef7e 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -30,6 +30,7 @@ # include "virpci.h" # include "virusb.h" # include "virscsi.h" +# include "virhost.h" # include "domain_conf.h" typedef enum { @@ -53,6 +54,7 @@ struct _virHostdevManager { virPCIDeviceListPtr inactivePCIHostdevs; virUSBDeviceListPtr activeUSBHostdevs; virSCSIDeviceListPtr activeSCSIHostdevs; + virHostDeviceListPtr activeHostHostdevs;

}; virHostdevManagerPtr virHostdevManagerGetDefault(void); @@ -87,6 +89,13 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr hostdev_mgr, virDomainHostdevDefPtr *hostdevs, int nhostdevs) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +int +virHostdevPrepareHostDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, @@ -109,6 +118,13 @@ virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, virDomainHostdevDefPtr *hostdevs, int nhostdevs) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +void +virHostdevReAttachHostDevices(virHostdevManagerPtr hostdev_mgr, + const char *drv_name, + const char *dom_name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, virDomainHostdevDefPtr *hostdevs, diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 78a224b..2c85140 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -24,6 +24,7 @@ #include "viralloc.h" #include "vircommand.h" #include "vircrypto.h" +#include "virhost.h" #include "virmock.h" #include "virnetdev.h" #include "virnetdevip.h" @@ -107,6 +108,14 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix ATTRIBUTE_UNUSED, } int +virHostOpenVhostSCSI(int *vhostfd) +{ + *vhostfd = STDERR_FILENO + 1; + + return 0; +} + +int virNetDevTapCreate(char **ifname, const char *tunpath ATTRIBUTE_UNUSED, int *tapfd,

On 11/11/2016 04:44 PM, John Ferlan wrote: > While perhaps mostly obvious - you need some sort of commit message here. > > On 11/08/2016 01:26 PM, Eric Farman wrote: >> Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; >> --- >> po/POTFILES.in | 1 + >> src/Makefile.am | 1 + >> src/libvirt_private.syms | 19 +++ >> src/util/virhost.c | 299 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> src/util/virhost.h | 72 ++++++++++++ >> src/util/virhostdev.c | 155 ++++++++++++++++++++++++ >> src/util/virhostdev.h | 16 +++ >> tests/qemuxml2argvmock.c | 9 ++ >> 8 files changed, 572 insertions(+) >> create mode 100644 src/util/virhost.c >> create mode 100644 src/util/virhost.h >> > I fear someone will equate "virhost" to host virtual functions as > opposed to what it is. You'll note there's virhostcpu, virhostdev, and > virhostmem - each of which are utility API's for that subsystem. > > So I think this needs to become 'virscsihost.{c,h}' and of course the > API's are 'virSCSIHostDevice' prefixed instead of 'virHostDevice'. > Alternatively, the functions could go in virscsi.c, but I do prefer the > separation. > >> diff --git a/po/POTFILES.in b/po/POTFILES.in >> index 1469240..a7cc542 100644 >> --- a/po/POTFILES.in >> +++ b/po/POTFILES.in >> @@ -199,6 +199,7 @@ src/util/virfirewall.c >> src/util/virfirmware.c >> src/util/virhash.c >> src/util/virhook.c >> +src/util/virhost.c >> src/util/virhostcpu.c >> src/util/virhostdev.c >> src/util/virhostmem.c >> diff --git a/src/Makefile.am b/src/Makefile.am >> index d417b6e..404c64e 100644 >> --- a/src/Makefile.am >> +++ b/src/Makefile.am >> @@ -122,6 +122,7 @@ UTIL_SOURCES = \ >> util/virhash.c util/virhash.h \ >> util/virhashcode.c util/virhashcode.h \ >> util/virhook.c util/virhook.h \ >> + util/virhost.c util/virhost.h \ >> util/virhostcpu.c util/virhostcpu.h util/virhostcpupriv.h \ >> util/virhostdev.c util/virhostdev.h \ >> util/virhostmem.c util/virhostmem.h \ >> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >> index 74dd527..ff535f9 100644 >> --- a/src/libvirt_private.syms >> +++ b/src/libvirt_private.syms >> @@ -1675,6 +1675,23 @@ virHookInitialize; >> virHookPresent; >> +# util/virhost.h >> +virHostDeviceFileIterate; >> +virHostDeviceFree; >> +virHostDeviceGetName; >> +virHostDeviceListAdd; >> +virHostDeviceListCount; >> +virHostDeviceListDel; >> +virHostDeviceListFind; >> +virHostDeviceListFindIndex; > This one is not used externally, so it could just be static to virhost.c > >> +virHostDeviceListGet; >> +virHostDeviceListNew; >> +virHostDeviceListSteal; >> +virHostDeviceNew; >> +virHostDeviceSetUsedBy; >> +virHostOpenVhostSCSI; >> + >> + >> # util/virhostdev.h >> virHostdevFindUSBDevice; >> virHostdevManagerGetDefault; >> @@ -1682,10 +1699,12 @@ virHostdevPCINodeDeviceDetach; >> virHostdevPCINodeDeviceReAttach; >> virHostdevPCINodeDeviceReset; >> virHostdevPrepareDomainDevices; >> +virHostdevPrepareHostDevices; As I alluded to in my response in patch 3, taking your s/Host/SCSIHost/ comment to heart breaks down here because "virHostdevPrepareSCSIHostDevices" was introduced by commit 17bddc46.

So now I'm torn in how far to correlate the changes. I currently have two (to-be-squashed) patches that does "s/virHost/virSCSIHost/" and "s/HOST/SCSIHOST/" within the context of this series, but going farther means qemuHostdevPrepareSCSIHostDevices calls virHostdevPrepareHostDevices, because qemuHostdevPrepareSCSIDevices calls virHostdevPrepareSCSIDevices, whihc may call virHostdevPrepareSCSIHostDevices

>> virHostdevPreparePCIDevices; >> virHostdevPrepareSCSIDevices; >> virHostdevPrepareUSBDevices; >> virHostdevReAttachDomainDevices; >> +virHostdevReAttachHostDevices; >> virHostdevReAttachPCIDevices; >> virHostdevReAttachSCSIDevices; >> virHostdevReAttachUSBDevices; >> diff --git a/src/util/virhost.c b/src/util/virhost.c >> new file mode 100644 >> index 0000000..9b5f524 >> --- /dev/null >> +++ b/src/util/virhost.c >> @@ -0,0 +1,299 @@ >> +/* >> + * virhost.c: helper APIs for managing scsi_host devices >> + * >> + * Copyright (C) 2016 IBM Corporation >> + * >> + * 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: >> + * Eric Farman <farman(a)linux.vnet.ibm.com&gt; >> + */ >> + >> +#include <config.h> >> +#include <fcntl.h> >> + >> +#include "virhost.h" >> +#include "virlog.h" >> +#include "viralloc.h" >> +#include "virerror.h" >> +#include "virfile.h" >> +#include "virstring.h" >> + >> +VIR_LOG_INIT("util.host"); >> + >> +#define SYSFS_VHOST_SCSI_DEVICES "/sys/kernel/config/target/vhost/" >> +#define VHOST_SCSI_DEVICE "/dev/vhost-scsi" >> + >> +struct _virUsedByInfo { >> + char *drvname; /* which driver */ >> + char *domname; /* which domain */ >> +}; >> +typedef struct _virUsedByInfo *virUsedByInfoPtr; > Hmm... seeing this makes me think the code should go in virscsi.c... > That would seem to then allow more code reuse... > > However, looking at your virHostdevPrepareHostDevices changes for how > this is implemented, I see no sharing allowed/going on. Thus, there's no > need for a list of used_by - rather it's just a single 'used_by_drvname' > and 'used_by_domname' in the virSCSIHostDevice structure (similar to PCI > and USB). > > Unless of course you think you want to add sharing.... which I cannot > imagine is a good idea... No, this is a fine idea. I cannot see a point in permitting this. > >> + >> +struct _virHostDevice { > s/Host/SCSIHost/ > >> + char *name; /* naa.<wwn> */ >> + char *path; >> + virUsedByInfoPtr *used_by; /* driver:domain(s) using this dev */ >> + size_t n_used_by; /* how many domains are using this dev */ > Looks like the above 2 get replaced by > > char *used_by_drvname; > char *used_by_domname; > > Also - looking at QEMU and "forward" thinking - there are options on the > qemu command line for things like boot_tpgt, num_queues, max_sectors, > cmd_per_lun, and bootindex... I can see the last one being asked for - > as in can we pass one of these to the guest to allow booting from a > specific LUN on the array... > >> +}; >> + >> +struct _virHostDeviceList { > s/Host/SCSIHost/ > >> + virObjectLockable parent; >> + size_t count; >> + virHostDevicePtr *devs; >> +}; >> + >> +static virClassPtr virHostDeviceListClass; > s/Host/SCSIHost/ > > (ad nauseum ;-)) > >> + >> +static void >> +virHostDeviceListDispose(void *obj) >> +{ >> + virHostDeviceListPtr list = obj; >> + size_t i; >> + >> + for (i = 0; i < list->count; i++) >> + virHostDeviceFree(list->devs[i]); >> + >> + VIR_FREE(list->devs); >> +} >> + >> + > You start w/ the newer convention of 2 spaces between functions, but it > ends here. After this there's always just 1 space between functions - > let's try to go w/ 2. OK. > >> +static int >> +virHostOnceInit(void) >> +{ >> + if (!(virHostDeviceListClass = >> virClassNew(virClassForObjectLockable(), >> + "virHostDeviceList", >> + >> sizeof(virHostDeviceList), >> + >> virHostDeviceListDispose))) >> + return -1; >> + >> + return 0; >> +} >> + >> +VIR_ONCE_GLOBAL_INIT(virHost) >> + >> +/* For virReportOOMError() and virReportSystemError() */ >> +#define VIR_FROM_THIS VIR_FROM_NONE >> + >> +int >> +virHostOpenVhostSCSI(int *vhostfd) >> +{ >> + if (!virFileExists(VHOST_SCSI_DEVICE)) >> + goto error; >> + >> + *vhostfd = open(VHOST_SCSI_DEVICE, O_RDWR); >> + >> + if (*vhostfd < 0) { >> + virReportSystemError(errno, _("Failed to open %s"), >> VHOST_SCSI_DEVICE); >> + goto error; >> + } >> + >> + return 0; >> + >> + error: >> + VIR_FORCE_CLOSE(*vhostfd); >> + >> + return -1; >> +} >> + >> +static void >> +virHostDeviceUsedByInfoFree(virUsedByInfoPtr used_by) >> +{ > I think this ends up going away since things aren't shareable. > >> + VIR_FREE(used_by->drvname); >> + VIR_FREE(used_by->domname); >> + VIR_FREE(used_by); >> +} >> + >> +void >> +virHostDeviceListDel(virHostDeviceListPtr list, >> + virHostDevicePtr dev, >> + const char *drvname, >> + const char *domname) >> +{ >> + virHostDevicePtr tmp = NULL; >> + size_t i; >> + > This will need to be adjusted since (I assume) you don't want shareable > scsi_host devices > >> + for (i = 0; i < dev->n_used_by; i++) { >> + if (STREQ_NULLABLE(dev->used_by[i]->drvname, drvname) && >> + STREQ_NULLABLE(dev->used_by[i]->domname, domname)) { >> + if (dev->n_used_by > 1) { >> + virHostDeviceUsedByInfoFree(dev->used_by[i]); >> + VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by); >> + } else { >> + tmp = virHostDeviceListSteal(list, dev); >> + virHostDeviceFree(tmp); >> + } >> + break; >> + } >> + } >> +} >> + >> +int > s/int/static int/ > >> +virHostDeviceListFindIndex(virHostDeviceListPtr list, >> virHostDevicePtr dev) >> +{ >> + size_t i; >> + >> + for (i = 0; i < list->count; i++) { >> + virHostDevicePtr other = list->devs[i]; >> + if (STREQ_NULLABLE(other->name, dev->name)) >> + return i; >> + } >> + return -1; >> +} >> + >> +virHostDevicePtr >> +virHostDeviceListGet(virHostDeviceListPtr list, int idx) >> +{ >> + if (idx >= list->count || idx < 0) >> + return NULL; >> + >> + return list->devs[idx]; >> +} >> + >> +size_t >> +virHostDeviceListCount(virHostDeviceListPtr list) >> +{ >> + return list->count; >> +} >> + >> +virHostDevicePtr >> +virHostDeviceListSteal(virHostDeviceListPtr list, >> + virHostDevicePtr dev) >> +{ >> + virHostDevicePtr ret = NULL; >> + size_t i; >> + >> + for (i = 0; i < list->count; i++) { >> + if (STREQ_NULLABLE(list->devs[i]->name, dev->name)) { >> + ret = list->devs[i]; >> + VIR_DELETE_ELEMENT(list->devs, i, list->count); >> + break; >> + } >> + } >> + >> + return ret; >> +} >> + >> +virHostDevicePtr >> +virHostDeviceListFind(virHostDeviceListPtr list, virHostDevicePtr dev) >> +{ >> + int idx; >> + >> + if ((idx = virHostDeviceListFindIndex(list, dev)) >= 0) >> + return list->devs[idx]; >> + else >> + return NULL; >> +} >> + >> +int >> +virHostDeviceListAdd(virHostDeviceListPtr list, >> + virHostDevicePtr dev) >> +{ >> + if (virHostDeviceListFind(list, dev)) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Device %s is already in use"), dev->name); >> + return -1; >> + } >> + return VIR_APPEND_ELEMENT(list->devs, list->count, dev); >> +} >> + >> +virHostDeviceListPtr >> +virHostDeviceListNew(void) >> +{ >> + virHostDeviceListPtr list; >> + >> + if (virHostInitialize() < 0) >> + return NULL; >> + >> + if (!(list = virObjectLockableNew(virHostDeviceListClass))) >> + return NULL; >> + >> + return list; > or simply return virObjectLockableNew(virSCSIHostDeviceListClass); and > then 'list' is not needed. > >> +} >> + >> +int >> +virHostDeviceSetUsedBy(virHostDevicePtr dev, >> + const char *drvname, >> + const char *domname) >> +{ > Without sharing, this mimics PCI/USB instead... > >> + virUsedByInfoPtr copy; >> + if (VIR_ALLOC(copy) < 0) >> + return -1; >> + if (VIR_STRDUP(copy->drvname, drvname) < 0 || >> + VIR_STRDUP(copy->domname, domname) < 0) >> + goto cleanup; >> + >> + if (VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy) < 0) >> + goto cleanup; >> + >> + return 0; >> + >> + cleanup: >> + virHostDeviceUsedByInfoFree(copy); >> + return -1; >> +} >> + >> +int >> +virHostDeviceFileIterate(virHostDevicePtr dev, >> + virHostDeviceFileActor actor, >> + void *opaque) >> +{ >> + return (actor)(dev, dev->path, opaque); >> +} >> + >> +const char * >> +virHostDeviceGetName(virHostDevicePtr dev) >> +{ >> + return dev->name; >> +} >> + >> +virHostDevicePtr >> +virHostDeviceNew(const char *name) >> +{ >> + virHostDevicePtr dev; >> + >> + if (VIR_ALLOC(dev) < 0) >> + return NULL; >> + >> + if (VIR_STRDUP(dev->name, name) < 0) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("dev->name buffer overflow: %s"), >> + name); >> + goto error; >> + } >> + >> + if (virAsprintf(&dev->path, "%s/%s", >> + SYSFS_VHOST_SCSI_DEVICES, name) < 0) >> + goto cleanup; >> + >> + VIR_DEBUG("%s: initialized", dev->name); >> + >> + cleanup: >> + return dev; >> + >> + error: >> + virHostDeviceFree(dev); >> + dev = NULL; >> + goto cleanup; >> +} >> + >> +void >> +virHostDeviceFree(virHostDevicePtr dev) >> +{ > size_t i; > >> + if (!dev) >> + return; >> + VIR_DEBUG("%s: freeing", dev->name); > Would be nice if it was freeing everything in 'dev' before freeing dev... Oops! > > Thus you have: > > VIR_FREE(dev->name); > VIR_FREE(dev->path); > VIR_FREE(dev->used_by_drvname); > VIR_FREE(dev->used_by_domname); > >> + VIR_FREE(dev); >> +} >> diff --git a/src/util/virhost.h b/src/util/virhost.h >> new file mode 100644 >> index 0000000..6d7b790 >> --- /dev/null >> +++ b/src/util/virhost.h >> @@ -0,0 +1,72 @@ >> +/* >> + * virhost.h: helper APIs for managing host scsi_host devices >> + * >> + * Copyright (C) 2016 IBM Corporation >> + * >> + * 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: >> + * Eric Farman <farman(a)linux.vnet.ibm.com&gt; >> + */ >> + >> +#ifndef __VIR_HOST_H__ >> +# define __VIR_HOST_H__ >> + >> +# include "internal.h" >> +# include "virobject.h" >> +# include "virutil.h" >> + >> +typedef struct _virHostDevice virHostDevice; >> +typedef virHostDevice *virHostDevicePtr; >> +typedef struct _virHostDeviceAddress virHostDeviceAddress; >> +typedef virHostDeviceAddress *virHostDeviceAddressPtr; >> +typedef struct _virHostDeviceList virHostDeviceList; >> +typedef virHostDeviceList *virHostDeviceListPtr; >> + >> +struct _virHostDeviceAddress { >> + char *wwpn; >> +}; > Hmmm.. this would seemingly duplicate name without the "naa.", but in > the long run it's not even used - so why is it necessary? > > Then again as it turns out a "vhost-scsi-pci" or "vhost-scsi-ccw" device > object is created. Each of those would seemingly need a controller to > use wouldn't they? Are you referring to a <controller> tag? Or something else?

> So the address would seem to be important, but is not > handled. Will check how the PCI handling is broken. Seemed okay for CCW, but maybe I've missed something there too.

- Eric > >> + >> +typedef int (*virHostDeviceFileActor)(virHostDevicePtr dev, >> + const char *name, void *opaque); >> + >> +int virHostDeviceFileIterate(virHostDevicePtr dev, >> + virHostDeviceFileActor actor, >> + void *opaque); >> +const char *virHostDeviceGetName(virHostDevicePtr dev); >> +virHostDevicePtr virHostDeviceListGet(virHostDeviceListPtr list, >> + int idx); >> +size_t virHostDeviceListCount(virHostDeviceListPtr list); >> +virHostDevicePtr virHostDeviceListSteal(virHostDeviceListPtr list, >> + virHostDevicePtr dev); >> +int virHostDeviceListFindIndex(virHostDeviceListPtr list, >> + virHostDevicePtr dev); > Remove the def since it's local to virhost.c > >> +virHostDevicePtr virHostDeviceListFind(virHostDeviceListPtr list, >> + virHostDevicePtr dev); >> +int virHostDeviceListAdd(virHostDeviceListPtr list, >> + virHostDevicePtr dev); >> +void virHostDeviceListDel(virHostDeviceListPtr list, >> + virHostDevicePtr dev, >> + const char *drvname, >> + const char *domname); >> +virHostDeviceListPtr virHostDeviceListNew(void); >> +virHostDevicePtr virHostDeviceNew(const char *name); >> +int virHostDeviceSetUsedBy(virHostDevicePtr dev, >> + const char *drvname, >> + const char *domname); >> +void virHostDeviceFree(virHostDevicePtr dev); >> +int virHostOpenVhostSCSI(int *vhostfd); >> + >> +#endif /* __VIR_HOST_H__ */ >> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c >> index 9c2262e..b92e246 100644 >> --- a/src/util/virhostdev.c >> +++ b/src/util/virhostdev.c >> @@ -146,6 +146,7 @@ virHostdevManagerDispose(void *obj) >> virObjectUnref(hostdevMgr->inactivePCIHostdevs); >> virObjectUnref(hostdevMgr->activeUSBHostdevs); >> virObjectUnref(hostdevMgr->activeSCSIHostdevs); >> + virObjectUnref(hostdevMgr->activeHostHostdevs); >> VIR_FREE(hostdevMgr->stateDir); >> } >> @@ -170,6 +171,9 @@ virHostdevManagerNew(void) >> if (!(hostdevMgr->activeSCSIHostdevs = virSCSIDeviceListNew())) >> goto error; >> + if (!(hostdevMgr->activeHostHostdevs = virHostDeviceListNew())) >> + goto error; >> + >> if (privileged) { >> if (VIR_STRDUP(hostdevMgr->stateDir, HOSTDEV_STATE_DIR) < 0) >> goto error; >> @@ -1472,6 +1476,102 @@ >> virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr, >> return -1; >> } >> +int >> +virHostdevPrepareHostDevices(virHostdevManagerPtr mgr, >> + const char *drv_name, >> + const char *dom_name, >> + virDomainHostdevDefPtr *hostdevs, >> + int nhostdevs) >> +{ >> + size_t i, j; >> + int count; >> + virHostDeviceListPtr list; >> + virHostDevicePtr tmp; >> + >> + if (!nhostdevs) >> + return 0; >> + >> + /* To prevent situation where scsi_host device is assigned to >> two domains >> + * we need to keep a list of currently assigned scsi_host devices. >> + * This is done in several loops which cannot be joined into one >> big >> + * loop. See virHostdevPreparePCIDevices() >> + */ >> + if (!(list = virHostDeviceListNew())) >> + goto cleanup; >> + >> + /* Loop 1: build temporary list */ >> + for (i = 0; i < nhostdevs; i++) { >> + virDomainHostdevDefPtr hostdev = hostdevs[i]; >> + virDomainHostdevSubsysHostPtr hostsrc = >> &hostdev->source.subsys.u.host; >> + >> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || >> + hostdev->source.subsys.type != >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) >> + continue; >> + >> + if (hostsrc->protocol != >> VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) { >> + continue; /* Not supported */ >> + } else { > The else would be unnecessary - just move the virSCSIHostDevicePtr up to > the beginning of the for loop. > >> + virHostDevicePtr host; >> + if (!(host = virHostDeviceNew(hostsrc->wwpn))) >> + goto cleanup; >> + >> + if (virHostDeviceListAdd(list, host) < 0) { >> + virHostDeviceFree(host); >> + 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(mgr->activeHostHostdevs); >> + count = virHostDeviceListCount(list); >> + >> + for (i = 0; i < count; i++) { >> + virHostDevicePtr host = virHostDeviceListGet(list, i); > See this code doesn't do any sort of sharing - so no need for a list, > just a single entry... > >> + if ((tmp = virHostDeviceListFind(mgr->activeHostHostdevs, >> host))) { >> + virReportError(VIR_ERR_OPERATION_INVALID, >> + _("SCSI_host device %s is already in use >> by " >> + "another domain"), >> + virHostDeviceGetName(tmp)); >> + goto error; >> + } else { >> + if (virHostDeviceSetUsedBy(host, drv_name, dom_name) < 0) >> + goto error; >> + >> + VIR_DEBUG("Adding %s to activeHostHostdevs", >> virHostDeviceGetName(host)); >> + >> + if (virHostDeviceListAdd(mgr->activeHostHostdevs, host) >> < 0) >> + goto error; >> + } >> + } >> + >> + virObjectUnlock(mgr->activeHostHostdevs); >> + >> + /* Loop 3: Temporary list was successfully merged with >> + * driver list, so steal all items to avoid freeing them >> + * when freeing temporary list. >> + */ >> + while (virHostDeviceListCount(list) > 0) { >> + tmp = virHostDeviceListGet(list, 0); >> + virHostDeviceListSteal(list, tmp); >> + } >> + >> + virObjectUnref(list); >> + return 0; >> + error: >> + for (j = 0; j < i; j++) { >> + tmp = virHostDeviceListGet(list, i); >> + virHostDeviceListSteal(mgr->activeHostHostdevs, tmp); >> + } >> + virObjectUnlock(mgr->activeHostHostdevs); >> + cleanup: >> + virObjectUnref(list); >> + return -1; >> +} >> + >> void >> virHostdevReAttachUSBDevices(virHostdevManagerPtr mgr, >> const char *drv_name, >> @@ -1604,6 +1704,61 @@ >> virHostdevReAttachSCSIDevices(virHostdevManagerPtr mgr, >> virObjectUnlock(mgr->activeSCSIHostdevs); >> } >> +void >> +virHostdevReAttachHostDevices(virHostdevManagerPtr mgr, >> + const char *drv_name, >> + const char *dom_name, >> + virDomainHostdevDefPtr *hostdevs, >> + int nhostdevs) >> +{ >> + size_t i; >> + virHostDevicePtr host, tmp; >> + >> + >> + if (!nhostdevs) >> + return; >> + >> + virObjectLock(mgr->activeHostHostdevs); >> + for (i = 0; i < nhostdevs; i++) { >> + virDomainHostdevDefPtr hostdev = hostdevs[i]; >> + virDomainHostdevSubsysHostPtr hostsrc = >> &hostdev->source.subsys.u.host; >> + >> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || >> + hostdev->source.subsys.type != >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) >> + continue; >> + >> + if (hostsrc->protocol != >> VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) >> + continue; /* Not supported */ >> + >> + if (!(host = virHostDeviceNew(hostsrc->wwpn))) { >> + VIR_WARN("Unable to reattach SCSI_host device %s on >> domain %s", >> + hostsrc->wwpn, dom_name); >> + virObjectUnlock(mgr->activeHostHostdevs); >> + return; >> + } >> + > I assume the following changes to match PCI/USB more closely since no > sharing is allowed. > >> + /* Only delete the devices which are marked as being used by >> @name, >> + * because qemuProcessStart could fail half way through. */ >> + >> + if (!(tmp = virHostDeviceListFind(mgr->activeHostHostdevs, >> host))) { >> + VIR_WARN("Unable to find device %s " >> + "in list of active SCSI_host devices", >> + hostsrc->wwpn); >> + virHostDeviceFree(host); >> + virObjectUnlock(mgr->activeHostHostdevs); >> + return; >> + } >> + >> + VIR_DEBUG("Removing %s dom=%s from activeHostHostdevs", >> + hostsrc->wwpn, dom_name); >> + >> + virHostDeviceListDel(mgr->activeHostHostdevs, tmp, >> + drv_name, dom_name); >> + virHostDeviceFree(host); >> + } >> + virObjectUnlock(mgr->activeHostHostdevs); >> +} >> + >> int >> virHostdevPCINodeDeviceDetach(virHostdevManagerPtr mgr, >> virPCIDevicePtr pci) >> diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h >> index f2f51bd..19cef7e 100644 >> --- a/src/util/virhostdev.h >> +++ b/src/util/virhostdev.h >> @@ -30,6 +30,7 @@ >> # include "virpci.h" >> # include "virusb.h" >> # include "virscsi.h" >> +# include "virhost.h" >> # include "domain_conf.h" >> typedef enum { >> @@ -53,6 +54,7 @@ struct _virHostdevManager { >> virPCIDeviceListPtr inactivePCIHostdevs; >> virUSBDeviceListPtr activeUSBHostdevs; >> virSCSIDeviceListPtr activeSCSIHostdevs; >> + virHostDeviceListPtr activeHostHostdevs; > s/activeHostHostdevs/activeSCSIHostHostdevs/ > > John > >> }; >> virHostdevManagerPtr virHostdevManagerGetDefault(void); >> @@ -87,6 +89,13 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr >> hostdev_mgr, >> virDomainHostdevDefPtr *hostdevs, >> int nhostdevs) >> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >> +int >> +virHostdevPrepareHostDevices(virHostdevManagerPtr hostdev_mgr, >> + const char *drv_name, >> + const char *dom_name, >> + virDomainHostdevDefPtr *hostdevs, >> + int nhostdevs) >> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >> void >> virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, >> const char *drv_name, >> @@ -109,6 +118,13 @@ >> virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, >> virDomainHostdevDefPtr *hostdevs, >> int nhostdevs) >> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >> +void >> +virHostdevReAttachHostDevices(virHostdevManagerPtr hostdev_mgr, >> + const char *drv_name, >> + const char *dom_name, >> + virDomainHostdevDefPtr *hostdevs, >> + int nhostdevs) >> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >> int >> virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, >> virDomainHostdevDefPtr *hostdevs, >> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c >> index 78a224b..2c85140 100644 >> --- a/tests/qemuxml2argvmock.c >> +++ b/tests/qemuxml2argvmock.c >> @@ -24,6 +24,7 @@ >> #include "viralloc.h" >> #include "vircommand.h" >> #include "vircrypto.h" >> +#include "virhost.h" >> #include "virmock.h" >> #include "virnetdev.h" >> #include "virnetdevip.h" >> @@ -107,6 +108,14 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix >> ATTRIBUTE_UNUSED, >> } >> int >> +virHostOpenVhostSCSI(int *vhostfd) >> +{ >> + *vhostfd = STDERR_FILENO + 1; >> + >> + return 0; >> +} >> + >> +int >> virNetDevTapCreate(char **ifname, >> const char *tunpath ATTRIBUTE_UNUSED, >> int *tapfd, >>

On 11/14/2016 04:40 PM, John Ferlan wrote: > > On 11/14/2016 03:38 PM, Eric Farman wrote: >> >> On 11/11/2016 04:44 PM, John Ferlan wrote: >>> While perhaps mostly obvious - you need some sort of commit message >>> here. >>> >>> On 11/08/2016 01:26 PM, Eric Farman wrote: >>>> Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; >>>> --- >>>> po/POTFILES.in | 1 + >>>> src/Makefile.am | 1 + >>>> src/libvirt_private.syms | 19 +++ >>>> src/util/virhost.c | 299 >>>> +++++++++++++++++++++++++++++++++++++++++++++++ >>>> src/util/virhost.h | 72 ++++++++++++ >>>> src/util/virhostdev.c | 155 ++++++++++++++++++++++++ >>>> src/util/virhostdev.h | 16 +++ >>>> tests/qemuxml2argvmock.c | 9 ++ >>>> 8 files changed, 572 insertions(+) >>>> create mode 100644 src/util/virhost.c >>>> create mode 100644 src/util/virhost.h >>>> >>> I fear someone will equate "virhost" to host virtual functions as >>> opposed to what it is. You'll note there's virhostcpu, virhostdev, and >>> virhostmem - each of which are utility API's for that subsystem. >>> >>> So I think this needs to become 'virscsihost.{c,h}' and of course the >>> API's are 'virSCSIHostDevice' prefixed instead of 'virHostDevice'. >>> Alternatively, the functions could go in virscsi.c, but I do prefer the >>> separation. >>> >>>> diff --git a/po/POTFILES.in b/po/POTFILES.in >>>> index 1469240..a7cc542 100644 >>>> --- a/po/POTFILES.in >>>> +++ b/po/POTFILES.in >>>> @@ -199,6 +199,7 @@ src/util/virfirewall.c >>>> src/util/virfirmware.c >>>> src/util/virhash.c >>>> src/util/virhook.c >>>> +src/util/virhost.c >>>> src/util/virhostcpu.c >>>> src/util/virhostdev.c >>>> src/util/virhostmem.c >>>> diff --git a/src/Makefile.am b/src/Makefile.am >>>> index d417b6e..404c64e 100644 >>>> --- a/src/Makefile.am >>>> +++ b/src/Makefile.am >>>> @@ -122,6 +122,7 @@ UTIL_SOURCES = \ >>>> util/virhash.c util/virhash.h \ >>>> util/virhashcode.c util/virhashcode.h \ >>>> util/virhook.c util/virhook.h \ >>>> + util/virhost.c util/virhost.h \ >>>> util/virhostcpu.c util/virhostcpu.h util/virhostcpupriv.h \ >>>> util/virhostdev.c util/virhostdev.h \ >>>> util/virhostmem.c util/virhostmem.h \ >>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >>>> index 74dd527..ff535f9 100644 >>>> --- a/src/libvirt_private.syms >>>> +++ b/src/libvirt_private.syms >>>> @@ -1675,6 +1675,23 @@ virHookInitialize; >>>> virHookPresent; >>>> +# util/virhost.h >>>> +virHostDeviceFileIterate; >>>> +virHostDeviceFree; >>>> +virHostDeviceGetName; >>>> +virHostDeviceListAdd; >>>> +virHostDeviceListCount; >>>> +virHostDeviceListDel; >>>> +virHostDeviceListFind; >>>> +virHostDeviceListFindIndex; >>> This one is not used externally, so it could just be static to >>> virhost.c >>> >>>> +virHostDeviceListGet; >>>> +virHostDeviceListNew; >>>> +virHostDeviceListSteal; >>>> +virHostDeviceNew; >>>> +virHostDeviceSetUsedBy; >>>> +virHostOpenVhostSCSI; >>>> + >>>> + >>>> # util/virhostdev.h >>>> virHostdevFindUSBDevice; >>>> virHostdevManagerGetDefault; >>>> @@ -1682,10 +1699,12 @@ virHostdevPCINodeDeviceDetach; >>>> virHostdevPCINodeDeviceReAttach; >>>> virHostdevPCINodeDeviceReset; >>>> virHostdevPrepareDomainDevices; >>>> +virHostdevPrepareHostDevices; >> As I alluded to in my response in patch 3, taking your s/Host/SCSIHost/ >> comment to heart breaks down here because >> "virHostdevPrepareSCSIHostDevices" was introduced by commit 17bddc46. > > Ugh... So many close names is making my eyes hurt. Mine too. :) > I was afraid we may > end with this dilemma. Trust me I waffled a lot about saying anything on > this, but it eventually got to a point where there were virHostXXX API's > that I really had to say something. My other thought along the way was > to use the "vHost" in some way (e.g. virSCSIHostvHost prefixes). > > I was hoping though that a generic solution would work, but felt it may > not work for everything and we could address those as we went along. > > For this particular one what you're adding I think would be: > > virHostdevPrepareSCSIHostvHostDevices > > That would say to me as a reader that I'm modifying a scsi_host device > using the vhost protocol. Search around for "SCSIiSCSI" to see some > really ugly names. Would "SCSIHostVHost" be acceptable? Writing "SCSIHostvHost" looks like a typo to me.

> >> So now I'm torn in how far to correlate the changes. I currently have >> two (to-be-squashed) patches that does "s/virHost/virSCSIHost/" and >> "s/HOST/SCSIHOST/" within the context of this series, but going farther >> means qemuHostdevPrepareSCSIHostDevices calls >> virHostdevPrepareHostDevices, because qemuHostdevPrepareSCSIDevices >> calls virHostdevPrepareSCSIDevices, whihc may call >> virHostdevPrepareSCSIHostDevices >> > I recall hitting something else during review, but now I cannot remember > what it was... There was some name that should have use SCSI, but > didn't. I thought I flagged it, but I might not of. Now I cannot > remember what it was. It was something I found while looking at the PCI > address code. <sigh> must've been interrupted and lost my train of > thought and didn't get back to it. I'll keep an eye out. As long as I'm mucking around in this code, if we can straighten some things out that would be a Good Thing. - Eric > >>>> virHostdevPreparePCIDevices; >>>> virHostdevPrepareSCSIDevices; >>>> virHostdevPrepareUSBDevices; >>>> virHostdevReAttachDomainDevices; >>>> +virHostdevReAttachHostDevices; >>>> virHostdevReAttachPCIDevices; >>>> virHostdevReAttachSCSIDevices; >>>> virHostdevReAttachUSBDevices; >>>> diff --git a/src/util/virhost.c b/src/util/virhost.c >>>> new file mode 100644 >>>> index 0000000..9b5f524 >>>> --- /dev/null >>>> +++ b/src/util/virhost.c >>>> @@ -0,0 +1,299 @@ >>>> +/* >>>> + * virhost.c: helper APIs for managing scsi_host devices >>>> + * >>>> + * Copyright (C) 2016 IBM Corporation >>>> + * >>>> + * 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: >>>> + * Eric Farman <farman(a)linux.vnet.ibm.com&gt; >>>> + */ >>>> + >>>> +#include <config.h> >>>> +#include <fcntl.h> >>>> + >>>> +#include "virhost.h" >>>> +#include "virlog.h" >>>> +#include "viralloc.h" >>>> +#include "virerror.h" >>>> +#include "virfile.h" >>>> +#include "virstring.h" >>>> + >>>> +VIR_LOG_INIT("util.host"); >>>> + >>>> +#define SYSFS_VHOST_SCSI_DEVICES "/sys/kernel/config/target/vhost/" >>>> +#define VHOST_SCSI_DEVICE "/dev/vhost-scsi" >>>> + >>>> +struct _virUsedByInfo { >>>> + char *drvname; /* which driver */ >>>> + char *domname; /* which domain */ >>>> +}; >>>> +typedef struct _virUsedByInfo *virUsedByInfoPtr; >>> Hmm... seeing this makes me think the code should go in virscsi.c... >>> That would seem to then allow more code reuse... >>> >>> However, looking at your virHostdevPrepareHostDevices changes for how >>> this is implemented, I see no sharing allowed/going on. Thus, >>> there's no >>> need for a list of used_by - rather it's just a single >>> 'used_by_drvname' >>> and 'used_by_domname' in the virSCSIHostDevice structure (similar to >>> PCI >>> and USB). >>> >>> Unless of course you think you want to add sharing.... which I cannot >>> imagine is a good idea... >> No, this is a fine idea. I cannot see a point in permitting this. >> >>>> + >>>> +struct _virHostDevice { >>> s/Host/SCSIHost/ >>> >>>> + char *name; /* naa.<wwn> */ >>>> + char *path; >>>> + virUsedByInfoPtr *used_by; /* driver:domain(s) using this dev */ >>>> + size_t n_used_by; /* how many domains are using this dev */ >>> Looks like the above 2 get replaced by >>> >>> char *used_by_drvname; >>> char *used_by_domname; >>> >>> Also - looking at QEMU and "forward" thinking - there are options on >>> the >>> qemu command line for things like boot_tpgt, num_queues, max_sectors, >>> cmd_per_lun, and bootindex... I can see the last one being asked for - >>> as in can we pass one of these to the guest to allow booting from a >>> specific LUN on the array... >>> >>>> +}; >>>> + >>>> +struct _virHostDeviceList { >>> s/Host/SCSIHost/ >>> >>>> + virObjectLockable parent; >>>> + size_t count; >>>> + virHostDevicePtr *devs; >>>> +}; >>>> + >>>> +static virClassPtr virHostDeviceListClass; >>> s/Host/SCSIHost/ >>> >>> (ad nauseum ;-)) >>> >>>> + >>>> +static void >>>> +virHostDeviceListDispose(void *obj) >>>> +{ >>>> + virHostDeviceListPtr list = obj; >>>> + size_t i; >>>> + >>>> + for (i = 0; i < list->count; i++) >>>> + virHostDeviceFree(list->devs[i]); >>>> + >>>> + VIR_FREE(list->devs); >>>> +} >>>> + >>>> + >>> You start w/ the newer convention of 2 spaces between functions, but it >>> ends here. After this there's always just 1 space between functions - >>> let's try to go w/ 2. >> OK. >> >>>> +static int >>>> +virHostOnceInit(void) >>>> +{ >>>> + if (!(virHostDeviceListClass = >>>> virClassNew(virClassForObjectLockable(), >>>> + "virHostDeviceList", >>>> + >>>> sizeof(virHostDeviceList), >>>> + >>>> virHostDeviceListDispose))) >>>> + return -1; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +VIR_ONCE_GLOBAL_INIT(virHost) >>>> + >>>> +/* For virReportOOMError() and virReportSystemError() */ >>>> +#define VIR_FROM_THIS VIR_FROM_NONE >>>> + >>>> +int >>>> +virHostOpenVhostSCSI(int *vhostfd) >>>> +{ >>>> + if (!virFileExists(VHOST_SCSI_DEVICE)) >>>> + goto error; >>>> + >>>> + *vhostfd = open(VHOST_SCSI_DEVICE, O_RDWR); >>>> + >>>> + if (*vhostfd < 0) { >>>> + virReportSystemError(errno, _("Failed to open %s"), >>>> VHOST_SCSI_DEVICE); >>>> + goto error; >>>> + } >>>> + >>>> + return 0; >>>> + >>>> + error: >>>> + VIR_FORCE_CLOSE(*vhostfd); >>>> + >>>> + return -1; >>>> +} >>>> + >>>> +static void >>>> +virHostDeviceUsedByInfoFree(virUsedByInfoPtr used_by) >>>> +{ >>> I think this ends up going away since things aren't shareable. >>> >>>> + VIR_FREE(used_by->drvname); >>>> + VIR_FREE(used_by->domname); >>>> + VIR_FREE(used_by); >>>> +} >>>> + >>>> +void >>>> +virHostDeviceListDel(virHostDeviceListPtr list, >>>> + virHostDevicePtr dev, >>>> + const char *drvname, >>>> + const char *domname) >>>> +{ >>>> + virHostDevicePtr tmp = NULL; >>>> + size_t i; >>>> + >>> This will need to be adjusted since (I assume) you don't want shareable >>> scsi_host devices >>> >>>> + for (i = 0; i < dev->n_used_by; i++) { >>>> + if (STREQ_NULLABLE(dev->used_by[i]->drvname, drvname) && >>>> + STREQ_NULLABLE(dev->used_by[i]->domname, domname)) { >>>> + if (dev->n_used_by > 1) { >>>> + virHostDeviceUsedByInfoFree(dev->used_by[i]); >>>> + VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by); >>>> + } else { >>>> + tmp = virHostDeviceListSteal(list, dev); >>>> + virHostDeviceFree(tmp); >>>> + } >>>> + break; >>>> + } >>>> + } >>>> +} >>>> + >>>> +int >>> s/int/static int/ >>> >>>> +virHostDeviceListFindIndex(virHostDeviceListPtr list, >>>> virHostDevicePtr dev) >>>> +{ >>>> + size_t i; >>>> + >>>> + for (i = 0; i < list->count; i++) { >>>> + virHostDevicePtr other = list->devs[i]; >>>> + if (STREQ_NULLABLE(other->name, dev->name)) >>>> + return i; >>>> + } >>>> + return -1; >>>> +} >>>> + >>>> +virHostDevicePtr >>>> +virHostDeviceListGet(virHostDeviceListPtr list, int idx) >>>> +{ >>>> + if (idx >= list->count || idx < 0) >>>> + return NULL; >>>> + >>>> + return list->devs[idx]; >>>> +} >>>> + >>>> +size_t >>>> +virHostDeviceListCount(virHostDeviceListPtr list) >>>> +{ >>>> + return list->count; >>>> +} >>>> + >>>> +virHostDevicePtr >>>> +virHostDeviceListSteal(virHostDeviceListPtr list, >>>> + virHostDevicePtr dev) >>>> +{ >>>> + virHostDevicePtr ret = NULL; >>>> + size_t i; >>>> + >>>> + for (i = 0; i < list->count; i++) { >>>> + if (STREQ_NULLABLE(list->devs[i]->name, dev->name)) { >>>> + ret = list->devs[i]; >>>> + VIR_DELETE_ELEMENT(list->devs, i, list->count); >>>> + break; >>>> + } >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +virHostDevicePtr >>>> +virHostDeviceListFind(virHostDeviceListPtr list, virHostDevicePtr >>>> dev) >>>> +{ >>>> + int idx; >>>> + >>>> + if ((idx = virHostDeviceListFindIndex(list, dev)) >= 0) >>>> + return list->devs[idx]; >>>> + else >>>> + return NULL; >>>> +} >>>> + >>>> +int >>>> +virHostDeviceListAdd(virHostDeviceListPtr list, >>>> + virHostDevicePtr dev) >>>> +{ >>>> + if (virHostDeviceListFind(list, dev)) { >>>> + virReportError(VIR_ERR_INTERNAL_ERROR, >>>> + _("Device %s is already in use"), dev->name); >>>> + return -1; >>>> + } >>>> + return VIR_APPEND_ELEMENT(list->devs, list->count, dev); >>>> +} >>>> + >>>> +virHostDeviceListPtr >>>> +virHostDeviceListNew(void) >>>> +{ >>>> + virHostDeviceListPtr list; >>>> + >>>> + if (virHostInitialize() < 0) >>>> + return NULL; >>>> + >>>> + if (!(list = virObjectLockableNew(virHostDeviceListClass))) >>>> + return NULL; >>>> + >>>> + return list; >>> or simply return virObjectLockableNew(virSCSIHostDeviceListClass); and >>> then 'list' is not needed. >>> >>>> +} >>>> + >>>> +int >>>> +virHostDeviceSetUsedBy(virHostDevicePtr dev, >>>> + const char *drvname, >>>> + const char *domname) >>>> +{ >>> Without sharing, this mimics PCI/USB instead... >>> >>>> + virUsedByInfoPtr copy; >>>> + if (VIR_ALLOC(copy) < 0) >>>> + return -1; >>>> + if (VIR_STRDUP(copy->drvname, drvname) < 0 || >>>> + VIR_STRDUP(copy->domname, domname) < 0) >>>> + goto cleanup; >>>> + >>>> + if (VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy) < 0) >>>> + goto cleanup; >>>> + >>>> + return 0; >>>> + >>>> + cleanup: >>>> + virHostDeviceUsedByInfoFree(copy); >>>> + return -1; >>>> +} >>>> + >>>> +int >>>> +virHostDeviceFileIterate(virHostDevicePtr dev, >>>> + virHostDeviceFileActor actor, >>>> + void *opaque) >>>> +{ >>>> + return (actor)(dev, dev->path, opaque); >>>> +} >>>> + >>>> +const char * >>>> +virHostDeviceGetName(virHostDevicePtr dev) >>>> +{ >>>> + return dev->name; >>>> +} >>>> + >>>> +virHostDevicePtr >>>> +virHostDeviceNew(const char *name) >>>> +{ >>>> + virHostDevicePtr dev; >>>> + >>>> + if (VIR_ALLOC(dev) < 0) >>>> + return NULL; >>>> + >>>> + if (VIR_STRDUP(dev->name, name) < 0) { >>>> + virReportError(VIR_ERR_INTERNAL_ERROR, >>>> + _("dev->name buffer overflow: %s"), >>>> + name); >>>> + goto error; >>>> + } >>>> + >>>> + if (virAsprintf(&dev->path, "%s/%s", >>>> + SYSFS_VHOST_SCSI_DEVICES, name) < 0) >>>> + goto cleanup; >>>> + >>>> + VIR_DEBUG("%s: initialized", dev->name); >>>> + >>>> + cleanup: >>>> + return dev; >>>> + >>>> + error: >>>> + virHostDeviceFree(dev); >>>> + dev = NULL; >>>> + goto cleanup; >>>> +} >>>> + >>>> +void >>>> +virHostDeviceFree(virHostDevicePtr dev) >>>> +{ >>> size_t i; >>> >>>> + if (!dev) >>>> + return; >>>> + VIR_DEBUG("%s: freeing", dev->name); >>> Would be nice if it was freeing everything in 'dev' before freeing >>> dev... >> Oops! >> >>> Thus you have: >>> >>> VIR_FREE(dev->name); >>> VIR_FREE(dev->path); >>> VIR_FREE(dev->used_by_drvname); >>> VIR_FREE(dev->used_by_domname); >>> >>>> + VIR_FREE(dev); >>>> +} >>>> diff --git a/src/util/virhost.h b/src/util/virhost.h >>>> new file mode 100644 >>>> index 0000000..6d7b790 >>>> --- /dev/null >>>> +++ b/src/util/virhost.h >>>> @@ -0,0 +1,72 @@ >>>> +/* >>>> + * virhost.h: helper APIs for managing host scsi_host devices >>>> + * >>>> + * Copyright (C) 2016 IBM Corporation >>>> + * >>>> + * 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: >>>> + * Eric Farman <farman(a)linux.vnet.ibm.com&gt; >>>> + */ >>>> + >>>> +#ifndef __VIR_HOST_H__ >>>> +# define __VIR_HOST_H__ >>>> + >>>> +# include "internal.h" >>>> +# include "virobject.h" >>>> +# include "virutil.h" >>>> + >>>> +typedef struct _virHostDevice virHostDevice; >>>> +typedef virHostDevice *virHostDevicePtr; >>>> +typedef struct _virHostDeviceAddress virHostDeviceAddress; >>>> +typedef virHostDeviceAddress *virHostDeviceAddressPtr; >>>> +typedef struct _virHostDeviceList virHostDeviceList; >>>> +typedef virHostDeviceList *virHostDeviceListPtr; >>>> + >>>> +struct _virHostDeviceAddress { >>>> + char *wwpn; >>>> +}; >>> Hmmm.. this would seemingly duplicate name without the "naa.", but in >>> the long run it's not even used - so why is it necessary? >>> >>> Then again as it turns out a "vhost-scsi-pci" or "vhost-scsi-ccw" >>> device >>> object is created. Each of those would seemingly need a controller to >>> use wouldn't they? >> Are you referring to a <controller> tag? Or something else? >> > Um.. oh yeah. I think at this point it wasn't totally clear what address > was going to be used in the guest. This is probably where I started > down the rabbit hole of figuring out how PCI addresses would be added to > the XML. > >>> So the address would seem to be important, but is not >>> handled. >> Will check how the PCI handling is broken. Seemed okay for CCW, but >> maybe I've missed something there too. >> > I think you're letting QEMU pick which may not be a good idea based on > our recent experience. > > John >> - Eric >> >>>> + >>>> +typedef int (*virHostDeviceFileActor)(virHostDevicePtr dev, >>>> + const char *name, void >>>> *opaque); >>>> + >>>> +int virHostDeviceFileIterate(virHostDevicePtr dev, >>>> + virHostDeviceFileActor actor, >>>> + void *opaque); >>>> +const char *virHostDeviceGetName(virHostDevicePtr dev); >>>> +virHostDevicePtr virHostDeviceListGet(virHostDeviceListPtr list, >>>> + int idx); >>>> +size_t virHostDeviceListCount(virHostDeviceListPtr list); >>>> +virHostDevicePtr virHostDeviceListSteal(virHostDeviceListPtr list, >>>> + virHostDevicePtr dev); >>>> +int virHostDeviceListFindIndex(virHostDeviceListPtr list, >>>> + virHostDevicePtr dev); >>> Remove the def since it's local to virhost.c >>> >>>> +virHostDevicePtr virHostDeviceListFind(virHostDeviceListPtr list, >>>> + virHostDevicePtr dev); >>>> +int virHostDeviceListAdd(virHostDeviceListPtr list, >>>> + virHostDevicePtr dev); >>>> +void virHostDeviceListDel(virHostDeviceListPtr list, >>>> + virHostDevicePtr dev, >>>> + const char *drvname, >>>> + const char *domname); >>>> +virHostDeviceListPtr virHostDeviceListNew(void); >>>> +virHostDevicePtr virHostDeviceNew(const char *name); >>>> +int virHostDeviceSetUsedBy(virHostDevicePtr dev, >>>> + const char *drvname, >>>> + const char *domname); >>>> +void virHostDeviceFree(virHostDevicePtr dev); >>>> +int virHostOpenVhostSCSI(int *vhostfd); >>>> + >>>> +#endif /* __VIR_HOST_H__ */ >>>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c >>>> index 9c2262e..b92e246 100644 >>>> --- a/src/util/virhostdev.c >>>> +++ b/src/util/virhostdev.c >>>> @@ -146,6 +146,7 @@ virHostdevManagerDispose(void *obj) >>>> virObjectUnref(hostdevMgr->inactivePCIHostdevs); >>>> virObjectUnref(hostdevMgr->activeUSBHostdevs); >>>> virObjectUnref(hostdevMgr->activeSCSIHostdevs); >>>> + virObjectUnref(hostdevMgr->activeHostHostdevs); >>>> VIR_FREE(hostdevMgr->stateDir); >>>> } >>>> @@ -170,6 +171,9 @@ virHostdevManagerNew(void) >>>> if (!(hostdevMgr->activeSCSIHostdevs = virSCSIDeviceListNew())) >>>> goto error; >>>> + if (!(hostdevMgr->activeHostHostdevs = >>>> virHostDeviceListNew())) >>>> + goto error; >>>> + >>>> if (privileged) { >>>> if (VIR_STRDUP(hostdevMgr->stateDir, HOSTDEV_STATE_DIR) >>>> < 0) >>>> goto error; >>>> @@ -1472,6 +1476,102 @@ >>>> virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr, >>>> return -1; >>>> } >>>> +int >>>> +virHostdevPrepareHostDevices(virHostdevManagerPtr mgr, >>>> + const char *drv_name, >>>> + const char *dom_name, >>>> + virDomainHostdevDefPtr *hostdevs, >>>> + int nhostdevs) >>>> +{ >>>> + size_t i, j; >>>> + int count; >>>> + virHostDeviceListPtr list; >>>> + virHostDevicePtr tmp; >>>> + >>>> + if (!nhostdevs) >>>> + return 0; >>>> + >>>> + /* To prevent situation where scsi_host device is assigned to >>>> two domains >>>> + * we need to keep a list of currently assigned scsi_host >>>> devices. >>>> + * This is done in several loops which cannot be joined into one >>>> big >>>> + * loop. See virHostdevPreparePCIDevices() >>>> + */ >>>> + if (!(list = virHostDeviceListNew())) >>>> + goto cleanup; >>>> + >>>> + /* Loop 1: build temporary list */ >>>> + for (i = 0; i < nhostdevs; i++) { >>>> + virDomainHostdevDefPtr hostdev = hostdevs[i]; >>>> + virDomainHostdevSubsysHostPtr hostsrc = >>>> &hostdev->source.subsys.u.host; >>>> + >>>> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || >>>> + hostdev->source.subsys.type != >>>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) >>>> + continue; >>>> + >>>> + if (hostsrc->protocol != >>>> VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) { >>>> + continue; /* Not supported */ >>>> + } else { >>> The else would be unnecessary - just move the virSCSIHostDevicePtr >>> up to >>> the beginning of the for loop. >>> >>>> + virHostDevicePtr host; >>>> + if (!(host = virHostDeviceNew(hostsrc->wwpn))) >>>> + goto cleanup; >>>> + >>>> + if (virHostDeviceListAdd(list, host) < 0) { >>>> + virHostDeviceFree(host); >>>> + 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(mgr->activeHostHostdevs); >>>> + count = virHostDeviceListCount(list); >>>> + >>>> + for (i = 0; i < count; i++) { >>>> + virHostDevicePtr host = virHostDeviceListGet(list, i); >>> See this code doesn't do any sort of sharing - so no need for a list, >>> just a single entry... >>> >>>> + if ((tmp = virHostDeviceListFind(mgr->activeHostHostdevs, >>>> host))) { >>>> + virReportError(VIR_ERR_OPERATION_INVALID, >>>> + _("SCSI_host device %s is already in use >>>> by " >>>> + "another domain"), >>>> + virHostDeviceGetName(tmp)); >>>> + goto error; >>>> + } else { >>>> + if (virHostDeviceSetUsedBy(host, drv_name, dom_name) < 0) >>>> + goto error; >>>> + >>>> + VIR_DEBUG("Adding %s to activeHostHostdevs", >>>> virHostDeviceGetName(host)); >>>> + >>>> + if (virHostDeviceListAdd(mgr->activeHostHostdevs, host) >>>> < 0) >>>> + goto error; >>>> + } >>>> + } >>>> + >>>> + virObjectUnlock(mgr->activeHostHostdevs); >>>> + >>>> + /* Loop 3: Temporary list was successfully merged with >>>> + * driver list, so steal all items to avoid freeing them >>>> + * when freeing temporary list. >>>> + */ >>>> + while (virHostDeviceListCount(list) > 0) { >>>> + tmp = virHostDeviceListGet(list, 0); >>>> + virHostDeviceListSteal(list, tmp); >>>> + } >>>> + >>>> + virObjectUnref(list); >>>> + return 0; >>>> + error: >>>> + for (j = 0; j < i; j++) { >>>> + tmp = virHostDeviceListGet(list, i); >>>> + virHostDeviceListSteal(mgr->activeHostHostdevs, tmp); >>>> + } >>>> + virObjectUnlock(mgr->activeHostHostdevs); >>>> + cleanup: >>>> + virObjectUnref(list); >>>> + return -1; >>>> +} >>>> + >>>> void >>>> virHostdevReAttachUSBDevices(virHostdevManagerPtr mgr, >>>> const char *drv_name, >>>> @@ -1604,6 +1704,61 @@ >>>> virHostdevReAttachSCSIDevices(virHostdevManagerPtr mgr, >>>> virObjectUnlock(mgr->activeSCSIHostdevs); >>>> } >>>> +void >>>> +virHostdevReAttachHostDevices(virHostdevManagerPtr mgr, >>>> + const char *drv_name, >>>> + const char *dom_name, >>>> + virDomainHostdevDefPtr *hostdevs, >>>> + int nhostdevs) >>>> +{ >>>> + size_t i; >>>> + virHostDevicePtr host, tmp; >>>> + >>>> + >>>> + if (!nhostdevs) >>>> + return; >>>> + >>>> + virObjectLock(mgr->activeHostHostdevs); >>>> + for (i = 0; i < nhostdevs; i++) { >>>> + virDomainHostdevDefPtr hostdev = hostdevs[i]; >>>> + virDomainHostdevSubsysHostPtr hostsrc = >>>> &hostdev->source.subsys.u.host; >>>> + >>>> + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || >>>> + hostdev->source.subsys.type != >>>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) >>>> + continue; >>>> + >>>> + if (hostsrc->protocol != >>>> VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) >>>> + continue; /* Not supported */ >>>> + >>>> + if (!(host = virHostDeviceNew(hostsrc->wwpn))) { >>>> + VIR_WARN("Unable to reattach SCSI_host device %s on >>>> domain %s", >>>> + hostsrc->wwpn, dom_name); >>>> + virObjectUnlock(mgr->activeHostHostdevs); >>>> + return; >>>> + } >>>> + >>> I assume the following changes to match PCI/USB more closely since no >>> sharing is allowed. >>> >>>> + /* Only delete the devices which are marked as being used by >>>> @name, >>>> + * because qemuProcessStart could fail half way through. */ >>>> + >>>> + if (!(tmp = virHostDeviceListFind(mgr->activeHostHostdevs, >>>> host))) { >>>> + VIR_WARN("Unable to find device %s " >>>> + "in list of active SCSI_host devices", >>>> + hostsrc->wwpn); >>>> + virHostDeviceFree(host); >>>> + virObjectUnlock(mgr->activeHostHostdevs); >>>> + return; >>>> + } >>>> + >>>> + VIR_DEBUG("Removing %s dom=%s from activeHostHostdevs", >>>> + hostsrc->wwpn, dom_name); >>>> + >>>> + virHostDeviceListDel(mgr->activeHostHostdevs, tmp, >>>> + drv_name, dom_name); >>>> + virHostDeviceFree(host); >>>> + } >>>> + virObjectUnlock(mgr->activeHostHostdevs); >>>> +} >>>> + >>>> int >>>> virHostdevPCINodeDeviceDetach(virHostdevManagerPtr mgr, >>>> virPCIDevicePtr pci) >>>> diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h >>>> index f2f51bd..19cef7e 100644 >>>> --- a/src/util/virhostdev.h >>>> +++ b/src/util/virhostdev.h >>>> @@ -30,6 +30,7 @@ >>>> # include "virpci.h" >>>> # include "virusb.h" >>>> # include "virscsi.h" >>>> +# include "virhost.h" >>>> # include "domain_conf.h" >>>> typedef enum { >>>> @@ -53,6 +54,7 @@ struct _virHostdevManager { >>>> virPCIDeviceListPtr inactivePCIHostdevs; >>>> virUSBDeviceListPtr activeUSBHostdevs; >>>> virSCSIDeviceListPtr activeSCSIHostdevs; >>>> + virHostDeviceListPtr activeHostHostdevs; >>> s/activeHostHostdevs/activeSCSIHostHostdevs/ >>> >>> John >>> >>>> }; >>>> virHostdevManagerPtr virHostdevManagerGetDefault(void); >>>> @@ -87,6 +89,13 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr >>>> hostdev_mgr, >>>> virDomainHostdevDefPtr *hostdevs, >>>> int nhostdevs) >>>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >>>> +int >>>> +virHostdevPrepareHostDevices(virHostdevManagerPtr hostdev_mgr, >>>> + const char *drv_name, >>>> + const char *dom_name, >>>> + virDomainHostdevDefPtr *hostdevs, >>>> + int nhostdevs) >>>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >>>> void >>>> virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, >>>> const char *drv_name, >>>> @@ -109,6 +118,13 @@ >>>> virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, >>>> virDomainHostdevDefPtr *hostdevs, >>>> int nhostdevs) >>>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >>>> +void >>>> +virHostdevReAttachHostDevices(virHostdevManagerPtr hostdev_mgr, >>>> + const char *drv_name, >>>> + const char *dom_name, >>>> + virDomainHostdevDefPtr *hostdevs, >>>> + int nhostdevs) >>>> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); >>>> int >>>> virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, >>>> virDomainHostdevDefPtr *hostdevs, >>>> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c >>>> index 78a224b..2c85140 100644 >>>> --- a/tests/qemuxml2argvmock.c >>>> +++ b/tests/qemuxml2argvmock.c >>>> @@ -24,6 +24,7 @@ >>>> #include "viralloc.h" >>>> #include "vircommand.h" >>>> #include "vircrypto.h" >>>> +#include "virhost.h" >>>> #include "virmock.h" >>>> #include "virnetdev.h" >>>> #include "virnetdevip.h" >>>> @@ -107,6 +108,14 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix >>>> ATTRIBUTE_UNUSED, >>>> } >>>> int >>>> +virHostOpenVhostSCSI(int *vhostfd) >>>> +{ >>>> + *vhostfd = STDERR_FILENO + 1; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +int >>>> virNetDevTapCreate(char **ifname, >>>> const char *tunpath ATTRIBUTE_UNUSED, >>>> int *tapfd, >>>>

Open /dev/vhost-scsi, and record the resulting file descriptor, so that the guest has access to the host device outside of the libvirt daemon. Pass this information, along with data parsed from the XML file, to build a device string for the qemu command line. That device string will be for either a vhost-scsi-ccw device in the case of an s390 machine, or vhost-scsi-pci for any others. Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_cgroup.c | 32 +++++++++++++++++ src/qemu/qemu_command.c | 79 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_command.h | 5 +++ src/qemu/qemu_domain_address.c | 10 ++++++ src/qemu/qemu_hostdev.c | 41 ++++++++++++++++++++++ src/qemu/qemu_hostdev.h | 8 +++++ 6 files changed, 175 insertions(+)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index ee31d14..a22a1bf 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -277,6 +277,25 @@ qemuSetupHostSCSIDeviceCgroup(virSCSIDevicePtr dev ATTRIBUTE_UNUSED, return ret; } +static int +qemuSetupHostHostDeviceCgroup(virHostDevicePtr dev ATTRIBUTE_UNUSED, + const char *path, + void *opaque) +{ + virDomainObjPtr vm = opaque; + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret; + + VIR_DEBUG("Process path '%s' for scsi_host device", path); + + ret = virCgroupAllowDevicePath(priv->cgroup, path, + VIR_CGROUP_DEVICE_RW, false); + + virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, "rw", ret == 0); + + return ret; +} + int qemuSetupHostdevCgroup(virDomainObjPtr vm, virDomainHostdevDefPtr dev) @@ -286,9 +305,11 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &dev->source.subsys.u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; + virDomainHostdevSubsysHostPtr hostsrc = &dev->source.subsys.u.host; virPCIDevicePtr pci = NULL; virUSBDevicePtr usb = NULL; virSCSIDevicePtr scsi = NULL; + virHostDevicePtr host = NULL; char *path = NULL; /* currently this only does something for PCI devices using vfio @@ -377,6 +398,16 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, } case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: { + if (hostsrc->protocol == + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) { + if ((host = virHostDeviceNew(hostsrc->wwpn)) == NULL)

+ goto cleanup; + + if (virHostDeviceFileIterate(host, + qemuSetupHostHostDeviceCgroup, + vm) < 0) + goto cleanup; + } break; } @@ -390,6 +421,7 @@ qemuSetupHostdevCgroup(virDomainObjPtr vm, virPCIDeviceFree(pci); virUSBDeviceFree(usb); virSCSIDeviceFree(scsi); + virHostDeviceFree(host); VIR_FREE(path); return ret; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9adf0fe..ecd3286 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4730,6 +4730,43 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) } char * +qemuBuildHostHostdevDevStr(const virDomainDef *def, + virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps, + char *vhostfdName) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + virDomainHostdevSubsysHostPtr hostsrc = &dev->source.subsys.u.host; + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_SCSI)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support vhost-scsi devices")); + goto cleanup; + } + + if (ARCH_IS_S390(def->os.arch)) + virBufferAddLit(&buf, "vhost-scsi-ccw"); + else + virBufferAddLit(&buf, "vhost-scsi-pci"); + + virBufferAsprintf(&buf, ",wwpn=%s", hostsrc->wwpn); + virBufferAsprintf(&buf, ",vhostfd=%s", vhostfdName); + virBufferAsprintf(&buf, ",id=%s", dev->info->alias);

+ + if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)

+ goto cleanup; + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + return virBufferContentAndReset(&buf); + + cleanup: + virBufferFreeAndReset(&buf); + return NULL; +} + +char * qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5210,6 +5247,48 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, return -1; } } + + /* SCSI_host */ + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) { + if (!virQEMUCapsGet(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 (hostdev->source.subsys.u.host.protocol == + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) { + char *vhostfdName = NULL;

+ int vhostfd = -1; + + if (virHostOpenVhostSCSI(&vhostfd) < 0) + return -1; + + if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) { + VIR_FORCE_CLOSE(vhostfd); + return -1; + } + + virCommandPassFD(cmd, vhostfd, + VIR_COMMAND_PASS_FD_CLOSE_PARENT); + + virCommandAddArg(cmd, "-device"); + if (!(devstr = qemuBuildHostHostdevDevStr(def, + hostdev, + qemuCaps, + vhostfdName))) { + VIR_FREE(vhostfdName); + VIR_FORCE_CLOSE(vhostfd); + return -1; + } + virCommandAddArg(cmd, devstr); + + VIR_FREE(vhostfdName); + VIR_FREE(devstr); + } + } } return 0; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 0ddaba4..0abf31e 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -163,6 +163,11 @@ char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev); char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); +char * +qemuBuildHostHostdevDevStr(const virDomainDef *def, + virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps, + char *vhostfdName); char *qemuBuildRedirdevDevStr(const virDomainDef *def, virDomainRedirdevDefPtr dev, diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b35a95f..90d3357 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -312,6 +312,16 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,

} } + for (i = 0; i < def->nhostdevs; i++) { + if (def->hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + def->hostdevs[i]->source.subsys.type == + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST && + def->hostdevs[i]->source.subsys.u.host.protocol == + VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST && + def->hostdevs[i]->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + def->hostdevs[i]->info->type = type; + } + if (def->memballoon && def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO && def->memballoon->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index dd3a3cf..29aedeb 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -292,6 +292,17 @@ qemuHostdevPrepareSCSIDevices(virQEMUDriverPtr driver, name, hostdevs, nhostdevs); } +int +qemuHostdevPrepareHostDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; + + return virHostdevPrepareHostDevices(hostdev_mgr, QEMU_DRIVER_NAME, + name, hostdevs, nhostdevs); +} int qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, @@ -315,6 +326,10 @@ qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, def->hostdevs, def->nhostdevs) < 0) return -1; + if (qemuHostdevPrepareHostDevices(driver, def->name, + def->hostdevs, def->nhostdevs) < 0) + return -1; + return 0; } @@ -370,6 +385,29 @@ qemuHostdevReAttachSCSIDevices(virQEMUDriverPtr driver, } void +qemuHostdevReAttachHostDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + size_t i; + virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; + + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + virDomainDeviceDef dev; + + dev.type = VIR_DOMAIN_DEVICE_HOSTDEV; + dev.data.hostdev = hostdev; + + ignore_value(qemuRemoveSharedDevice(driver, &dev, name)); + } + + virHostdevReAttachHostDevices(hostdev_mgr, QEMU_DRIVER_NAME, + name, hostdevs, nhostdevs); +} + +void qemuHostdevReAttachDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def) { @@ -384,4 +422,7 @@ qemuHostdevReAttachDomainDevices(virQEMUDriverPtr driver, qemuHostdevReAttachSCSIDevices(driver, def->name, def->hostdevs, def->nhostdevs); + + qemuHostdevReAttachHostDevices(driver, def->name, def->hostdevs, + def->nhostdevs); } diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 0a3c715..1788d12 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -55,6 +55,10 @@ int qemuHostdevPrepareSCSIDevices(virQEMUDriverPtr driver, const char *name, virDomainHostdevDefPtr *hostdevs, int nhostdevs); +int qemuHostdevPrepareHostDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs); int qemuHostdevPrepareDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, @@ -72,6 +76,10 @@ void qemuHostdevReAttachSCSIDevices(virQEMUDriverPtr driver, const char *name, virDomainHostdevDefPtr *hostdevs, int nhostdevs); +void qemuHostdevReAttachHostDevices(virQEMUDriverPtr driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs); void qemuHostdevReAttachDomainDevices(virQEMUDriverPtr driver, virDomainDefPtr def);

Adjust the device string that is built for vhost-scsi devices so that it can be invoked from hotplug. >From the QEMU command line, the file descriptors are expect to be numeric only.

However, for hotplug, the file descriptors are expected to begin with at least one alphabetic character else this error occurs: # virsh attach-device guest_0001 ~/vhost.xml error: Failed to attach device from /root/vhost.xml error: internal error: unable to execute QEMU command 'getfd': Parameter 'fdname' expects a name not starting with a digit We also close the file descriptor in this case, so that shutting down the guest cleans up the host cgroup entries and allows future guests to use vhost-scsi devices. (Otherwise the guest will silently end.)

Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_hotplug.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2d6b086..d503212 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2425,6 +2425,120 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, goto cleanup; } +static int +qemuDomainAttachHostSCSIHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData;

+ virDomainCCWAddressSetPtr ccwaddrs = NULL; + char *vhostfdName = NULL; + int vhostfd = -1; + char *devstr = NULL; + bool teardowncgroup = false; + bool teardownlabel = false; + bool releaseaddr = false; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("SCSI passthrough is not supported by this version of qemu")); + goto cleanup; + }

+ + if (qemuHostdevPrepareHostDevices(driver, vm->def->name, &hostdev, 1) < 0) { + virDomainHostdevSubsysHostPtr hostsrc = &hostdev->source.subsys.u.host; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to prepare scsi_host hostdev: %s"), + hostsrc->wwpn); + goto cleanup; + } + + if (qemuSetupHostdevCgroup(vm, hostdev) < 0) + goto cleanup; + teardowncgroup = true; + + if (virSecurityManagerSetHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) + goto cleanup; + teardownlabel = true; + + if (virHostOpenVhostSCSI(&vhostfd) < 0) + goto cleanup; + + if (virAsprintf(&vhostfdName, "vhostfd-%d", vhostfd) < 0) + goto cleanup; + + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + if (qemuDomainMachineIsS390CCW(vm->def) && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; + } + + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || + hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, hostdev->info) < 0)

+ goto cleanup; + } else if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def))) + goto cleanup; + if (virDomainCCWAddressAssign(hostdev->info, ccwaddrs, + !hostdev->info->addr.ccw.assigned) < 0) + goto cleanup; + } + releaseaddr = true; + + if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) + goto cleanup; + + if (!(devstr = qemuBuildHostHostdevDevStr(vm->def, + hostdev, + priv->qemuCaps, + vhostfdName)))

+ goto cleanup; + + qemuDomainObjEnterMonitor(driver, vm); + + ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd, vhostfdName);

+ + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto cleanup;

+ } + + virDomainAuditHostdev(vm, hostdev, "attach", (ret == 0)); + + if (ret < 0) + goto cleanup; + + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) + goto cleanup;

+ + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; +

+ virDomainCCWAddressSetFree(ccwaddrs); + + VIR_FORCE_CLOSE(vhostfd); + VIR_FREE(vhostfdName); + VIR_FREE(devstr); + return 0; + + cleanup:

+ if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail"); + if (teardownlabel && + virSecurityManagerRestoreHostdevLabel(driver->securityManager, + vm->def, hostdev, NULL) < 0) + VIR_WARN("Unable to restore host device labelling on hotplug fail"); + if (releaseaddr) + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); +

+ VIR_FORCE_CLOSE(vhostfd); + VIR_FREE(vhostfdName); + VIR_FREE(devstr); + return ret; +} + int qemuDomainAttachHostDevice(virConnectPtr conn, @@ -2458,6 +2572,11 @@ qemuDomainAttachHostDevice(virConnectPtr conn, goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: + if (qemuDomainAttachHostSCSIHostDevice(driver, vm, hostdev) < 0) + goto error; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -3549,6 +3668,14 @@ qemuDomainRemoveSCSIHostDevice(virQEMUDriverPtr driver, qemuHostdevReAttachSCSIDevices(driver, vm->def->name, &hostdev, 1); } +static void +qemuDomainRemoveHostSCSIHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr hostdev) +{ + qemuHostdevReAttachHostDevices(driver, vm->def->name, &hostdev, 1); +} + static int qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3627,6 +3754,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, qemuDomainRemoveSCSIHostDevice(driver, vm, hostdev); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: + qemuDomainRemoveHostSCSIHostDevice(driver, vm, hostdev); break; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: break; @@ -4477,6 +4605,31 @@ qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver, } static int +qemuDomainDetachHostSCSIHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr detach) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret = -1; + + if (!detach->info->alias) { + virReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("device cannot be detached without a device alias")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, detach->info); + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + + return ret; +} + +static int qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainHostdevDefPtr detach) @@ -4498,6 +4651,9 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: ret = qemuDomainDetachHostSCSIDevice(driver, vm, detach); break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: + ret = qemuDomainDetachHostSCSIHostDevice(driver, vm, detach); + break; default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev subsys type '%s' not supported"), @@ -4575,6 +4731,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, } break; } + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected hostdev type %d"), subsys->type);

On 11/11/2016 04:47 PM, John Ferlan wrote: > > On 11/08/2016 01:26 PM, Eric Farman wrote: >> Adjust the device string that is built for vhost-scsi devices so that it >> can be invoked from hotplug. >> >> >From the QEMU command line, the file descriptors are expect to be >> numeric only. > s/>// > > Looks like a cut-n-paste carryover > >> However, for hotplug, the file descriptors are expected to begin with >> at least >> one alphabetic character else this error occurs: >> >> # virsh attach-device guest_0001 ~/vhost.xml >> error: Failed to attach device from /root/vhost.xml >> error: internal error: unable to execute QEMU command 'getfd': >> Parameter 'fdname' expects a name not starting with a digit >> >> We also close the file descriptor in this case, so that shutting down >> the >> guest cleans up the host cgroup entries and allows future guests to use >> vhost-scsi devices. (Otherwise the guest will silently end.) > See you're following the lead of qemuDomainAttachHostPCIDevice for the > 'configfd' processing > >> Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; >> --- >> src/qemu/qemu_hotplug.c | 158 >> ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 158 insertions(+) >> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index 2d6b086..d503212 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -2425,6 +2425,120 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr >> conn, >> goto cleanup; >> } >> +static int >> +qemuDomainAttachHostSCSIHostDevice(virQEMUDriverPtr driver, >> + virDomainObjPtr vm, >> + virDomainHostdevDefPtr hostdev) >> +{ >> + int ret = -1; >> + qemuDomainObjPrivatePtr priv = vm->privateData; > virErrorPtr orig_err; > >> + virDomainCCWAddressSetPtr ccwaddrs = NULL; >> + char *vhostfdName = NULL; >> + int vhostfd = -1; >> + char *devstr = NULL; >> + bool teardowncgroup = false; >> + bool teardownlabel = false; >> + bool releaseaddr = false; >> + >> + if (!virQEMUCapsGet(priv->qemuCaps, >> QEMU_CAPS_DEVICE_SCSI_GENERIC)) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("SCSI passthrough is not supported by this >> version of qemu")); >> + goto cleanup; >> + } > Still not clear why SCSI_GENERIC is required - what is the guest device? (jumping out of order in my replies... You brought this up earlier, but there's less to digest in this one...) What's visible in the guest is one or more /dev/sdX devices and their /dev/sgX counterparts, just as there would be if using virtio-scsi. The difference is that there'd be one <hostdev> tag for each disk for virtio, but here a single <hostdev> can contain many disks within it.

Now, the question of why the check has to be made twice? Er, well, uh... I don't have an answer for that.

> >> + >> + if (qemuHostdevPrepareHostDevices(driver, vm->def->name, >> &hostdev, 1) < 0) { >> + virDomainHostdevSubsysHostPtr hostsrc = >> &hostdev->source.subsys.u.host; >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Unable to prepare scsi_host hostdev: %s"), >> + hostsrc->wwpn); >> + goto cleanup; >> + } >> + >> + if (qemuSetupHostdevCgroup(vm, hostdev) < 0) >> + goto cleanup; >> + teardowncgroup = true; >> + >> + if (virSecurityManagerSetHostdevLabel(driver->securityManager, >> + vm->def, hostdev, NULL) < 0) >> + goto cleanup; >> + teardownlabel = true; >> + >> + if (virHostOpenVhostSCSI(&vhostfd) < 0) >> + goto cleanup; >> + >> + if (virAsprintf(&vhostfdName, "vhostfd-%d", vhostfd) < 0) >> + goto cleanup; >> + >> + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { >> + if (qemuDomainMachineIsS390CCW(vm->def) && >> + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) >> + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; >> + } >> + >> + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || >> + hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { >> + if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, >> hostdev->info) < 0) > Here we are doing the PCI address thing, but I don't see patch7 > addressing that... That is - no address is defined on the command line > (as seen the the patch9 .args file) > >> + goto cleanup; >> + } else if (hostdev->info->type == >> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { >> + if (!(ccwaddrs = >> qemuDomainCCWAddrSetCreateFromDomain(vm->def))) >> + goto cleanup; >> + if (virDomainCCWAddressAssign(hostdev->info, ccwaddrs, >> + >> !hostdev->info->addr.ccw.assigned) < 0) >> + goto cleanup; >> + } >> + releaseaddr = true; >> + >> + if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, >> -1) < 0) >> + goto cleanup; >> + >> + if (!(devstr = qemuBuildHostHostdevDevStr(vm->def, >> + hostdev, >> + priv->qemuCaps, >> + vhostfdName))) > If I look at the only other caller of qemuMonitorAddDeviceWithFd I note > that it does this slightly differently... You may want to check that > out as they should be consistent. Okay. > > In particular, the configfd_name is a combination of "fd-%s" where %s is > the alias (e.g. perhaps "fd-hostdev4"; whereas, this would seem to > generate "vhostfd-hostdev4" I see that I started this from the net code that has both tapfd and vhostfd, but calls Monitor via qemuMonitorAddNetdev instead of how configfd handles things. So I presumed that the string was meant to be more descriptive, than following a particular naming convention. Will change with suggestions below. - Eric > >> + goto cleanup; >> + >> + qemuDomainObjEnterMonitor(driver, vm); >> + >> + ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd, >> vhostfdName); > if (qemuMonitorAddDeviceWithFd(...) < 0) > goto exit_monitor; >> + >> + if (qemuDomainObjExitMonitor(driver, vm) < 0) { >> + ret = -1; >> + goto cleanup; > s/cleanup/audit [1] > > The ret = -1 would be pointless and we should just able to alter the > subsequent lines a bit... [1] (see redirdevs for my example) > >> + } >> + >> + virDomainAuditHostdev(vm, hostdev, "attach", (ret == 0)); >> + >> + if (ret < 0) >> + goto cleanup; >> + >> + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0) >> + goto cleanup; > Hmmm... so if this fails now, we have the device added to the domain, > but we're failing so we should remove the device... See the problem? > > That's why other functions do their REALLOC_N before monitor > interactions... then just add it (as follows) - so move the above 2 > lines before the EnterMonitor >> + >> + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; >> + > [1] > vm->def->hostdevs[vm->def->nhostdevs++] = hostdev; > ret = 0; > > audit: > virDomainAuditHostdev(vm, hostdev, "attach", (ret == 0)); > > cleanup: > >> + virDomainCCWAddressSetFree(ccwaddrs); >> + >> + VIR_FORCE_CLOSE(vhostfd); >> + VIR_FREE(vhostfdName); >> + VIR_FREE(devstr); >> + return 0; >> + >> + cleanup: > s/cleanup/exit_monitor > > orig_err = virSaveLastError(); > >> + if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) >> + VIR_WARN("Unable to remove host device cgroup ACL on hotplug >> fail"); >> + if (teardownlabel && >> + virSecurityManagerRestoreHostdevLabel(driver->securityManager, >> + vm->def, hostdev, >> NULL) < 0) >> + VIR_WARN("Unable to restore host device labelling on hotplug >> fail"); >> + if (releaseaddr) >> + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL); >> + > if (orig_err) { > virSetError(orig_err); > virFreeError(orig_err); > } > > goto audit; > > Meaning the next 4 aren't necessary. > > John > >> + VIR_FORCE_CLOSE(vhostfd); >> + VIR_FREE(vhostfdName); >> + VIR_FREE(devstr); >> + return ret; >> +} >> + >> int >> qemuDomainAttachHostDevice(virConnectPtr conn, >> @@ -2458,6 +2572,11 @@ qemuDomainAttachHostDevice(virConnectPtr conn, >> goto error; >> break; >> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: >> + if (qemuDomainAttachHostSCSIHostDevice(driver, vm, hostdev) >> < 0) >> + goto error; >> + break; >> + >> default: >> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> _("hostdev subsys type '%s' not supported"), >> @@ -3549,6 +3668,14 @@ >> qemuDomainRemoveSCSIHostDevice(virQEMUDriverPtr driver, >> qemuHostdevReAttachSCSIDevices(driver, vm->def->name, &hostdev, >> 1); >> } >> +static void >> +qemuDomainRemoveHostSCSIHostDevice(virQEMUDriverPtr driver, >> + virDomainObjPtr vm, >> + virDomainHostdevDefPtr hostdev) >> +{ >> + qemuHostdevReAttachHostDevices(driver, vm->def->name, &hostdev, 1); >> +} >> + >> static int >> qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, >> virDomainObjPtr vm, >> @@ -3627,6 +3754,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr >> driver, >> qemuDomainRemoveSCSIHostDevice(driver, vm, hostdev); >> break; >> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: >> + qemuDomainRemoveHostSCSIHostDevice(driver, vm, hostdev); >> break; >> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: >> break; >> @@ -4477,6 +4605,31 @@ >> qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver, >> } >> static int >> +qemuDomainDetachHostSCSIHostDevice(virQEMUDriverPtr driver, >> + virDomainObjPtr vm, >> + virDomainHostdevDefPtr detach) >> +{ >> + qemuDomainObjPrivatePtr priv = vm->privateData; >> + int ret = -1; >> + >> + if (!detach->info->alias) { >> + virReportError(VIR_ERR_OPERATION_FAILED, >> + "%s", _("device cannot be detached without a >> device alias")); >> + return -1; >> + } >> + >> + qemuDomainMarkDeviceForRemoval(vm, detach->info); >> + >> + qemuDomainObjEnterMonitor(driver, vm); >> + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias); >> + >> + if (qemuDomainObjExitMonitor(driver, vm) < 0) >> + return -1; >> + >> + return ret; >> +} >> + >> +static int >> qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, >> virDomainObjPtr vm, >> virDomainHostdevDefPtr detach) >> @@ -4498,6 +4651,9 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr >> driver, >> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: >> ret = qemuDomainDetachHostSCSIDevice(driver, vm, detach); >> break; >> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: >> + ret = qemuDomainDetachHostSCSIHostDevice(driver, vm, detach); >> + break; >> default: >> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> _("hostdev subsys type '%s' not supported"), >> @@ -4575,6 +4731,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr >> driver, >> } >> break; >> } >> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: >> + break; >> default: >> virReportError(VIR_ERR_INTERNAL_ERROR, >> _("unexpected hostdev type %d"), >> subsys->type); >>

Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; --- docs/schemas/domaincommon.rng | 23 ++++++++++++ src/conf/domain_audit.c | 7 ++++ src/conf/domain_conf.c | 81 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 109 insertions(+), 2 deletions(-)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 19d45fd..bb903ef 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3974,6 +3974,7 @@ <ref name="hostdevsubsyspci"/> <ref name="hostdevsubsysusb"/> <ref name="hostdevsubsysscsi"/> + <ref name="hostdevsubsyshost"/> </choice> </define> @@ -4102,6 +4103,28 @@ </element> </define> + <define name="hostdevsubsyshost"> + <attribute name="type"> + <value>scsi_host</value> + </attribute> + <element name="source"> + <choice> + <group> + <attribute name="protocol"> + <choice> + <value>vhost</value> <!-- vhost, required --> + </choice> + </attribute> + <attribute name="wwpn"> + <data type="string"> + <param name="pattern">(naa\.)[0-9a-fA-F]{16}</param> + </data> + </attribute> + </group> + </choice> + </element> + </define> + <define name="hostdevcapsstorage"> <attribute name="type"> <value>storage</value> diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 2decf02..844b3cd 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -392,6 +392,7 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &hostdev->source.subsys.u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + virDomainHostdevSubsysHostPtr hostsrc = &hostdev->source.subsys.u.host; virUUIDFormat(vm->def->uuid, uuidstr); if (!(vmname = virAuditEncode("vm", vm->def->name))) { @@ -444,6 +445,12 @@ virDomainAuditHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr hostdev, } break; } + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: + if (VIR_STRDUP_QUIET(address, hostsrc->wwpn) < 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 b8a3366..75cacd9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2323,6 +2323,9 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def) } else { VIR_FREE(scsisrc->u.host.adapter); } + } else if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) { + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host; + VIR_FREE(hostsrc->wwpn); } break; } @@ -6092,6 +6095,55 @@ virDomainHostdevSubsysSCSIDefParseXML(xmlNodePtr sourcenode, return ret; } +static int +virDomainHostdevSubsysHostDefParseXML(xmlNodePtr sourcenode, + virDomainHostdevDefPtr def) +{ + char *protocol = NULL; + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host; + + if ((protocol = virXMLPropString(sourcenode, "protocol"))) { + hostsrc->protocol = + virDomainHostdevSubsysHostProtocolTypeFromString(protocol); + if (hostsrc->protocol <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unknown scsi_host subsystem protocol '%s'"), + protocol); + goto cleanup; + } + }

+ + switch ((virDomainHostdevSubsysHostProtocolType) hostsrc->protocol) { + case VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST: + if (!(hostsrc->wwpn = virXMLPropString(sourcenode, "wwpn"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing vhost-scsi hostdev source path name"));

+ goto cleanup; + } + + if (!STRPREFIX(hostsrc->wwpn, "naa.") || + strlen(hostsrc->wwpn) != strlen("naa.") + 16) {

+ virReportError(VIR_ERR_XML_ERROR, "%s", _("malformed 'wwpn' value")); + goto cleanup; + } + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_NONE: + case VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_LAST: + virReportError(VIR_ERR_XML_ERROR, + _("Invalid hostdev protocol '%s'"), + virDomainHostdevSubsysHostProtocolTypeToString(def->source.subsys.type));

+ goto cleanup; + break; + } + + return 0; + + cleanup: + VIR_FREE(hostsrc->wwpn); + VIR_FREE(protocol); + return -1; +} + static int virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, @@ -6216,6 +6268,11 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, goto error; break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: + if (virDomainHostdevSubsysHostDefParseXML(sourcenode, def) < 0) + goto error; + break; + default: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("address type='%s' not supported in hostdev interfaces"), @@ -13908,7 +13965,13 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a, else return virDomainHostdevMatchSubsysSCSIHost(a, b); case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: - /* Fall through for now */ + if (a->source.subsys.u.host.protocol != + b->source.subsys.u.host.protocol) + return 0; + if (STREQ(a->source.subsys.u.host.wwpn, b->source.subsys.u.host.wwpn)) + return 1; + else + return 0; case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: return 0; } @@ -20815,9 +20878,11 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, unsigned int flags, bool includeTypeInAddr) { + bool closedSource = false; virDomainHostdevSubsysUSBPtr usbsrc = &def->source.subsys.u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &def->source.subsys.u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi; + virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host; virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; @@ -20858,6 +20923,15 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, protocol, iscsisrc->path); } + if (def->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST) { + const char *protocol = + virDomainHostdevSubsysHostProtocolTypeToString(hostsrc->protocol); + closedSource = true; + + virBufferAsprintf(buf, " protocol='%s' wwpn='%s'/", + protocol, hostsrc->wwpn); + } + virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); @@ -20911,6 +20985,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, scsihostsrc->unit); } break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST: + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected hostdev type %d"), @@ -20926,7 +21002,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, } virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</source>\n"); + if (!closedSource) + virBufferAddLit(buf, "</source>\n"); return 0; }