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@xxxxxxxxxxxxxxxxxx>
---
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>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list