On Wed, 9 Sep 2020 08:18:08 +0200 Bjoern Walk <bwalk@xxxxxxxxxxxxx> wrote: > Erik Skultety <eskultet@xxxxxxxxxx> [2020-09-08, 05:46PM +0200]: > > On Mon, Aug 24, 2020 at 01:59:12PM +0200, Bjoern Walk wrote: > > > From: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxx> > > > Signed-off-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> > > > --- > > > 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_SA22-7832-03.pdf > > 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.
Attachment:
pgpmtjJMPcqsN.pgp
Description: OpenPGP digital signature
