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

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



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_SA22-7832-03.pdf

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

Attachment: signature.asc
Description: PGP signature


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