Re: [libvirt] [PATCH v3 03/10] Introduce a "scsi_host" hostdev type

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

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

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

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