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

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(a)linux.vnet.ibm.com&gt; >>>> --- >>>> src/conf/domain_conf.c | 12 +++++++++++- >>>> src/conf/domain_conf.h | 18 ++++++++++++++++++ >>>> src/qemu/qemu_cgroup.c | 7 +++++++ >>>> src/qemu/qemu_hotplug.c | 2 ++ >>>> src/security/security_apparmor.c | 4 ++++ >>>> src/security/security_dac.c | 8 ++++++++ >>>> src/security/security_selinux.c | 8 ++++++++ >>>> tests/domaincapsschemadata/full.xml | 1 + >>>> 8 files changed, 59 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >>>> index 043f0e2..b8a3366 100644 >>>> --- a/src/conf/domain_conf.c >>>> +++ b/src/conf/domain_conf.c >>>> @@ -647,7 +647,8 @@ VIR_ENUM_IMPL(virDomainHostdevMode, >>>> VIR_DOMAIN_HOSTDEV_MODE_LAST, >>>> VIR_ENUM_IMPL(virDomainHostdevSubsys, >>>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST, >>>> "usb", >>>> "pci", >>>> - "scsi") >>>> + "scsi", >>>> + "scsi_host") >>>> VIR_ENUM_IMPL(virDomainHostdevSubsysPCIBackend, >>>> VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST, >>>> @@ -661,6 +662,11 @@ >>>> VIR_ENUM_IMPL(virDomainHostdevSubsysSCSIProtocol, >>>> "adapter", >>>> "iscsi") >>>> +VIR_ENUM_IMPL(virDomainHostdevSubsysHostProtocol, >>> In order to "follow" convention for structure names already >>> >>> s/SubsysHostProtocol/SubsysSCSIHostProtocol/ >>> >>> Of course this has ramifications beyond this patch... >> I avoided this (and many of the subsequent comments below) because the >> existing struct virDomainHostdevSubsysSCSI has a union for both iSCSI >> and it's version of host, and the latter is of course named >> virDomainHostdevSubsysSCSIHost(Ptr). If that's not much of a concern, >> then I'll go shake out the variations of host->scsihost you describe >> below. >> > Out of order, but didn't want to lose this... So there's: > > > typedef struct _virDomainHostdevSubsysSCSI virDomainHostdevSubsysSCSI; > typedef virDomainHostdevSubsysSCSI *virDomainHostdevSubsysSCSIPtr; > struct _virDomainHostdevSubsysSCSI { > int protocol; /* enum virDomainHostdevSCSIProtocolType */ > int sgio; /* enum virDomainDeviceSGIO */ > int rawio; /* enum virTristateBool */ > union { > virDomainHostdevSubsysSCSIHost host; > virDomainHostdevSubsysSCSIiSCSI iscsi; > } u; > }; > > > and when I first went through your changes I thought - why not just > alter the virDomainHostdevSCSIProtocolType protocol here to add vHost > and then add a SubsysSCSIvHost vhost structure at this point. But I > think I had one of those *pfft* moments where the brain just explodes > thinking about the details. > > The SCSIHost one is for XML: > > <devices> > <hostdev mode='subsystem' type='scsi' sgio='filtered' rawio='yes'> > <source> > <adapter name='scsi_host0'/> > <address bus='0' target='0' unit='0'/> > </source> > <readonly/> > <address type='drive' controller='0' bus='0' target='0' unit='0'/> > </hostdev> > </devices> > > > while the SCSIiSCSI is for XML: > > <devices> > <hostdev mode='subsystem' type='scsi'> > <source protocol='iscsi' > name='iqn.2014-08.com.example:iscsi-nopool/1'> > <host name='example.com' port='3260'/> > <auth username='myuser'> > <secret type='iscsi' usage='libvirtiscsi'/> > </auth> > </source> > <address type='drive' controller='0' bus='0' target='0' unit='0'/> > </hostdev> > </devices> > > whereas your proposed XML is > > <hostdev mode='subsystem' type='scsi_host'> > <source protocol='vhost' wwpn='naa.5001405df3e54061'/> > <address type='ccw' cssid='0xfe' ssid='0x0' devno='0x1000'/> > </hostdev> > > > So yes, I suppose it could fit if you change the 'wwpn' to be 'name', > then go through the process of altering existing code to be able handle > the 'iscsi' protocol and the 'vhost' protocol. But the big difference I > saw which changed my mind was the "hostdev ... type='scsi_host'", so > that's why I left it alone since it's a different abstraction. 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> >>>>