[libvirt] [PATCH v3 03/10] Introduce a "scsi_host" hostdev type
Eric Farman
farman at linux.vnet.ibm.com
Mon Nov 14 22:20:47 UTC 2016
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.
- 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