Re: [PATCH 2/5] node_device: detect CSS devices

Erik Skultety <eskultet(a)redhat.com&gt; [2020-09-08, 05:46PM +0200]: > On Mon, Aug 24, 2020 at 01:59:12PM +0200, Bjoern Walk wrote: > > From: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > > > > 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(a)linux.ibm.com&gt; > > Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > > --- > > 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... 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.