[libvirt] [PATCH v3 03/10] Introduce a "scsi_host" hostdev type
Eric Farman
farman at linux.vnet.ibm.com
Thu Nov 17 22:28:49 UTC 2016
On 11/14/2016 05:20 PM, Eric Farman wrote:
>
>
> 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 at 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.
John,
I sent an RFC patch [1] separate from this series the other day, but
thought that I had the remainder of your comments addressed and so maybe
I'd combine everything into one series. Then my brain exploded:
Before:
// These three are all existing code
virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi;
virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
// The next one is new
virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
After:
// These three are all existing code
virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi;
virDomainHostdevSubsysSCSISCSIHostPtr scsiscsihostsrc =
&scsisrc->u.host;
virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
// The next one is new
virDomainHostdevSubsysSCSIHostPtr scsihostsrc =
&def->source.subsys.u.scsihost;
So, uh, ugh. (And it still has an inconsistency because I had to
prepend another "scsi" on the existing "scsihostsrc" ... which means
there's probably a better reworking to have happen here.)
I could take your other suggestion of "SCSIHostVHost", but I still worry
that that gets viewed as a subset of the existing SCSIHost stuff (which
is a type='scsi' sourceadapter='scsi_host' hostdev), without somehow
cleaning up the existing code.
For now, I've stashed these changes off to the side. I could spin a v4
of the vhost-scsi series without any of the s/host/scsihost/ variations
you asked for, but this rabbit hole is probably going to consume me
until the next freeze/holiday. Thoughts?
- Eric
[1] https://www.redhat.com/archives/libvir-list/2016-November/msg00808.html
>
> - 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>
>>>>>
>
More information about the libvir-list
mailing list