Re: [PATCH RFC 13/20] Documentation: ctlv: Describe how to write a family

[Jason Gunthorpe]

> [ ... 34 lines skipped ... ]
> +Add the specification, registry entry, and output manifest entry in one change::
> +
> +    Documentation/ctlv/specs/ctlv-example.yaml
> +    Documentation/ctlv/families.yaml
> +    Documentation/ctlv/generated-files.yaml
> +
> +``families.yaml`` is append-only. Its array index is the family ID and is part
> +of every generated attribute and operation ID. Never reorder, remove, rename,
> +or reuse an entry. ``Documentation/ctlv/abi/families.registry.yaml`` is the
> +checked-in registry ABI. The output manifest selects the generated UAPI,
> +kernel and userspace paths and generation modes.
> +
> +Generate and verify them with::
> +
> +    make -C tools/ctlv regen
> +    make -C tools/ctlv all
> +

Maybe mention to inspect git diff and ensure nothing unexpected
changed?

> +Never edit a generated output. Change the YAML or generator and regenerate it.
> +Optional RST reference pages are written to
> +``Documentation/output/ctlv/families`` by::
> +
> +    make -C tools/ctlv docs
> +
> +Write the specification
> +=======================
> +

Before introducing the YAML, write out in a more plain text form
exactly what system calls the example is aiming to create with. I came
up with:

For an example, define two system calls from userspace that look like this::

    item_set_action(u64 index, string label?, blob data?) -> status;
    items_dump_query() -> { u64 index; string label?; }[];

And one event::

    item_changed_event -> { u64 index; string label?; };

``index`` is 0..4095, ``label`` is at most 63 bytes excluding NUL, and ``data``
is at most 1 MiB with 8-byte alignment.

> +.. code-block:: yaml
> +
> +    # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
> +    %YAML 1.2
> +    ---
> +    family:
> +      name: ctlv-example
> +      inherits: ctlv
> +      post-init: true
> +      pre-fini: true
> +
> +    attr-groups:
> +      - name: item
> +        attrs:
> +          - name: index
> +            type: uint
> +            max: 4095
> +          - name: label
> +            type: string
> +            max-length: 63
> +          - name: data
> +            blob:
> +              direction: input
> +              allowed-backings: [ va ]
> +              max-length: 1048576
> +              alignment: 8
> +      - name: list
> +        attrs:
> +          - name: item
> +            attrs: item-reply
> +            max-occurrences: unlimited
> +
> +    attr-sets:
> +      - name: item-request
> +        attrs: item.index
> +        optional-attrs:
> +          - item.label
> +          - item.data
> +      - name: item-reply
> +        attrs: item.index
> +        optional-attrs: item.label
> +      - name: items-reply
> +        optional-attrs: list.item

I haven't quite figured out what attr-sets is about. I think these
blocks should have some # comments with more guidance what is going on.

> +
> +    ops:
> +      - name: item-set
> +        type: action
> +        optional: true
> +        request:
> +          attrs: item-request
> +      - name: items-dump
> +        type: query
> +        availability: dynamic
> +        pre-post: true
> +        reply:
> +          attrs: items-reply
> +        suggested-reply-length: 4096
> +      - name: item-changed
> +        type: event
> +        reply:
> +          attrs: item-reply
> +
> +An attribute has one payload form: ``type``, ``enum``, ``flags``, ``attrs``, or
> +``blob``. An attr set names one message level. ``attrs`` are required and
> +``optional-attrs`` are optional. Its bounds and membership are ABI.

The attribute first point of view is maybe a little too netlinky, I
prefer to think of this as a system call first, with a first class
'query' to get a list of things

Alot of stuff this is going to deal with won't be the netlink esque
'dump a table, set a table' kind of flow, but a bunch of different
system calls with arguments.

> +
> +Inheritance and extension points
> +================================
> +
> +A descendant implements only the operations it declares. Inherited operations
> +keep the callbacks and operation hooks of their declaring family. Every module
> +therefore depends on the module containing its parent implementation.
> +

This section seems hard to understand, it is missing something
explaining the purpose an intended usage of this mechanism. I think I
get it.. 

Maybe structure this to be an improvement to the first example solving
a specific problem 'like the HW driver need to add per-driver
things to the scheme'

Also please use a more neutral term like "HW driver" not vendor
throughout.

> +A parent reserves an extension point with an empty stub set:
> +
> +.. code-block:: yaml
> +
> +    attr-groups:
> +      - name: device
> +        attrs:
> +          - name: details
> +            attrs: details-extension
> +
> +    attr-sets:
> +      - name: details-extension
> +        stub: true
> +      - name: device-reply
> +        optional-attrs: device.details
> +
> +A child fills that same set:
> +
> +.. code-block:: yaml
> +
> +    family:
> +      name: ctlv-example-vendor
> +      inherits: ctlv-example
> +      pre-init: true
> +
> +    attr-groups:
> +      - name: vendor
> +        attrs:
> +          - name: serial
> +            type: uint
> +
> +    attr-sets:
> +      - name: details-extension
> +        optional-attrs: vendor.serial
> +
> +The parent implementation opens the physical nest and delegates its members:
> +
> +.. code-block:: c
> +
> +    if (ctlv_example_device_details_start(reply, &details)) {
> +            ret = ctlv_stub_fill(ctx, request_details, &details);
> +            ctlv_nest_end(&details);
> +            if (ret)
> +                    return ret;
> +    }
> +
> +The generated child callback fills it:
> +
> +.. code-block:: c
> +
> +    int ctlv_example_vendor_details_extension_fill(
> +                            void *priv, struct ctlv_op_ctx *ctx,
> +                            const struct ctlv_cursor *request,
> +                            struct ctlv_cursor *nest)
> +    {
> +            ctlv_example_vendor_vendor_serial_put(nest,
> +                                                   example_serial(priv));
> +            return 0;
> +    }

I suppose this like RDMA and each HW driver using this gets its own
full scheme tree that only has its own stuff in it?

What about userspace, it "knows" somehow what to do?

> +
> +An unfilled stub contributes no nest or message length. Neither side names the
> +family on the other side.

One of the things I feel strongly about from iommufd is that it should
be really obvious what HW drivers is being affiliated with these
operations that have a HW driver struct linked to them.

So, like, if userspace does a query and gets back an item that has one
of these extended structs it should have a trivial way to branch to
the right handler for it. Inspecting the attributes doesn't seem like
a great way to go about that?

Is there some place to put an ID of 'ctlv-example-vendor' someplace?
Like in the nest start somehow?

-- 
Jason