On 11/08/2016 01:26 PM, Eric Farman wrote:

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@linux.vnet.ibm.com> --- 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;

Since virDomainHostdevDefParseXMLSubsys will validate that 'type' is valid w/ a call to virDomainHostdevSubsysTypeFromString, that means for this code there's no way the subsys.type could be invalid. So no need to jump to error. FWIW: Even if you did jump to error, there should be an error message. In fact both USB and LAST "fall through" for now. Later though when SCSI_HOST is added, that's when you can do some more address validation since it'll be a new type. As opposed to the lack of validation for USB since USB existed before we added having <address> validation (which is in the post parse callback IIRC). I will adjust this before pushing (shortly)... ACK - John

+ 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; }

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@linux.vnet.ibm.com> --- 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. - 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>

On 11/14/2016 04:59 PM, John Ferlan wrote:

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@linux.vnet.ibm.com> --- 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.

Yeah, this was the direction I started in, extending type='scsi' to include a 'vhost' protocol. But that caused a lot of other rework that was messy. I like this construction more, but it does cause a bit more upheaval in other areas.

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...

I don't think the current code naming is incorrect, but it does slightly paint us into a box with this work. I'll mull this over overnight, and maybe cook up a cleanup patch separate from this series. Or perhaps take your other suggestion and go with the inclusion of "vhost" in the functions. - Eric

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>

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@linux.vnet.ibm.com> --- 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; 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@linux.vnet.ibm.com> + */ + +#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...

+ +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.

+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... 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@linux.vnet.ibm.com> + */ + +#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? So the address would seem to be important, but is not handled.

+ +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 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@linux.vnet.ibm.com> --- 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. 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.

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.

...

...

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@linux.vnet.ibm.com> + */ + +#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@linux.vnet.ibm.com> + */ + +#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,

On 11/14/2016 05:06 PM, Eric Farman wrote:

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@linux.vnet.ibm.com> --- 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.

That's fine... It's funny reading HostvHost alone gets me to thinking of the short hand for sports matches "verses". John

...

...

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@linux.vnet.ibm.com> + */ + +#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@linux.vnet.ibm.com> + */ + +#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,

On 11/08/2016 01:26 PM, Eric Farman wrote:

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@linux.vnet.ibm.com> --- 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(+)

Beyond the aforementioned "Host" to "SCSIHost" that will need to persist into here...

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)

if (!host = vir...())) is the more commonly used approach although yes I see you copied SCSI

+ 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);

These could be combined into one formatted print...

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

and yet no AddressStr is added to the command line - so we're missing a place to configure that.

+ 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)) {

Is this the right check? I guess it's unclear why scsi_generic is needed - is that necessary for vhost-scsi? I know it's used in order to pass SCSI LUN's to the guest (eg the SCSI hostdev code).

+ 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;

Once I read ahead to patch 6 this made more sense - although it's a bit strange to read... This is essentially following the PCI 'configfd' processing code.

+ 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,

Is this CCW addresses are typically assigned? It just seems odd that a "PrimeVirtioDeviceAddresses" is being used to prime the vhost-scsi-ccw address. It just doesn't feel like the right place. Certain the PCI address isn't being set here as only TYPE_VIRTIO_CCW or _MMIO is passed. Theoretically speaking it wouldn't be necessary if the address was assigned during device post parse... John

} }

+ 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);

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@linux.vnet.ibm.com> --- 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?

+ + 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. 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"

+ 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);

On 11/14/2016 09:08 AM, Eric Farman wrote:

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@linux.vnet.ibm.com> --- 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.

Ah - OK. I hadn't got around to "seeing" this working on my test system yet (it's still out of commission). Your explanation makes sense.

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

yeah well I do this too... It's easy to get lost in what you've done or haven't done John

...

...

+ + 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);

need a commit message here. On 11/08/2016 01:26 PM, Eric Farman wrote:

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

Beyond the "Host" to "SCSIHost" type changes... Since this is where the device is being added - this is when the address adjustment functions should be addressed. I got really hung up to day trying to look for examples - hopefully I didn't make too much of a mess... First off, the virDomainHostdevDefParseXML should be adjusted to ensure that if the user does provide an address - that it is valid for the device. IOW: Follow the SUBSYS_TYPE_PCI: case more or less to ensure the def->info->type if not NONE is either TYPE_PCI or TYPE_CCW. The virDomainDeviceDefPostParseInternal post processing code should handle setting an address if a valid one (checked earlier) isn't supplied. This would save the address in the domain/config XML. For PCI it would be qemuDomainAssignDevicePCISlots... I'm not clear if doing the same for CCW is ever done. In any case, qemuDomainAssignDevicePCISlots does go through the hostdev list and reserves PCI address for PCI hostdevs, which I believe would also work for this would be. Again, I'm not clear if/why CCW would be handled. NB: I haven't gone and looked for every new subsys case to ensure that things were addressed for the case's type - just ran out of time and energy. But that is something that should be done by the end of patch8 (which I assume now gets merged into here eventually).

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; + } + }

Since protocol is required the logic is : if (!(protocol = virXMLPropString(sourcenode, "protocol"))) virReportError(... _("Missing vhost-scsi 'protocol' attribute'")); return -1; } if ((hostsrc->protocol = virDomainHostdevSubsysHostProtocolTypeFromString(protocol)) <= 0) { virReportError(...); 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"));

Technically it's missing the required 'wwpn' value

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

I assume the wwpn minus the naa. needs to be valid - see virValidateWWN and just pass wwpn + 4

+ 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));

Use hostsrc->protocol for the argument John

+ 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; }