[PATCH 0/5] Support CSS and DASD devices in node_device driver
This series enables support for channel subsystem (CSS) devices, Direct Access Storage Devices (DASDs) in the node_device driver and adds support to create vfio-ccw mdev devices in the node_device driver. Boris Fiuczynski (5): node_device: refactor udevProcessCCW node_device: detect CSS devices virsh: nodedev: ability to filter CSS capabilities node_device: detect DASD devices node_device: mdev vfio-ccw support docs/formatnode.html.in | 12 +++ docs/manpages/virsh.rst | 2 +- docs/schemas/nodedev.rng | 16 ++++ include/libvirt/libvirt-nodedev.h | 1 + src/conf/domain_addr.c | 2 +- src/conf/domain_addr.h | 3 + src/conf/node_device_conf.c | 5 ++ src/conf/node_device_conf.h | 4 +- src/conf/virnodedeviceobj.c | 4 +- src/libvirt-nodedev.c | 1 + src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 49 +++++++++--- src/node_device/node_device_udev.c | 77 ++++++++++++++++--- .../ccw_0_0_10000-invalid.xml | 4 +- tests/nodedevschemadata/ccw_0_0_ffff.xml | 4 +- tests/nodedevschemadata/css_0_0_ffff.xml | 10 +++ tests/nodedevxml2xmltest.c | 1 + tools/virsh-nodedev.c | 3 + 18 files changed, 172 insertions(+), 27 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_ffff.xml -- 2.24.1
From: Boris Fiuczynski <fiuczy@linux.ibm.com> Refactor out CCW address parsing for later reuse. Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_udev.c | 31 ++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index ff558efb..d478a673 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1058,27 +1058,38 @@ udevProcessMediatedDevice(struct udev_device *dev, static int -udevProcessCCW(struct udev_device *device, - virNodeDeviceDefPtr def) +udevGetCCWAddress(const char *sysfs_path, + virNodeDevCapDataPtr data) { - int online; char *p; - virNodeDevCapDataPtr data = &def->caps->data; - - /* process only online devices to keep the list sane */ - if (udevGetIntSysfsAttr(device, "online", &online, 0) < 0 || online != 1) - return -1; - if ((p = strrchr(def->sysfs_path, '/')) == NULL || + if ((p = strrchr(sysfs_path, '/')) == NULL || virStrToLong_ui(p + 1, &p, 16, &data->ccw_dev.cssid) < 0 || p == NULL || virStrToLong_ui(p + 1, &p, 16, &data->ccw_dev.ssid) < 0 || p == NULL || virStrToLong_ui(p + 1, &p, 16, &data->ccw_dev.devno) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse the CCW address from sysfs path: '%s'"), - def->sysfs_path); + sysfs_path); return -1; } + return 0; +} + + +static int +udevProcessCCW(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + int online; + + /* process only online devices to keep the list sane */ + if (udevGetIntSysfsAttr(device, "online", &online, 0) < 0 || online != 1) + return -1; + + if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0) + return -1; + if (udevGenerateDeviceName(device, def, NULL) != 0) return -1; -- 2.24.1
On Mon, Aug 24, 2020 at 01:59:11PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Refactor out CCW address parsing for later reuse.
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Erik Skultety <eskultet@redhat.com>
From: Boris Fiuczynski <fiuczy@linux.ibm.com> Make channel subsystem (CSS) devices available in the node_device driver. The CCS devices reside in the computer system and provide CCW devices, e.g.: +- css_0_0_003a | +- ccw_0_0_1a2b | +- scsi_host0 | +- scsi_target0_0_0 | +- scsi_0_0_0_0 Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/schemas/nodedev.rng | 16 ++++++++++++++ src/conf/node_device_conf.c | 5 +++++ src/conf/node_device_conf.h | 1 + src/conf/virnodedeviceobj.c | 1 + src/node_device/node_device_udev.c | 22 +++++++++++++++++++ .../ccw_0_0_10000-invalid.xml | 4 ++-- tests/nodedevschemadata/ccw_0_0_ffff.xml | 4 ++-- tests/nodedevschemadata/css_0_0_ffff.xml | 10 +++++++++ tests/nodedevxml2xmltest.c | 1 + tools/virsh-nodedev.c | 1 + 10 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_ffff.xml diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 4b2b350f..f7f517b5 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -85,6 +85,7 @@ <ref name="capdrm"/> <ref name="capmdev"/> <ref name="capccwdev"/> + <ref name="capcssdev"/> </choice> </element> </define> @@ -659,6 +660,21 @@ </element> </define> + <define name='capcssdev'> + <attribute name='type'> + <value>css</value> + </attribute> + <element name='cssid'> + <ref name='ccwCssidRange'/> + </element> + <element name='ssid'> + <ref name='ccwSsidRange'/> + </element> + <element name='devno'> + <ref name='ccwDevnoRange'/> + </element> + </define> + <define name='address'> <element name='address'> <attribute name='domain'><ref name='hexuint'/></attribute> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 851ef0a8..8694df4b 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -65,6 +65,7 @@ VIR_ENUM_IMPL(virNodeDevCap, "mdev_types", "mdev", "ccw", + "css", ); VIR_ENUM_IMPL(virNodeDevNetCap, @@ -602,6 +603,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def) virNodeDeviceCapMdevDefFormat(&buf, data); break; case VIR_NODE_DEV_CAP_CCW_DEV: + case VIR_NODE_DEV_CAP_CSS_DEV: virBufferAsprintf(&buf, "<cssid>0x%x</cssid>\n", data->ccw_dev.cssid); virBufferAsprintf(&buf, "<ssid>0x%x</ssid>\n", @@ -1904,6 +1906,7 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, ret = virNodeDevCapMdevParseXML(ctxt, def, node, &caps->data.mdev); break; case VIR_NODE_DEV_CAP_CCW_DEV: + case VIR_NODE_DEV_CAP_CSS_DEV: ret = virNodeDevCapCCWParseXML(ctxt, def, node, &caps->data.ccw_dev); break; case VIR_NODE_DEV_CAP_MDEV_TYPES: @@ -2228,6 +2231,7 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_CCW_DEV: + case VIR_NODE_DEV_CAP_CSS_DEV: case VIR_NODE_DEV_CAP_LAST: /* This case is here to shutup the compiler */ break; @@ -2281,6 +2285,7 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def) case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_CCW_DEV: + case VIR_NODE_DEV_CAP_CSS_DEV: case VIR_NODE_DEV_CAP_LAST: break; } diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 9b8c7aad..47669d42 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -64,6 +64,7 @@ typedef enum { VIR_NODE_DEV_CAP_MDEV_TYPES, /* Device capable of mediated devices */ VIR_NODE_DEV_CAP_MDEV, /* Mediated device */ VIR_NODE_DEV_CAP_CCW_DEV, /* s390 CCW device */ + VIR_NODE_DEV_CAP_CSS_DEV, /* s390 channel subsystem device */ VIR_NODE_DEV_CAP_LAST } virNodeDevCapType; diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index bfd52412..e234432b 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -710,6 +710,7 @@ virNodeDeviceObjHasCap(const virNodeDeviceObj *obj, case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_MDEV: case VIR_NODE_DEV_CAP_CCW_DEV: + case VIR_NODE_DEV_CAP_CSS_DEV: case VIR_NODE_DEV_CAP_LAST: break; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index d478a673..1c4df709 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1097,6 +1097,24 @@ udevProcessCCW(struct udev_device *device, } +static int +udevProcessCSS(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + /* do not process EADM and CHSC devices to keep the list sane */ + if (STREQ(def->driver, "eadm_subchannel") || + STREQ(def->driver, "chsc_subchannel")) + return -1; + + if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0) + return -1; + + if (udevGenerateDeviceName(device, def, NULL) != 0) + return -1; + + return 0; +} + static int udevGetDeviceNodes(struct udev_device *device, virNodeDeviceDefPtr def) @@ -1175,6 +1193,8 @@ udevGetDeviceType(struct udev_device *device, *type = VIR_NODE_DEV_CAP_MDEV; else if (STREQ_NULLABLE(subsystem, "ccw")) *type = VIR_NODE_DEV_CAP_CCW_DEV; + else if (STREQ_NULLABLE(subsystem, "css")) + *type = VIR_NODE_DEV_CAP_CSS_DEV; VIR_FREE(subsystem); } @@ -1219,6 +1239,8 @@ udevGetDeviceDetails(struct udev_device *device, return udevProcessMediatedDevice(device, def); case VIR_NODE_DEV_CAP_CCW_DEV: return udevProcessCCW(device, def); + case VIR_NODE_DEV_CAP_CSS_DEV: + return udevProcessCSS(device, def); case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: diff --git a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml index d840555c..f3cf0c1c 100644 --- a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml +++ b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml @@ -1,7 +1,7 @@ <device> <name>ccw_0_0_10000</name> - <path>/sys/devices/css0/0.0.0000/0.0.10000</path> - <parent>computer</parent> + <path>/sys/devices/css0/0.0.0070/0.0.10000</path> + <parent>css_0_0_0070</parent> <capability type='ccw'> <cssid>0x0</cssid> <ssid>0x0</ssid> diff --git a/tests/nodedevschemadata/ccw_0_0_ffff.xml b/tests/nodedevschemadata/ccw_0_0_ffff.xml index 5ecd0b0a..3b8ea46e 100644 --- a/tests/nodedevschemadata/ccw_0_0_ffff.xml +++ b/tests/nodedevschemadata/ccw_0_0_ffff.xml @@ -1,7 +1,7 @@ <device> <name>ccw_0_0_ffff</name> - <path>/sys/devices/css0/0.0.0000/0.0.ffff</path> - <parent>computer</parent> + <path>/sys/devices/css0/0.0.0070/0.0.ffff</path> + <parent>css_0_0_0070</parent> <capability type='ccw'> <cssid>0x0</cssid> <ssid>0x0</ssid> diff --git a/tests/nodedevschemadata/css_0_0_ffff.xml b/tests/nodedevschemadata/css_0_0_ffff.xml new file mode 100644 index 00000000..312e07fe --- /dev/null +++ b/tests/nodedevschemadata/css_0_0_ffff.xml @@ -0,0 +1,10 @@ +<device> + <name>css_0_0_ffff</name> + <path>/sys/devices/css0/0.0.ffff</path> + <parent>computer</parent> + <capability type='css'> + <cssid>0x0</cssid> + <ssid>0x0</ssid> + <devno>0xffff</devno> + </capability> +</device> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index 6168c29c..3cb23b1d 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -123,6 +123,7 @@ mymain(void) DO_TEST("pci_0000_02_10_7_mdev_types"); DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b"); DO_TEST("ccw_0_0_ffff"); + DO_TEST("css_0_0_ffff"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index e576b3c8..ff4b7441 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -461,6 +461,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) case VIR_NODE_DEV_CAP_CCW_DEV: flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV; break; + case VIR_NODE_DEV_CAP_CSS_DEV: case VIR_NODE_DEV_CAP_LAST: break; } -- 2.24.1
On Mon, Aug 24, 2020 at 01:59:12PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Make channel subsystem (CSS) devices available in the node_device driver.o
Can there be more CSS devices per CPC?
The CCS devices reside in the computer system and provide CCW devices, e.g.:
+- css_0_0_003a | +- ccw_0_0_1a2b | +- scsi_host0 | +- scsi_target0_0_0 | +- scsi_0_0_0_0
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/schemas/nodedev.rng | 16 ++++++++++++++ src/conf/node_device_conf.c | 5 +++++ src/conf/node_device_conf.h | 1 + src/conf/virnodedeviceobj.c | 1 + src/node_device/node_device_udev.c | 22 +++++++++++++++++++ .../ccw_0_0_10000-invalid.xml | 4 ++-- tests/nodedevschemadata/ccw_0_0_ffff.xml | 4 ++-- tests/nodedevschemadata/css_0_0_ffff.xml | 10 +++++++++ tests/nodedevxml2xmltest.c | 1 + tools/virsh-nodedev.c | 1 + 10 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_ffff.xml
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 4b2b350f..f7f517b5 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -85,6 +85,7 @@ <ref name="capdrm"/> <ref name="capmdev"/> <ref name="capccwdev"/> + <ref name="capcssdev"/> </choice> </element> </define> @@ -659,6 +660,21 @@ </element> </define>
+ <define name='capcssdev'> + <attribute name='type'> + <value>css</value> + </attribute> + <element name='cssid'>
Is ^this one just a different name for CHPID or those are completely unrelated?
+ <ref name='ccwCssidRange'/> + </element> + <element name='ssid'> + <ref name='ccwSsidRange'/> + </element> + <element name='devno'> + <ref name='ccwDevnoRange'/> + </element> + </define> +
Are ^these attributes documented a little more somewhere? I didn't find those in [1]. I suppose it is available in the z/Architecture: Principles of Operation publication for which I'd have to get an IBM account. [snip]
+static int +udevProcessCSS(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + /* do not process EADM and CHSC devices to keep the list sane */ + if (STREQ(def->driver, "eadm_subchannel") || + STREQ(def->driver, "chsc_subchannel"))
[2] Also mentions message subchannel, although apparently there are no Linux kernel drivers for that yet, IIUC that one would have to be added here as well as some point, is that correct?
+ return -1; + + if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0) + return -1; + + if (udevGenerateDeviceName(device, def, NULL) != 0) + return -1; + + return 0; +} + static int udevGetDeviceNodes(struct udev_device *device, virNodeDeviceDefPtr def) @@ -1175,6 +1193,8 @@ udevGetDeviceType(struct udev_device *device, *type = VIR_NODE_DEV_CAP_MDEV; else if (STREQ_NULLABLE(subsystem, "ccw")) *type = VIR_NODE_DEV_CAP_CCW_DEV; + else if (STREQ_NULLABLE(subsystem, "css")) + *type = VIR_NODE_DEV_CAP_CSS_DEV;
VIR_FREE(subsystem); } @@ -1219,6 +1239,8 @@ udevGetDeviceDetails(struct udev_device *device, return udevProcessMediatedDevice(device, def); case VIR_NODE_DEV_CAP_CCW_DEV: return udevProcessCCW(device, def); + case VIR_NODE_DEV_CAP_CSS_DEV: + return udevProcessCSS(device, def); case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: diff --git a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml index d840555c..f3cf0c1c 100644 --- a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml +++ b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml @@ -1,7 +1,7 @@ <device> <name>ccw_0_0_10000</name> - <path>/sys/devices/css0/0.0.0000/0.0.10000</path> - <parent>computer</parent> + <path>/sys/devices/css0/0.0.0070/0.0.10000</path> + <parent>css_0_0_0070</parent>
Looking at the architecture diagram at [1], I'm wondering why haven't we add the CSS channel right away and instead only added CCW devices, which are basically just end points on the CSS subsystem. The changes otherwise look good, so I'm inclined to give it an ACK, but I'd like to understand CSS a bit more before that (since I don't have HW to try this on) Erik
Erik Skultety <eskultet@redhat.com> [2020-09-08, 05:46PM +0200]:
On Mon, Aug 24, 2020 at 01:59:12PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Make channel subsystem (CSS) devices available in the node_device driver.o
Can there be more CSS devices per CPC?
Yes, several typically. One for each I/O device attached to the LPAR.
The CCS devices reside in the computer system and provide CCW devices, e.g.:
+- css_0_0_003a | +- ccw_0_0_1a2b | +- scsi_host0 | +- scsi_target0_0_0 | +- scsi_0_0_0_0
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/schemas/nodedev.rng | 16 ++++++++++++++ src/conf/node_device_conf.c | 5 +++++ src/conf/node_device_conf.h | 1 + src/conf/virnodedeviceobj.c | 1 + src/node_device/node_device_udev.c | 22 +++++++++++++++++++ .../ccw_0_0_10000-invalid.xml | 4 ++-- tests/nodedevschemadata/ccw_0_0_ffff.xml | 4 ++-- tests/nodedevschemadata/css_0_0_ffff.xml | 10 +++++++++ tests/nodedevxml2xmltest.c | 1 + tools/virsh-nodedev.c | 1 + 10 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_ffff.xml
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 4b2b350f..f7f517b5 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -85,6 +85,7 @@ <ref name="capdrm"/> <ref name="capmdev"/> <ref name="capccwdev"/> + <ref name="capcssdev"/> </choice> </element> </define> @@ -659,6 +660,21 @@ </element> </define>
+ <define name='capcssdev'> + <attribute name='type'> + <value>css</value> + </attribute> + <element name='cssid'>
Is ^this one just a different name for CHPID or those are completely unrelated?
As far as I understood, there is a 1-to-1 relation between CHPIDs and channel paths, but they are not the same. For example, there are only 256 CHPIDs per system available, compared to 2^16 channel paths.
+ <ref name='ccwCssidRange'/> + </element> + <element name='ssid'> + <ref name='ccwSsidRange'/> + </element> + <element name='devno'> + <ref name='ccwDevnoRange'/> + </element> + </define> +
Are ^these attributes documented a little more somewhere? I didn't find those in [1]. I suppose it is available in the z/Architecture: Principles of Operation publication for which I'd have to get an IBM account.
Here's a freely available version of the PoP: https://www.ibm.com/support/pages/sites/default/files/inline-files/690450_SA... I/O is described in chapter 13.
[snip]
+static int +udevProcessCSS(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + /* do not process EADM and CHSC devices to keep the list sane */ + if (STREQ(def->driver, "eadm_subchannel") || + STREQ(def->driver, "chsc_subchannel"))
[2] Also mentions message subchannel, although apparently there are no Linux kernel drivers for that yet, IIUC that one would have to be added here as well as some point, is that correct?
We have never seen those, but we would want to add them here if they have no relevance for the user.
+ return -1; + + if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0) + return -1; + + if (udevGenerateDeviceName(device, def, NULL) != 0) + return -1; + + return 0; +} + static int udevGetDeviceNodes(struct udev_device *device, virNodeDeviceDefPtr def) @@ -1175,6 +1193,8 @@ udevGetDeviceType(struct udev_device *device, *type = VIR_NODE_DEV_CAP_MDEV; else if (STREQ_NULLABLE(subsystem, "ccw")) *type = VIR_NODE_DEV_CAP_CCW_DEV; + else if (STREQ_NULLABLE(subsystem, "css")) + *type = VIR_NODE_DEV_CAP_CSS_DEV;
VIR_FREE(subsystem); } @@ -1219,6 +1239,8 @@ udevGetDeviceDetails(struct udev_device *device, return udevProcessMediatedDevice(device, def); case VIR_NODE_DEV_CAP_CCW_DEV: return udevProcessCCW(device, def); + case VIR_NODE_DEV_CAP_CSS_DEV: + return udevProcessCSS(device, def); case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: diff --git a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml index d840555c..f3cf0c1c 100644 --- a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml +++ b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml @@ -1,7 +1,7 @@ <device> <name>ccw_0_0_10000</name> - <path>/sys/devices/css0/0.0.0000/0.0.10000</path> - <parent>computer</parent> + <path>/sys/devices/css0/0.0.0070/0.0.10000</path> + <parent>css_0_0_0070</parent>
Looking at the architecture diagram at [1], I'm wondering why haven't we add the CSS channel right away and instead only added CCW devices, which are basically just end points on the CSS subsystem.
Simply because they were not relevant for the user at that time. For the typical Linux user, the channel subsystem was an implementaion detail of I/O and every interaction with I/O devices happenend via the corresponding CCW device. That's why I didn't implement them in the first place. This changes now (unfortunately) with mdevs, so we might want this information in libvirt.
The changes otherwise look good, so I'm inclined to give it an ACK, but I'd like to understand CSS a bit more before that (since I don't have HW to try this on)
Thanks a bunch. Understandably, I am not too deeply into I/O and I am also confused more times than I'd like to admit. Maybe Boris can give some more details.
Erik
-- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, 9 Sep 2020 08:18:08 +0200 Bjoern Walk <bwalk@linux.ibm.com> wrote:
Erik Skultety <eskultet@redhat.com> [2020-09-08, 05:46PM +0200]:
On Mon, Aug 24, 2020 at 01:59:12PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Make channel subsystem (CSS) devices available in the node_device driver.o
Can there be more CSS devices per CPC?
Yes, several typically. One for each I/O device attached to the LPAR.
The CCS devices reside in the computer system and provide CCW devices, e.g.:
+- css_0_0_003a | +- ccw_0_0_1a2b | +- scsi_host0 | +- scsi_target0_0_0 | +- scsi_0_0_0_0
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/schemas/nodedev.rng | 16 ++++++++++++++ src/conf/node_device_conf.c | 5 +++++ src/conf/node_device_conf.h | 1 + src/conf/virnodedeviceobj.c | 1 + src/node_device/node_device_udev.c | 22 +++++++++++++++++++ .../ccw_0_0_10000-invalid.xml | 4 ++-- tests/nodedevschemadata/ccw_0_0_ffff.xml | 4 ++-- tests/nodedevschemadata/css_0_0_ffff.xml | 10 +++++++++ tests/nodedevxml2xmltest.c | 1 + tools/virsh-nodedev.c | 1 + 10 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_ffff.xml
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 4b2b350f..f7f517b5 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -85,6 +85,7 @@ <ref name="capdrm"/> <ref name="capmdev"/> <ref name="capccwdev"/> + <ref name="capcssdev"/> </choice> </element> </define> @@ -659,6 +660,21 @@ </element> </define>
+ <define name='capcssdev'> + <attribute name='type'> + <value>css</value> + </attribute> + <element name='cssid'>
Is ^this one just a different name for CHPID or those are completely unrelated?
As far as I understood, there is a 1-to-1 relation between CHPIDs and channel paths, but they are not the same. For example, there are only 256 CHPIDs per system available, compared to 2^16 channel paths.
'CHPID' is short for 'channel path identifier'. You can have up to 256 channel paths per channel subsystem image (a given LPAR only sees one channel subsystem image[a]). Any subchannel can use up to 8 channel paths, and a given channel path is usually used by many subchannels. The 'cssid' is the number of the channel subsystem image for the subchannel.[b]
+ <ref name='ccwCssidRange'/> + </element> + <element name='ssid'> + <ref name='ccwSsidRange'/> + </element> + <element name='devno'> + <ref name='ccwDevnoRange'/> + </element> + </define> +
Are ^these attributes documented a little more somewhere? I didn't find those in [1]. I suppose it is available in the z/Architecture: Principles of
[BTW: what are [1] and [2] referring to?]
Operation publication for which I'd have to get an IBM account.
Here's a freely available version of the PoP:
https://www.ibm.com/support/pages/sites/default/files/inline-files/690450_SA...
I/O is described in chapter 13.
Let me also point to a series of articles I did on my blog: https://virtualpenguins.blogspot.com/2017/02/channel-io-demystified.html
[snip]
+static int +udevProcessCSS(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + /* do not process EADM and CHSC devices to keep the list sane */ + if (STREQ(def->driver, "eadm_subchannel") || + STREQ(def->driver, "chsc_subchannel"))
[2] Also mentions message subchannel, although apparently there are no Linux kernel drivers for that yet, IIUC that one would have to be added here as well as some point, is that correct?
We have never seen those, but we would want to add them here if they have no relevance for the user.
I think you want to filter message subchannels as well, my assumption is that libvirt will only care about I/O subchannels.
+ return -1; + + if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0) + return -1; + + if (udevGenerateDeviceName(device, def, NULL) != 0) + return -1; + + return 0; +} + static int udevGetDeviceNodes(struct udev_device *device, virNodeDeviceDefPtr def) @@ -1175,6 +1193,8 @@ udevGetDeviceType(struct udev_device *device, *type = VIR_NODE_DEV_CAP_MDEV; else if (STREQ_NULLABLE(subsystem, "ccw")) *type = VIR_NODE_DEV_CAP_CCW_DEV; + else if (STREQ_NULLABLE(subsystem, "css")) + *type = VIR_NODE_DEV_CAP_CSS_DEV;
VIR_FREE(subsystem); } @@ -1219,6 +1239,8 @@ udevGetDeviceDetails(struct udev_device *device, return udevProcessMediatedDevice(device, def); case VIR_NODE_DEV_CAP_CCW_DEV: return udevProcessCCW(device, def); + case VIR_NODE_DEV_CAP_CSS_DEV: + return udevProcessCSS(device, def); case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: diff --git a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml index d840555c..f3cf0c1c 100644 --- a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml +++ b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml @@ -1,7 +1,7 @@ <device> <name>ccw_0_0_10000</name> - <path>/sys/devices/css0/0.0.0000/0.0.10000</path> - <parent>computer</parent> + <path>/sys/devices/css0/0.0.0070/0.0.10000</path> + <parent>css_0_0_0070</parent>
Looking at the architecture diagram at [1], I'm wondering why haven't we add the CSS channel right away and instead only added CCW devices, which are basically just end points on the CSS subsystem.
Simply because they were not relevant for the user at that time. For the typical Linux user, the channel subsystem was an implementaion detail of I/O and every interaction with I/O devices happenend via the corresponding CCW device. That's why I didn't implement them in the first place. This changes now (unfortunately) with mdevs, so we might want this information in libvirt.
The changes otherwise look good, so I'm inclined to give it an ACK, but I'd like to understand CSS a bit more before that (since I don't have HW to try this on)
Thanks a bunch. Understandably, I am not too deeply into I/O and I am also confused more times than I'd like to admit. Maybe Boris can give some more details.
I know I/O, but I'm not that deeply into libvirt; I hope I could help a bit nevertheless :) [a] You could have more than one channel subsystem image visible in theory, if something called MCSS-E is available and enabled by the operating system. AFAIK, the only implementation of MCSS-E is the one in QEMU, and there has never been any operating system that supported it, so it's probably best to just ignore this. [b] Always 0 on LPARs, and within guests. We wanted to use 0xfe for virtio devices (see QEMU device definitions), but as MCSS-E turned out to be never properly implemented, they show up as 0 as well, which makes it a bit useless.
...
diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 4b2b350f..f7f517b5 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -85,6 +85,7 @@ <ref name="capdrm"/> <ref name="capmdev"/> <ref name="capccwdev"/> + <ref name="capcssdev"/> </choice> </element> </define> @@ -659,6 +660,21 @@ </element> </define>
+ <define name='capcssdev'> + <attribute name='type'> + <value>css</value> + </attribute> + <element name='cssid'>
Is ^this one just a different name for CHPID or those are completely unrelated?
As far as I understood, there is a 1-to-1 relation between CHPIDs and channel paths, but they are not the same. For example, there are only 256 CHPIDs per system available, compared to 2^16 channel paths.
'CHPID' is short for 'channel path identifier'. You can have up to 256 channel paths per channel subsystem image (a given LPAR only sees one channel subsystem image[a]). Any subchannel can use up to 8 channel paths, and a given channel path is usually used by many subchannels.
The 'cssid' is the number of the channel subsystem image for the subchannel.[b]
+ <ref name='ccwCssidRange'/> + </element> + <element name='ssid'> + <ref name='ccwSsidRange'/> + </element> + <element name='devno'> + <ref name='ccwDevnoRange'/> + </element> + </define> +
Are ^these attributes documented a little more somewhere? I didn't find those in [1]. I suppose it is available in the z/Architecture: Principles of
[BTW: what are [1] and [2] referring to?]
Urgh...not again... [1] https://www.ibm.com/support/knowledgecenter/zosbasics/com.ibm.zos.znetwork/z... [2] https://www.kernel.org/doc/html/latest/driver-api/s390-drivers.html Sorry...
Operation publication for which I'd have to get an IBM account.
Here's a freely available version of the PoP:
https://www.ibm.com/support/pages/sites/default/files/inline-files/690450_SA...
I/O is described in chapter 13.
Let me also point to a series of articles I did on my blog: https://virtualpenguins.blogspot.com/2017/02/channel-io-demystified.html
Thanks, I've looked at both, it's still fairly cloudy though.
[snip]
+static int +udevProcessCSS(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + /* do not process EADM and CHSC devices to keep the list sane */ + if (STREQ(def->driver, "eadm_subchannel") || + STREQ(def->driver, "chsc_subchannel"))
[2] Also mentions message subchannel, although apparently there are no Linux kernel drivers for that yet, IIUC that one would have to be added here as well as some point, is that correct?
We have never seen those, but we would want to add them here if they have no relevance for the user.
I think you want to filter message subchannels as well, my assumption is that libvirt will only care about I/O subchannels.
Yes, from my limited understanding of [2] and the architecture overview, we should filter those too.
+ return -1; + + if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0) + return -1; + + if (udevGenerateDeviceName(device, def, NULL) != 0) + return -1; + + return 0; +} + static int udevGetDeviceNodes(struct udev_device *device, virNodeDeviceDefPtr def) @@ -1175,6 +1193,8 @@ udevGetDeviceType(struct udev_device *device, *type = VIR_NODE_DEV_CAP_MDEV; else if (STREQ_NULLABLE(subsystem, "ccw")) *type = VIR_NODE_DEV_CAP_CCW_DEV; + else if (STREQ_NULLABLE(subsystem, "css")) + *type = VIR_NODE_DEV_CAP_CSS_DEV;
VIR_FREE(subsystem); } @@ -1219,6 +1239,8 @@ udevGetDeviceDetails(struct udev_device *device, return udevProcessMediatedDevice(device, def); case VIR_NODE_DEV_CAP_CCW_DEV: return udevProcessCCW(device, def); + case VIR_NODE_DEV_CAP_CSS_DEV: + return udevProcessCSS(device, def); case VIR_NODE_DEV_CAP_MDEV_TYPES: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_FC_HOST: diff --git a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml index d840555c..f3cf0c1c 100644 --- a/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml +++ b/tests/nodedevschemadata/ccw_0_0_10000-invalid.xml @@ -1,7 +1,7 @@ <device> <name>ccw_0_0_10000</name> - <path>/sys/devices/css0/0.0.0000/0.0.10000</path> - <parent>computer</parent> + <path>/sys/devices/css0/0.0.0070/0.0.10000</path> + <parent>css_0_0_0070</parent>
Looking at the architecture diagram at [1], I'm wondering why haven't we add the CSS channel right away and instead only added CCW devices, which are basically just end points on the CSS subsystem.
Simply because they were not relevant for the user at that time. For the typical Linux user, the channel subsystem was an implementaion detail of I/O and every interaction with I/O devices happenend via the corresponding CCW device. That's why I didn't implement them in the first place. This changes now (unfortunately) with mdevs, so we might want this information in libvirt.
Hmm, I think having a proper representation of the architecture in the nodedev driver is desirable regardless of mdev.
The changes otherwise look good, so I'm inclined to give it an ACK, but I'd like to understand CSS a bit more before that (since I don't have HW to try this on)
Thanks a bunch. Understandably, I am not too deeply into I/O and I am also confused more times than I'd like to admit. Maybe Boris can give some more details.
I know I/O, but I'm not that deeply into libvirt; I hope I could help a bit nevertheless :)
Appreciated Connie, thanks. Erik
[a] You could have more than one channel subsystem image visible in theory, if something called MCSS-E is available and enabled by the operating system. AFAIK, the only implementation of MCSS-E is the one in QEMU, and there has never been any operating system that supported it, so it's probably best to just ignore this. [b] Always 0 on LPARs, and within guests. We wanted to use 0xfe for virtio devices (see QEMU device definitions), but as MCSS-E turned out to be never properly implemented, they show up as 0 as well, which makes it a bit useless.
On Wed, 9 Sep 2020 14:46:00 +0200 Erik Skultety <eskultet@redhat.com> wrote:
Are ^these attributes documented a little more somewhere? I didn't find those in [1]. I suppose it is available in the z/Architecture: Principles of
[BTW: what are [1] and [2] referring to?]
Urgh...not again... [1] https://www.ibm.com/support/knowledgecenter/zosbasics/com.ibm.zos.znetwork/z... [2] https://www.kernel.org/doc/html/latest/driver-api/s390-drivers.html
TBH, I'm not sure how useful [2] is; it's mostly a vehicle for pulling in kerneldoc documentation of structs and functions. There's also https://www.kernel.org/doc/html/latest/s390/driver-model.html, not sure if that is helpful.
Sorry...
Operation publication for which I'd have to get an IBM account.
Here's a freely available version of the PoP:
https://www.ibm.com/support/pages/sites/default/files/inline-files/690450_SA...
I/O is described in chapter 13.
Let me also point to a series of articles I did on my blog: https://virtualpenguins.blogspot.com/2017/02/channel-io-demystified.html
Thanks, I've looked at both, it's still fairly cloudy though.
Let me know if I can do anything to make it clearer. This channel I/O stuff probably can be a bit daunting...
On Wed, Sep 09, 2020 at 03:01:50PM +0200, Cornelia Huck wrote:
On Wed, 9 Sep 2020 14:46:00 +0200 Erik Skultety <eskultet@redhat.com> wrote:
Are ^these attributes documented a little more somewhere? I didn't find those in [1]. I suppose it is available in the z/Architecture: Principles of
[BTW: what are [1] and [2] referring to?]
Urgh...not again... [1] https://www.ibm.com/support/knowledgecenter/zosbasics/com.ibm.zos.znetwork/z... [2] https://www.kernel.org/doc/html/latest/driver-api/s390-drivers.html
TBH, I'm not sure how useful [2] is; it's mostly a vehicle for pulling
It wasn't, but it was the only resource mentioning something about the ccw bus which was clearly relevant to patch 2/5 (udevProcessCSS).
in kerneldoc documentation of structs and functions. There's also https://www.kernel.org/doc/html/latest/s390/driver-model.html, not sure if that is helpful.
Sorry...
Operation publication for which I'd have to get an IBM account.
Here's a freely available version of the PoP:
https://www.ibm.com/support/pages/sites/default/files/inline-files/690450_SA...
I/O is described in chapter 13.
Let me also point to a series of articles I did on my blog: https://virtualpenguins.blogspot.com/2017/02/channel-io-demystified.html
Thanks, I've looked at both, it's still fairly cloudy though.
Let me know if I can do anything to make it clearer. This channel I/O stuff probably can be a bit daunting...
Sure, you could shed a bit more light on this one: https://www.redhat.com/archives/libvir-list/2020-September/msg00492.html Erik
On 9/9/20 9:03 AM, Cornelia Huck wrote:
[snip]
+static int +udevProcessCSS(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + /* do not process EADM and CHSC devices to keep the list sane */ + if (STREQ(def->driver, "eadm_subchannel") || + STREQ(def->driver, "chsc_subchannel"))
[2] Also mentions message subchannel, although apparently there are no Linux kernel drivers for that yet, IIUC that one would have to be added here as well as some point, is that correct? We have never seen those, but we would want to add them here if they have no relevance for the user. I think you want to filter message subchannels as well, my assumption is that libvirt will only care about I/O subchannels.
In https://www.kernel.org/doc/html/latest/driver-api/s390-drivers.html it is stated that for Message subchannels. No Linux driver currently exists. Therefore I do not know how I could currently filter message subchannels out. Due you want me to reverse the logic and just filter out everything but "io_subchannel" instead (which I would regard as fencing the unknown)? -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, Sep 11, 2020 at 06:11:49PM +0200, Boris Fiuczynski wrote:
On 9/9/20 9:03 AM, Cornelia Huck wrote:
[snip]
+static int +udevProcessCSS(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + /* do not process EADM and CHSC devices to keep the list sane */ + if (STREQ(def->driver, "eadm_subchannel") || + STREQ(def->driver, "chsc_subchannel"))
[2] Also mentions message subchannel, although apparently there are no Linux kernel drivers for that yet, IIUC that one would have to be added here as well as some point, is that correct? We have never seen those, but we would want to add them here if they have no relevance for the user. I think you want to filter message subchannels as well, my assumption is that libvirt will only care about I/O subchannels.
In https://www.kernel.org/doc/html/latest/driver-api/s390-drivers.html it is stated that for Message subchannels. No Linux driver currently exists.
Therefore I do not know how I could currently filter message subchannels out. Due you want me to reverse the logic and just filter out everything but "io_subchannel" instead (which I would regard as fencing the unknown)?
Well, I don't see a problem filtering out everything but the single thing you actually care about, rather than enumerating the things you don't care about and won't care about in the future, so yeah, I'd go with that. Erik
On 9/14/20 7:41 AM, Erik Skultety wrote:
On Fri, Sep 11, 2020 at 06:11:49PM +0200, Boris Fiuczynski wrote:
On 9/9/20 9:03 AM, Cornelia Huck wrote:
[snip]
+static int +udevProcessCSS(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + /* do not process EADM and CHSC devices to keep the list sane */ + if (STREQ(def->driver, "eadm_subchannel") || + STREQ(def->driver, "chsc_subchannel"))
[2] Also mentions message subchannel, although apparently there are no Linux kernel drivers for that yet, IIUC that one would have to be added here as well as some point, is that correct? We have never seen those, but we would want to add them here if they have no relevance for the user. I think you want to filter message subchannels as well, my assumption is that libvirt will only care about I/O subchannels.
In https://www.kernel.org/doc/html/latest/driver-api/s390-drivers.html it is stated that for Message subchannels. No Linux driver currently exists.
Therefore I do not know how I could currently filter message subchannels out. Due you want me to reverse the logic and just filter out everything but "io_subchannel" instead (which I would regard as fencing the unknown)?
Well, I don't see a problem filtering out everything but the single thing you actually care about, rather than enumerating the things you don't care about and won't care about in the future, so yeah, I'd go with that.
Erik
OK, I will do that. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
From: Boris Fiuczynski <fiuczy@linux.ibm.com> Allow to filter for CSS devices. Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- docs/formatnode.html.in | 12 ++++++++++++ docs/manpages/virsh.rst | 2 +- include/libvirt/libvirt-nodedev.h | 1 + src/conf/node_device_conf.h | 3 ++- src/conf/virnodedeviceobj.c | 3 ++- src/libvirt-nodedev.c | 1 + tools/virsh-nodedev.c | 2 ++ 7 files changed, 21 insertions(+), 3 deletions(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index 8a51c4da..4c7de50a 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -420,6 +420,18 @@ <dd>The device number.</dd> </dl> </dd> + <dt><code>css</code></dt> + <dd>Describes a Channel SubSystem (CSS) device commonly found on + the S390 architecture. Sub-elements include: + <dl> + <dt><code>cssid</code></dt> + <dd>The channel subsystem identifier.</dd> + <dt><code>ssid</code></dt> + <dd>The subchannel-set identifier.</dd> + <dt><code>devno</code></dt> + <dd>The device number.</dd> + </dl> + </dd> </dl> </dd> </dl> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 5731656b..ee287ac5 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4908,7 +4908,7 @@ List all of the devices available on the node that are known by libvirt. separated by comma, e.g. --cap pci,scsi. Valid capability types include 'system', 'pci', 'usb_device', 'usb', 'net', 'scsi_host', 'scsi_target', 'scsi', 'storage', 'fc_host', 'vports', 'scsi_generic', 'drm', 'mdev', -'mdev_types', 'ccw'. +'mdev_types', 'ccw', 'css'. If *--tree* is used, the output is formatted in a tree representing parents of each node. *cap* and *--tree* are mutually exclusive. diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h index a2ad61ac..dd2ffd57 100644 --- a/include/libvirt/libvirt-nodedev.h +++ b/include/libvirt/libvirt-nodedev.h @@ -81,6 +81,7 @@ typedef enum { VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV_TYPES = 1 << 13, /* Capable of mediated devices */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV = 1 << 14, /* Mediated device */ VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV = 1 << 15, /* CCW device */ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV = 1 << 16, /* CSS device */ } virConnectListAllNodeDeviceFlags; int virConnectListAllNodeDevices (virConnectPtr conn, diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 47669d42..5484bc34 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -368,7 +368,8 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps); VIR_CONNECT_LIST_NODE_DEVICES_CAP_DRM | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV_TYPES | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV | \ - VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV) + VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV | \ + VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV) int virNodeDeviceGetSCSIHostCaps(virNodeDevCapSCSIHostPtr scsi_host); diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index e234432b..8aefd15e 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -861,7 +861,8 @@ virNodeDeviceObjMatch(virNodeDeviceObjPtr obj, MATCH(DRM) || MATCH(MDEV_TYPES) || MATCH(MDEV) || - MATCH(CCW_DEV))) + MATCH(CCW_DEV) || + MATCH(CSS_DEV))) return false; } diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c index ca6c2bb0..28765b9c 100644 --- a/src/libvirt-nodedev.c +++ b/src/libvirt-nodedev.c @@ -101,6 +101,7 @@ virNodeNumOfDevices(virConnectPtr conn, const char *cap, unsigned int flags) * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV_TYPES * VIR_CONNECT_LIST_NODE_DEVICES_CAP_MDEV * VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV + * VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV * * Returns the number of node devices found or -1 and sets @devices to NULL in * case of error. On success, the array stored into @devices is guaranteed to diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index ff4b7441..9f29181a 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -462,6 +462,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED) flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_CCW_DEV; break; case VIR_NODE_DEV_CAP_CSS_DEV: + flags |= VIR_CONNECT_LIST_NODE_DEVICES_CAP_CSS_DEV; + break; case VIR_NODE_DEV_CAP_LAST: break; } -- 2.24.1
On Mon, Aug 24, 2020 at 01:59:13PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Allow to filter for CSS devices.
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
From: Boris Fiuczynski <fiuczy@linux.ibm.com> Make Direct Access Storage Devices (DASDs) available in the node_device driver. Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_udev.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1c4df709..5f9d67cc 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -871,6 +871,19 @@ udevProcessSD(struct udev_device *device, } +static int +udevProcessDasd(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + virNodeDevCapStoragePtr storage = &def->caps->data.storage; + + if (udevGetStringSysfsAttr(device, "device/uid", &storage->serial) < 0) + return -1; + + return udevProcessDisk(device, def); +} + + /* This function exists to deal with the case in which a driver does * not provide a device type in the usual place, but udev told us it's * a storage device, and we can make a good guess at what kind of @@ -891,6 +904,15 @@ udevKludgeStorageType(virNodeDeviceDefPtr def) def->sysfs_path); return 0; } + /* dasd disk */ + if (STRPREFIX(def->caps->data.storage.block, "/dev/dasd")) { + def->caps->data.storage.drive_type = g_strdup("dasd"); + VIR_DEBUG("Found storage type '%s' for device " + "with sysfs path '%s'", + def->caps->data.storage.drive_type, + def->sysfs_path); + return 0; + } VIR_DEBUG("Could not determine storage type " "for device with sysfs path '%s'", def->sysfs_path); return -1; @@ -978,6 +1000,8 @@ udevProcessStorage(struct udev_device *device, ret = udevProcessFloppy(device, def); } else if (STREQ(def->caps->data.storage.drive_type, "sd")) { ret = udevProcessSD(device, def); + } else if (STREQ(def->caps->data.storage.drive_type, "dasd")) { + ret = udevProcessDasd(device, def); } else { VIR_DEBUG("Unsupported storage type '%s'", def->caps->data.storage.drive_type); -- 2.24.1
On Mon, Aug 24, 2020 at 01:59:14PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Make Direct Access Storage Devices (DASDs) available in the node_device driver.
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_udev.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1c4df709..5f9d67cc 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -871,6 +871,19 @@ udevProcessSD(struct udev_device *device, }
+static int +udevProcessDasd(struct udev_device *device, + virNodeDeviceDefPtr def)
s/Dasd/DASD
+{ + virNodeDevCapStoragePtr storage = &def->caps->data.storage; + + if (udevGetStringSysfsAttr(device, "device/uid", &storage->serial) < 0) + return -1; + + return udevProcessDisk(device, def); +} + + /* This function exists to deal with the case in which a driver does * not provide a device type in the usual place, but udev told us it's * a storage device, and we can make a good guess at what kind of @@ -891,6 +904,15 @@ udevKludgeStorageType(virNodeDeviceDefPtr def) def->sysfs_path); return 0; } + /* dasd disk */ + if (STRPREFIX(def->caps->data.storage.block, "/dev/dasd")) { + def->caps->data.storage.drive_type = g_strdup("dasd"); + VIR_DEBUG("Found storage type '%s' for device " + "with sysfs path '%s'", + def->caps->data.storage.drive_type, + def->sysfs_path);
I understand why we would need it for /dev/vdX, but can udev not know the drive_type from kernel? IOW Do we really need ^this hunk?
+ return 0; + } VIR_DEBUG("Could not determine storage type " "for device with sysfs path '%s'", def->sysfs_path); return -1; @@ -978,6 +1000,8 @@ udevProcessStorage(struct udev_device *device, ret = udevProcessFloppy(device, def); } else if (STREQ(def->caps->data.storage.drive_type, "sd")) { ret = udevProcessSD(device, def); + } else if (STREQ(def->caps->data.storage.drive_type, "dasd")) { + ret = udevProcessDasd(device, def); } else { VIR_DEBUG("Unsupported storage type '%s'", def->caps->data.storage.drive_type);
The rest looks good: Reviewed-by: Erik Skultety <eskultet@redhat.com>
On 9/8/20 5:50 PM, Erik Skultety wrote:
On Mon, Aug 24, 2020 at 01:59:14PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Make Direct Access Storage Devices (DASDs) available in the node_device driver.
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/node_device/node_device_udev.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 1c4df709..5f9d67cc 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -871,6 +871,19 @@ udevProcessSD(struct udev_device *device, }
+static int +udevProcessDasd(struct udev_device *device, + virNodeDeviceDefPtr def)
s/Dasd/DASD
Changed it
+{ + virNodeDevCapStoragePtr storage = &def->caps->data.storage; + + if (udevGetStringSysfsAttr(device, "device/uid", &storage->serial) < 0) + return -1; + + return udevProcessDisk(device, def); +} + + /* This function exists to deal with the case in which a driver does * not provide a device type in the usual place, but udev told us it's * a storage device, and we can make a good guess at what kind of @@ -891,6 +904,15 @@ udevKludgeStorageType(virNodeDeviceDefPtr def) def->sysfs_path); return 0; } + /* dasd disk */ + if (STRPREFIX(def->caps->data.storage.block, "/dev/dasd")) { + def->caps->data.storage.drive_type = g_strdup("dasd"); + VIR_DEBUG("Found storage type '%s' for device " + "with sysfs path '%s'", + def->caps->data.storage.drive_type, + def->sysfs_path);
I understand why we would need it for /dev/vdX, but can udev not know the drive_type from kernel? IOW Do we really need ^this hunk?
For DASDs there are currently no identifies in udev besides ID_PATH. ID_TYPE=disk does not exist. That's why the DASDs fall through the udevProcessStorage detection logic. Without this hunk the dasd devices are being detected as storage devices but than end up as "Unsupported storage type". The short answer is yes. :-)
+ return 0; + } VIR_DEBUG("Could not determine storage type " "for device with sysfs path '%s'", def->sysfs_path); return -1; @@ -978,6 +1000,8 @@ udevProcessStorage(struct udev_device *device, ret = udevProcessFloppy(device, def); } else if (STREQ(def->caps->data.storage.drive_type, "sd")) { ret = udevProcessSD(device, def); + } else if (STREQ(def->caps->data.storage.drive_type, "dasd")) { + ret = udevProcessDasd(device, def); } else { VIR_DEBUG("Unsupported storage type '%s'", def->caps->data.storage.drive_type);
The rest looks good: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Thanks -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
...
} + /* dasd disk */ + if (STRPREFIX(def->caps->data.storage.block, "/dev/dasd")) { + def->caps->data.storage.drive_type = g_strdup("dasd"); + VIR_DEBUG("Found storage type '%s' for device " + "with sysfs path '%s'", + def->caps->data.storage.drive_type, + def->sysfs_path);
I understand why we would need it for /dev/vdX, but can udev not know the drive_type from kernel? IOW Do we really need ^this hunk?
For DASDs there are currently no identifies in udev besides ID_PATH. ID_TYPE=disk does not exist. That's why the DASDs fall through the udevProcessStorage detection logic. Without this hunk the dasd devices are being detected as storage devices but than end up as "Unsupported storage type". The short answer is yes. :-)
Okay, can you put a concise version of ^this in a commentary then? Erik
On 9/14/20 7:17 AM, Erik Skultety wrote:
...
} + /* dasd disk */ + if (STRPREFIX(def->caps->data.storage.block, "/dev/dasd")) { + def->caps->data.storage.drive_type = g_strdup("dasd"); + VIR_DEBUG("Found storage type '%s' for device " + "with sysfs path '%s'", + def->caps->data.storage.drive_type, + def->sysfs_path);
I understand why we would need it for /dev/vdX, but can udev not know the drive_type from kernel? IOW Do we really need ^this hunk?
For DASDs there are currently no identifies in udev besides ID_PATH. ID_TYPE=disk does not exist. That's why the DASDs fall through the udevProcessStorage detection logic. Without this hunk the dasd devices are being detected as storage devices but than end up as "Unsupported storage type". The short answer is yes. :-)
Okay, can you put a concise version of ^this in a commentary then?
Erik
Done, I am going to send a v2 later. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
From: Boris Fiuczynski <fiuczy@linux.ibm.com> Allow vfio-ccw mdev devices to be created besides vfio-pci mdev devices as well. Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_addr.c | 2 +- src/conf/domain_addr.h | 3 ++ src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 49 +++++++++++++++++++++++----- 4 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 1068cbf1..1bfa164a 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -1365,7 +1365,7 @@ virDomainPCIAddressSetAllMulti(virDomainDefPtr def) } -static char* +char* virDomainCCWAddressAsString(virDomainDeviceCCWAddressPtr addr) { return g_strdup_printf("%x.%x.%04x", addr->cssid, addr->ssid, addr->devno); diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index c1363c14..a0460b40 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -208,6 +208,9 @@ int virDomainCCWAddressAssign(virDomainDeviceInfoPtr dev, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr addrs); +char* virDomainCCWAddressAsString(virDomainDeviceCCWAddressPtr addr) + ATTRIBUTE_NONNULL(1); + virDomainCCWAddressSetPtr virDomainCCWAddressSetCreateFromDomain(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 01c2e710..e1342569 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -143,6 +143,7 @@ virPCIDeviceAddressParseXML; # conf/domain_addr.h virDomainCCWAddressAssign; +virDomainCCWAddressAsString; virDomainCCWAddressSetCreateFromDomain; virDomainCCWAddressSetFree; virDomainPCIAddressBusIsFullyReserved; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index e89c8b0e..bee2ef93 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -28,6 +28,7 @@ #include "virerror.h" #include "datatypes.h" +#include "domain_addr.h" #include "viralloc.h" #include "virfile.h" #include "virjson.h" @@ -628,7 +629,7 @@ nodeDeviceFindAddressByName(const char *name) { virNodeDeviceDefPtr def = NULL; virNodeDevCapsDefPtr caps = NULL; - char *pci_addr = NULL; + char *addr = NULL; virNodeDeviceObjPtr dev = virNodeDeviceObjListFindByName(driver->devs, name); if (!dev) { @@ -639,22 +640,52 @@ nodeDeviceFindAddressByName(const char *name) def = virNodeDeviceObjGetDef(dev); for (caps = def->caps; caps != NULL; caps = caps->next) { - if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - virPCIDeviceAddress addr = { + switch ((virNodeDevCapType) caps->data.type) { + case VIR_NODE_DEV_CAP_PCI_DEV: { + virPCIDeviceAddress pci_addr = { .domain = caps->data.pci_dev.domain, .bus = caps->data.pci_dev.bus, .slot = caps->data.pci_dev.slot, .function = caps->data.pci_dev.function }; - pci_addr = virPCIDeviceAddressAsString(&addr); + addr = virPCIDeviceAddressAsString(&pci_addr); break; } + case VIR_NODE_DEV_CAP_CSS_DEV: { + virDomainDeviceCCWAddress ccw_addr = { + .cssid = caps->data.ccw_dev.cssid, + .ssid = caps->data.ccw_dev.ssid, + .devno = caps->data.ccw_dev.devno + }; + + addr = virDomainCCWAddressAsString(&ccw_addr); + break; + } + + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_HOST: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_MDEV: + case VIR_NODE_DEV_CAP_CCW_DEV: + case VIR_NODE_DEV_CAP_LAST: + continue; + } } virNodeDeviceObjEndAPI(&dev); - return pci_addr; + return addr; } @@ -664,11 +695,11 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, { virCommandPtr cmd; g_autofree char *json = NULL; - g_autofree char *parent_pci = nodeDeviceFindAddressByName(def->parent); + g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent); - if (!parent_pci) { + if (!parent_addr) { virReportError(VIR_ERR_NO_NODE_DEVICE, - _("unable to find PCI address for parent device '%s'"), def->parent); + _("unable to find address for parent device '%s'"), def->parent); return NULL; } @@ -679,7 +710,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, } cmd = virCommandNewArgList(MDEVCTL, "start", - "-p", parent_pci, + "-p", parent_addr, "--jsonfile", "/dev/stdin", NULL); -- 2.24.1
On Mon, Aug 24, 2020 at 01:59:15PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com>
Allow vfio-ccw mdev devices to be created besides vfio-pci mdev devices as well.
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_addr.c | 2 +- src/conf/domain_addr.h | 3 ++ src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 49 +++++++++++++++++++++++----- 4 files changed, 45 insertions(+), 10 deletions(-) [snip]
@@ -639,22 +640,52 @@ nodeDeviceFindAddressByName(const char *name)
def = virNodeDeviceObjGetDef(dev); for (caps = def->caps; caps != NULL; caps = caps->next) { - if (caps->data.type == VIR_NODE_DEV_CAP_PCI_DEV) { - virPCIDeviceAddress addr = { + switch ((virNodeDevCapType) caps->data.type) { + case VIR_NODE_DEV_CAP_PCI_DEV: { + virPCIDeviceAddress pci_addr = { .domain = caps->data.pci_dev.domain, .bus = caps->data.pci_dev.bus, .slot = caps->data.pci_dev.slot, .function = caps->data.pci_dev.function };
- pci_addr = virPCIDeviceAddressAsString(&addr); + addr = virPCIDeviceAddressAsString(&pci_addr); break; } + case VIR_NODE_DEV_CAP_CSS_DEV: { + virDomainDeviceCCWAddress ccw_addr = { + .cssid = caps->data.ccw_dev.cssid, + .ssid = caps->data.ccw_dev.ssid, + .devno = caps->data.ccw_dev.devno + }; + + addr = virDomainCCWAddressAsString(&ccw_addr); + break; + } + + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_HOST: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_MDEV: + case VIR_NODE_DEV_CAP_CCW_DEV:
What about ccw mdevs? I would think that it's CCW end point device that we would want to assign concurrently, but we're only "slicing" the root channel subsystem device. If you have and CSS mdev, are the CCW subchannels separate for each VM or does it behave kind of like NPIV with its vHBAs? OR I'm completely off here?
+ case VIR_NODE_DEV_CAP_LAST: + continue; + } }
virNodeDeviceObjEndAPI(&dev);
- return pci_addr; + return addr; }
@@ -664,11 +695,11 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, { virCommandPtr cmd; g_autofree char *json = NULL; - g_autofree char *parent_pci = nodeDeviceFindAddressByName(def->parent); + g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
- if (!parent_pci) { + if (!parent_addr) { virReportError(VIR_ERR_NO_NODE_DEVICE, - _("unable to find PCI address for parent device '%s'"), def->parent); + _("unable to find address for parent device '%s'"), def->parent);
I'm wondering whether "unable to find parent device '%s'" would not suffice, since we're not specifying what type of address we were not able to find - I'm not even sure the address information is important at all. Erik
return NULL; }
@@ -679,7 +710,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, }
cmd = virCommandNewArgList(MDEVCTL, "start", - "-p", parent_pci, + "-p", parent_addr, "--jsonfile", "/dev/stdin", NULL);
-- 2.24.1
Erik Skultety <eskultet@redhat.com> [2020-09-08, 06:01PM +0200]:
On Mon, Aug 24, 2020 at 01:59:15PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com> + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_HOST: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_MDEV: + case VIR_NODE_DEV_CAP_CCW_DEV:
What about ccw mdevs? I would think that it's CCW end point device that we would want to assign concurrently, but we're only "slicing" the root channel subsystem device. If you have and CSS mdev, are the CCW subchannels separate for each VM or does it behave kind of like NPIV with its vHBAs? OR I'm completely off here?
You are correct. This is an unfortunate architectural decision on our platform. For vfio-ccw, we instatiate the mdevs on the CSS layer, which has a 1-to-1 relation to the underlying CCW device. That's why we need the information about the CSS devices in libvirt, to give the user a chance to easily find this relation when he wants to do passthrough with a DASD for example. It's a bit unintuitive and even silly for the end user, but it's architecturally correct and hence it was implemented like this in the vfio-ccw kernel driver. We don't even get the benefits of something like NPIV because for every subchannel there is only one CCW device (at least to my knowledge).
+ case VIR_NODE_DEV_CAP_LAST: + continue; + } }
virNodeDeviceObjEndAPI(&dev);
- return pci_addr; + return addr; }
@@ -664,11 +695,11 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, { virCommandPtr cmd; g_autofree char *json = NULL; - g_autofree char *parent_pci = nodeDeviceFindAddressByName(def->parent); + g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
- if (!parent_pci) { + if (!parent_addr) { virReportError(VIR_ERR_NO_NODE_DEVICE, - _("unable to find PCI address for parent device '%s'"), def->parent); + _("unable to find address for parent device '%s'"), def->parent);
I'm wondering whether "unable to find parent device '%s'" would not suffice, since we're not specifying what type of address we were not able to find - I'm not even sure the address information is important at all.
Erik
return NULL; }
@@ -679,7 +710,7 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, }
cmd = virCommandNewArgList(MDEVCTL, "start", - "-p", parent_pci, + "-p", parent_addr, "--jsonfile", "/dev/stdin", NULL);
-- 2.24.1
-- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, Sep 09, 2020 at 08:23:39AM +0200, Bjoern Walk wrote:
Erik Skultety <eskultet@redhat.com> [2020-09-08, 06:01PM +0200]:
On Mon, Aug 24, 2020 at 01:59:15PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com> + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_HOST: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_MDEV: + case VIR_NODE_DEV_CAP_CCW_DEV:
What about ccw mdevs? I would think that it's CCW end point device that we would want to assign concurrently, but we're only "slicing" the root channel subsystem device. If you have and CSS mdev, are the CCW subchannels separate for each VM or does it behave kind of like NPIV with its vHBAs? OR I'm completely off here?
You are correct. This is an unfortunate architectural decision on our platform. For vfio-ccw, we instatiate the mdevs on the CSS layer, which has a 1-to-1 relation to the underlying CCW device. That's why we need the information about the CSS devices in libvirt, to give the user a chance to easily find this relation when he wants to do passthrough with a DASD for example. It's a bit unintuitive and even silly for the end user, but it's architecturally correct and hence it was implemented like this in the vfio-ccw kernel driver. We don't even get the benefits of something like NPIV because for every subchannel there is only one CCW device (at least to my knowledge).
Having read both Connie's blog on channels and the I/O chapter in the free architectural publication you linked in the other reply, I now have a very vague understating of the architecture. One thing that still puzzles me though is that if there's a 1:1 subchannel mapping between CSS and an CCW device which then communicates via a control unit (which there may be more than 1) with CSS over up to 8 channel paths (this at least to me resembles NPIV), what does the mdev really solve, can't you assign one of the channel paths of one of the several control units the device is connected to to plug into the VM? Well, clearly not, I'm just being curious and trying to understand the use case here. Regards, Erik
On Wed, 9 Sep 2020 14:40:33 +0200 Erik Skultety <eskultet@redhat.com> wrote:
On Wed, Sep 09, 2020 at 08:23:39AM +0200, Bjoern Walk wrote:
Erik Skultety <eskultet@redhat.com> [2020-09-08, 06:01PM +0200]:
On Mon, Aug 24, 2020 at 01:59:15PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com> + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_HOST: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_MDEV: + case VIR_NODE_DEV_CAP_CCW_DEV:
What about ccw mdevs? I would think that it's CCW end point device that we would want to assign concurrently, but we're only "slicing" the root channel subsystem device. If you have and CSS mdev, are the CCW subchannels separate for each VM or does it behave kind of like NPIV with its vHBAs? OR I'm completely off here?
You are correct. This is an unfortunate architectural decision on our platform. For vfio-ccw, we instatiate the mdevs on the CSS layer, which has a 1-to-1 relation to the underlying CCW device. That's why we need the information about the CSS devices in libvirt, to give the user a chance to easily find this relation when he wants to do passthrough with a DASD for example. It's a bit unintuitive and even silly for the end user, but it's architecturally correct and hence it was implemented like this in the vfio-ccw kernel driver. We don't even get the benefits of something like NPIV because for every subchannel there is only one CCW device (at least to my knowledge).
Having read both Connie's blog on channels and the I/O chapter in the free architectural publication you linked in the other reply, I now have a very vague understating of the architecture. One thing that still puzzles me though is that if there's a 1:1 subchannel mapping between CSS and an CCW device which then communicates via a control unit (which there may be more than 1) with CSS over up to 8 channel paths (this at least to me resembles NPIV), what does the mdev really solve, can't you assign one of the channel paths of one of the several control units the device is connected to to plug into the VM? Well, clearly not, I'm just being curious and trying to understand the use case here.
I think it's easier to leave control units mostly out of the picture, as they are not really relevant for the modelling here. And yes, there is always exactly one subchannel for an I/O device. Subchannels (mostly a concept describing how to interact with a device) and channel paths (mostly corresponding to the way data flows to/from the device) are probably best understood as different ways to describe how devices are accessed. You don't really want to pass channel paths (as they are shared between many devices); the subchannel is a more natural unit to pass. The operating system also has way more control over subchannels than over individual channel paths. Regarding mdev, the idea here is less to be able to slice up a device as you do with GPUs, but more to be able to add a vendor driver that does the actual interesting work (channel program translations etc.) As to why we use the subchannel instead of the device, that's mostly following from how the low-level channel I/O layer in the Linux kernel is organized: The default I/O subchannel driver does a lot of things that a normal device driver should not need to deal with (path grouping, verification, status accumulation, etc.), but which would get into the way if you wanted to assign the device via vfio. Therefore, vfio-ccw implements an alternate I/O subchannel driver. HTH (at least a little bit).
On Wed, Sep 09, 2020 at 05:29:20PM +0200, Cornelia Huck wrote:
On Wed, 9 Sep 2020 14:40:33 +0200 Erik Skultety <eskultet@redhat.com> wrote:
On Wed, Sep 09, 2020 at 08:23:39AM +0200, Bjoern Walk wrote:
Erik Skultety <eskultet@redhat.com> [2020-09-08, 06:01PM +0200]:
On Mon, Aug 24, 2020 at 01:59:15PM +0200, Bjoern Walk wrote:
From: Boris Fiuczynski <fiuczy@linux.ibm.com> + case VIR_NODE_DEV_CAP_SYSTEM: + case VIR_NODE_DEV_CAP_USB_DEV: + case VIR_NODE_DEV_CAP_USB_INTERFACE: + case VIR_NODE_DEV_CAP_NET: + case VIR_NODE_DEV_CAP_SCSI_HOST: + case VIR_NODE_DEV_CAP_SCSI_TARGET: + case VIR_NODE_DEV_CAP_SCSI: + case VIR_NODE_DEV_CAP_STORAGE: + case VIR_NODE_DEV_CAP_FC_HOST: + case VIR_NODE_DEV_CAP_VPORTS: + case VIR_NODE_DEV_CAP_SCSI_GENERIC: + case VIR_NODE_DEV_CAP_DRM: + case VIR_NODE_DEV_CAP_MDEV_TYPES: + case VIR_NODE_DEV_CAP_MDEV: + case VIR_NODE_DEV_CAP_CCW_DEV:
What about ccw mdevs? I would think that it's CCW end point device that we would want to assign concurrently, but we're only "slicing" the root channel subsystem device. If you have and CSS mdev, are the CCW subchannels separate for each VM or does it behave kind of like NPIV with its vHBAs? OR I'm completely off here?
You are correct. This is an unfortunate architectural decision on our platform. For vfio-ccw, we instatiate the mdevs on the CSS layer, which has a 1-to-1 relation to the underlying CCW device. That's why we need the information about the CSS devices in libvirt, to give the user a chance to easily find this relation when he wants to do passthrough with a DASD for example. It's a bit unintuitive and even silly for the end user, but it's architecturally correct and hence it was implemented like this in the vfio-ccw kernel driver. We don't even get the benefits of something like NPIV because for every subchannel there is only one CCW device (at least to my knowledge).
Having read both Connie's blog on channels and the I/O chapter in the free architectural publication you linked in the other reply, I now have a very vague understating of the architecture. One thing that still puzzles me though is that if there's a 1:1 subchannel mapping between CSS and an CCW device which then communicates via a control unit (which there may be more than 1) with CSS over up to 8 channel paths (this at least to me resembles NPIV), what does the mdev really solve, can't you assign one of the channel paths of one of the several control units the device is connected to to plug into the VM? Well, clearly not, I'm just being curious and trying to understand the use case here.
I think it's easier to leave control units mostly out of the picture, as they are not really relevant for the modelling here. And yes, there is always exactly one subchannel for an I/O device.
Subchannels (mostly a concept describing how to interact with a device) and channel paths (mostly corresponding to the way data flows to/from the device) are probably best understood as different ways to describe how devices are accessed. You don't really want to pass channel paths (as they are shared between many devices); the subchannel is a more natural unit to pass. The operating system also has way more control over subchannels than over individual channel paths.
Gotcha.
Regarding mdev, the idea here is less to be able to slice up a device as you do with GPUs, but more to be able to add a vendor driver that does the actual interesting work (channel program translations etc.)
As to why we use the subchannel instead of the device, that's mostly following from how the low-level channel I/O layer in the Linux kernel is organized: The default I/O subchannel driver does a lot of things that a normal device driver should not need to deal with (path grouping, verification, status accumulation, etc.), but which would get into the way if you wanted to assign the device via vfio. Therefore, vfio-ccw implements an alternate I/O subchannel driver.
Okay, now it's much clearer what the mdev use case is. Thanks Connie. Erik
On 9/8/20 6:01 PM, Erik Skultety wrote:
@@ -664,11 +695,11 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, { virCommandPtr cmd; g_autofree char *json = NULL; - g_autofree char *parent_pci = nodeDeviceFindAddressByName(def->parent); + g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
- if (!parent_pci) { + if (!parent_addr) { virReportError(VIR_ERR_NO_NODE_DEVICE, - _("unable to find PCI address for parent device '%s'"), def->parent); + _("unable to find address for parent device '%s'"), def->parent); I'm wondering whether "unable to find parent device '%s'" would not suffice, since we're not specifying what type of address we were not able to find - I'm not even sure the address information is important at all.
Erik
Erik, how about _("unable to find parent device '%s' by its address"), def->parent); just to indicate the search criteria but I could also agree to a simple _("unable to find parent device '%s'"), def->parent); Your choice. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, Sep 11, 2020 at 05:34:24PM +0200, Boris Fiuczynski wrote:
On 9/8/20 6:01 PM, Erik Skultety wrote:
@@ -664,11 +695,11 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, { virCommandPtr cmd; g_autofree char *json = NULL; - g_autofree char *parent_pci = nodeDeviceFindAddressByName(def->parent); + g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
- if (!parent_pci) { + if (!parent_addr) { virReportError(VIR_ERR_NO_NODE_DEVICE, - _("unable to find PCI address for parent device '%s'"), def->parent); + _("unable to find address for parent device '%s'"), def->parent); I'm wondering whether "unable to find parent device '%s'" would not suffice, since we're not specifying what type of address we were not able to find - I'm not even sure the address information is important at all.
Erik
Erik, how about
_("unable to find parent device '%s' by its address"), def->parent);
just to indicate the search criteria but I could also agree to a simple
_("unable to find parent device '%s'"), def->parent);
I'd still go with the latter. Erik
On 9/14/20 7:34 AM, Erik Skultety wrote:
On Fri, Sep 11, 2020 at 05:34:24PM +0200, Boris Fiuczynski wrote:
On 9/8/20 6:01 PM, Erik Skultety wrote:
@@ -664,11 +695,11 @@ nodeDeviceGetMdevctlStartCommand(virNodeDeviceDefPtr def, { virCommandPtr cmd; g_autofree char *json = NULL; - g_autofree char *parent_pci = nodeDeviceFindAddressByName(def->parent); + g_autofree char *parent_addr = nodeDeviceFindAddressByName(def->parent);
- if (!parent_pci) { + if (!parent_addr) { virReportError(VIR_ERR_NO_NODE_DEVICE, - _("unable to find PCI address for parent device '%s'"), def->parent); + _("unable to find address for parent device '%s'"), def->parent); I'm wondering whether "unable to find parent device '%s'" would not suffice, since we're not specifying what type of address we were not able to find - I'm not even sure the address information is important at all.
Erik
Erik, how about
_("unable to find parent device '%s' by its address"), def->parent);
just to indicate the search criteria but I could also agree to a simple
_("unable to find parent device '%s'"), def->parent);
I'd still go with the latter.
Erik
OK, changed. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Bjoern Walk <bwalk@linux.ibm.com> [2020-08-24, 01:59PM +0200]:
This series enables support for channel subsystem (CSS) devices, Direct Access Storage Devices (DASDs) in the node_device driver and adds support to create vfio-ccw mdev devices in the node_device driver.
Boris Fiuczynski (5): node_device: refactor udevProcessCCW node_device: detect CSS devices virsh: nodedev: ability to filter CSS capabilities node_device: detect DASD devices node_device: mdev vfio-ccw support
docs/formatnode.html.in | 12 +++ docs/manpages/virsh.rst | 2 +- docs/schemas/nodedev.rng | 16 ++++ include/libvirt/libvirt-nodedev.h | 1 + src/conf/domain_addr.c | 2 +- src/conf/domain_addr.h | 3 + src/conf/node_device_conf.c | 5 ++ src/conf/node_device_conf.h | 4 +- src/conf/virnodedeviceobj.c | 4 +- src/libvirt-nodedev.c | 1 + src/libvirt_private.syms | 1 + src/node_device/node_device_driver.c | 49 +++++++++--- src/node_device/node_device_udev.c | 77 ++++++++++++++++--- .../ccw_0_0_10000-invalid.xml | 4 +- tests/nodedevschemadata/ccw_0_0_ffff.xml | 4 +- tests/nodedevschemadata/css_0_0_ffff.xml | 10 +++ tests/nodedevxml2xmltest.c | 1 + tools/virsh-nodedev.c | 3 + 18 files changed, 172 insertions(+), 27 deletions(-) create mode 100644 tests/nodedevschemadata/css_0_0_ffff.xml
-- 2.24.1
ping -- IBM Systems Linux on Z & Virtualization Development -------------------------------------------------- IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 -------------------------------------------------- Vorsitzende des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, Sep 01, 2020 at 09:49:46AM +0200, Bjoern Walk wrote:
Bjoern Walk <bwalk@linux.ibm.com> [2020-08-24, 01:59PM +0200]:
This series enables support for channel subsystem (CSS) devices, Direct Access Storage Devices (DASDs) in the node_device driver and adds support to create vfio-ccw mdev devices in the node_device driver.
From a quick look I don't really have any design comments. I'm leaving for PTO tomorrow and be back on Monday, so I'll do a proper review next week unless someone else beats me to it.
Regards, Erik
participants (4)
-
Bjoern Walk -
Boris Fiuczynski -
Cornelia Huck -
Erik Skultety