[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



...
> > > > 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/znetwork_59.htm
[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_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

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.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]