3:59 p.m.
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>
>> ---
>> 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.
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...
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>
>>