[PATCH net-next v4 0/7] ynl: add support for user headers and struct attrs

[PATCH net-next v4 0/7] ynl: add support for user headers and struct attrs

[Donald Hunter]

Add support for user headers and struct attrs to YNL. This patchset adds
features to ynl and add a partial spec for openvswitch that demonstrates
use of the features.

Patch 1-4 add features to ynl
Patch 5 adds partial openvswitch specs that demonstrate the new features
Patch 6-7 add documentation for legacy structs and for sub-type

v3 - v4:
 - Rebase to net-next after net-6.3-rc4 merge

v2 - v3: all requested by Jakub Kicinski
 - Drop genlmsg fix that was applied separately
 - Don't mention 'kernel' types, leave it to schema
 - Avoid passing fixed header around in python code
 - Use 'binary' with 'sub-type' for C arrays
 - Use 'binary' with 'struct' for C structs
 - Add docs for structs and sub-type

v1 - v2: all requested by Jakub Kicinski
 - Split ynl changes into separate patches
 - Rename user-header to fixed-header and improve description
 - Move fixed-header to operations section of spec
 - Introduce objects to represent struct config in nlspec
 - Use kebab-case throughout openvswitch specs

Donald Hunter (7):
  tools: ynl: Add struct parsing to nlspec
  tools: ynl: Add C array attribute decoding to ynl
  tools: ynl: Add struct attr decoding to ynl
  tools: ynl: Add fixed-header support to ynl
  netlink: specs: add partial specification for openvswitch
  docs: netlink: document struct support for genetlink-legacy
  docs: netlink: document the sub-type attribute property

 Documentation/netlink/genetlink-legacy.yaml   |  15 ++
 Documentation/netlink/specs/ovs_datapath.yaml | 153 ++++++++++++++++++
 Documentation/netlink/specs/ovs_vport.yaml    | 139 ++++++++++++++++
 .../netlink/genetlink-legacy.rst              |  68 +++++++-
 Documentation/userspace-api/netlink/specs.rst |   9 ++
 tools/net/ynl/lib/nlspec.py                   |  64 +++++++-
 tools/net/ynl/lib/ynl.py                      |  55 ++++++-
 7 files changed, 488 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/netlink/specs/ovs_datapath.yaml
 create mode 100644 Documentation/netlink/specs/ovs_vport.yaml

-- 
2.39.0

[PATCH net-next v4 1/7] tools: ynl: Add struct parsing to nlspec

[Donald Hunter]

Add python classes for struct definitions to nlspec

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/nlspec.py | 43 +++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index d04450c2a44a..a08f6dda5b79 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -214,6 +214,44 @@ class SpecAttrSet(SpecElement):
         return self.attrs.items()
 
 
+class SpecStructMember(SpecElement):
+    """Struct member attribute
+
+    Represents a single struct member attribute.
+
+    Attributes:
+        type    string, type of the member attribute
+    """
+    def __init__(self, family, yaml):
+        super().__init__(family, yaml)
+        self.type = yaml['type']
+
+
+class SpecStruct(SpecElement):
+    """Netlink struct type
+
+    Represents a C struct definition.
+
+    Attributes:
+        members   ordered list of struct members
+    """
+    def __init__(self, family, yaml):
+        super().__init__(family, yaml)
+
+        self.members = []
+        for member in yaml.get('members', []):
+            self.members.append(self.new_member(family, member))
+
+    def new_member(self, family, elem):
+        return SpecStructMember(family, elem)
+
+    def __iter__(self):
+        yield from self.members
+
+    def items(self):
+        return self.members.items()
+
+
 class SpecOperation(SpecElement):
     """Netlink Operation
 
@@ -344,6 +382,9 @@ class SpecFamily(SpecElement):
     def new_attr_set(self, elem):
         return SpecAttrSet(self, elem)
 
+    def new_struct(self, elem):
+        return SpecStruct(self, elem)
+
     def new_operation(self, elem, req_val, rsp_val):
         return SpecOperation(self, elem, req_val, rsp_val)
 
@@ -399,6 +440,8 @@ class SpecFamily(SpecElement):
         for elem in definitions:
             if elem['type'] == 'enum' or elem['type'] == 'flags':
                 self.consts[elem['name']] = self.new_enum(elem)
+            elif elem['type'] == 'struct':
+                self.consts[elem['name']] = self.new_struct(elem)
             else:
                 self.consts[elem['name']] = elem
 
-- 
2.39.0

[PATCH net-next v4 2/7] tools: ynl: Add C array attribute decoding to ynl

[Donald Hunter]

Add support for decoding C arrays from binay blobs in genetlink-legacy
messages.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/ynl.py | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 98ecfdb44a83..b635d147175c 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -68,6 +68,11 @@ class Netlink:
 
 
 class NlAttr:
+    type_formats = { 'u8' : ('B', 1), 's8' : ('b', 1),
+                     'u16': ('H', 2), 's16': ('h', 2),
+                     'u32': ('I', 4), 's32': ('i', 4),
+                     'u64': ('Q', 8), 's64': ('q', 8) }
+
     def __init__(self, raw, offset):
         self._len, self._type = struct.unpack("HH", raw[offset:offset + 4])
         self.type = self._type & ~Netlink.NLA_TYPE_MASK
@@ -93,6 +98,10 @@ class NlAttr:
     def as_bin(self):
         return self.raw
 
+    def as_c_array(self, type):
+        format, _ = self.type_formats[type]
+        return list({ x[0] for x in struct.iter_unpack(format, self.raw) })
+
     def __repr__(self):
         return f"[type:{self.type} len:{self._len}] {self.raw}"
 
@@ -363,6 +372,14 @@ class YnlFamily(SpecFamily):
             value = enum.entries_by_val[raw - i].name
         rsp[attr_spec['name']] = value
 
+    def _decode_binary(self, attr, attr_spec):
+        sub_type = attr_spec.get('sub-type')
+        if sub_type:
+            decoded = attr.as_c_array(sub_type)
+        else:
+            decoded = attr.as_bin()
+        return decoded
+
     def _decode(self, attrs, space):
         attr_space = self.attr_sets[space]
         rsp = dict()
@@ -380,7 +397,7 @@ class YnlFamily(SpecFamily):
             elif attr_spec["type"] == 'string':
                 decoded = attr.as_strz()
             elif attr_spec["type"] == 'binary':
-                decoded = attr.as_bin()
+                decoded = self._decode_binary(attr, attr_spec)
             elif attr_spec["type"] == 'flag':
                 decoded = True
             else:
-- 
2.39.0

[PATCH net-next v4 3/7] tools: ynl: Add struct attr decoding to ynl

[Donald Hunter]

Add support for decoding attributes that contain C structs.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/genetlink-legacy.yaml |  5 +++++
 tools/net/ynl/lib/ynl.py                    | 15 ++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index 5dc6f1c07a97..d50c78b9f42d 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -218,6 +218,11 @@ properties:
                     description: Max length for a string or a binary attribute.
                     $ref: '#/$defs/len-or-define'
               sub-type: *attr-type
+              # Start genetlink-legacy
+              struct:
+                description: Name of the struct type used for the attribute.
+                type: string
+              # End genetlink-legacy
 
       # Make sure name-prefix does not appear in subsets (subsets inherit naming)
       dependencies:
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index b635d147175c..af1d6d380035 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -102,6 +102,16 @@ class NlAttr:
         format, _ = self.type_formats[type]
         return list({ x[0] for x in struct.iter_unpack(format, self.raw) })
 
+    def as_struct(self, members):
+        value = dict()
+        offset = 0
+        for m in members:
+            format, size = self.type_formats[m.type]
+            decoded = struct.unpack_from(format, self.raw, offset)
+            offset += size
+            value[m.name] = decoded[0]
+        return value
+
     def __repr__(self):
         return f"[type:{self.type} len:{self._len}] {self.raw}"
 
@@ -373,8 +383,11 @@ class YnlFamily(SpecFamily):
         rsp[attr_spec['name']] = value
 
     def _decode_binary(self, attr, attr_spec):
+        struct_name = attr_spec.get('struct')
         sub_type = attr_spec.get('sub-type')
-        if sub_type:
+        if struct_name:
+            decoded = attr.as_struct(self.consts[struct_name])
+        elif sub_type:
             decoded = attr.as_c_array(sub_type)
         else:
             decoded = attr.as_bin()
-- 
2.39.0

[PATCH net-next v4 4/7] tools: ynl: Add fixed-header support to ynl

[Donald Hunter]

Add support for netlink families that add an optional fixed header structure
after the genetlink header and before any attributes. The fixed-header can be
specified on a per op basis, or once for all operations, which serves as a
default value that can be overridden.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/genetlink-legacy.yaml | 10 +++++++++
 tools/net/ynl/lib/nlspec.py                 | 21 ++++++++++++-------
 tools/net/ynl/lib/ynl.py                    | 23 +++++++++++++++++----
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index d50c78b9f42d..3b8984122383 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -261,6 +261,13 @@ properties:
       async-enum:
         description: Name for the enum type with notifications/events.
         type: string
+      # Start genetlink-legacy
+      fixed-header: &fixed-header
+        description: |
+          Name of the structure defininig the optional fixed-length protocol header. This header is
+          placed in a message after the netlink and genetlink headers and before any attributes.
+        type: string
+      # End genetlink-legacy
       list:
         description: List of commands
         type: array
@@ -293,6 +300,9 @@ properties:
               type: array
               items:
                 enum: [ strict, dump ]
+            # Start genetlink-legacy
+            fixed-header: *fixed-header
+            # End genetlink-legacy
             do: &subop-type
               description: Main command handler.
               type: object
diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index a08f6dda5b79..09dbb6c51ee9 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -258,16 +258,17 @@ class SpecOperation(SpecElement):
     Information about a single Netlink operation.
 
     Attributes:
-        value       numerical ID when serialized, None if req/rsp values differ
+        value           numerical ID when serialized, None if req/rsp values differ
 
-        req_value   numerical ID when serialized, user -> kernel
-        rsp_value   numerical ID when serialized, user <- kernel
-        is_call     bool, whether the operation is a call
-        is_async    bool, whether the operation is a notification
-        is_resv     bool, whether the operation does not exist (it's just a reserved ID)
-        attr_set    attribute set name
+        req_value       numerical ID when serialized, user -> kernel
+        rsp_value       numerical ID when serialized, user <- kernel
+        is_call         bool, whether the operation is a call
+        is_async        bool, whether the operation is a notification
+        is_resv         bool, whether the operation does not exist (it's just a reserved ID)
+        attr_set        attribute set name
+        fixed_header    string, optional name of fixed header struct
 
-        yaml        raw spec as loaded from the spec file
+        yaml            raw spec as loaded from the spec file
     """
     def __init__(self, family, yaml, req_value, rsp_value):
         super().__init__(family, yaml)
@@ -279,6 +280,7 @@ class SpecOperation(SpecElement):
         self.is_call = 'do' in yaml or 'dump' in yaml
         self.is_async = 'notify' in yaml or 'event' in yaml
         self.is_resv = not self.is_async and not self.is_call
+        self.fixed_header = self.yaml.get('fixed-header', family.fixed_header)
 
         # Added by resolve:
         self.attr_set = None
@@ -319,6 +321,7 @@ class SpecFamily(SpecElement):
         msgs_by_value  dict of all messages (indexed by name)
         ops        dict of all valid requests / responses
         consts     dict of all constants/enums
+        fixed_header  string, optional name of family default fixed header struct
     """
     def __init__(self, spec_path, schema_path=None):
         with open(spec_path, "r") as stream:
@@ -392,6 +395,7 @@ class SpecFamily(SpecElement):
         self._resolution_list.append(elem)
 
     def _dictify_ops_unified(self):
+        self.fixed_header = self.yaml['operations'].get('fixed-header')
         val = 1
         for elem in self.yaml['operations']['list']:
             if 'value' in elem:
@@ -403,6 +407,7 @@ class SpecFamily(SpecElement):
             self.msgs[op.name] = op
 
     def _dictify_ops_directional(self):
+        self.fixed_header = self.yaml['operations'].get('fixed-header')
         req_val = rsp_val = 1
         for elem in self.yaml['operations']['list']:
             if 'notify' in elem:
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index af1d6d380035..4d206c46a087 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -277,14 +277,22 @@ def _genl_load_families():
 
 
 class GenlMsg:
-    def __init__(self, nl_msg):
+    def __init__(self, nl_msg, fixed_header_members = []):
         self.nl = nl_msg
 
         self.hdr = nl_msg.raw[0:4]
-        self.raw = nl_msg.raw[4:]
+        offset = 4
 
         self.genl_cmd, self.genl_version, _ = struct.unpack("BBH", self.hdr)
 
+        self.fixed_header_attrs = dict()
+        for m in fixed_header_members:
+            format, size = NlAttr.type_formats[m.type]
+            decoded = struct.unpack_from(format, nl_msg.raw, offset)
+            offset += size
+            self.fixed_header_attrs[m.name] = decoded[0]
+
+        self.raw = nl_msg.raw[offset:]
         self.raw_attrs = NlAttrs(self.raw)
 
     def __repr__(self):
@@ -504,6 +512,13 @@ class YnlFamily(SpecFamily):
 
         req_seq = random.randint(1024, 65535)
         msg = _genl_msg(self.family.family_id, nl_flags, op.req_value, 1, req_seq)
+        fixed_header_members = []
+        if op.fixed_header:
+            fixed_header_members = self.consts[op.fixed_header].members
+            for m in fixed_header_members:
+                value = vals.pop(m.name)
+                format, _ = NlAttr.type_formats[m.type]
+                msg += struct.pack(format, value)
         for name, value in vals.items():
             msg += self._add_attr(op.attr_set.name, name, value)
         msg = _genl_msg_finalize(msg)
@@ -530,7 +545,7 @@ class YnlFamily(SpecFamily):
                     done = True
                     break
 
-                gm = GenlMsg(nl_msg)
+                gm = GenlMsg(nl_msg, fixed_header_members)
                 # Check if this is a reply to our request
                 if nl_msg.nl_seq != req_seq or gm.genl_cmd != op.rsp_value:
                     if gm.genl_cmd in self.async_msg_ids:
@@ -540,7 +555,7 @@ class YnlFamily(SpecFamily):
                         print('Unexpected message: ' + repr(gm))
                         continue
 
-                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name))
+                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name) | gm.fixed_header_attrs)
 
         if not rsp:
             return None
-- 
2.39.0

[PATCH net-next v4 5/7] netlink: specs: add partial specification for openvswitch

[Donald Hunter]

The openvswitch family has a fixed header, uses struct attrs and has array
values. This partial spec demonstrates these features in the YNL CLI. These
specs are sufficient to create, delete and dump datapaths and to dump vports:

$ ./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/ovs_datapath.yaml \
    --do dp-new --json '{ "dp-ifindex": 0, "name": "demo", "upcall-pid": 0}'
None

$ ./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/ovs_datapath.yaml \
    --dump dp-get --json '{ "dp-ifindex": 0 }'
[{'dp-ifindex': 3,
  'masks-cache-size': 256,
  'megaflow-stats': {'cache-hits': 0,
                     'mask-hit': 0,
                     'masks': 0,
                     'pad1': 0,
                     'padding': 0},
  'name': 'test',
  'stats': {'flows': 0, 'hit': 0, 'lost': 0, 'missed': 0},
  'user-features': {'dispatch-upcall-per-cpu',
                    'tc-recirc-sharing',
                    'unaligned'}},
 {'dp-ifindex': 48,
  'masks-cache-size': 256,
  'megaflow-stats': {'cache-hits': 0,
                     'mask-hit': 0,
                     'masks': 0,
                     'pad1': 0,
                     'padding': 0},
  'name': 'demo',
  'stats': {'flows': 0, 'hit': 0, 'lost': 0, 'missed': 0},
  'user-features': set()}]

$ ./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/ovs_datapath.yaml \
    --do dp-del --json '{ "dp-ifindex": 0, "name": "demo"}'
None

$ ./tools/net/ynl/cli.py \
    --spec Documentation/netlink/specs/ovs_vport.yaml \
    --dump vport-get --json '{ "dp-ifindex": 3 }'
[{'dp-ifindex': 3,
  'ifindex': 3,
  'name': 'test',
  'port-no': 0,
  'stats': {'rx-bytes': 0,
            'rx-dropped': 0,
            'rx-errors': 0,
            'rx-packets': 0,
            'tx-bytes': 0,
            'tx-dropped': 0,
            'tx-errors': 0,
            'tx-packets': 0},
  'type': 'internal',
  'upcall-pid': [0],
  'upcall-stats': {'fail': 0, 'success': 0}}]

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 Documentation/netlink/specs/ovs_datapath.yaml | 153 ++++++++++++++++++
 Documentation/netlink/specs/ovs_vport.yaml    | 139 ++++++++++++++++
 2 files changed, 292 insertions(+)
 create mode 100644 Documentation/netlink/specs/ovs_datapath.yaml
 create mode 100644 Documentation/netlink/specs/ovs_vport.yaml

diff --git a/Documentation/netlink/specs/ovs_datapath.yaml b/Documentation/netlink/specs/ovs_datapath.yaml
new file mode 100644
index 000000000000..6d71db8c4416
--- /dev/null
+++ b/Documentation/netlink/specs/ovs_datapath.yaml
@@ -0,0 +1,153 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: ovs_datapath
+version: 2
+protocol: genetlink-legacy
+
+doc:
+  OVS datapath configuration over generic netlink.
+
+definitions:
+  -
+    name: ovs-header
+    type: struct
+    members:
+      -
+        name: dp-ifindex
+        type: u32
+  -
+    name: user-features
+    type: flags
+    entries:
+      -
+        name: unaligned
+        doc: Allow last Netlink attribute to be unaligned
+      -
+        name: vport-pids
+        doc: Allow datapath to associate multiple Netlink PIDs to each vport
+      -
+        name: tc-recirc-sharing
+        doc: Allow tc offload recirc sharing
+      -
+        name: dispatch-upcall-per-cpu
+        doc: Allow per-cpu dispatch of upcalls
+  -
+    name: datapath-stats
+    type: struct
+    members:
+      -
+        name: hit
+        type: u64
+      -
+        name: missed
+        type: u64
+      -
+        name: lost
+        type: u64
+      -
+        name: flows
+        type: u64
+  -
+    name: megaflow-stats
+    type: struct
+    members:
+      -
+        name: mask-hit
+        type: u64
+      -
+        name: masks
+        type: u32
+      -
+        name: padding
+        type: u32
+      -
+        name: cache-hits
+        type: u64
+      -
+        name: pad1
+        type: u64
+
+attribute-sets:
+  -
+    name: datapath
+    attributes:
+      -
+        name: name
+        type: string
+      -
+        name: upcall-pid
+        doc: upcall pid
+        type: u32
+      -
+        name: stats
+        type: binary
+        struct: datapath-stats
+      -
+        name: megaflow-stats
+        type: binary
+        struct: megaflow-stats
+      -
+        name: user-features
+        type: u32
+        enum: user-features
+        enum-as-flags: true
+      -
+        name: pad
+        type: unused
+      -
+        name: masks-cache-size
+        type: u32
+      -
+        name: per-cpu-pids
+        type: binary
+        sub-type: u32
+
+operations:
+  fixed-header: ovs-header
+  list:
+    -
+      name: dp-get
+      doc: Get / dump OVS data path configuration and state
+      value: 3
+      attribute-set: datapath
+      do: &dp-get-op
+        request:
+          attributes:
+            - name
+        reply:
+          attributes:
+            - name
+            - upcall-pid
+            - stats
+            - megaflow-stats
+            - user-features
+            - masks-cache-size
+            - per-cpu-pids
+      dump: *dp-get-op
+    -
+      name: dp-new
+      doc: Create new OVS data path
+      value: 1
+      attribute-set: datapath
+      do:
+        request:
+          attributes:
+            - dp-ifindex
+            - name
+            - upcall-pid
+            - user-features
+    -
+      name: dp-del
+      doc: Delete existing OVS data path
+      value: 2
+      attribute-set: datapath
+      do:
+        request:
+          attributes:
+            - dp-ifindex
+            - name
+
+mcast-groups:
+  list:
+    -
+      name: ovs_datapath
diff --git a/Documentation/netlink/specs/ovs_vport.yaml b/Documentation/netlink/specs/ovs_vport.yaml
new file mode 100644
index 000000000000..8e55622ddf11
--- /dev/null
+++ b/Documentation/netlink/specs/ovs_vport.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
+
+name: ovs_vport
+version: 2
+protocol: genetlink-legacy
+
+doc:
+  OVS vport configuration over generic netlink.
+
+definitions:
+  -
+    name: ovs-header
+    type: struct
+    members:
+      -
+        name: dp-ifindex
+        type: u32
+  -
+    name: vport-type
+    type: enum
+    entries: [ unspec, netdev, internal, gre, vxlan, geneve ]
+  -
+    name: vport-stats
+    type: struct
+    members:
+      -
+        name: rx-packets
+        type: u64
+      -
+        name: tx-packets
+        type: u64
+      -
+        name: rx-bytes
+        type: u64
+      -
+        name: tx-bytes
+        type: u64
+      -
+        name: rx-errors
+        type: u64
+      -
+        name: tx-errors
+        type: u64
+      -
+        name: rx-dropped
+        type: u64
+      -
+        name: tx-dropped
+        type: u64
+
+attribute-sets:
+  -
+    name: vport-options
+    attributes:
+      -
+        name: dst-port
+        type: u32
+      -
+        name: extension
+        type: u32
+  -
+    name: upcall-stats
+    attributes:
+      -
+        name: success
+        type: u64
+        value: 0
+      -
+        name: fail
+        type: u64
+  -
+    name: vport
+    attributes:
+      -
+        name: port-no
+        type: u32
+      -
+        name: type
+        type: u32
+        enum: vport-type
+      -
+        name: name
+        type: string
+      -
+        name: options
+        type: nest
+        nested-attributes: vport-options
+      -
+        name: upcall-pid
+        type: binary
+        sub-type: u32
+      -
+        name: stats
+        type: binary
+        struct: vport-stats
+      -
+        name: pad
+        type: unused
+      -
+        name: ifindex
+        type: u32
+      -
+        name: netnsid
+        type: u32
+      -
+        name: upcall-stats
+        type: nest
+        nested-attributes: upcall-stats
+
+operations:
+  list:
+    -
+      name: vport-get
+      doc: Get / dump OVS vport configuration and state
+      value: 3
+      attribute-set: vport
+      fixed-header: ovs-header
+      do: &vport-get-op
+        request:
+          attributes:
+            - dp-ifindex
+            - name
+        reply: &dev-all
+          attributes:
+            - dp-ifindex
+            - port-no
+            - type
+            - name
+            - upcall-pid
+            - stats
+            - ifindex
+            - netnsid
+            - upcall-stats
+      dump: *vport-get-op
+
+mcast-groups:
+  list:
+    -
+      name: ovs_vport
-- 
2.39.0

[PATCH net-next v4 6/7] docs: netlink: document struct support for genetlink-legacy

[Donald Hunter]

Describe the genetlink-legacy support for using struct definitions
for fixed headers and for binary attributes.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 .../netlink/genetlink-legacy.rst              | 54 ++++++++++++++++++-
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/netlink/genetlink-legacy.rst b/Documentation/userspace-api/netlink/genetlink-legacy.rst
index 3bf0bcdf21d8..6b385a9e6d0b 100644
--- a/Documentation/userspace-api/netlink/genetlink-legacy.rst
+++ b/Documentation/userspace-api/netlink/genetlink-legacy.rst
@@ -163,8 +163,58 @@ Structures
 ----------
 
 Legacy families can define C structures both to be used as the contents
-of an attribute and as a fixed message header. The plan is to define
-the structs in ``definitions`` and link the appropriate attrs.
+of an attribute and as a fixed message header. Structs are defined
+in ``definitions`` and referenced in operations or attributes.
+
+.. code-block:: yaml
+
+  definitions:
+    -
+      name: message-header
+      type: struct
+      members:
+        -
+          name: a
+          type: u32
+        -
+          name: b
+          type: string
+
+Fixed Headers
+~~~~~~~~~~~~~
+
+Fixed message headers can be added to operations using ``fixed-header``.
+The default ``fixed-header`` can be set in ``operations`` and it can be set
+or overridden for each operation.
+
+.. code-block:: yaml
+
+  operations:
+    fixed-header: message-header
+    list:
+      -
+        name: get
+        fixed-header: custom-header
+        attribute-set: message-attrs
+
+Attributes
+~~~~~~~~~~
+
+A ``binary`` attribute can be interpreted as a C structure using a
+``struct`` property with the name of the structure definition. The
+``struct`` property implies ``sub-type: struct`` so it is not necessary to
+specify a sub-type.
+
+.. code-block:: yaml
+
+  attribute-sets:
+    -
+      name: stats-attrs
+      attributes:
+        -
+          name: stats
+          type: binary
+          struct: vport-stats
 
 Multi-message DO
 ----------------
-- 
2.39.0

[PATCH net-next v4 7/7] docs: netlink: document the sub-type attribute property

[Donald Hunter]

Add a definition for sub-type to the protocol spec doc and a description of
its usage for C arrays in genetlink-legacy.

Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
---
 .../userspace-api/netlink/genetlink-legacy.rst     | 14 ++++++++++++++
 Documentation/userspace-api/netlink/specs.rst      |  9 +++++++++
 2 files changed, 23 insertions(+)

diff --git a/Documentation/userspace-api/netlink/genetlink-legacy.rst b/Documentation/userspace-api/netlink/genetlink-legacy.rst
index 6b385a9e6d0b..afd9c4947a1c 100644
--- a/Documentation/userspace-api/netlink/genetlink-legacy.rst
+++ b/Documentation/userspace-api/netlink/genetlink-legacy.rst
@@ -216,6 +216,20 @@ specify a sub-type.
           type: binary
           struct: vport-stats
 
+C Arrays
+--------
+
+Legacy families also use ``binary`` attributes to encapsulate C arrays. The
+``sub-type`` is used to identify the type of scalar to extract.
+
+.. code-block:: yaml
+
+  attributes:
+    -
+      name: ports
+      type: binary
+      sub-type: u32
+
 Multi-message DO
 ----------------
 
diff --git a/Documentation/userspace-api/netlink/specs.rst b/Documentation/userspace-api/netlink/specs.rst
index a22442ba1d30..7931322d3238 100644
--- a/Documentation/userspace-api/netlink/specs.rst
+++ b/Documentation/userspace-api/netlink/specs.rst
@@ -254,6 +254,15 @@ rather than depend on what is specified in the spec file.
 The validation policy in the kernel is formed by combining the type
 definition (``type`` and ``nested-attributes``) and the ``checks``.
 
+sub-type
+~~~~~~~~
+
+Attributes can have a ``sub-type`` that is interpreted in a ``type``
+specific way. For example, an attribute with ``type: binary`` can have
+``sub-type: u32`` which says to interpret the binary blob as an array of
+``u32``. Binary types are described in more detail in
+:doc:`genetlink-legacy`.
+
 operations
 ----------
 
-- 
2.39.0

Re: [PATCH net-next v4 1/7] tools: ynl: Add struct parsing to nlspec

[Jakub Kicinski]

On Fri, 24 Mar 2023 19:18:54 +0000 Donald Hunter wrote:
> Add python classes for struct definitions to nlspec
> 
> Signed-off-by: Donald Hunter <donald.hunter@gmail.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next v4 2/7] tools: ynl: Add C array attribute decoding to ynl

[Jakub Kicinski]

On Fri, 24 Mar 2023 19:18:55 +0000 Donald Hunter wrote:
> Add support for decoding C arrays from binay blobs in genetlink-legacy
> messages.
> 
> Signed-off-by: Donald Hunter <donald.hunter@gmail.com>

If Stan's byte-order support gets in first consider supporting
byte-order in arrays as well:
https://lore.kernel.org/r/20230324225656.3999785-2-sdf@google.com/

Otherwise LGTM:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next v4 3/7] tools: ynl: Add struct attr decoding to ynl

[Jakub Kicinski]

On Fri, 24 Mar 2023 19:18:56 +0000 Donald Hunter wrote:
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index b635d147175c..af1d6d380035 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -102,6 +102,16 @@ class NlAttr:
>          format, _ = self.type_formats[type]
>          return list({ x[0] for x in struct.iter_unpack(format, self.raw) })
>  
> +    def as_struct(self, members):
> +        value = dict()
> +        offset = 0
> +        for m in members:

Maybe add a TODO here for string and binary?

> +            format, size = self.type_formats[m.type]
> +            decoded = struct.unpack_from(format, self.raw, offset)
> +            offset += size
> +            value[m.name] = decoded[0]
> +        return value
> +
>      def __repr__(self):
>          return f"[type:{self.type} len:{self._len}] {self.raw}"
>  
> @@ -373,8 +383,11 @@ class YnlFamily(SpecFamily):
>          rsp[attr_spec['name']] = value
>  
>      def _decode_binary(self, attr, attr_spec):
> +        struct_name = attr_spec.get('struct')
>          sub_type = attr_spec.get('sub-type')

Could you add these as fields in class SpecAttr, like is_multi
and access the fields here instead of the get()s?

> -        if sub_type:
> +        if struct_name:
> +            decoded = attr.as_struct(self.consts[struct_name])
> +        elif sub_type:
>              decoded = attr.as_c_array(sub_type)
>          else:
>              decoded = attr.as_bin()

Re: [PATCH net-next v4 4/7] tools: ynl: Add fixed-header support to ynl

[Jakub Kicinski]

On Fri, 24 Mar 2023 19:18:57 +0000 Donald Hunter wrote:
> diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
> index d50c78b9f42d..3b8984122383 100644
> --- a/Documentation/netlink/genetlink-legacy.yaml
> +++ b/Documentation/netlink/genetlink-legacy.yaml
> @@ -261,6 +261,13 @@ properties:
>        async-enum:
>          description: Name for the enum type with notifications/events.
>          type: string
> +      # Start genetlink-legacy
> +      fixed-header: &fixed-header
> +        description: |
> +          Name of the structure defininig the optional fixed-length protocol header. This header is

Typo in 'defininig', could you also wrap at 80 chars?
Old school kernel style.

> +          placed in a message after the netlink and genetlink headers and before any attributes.
> +        type: string
> +      # End genetlink-legacy

>  class GenlMsg:
> -    def __init__(self, nl_msg):
> +    def __init__(self, nl_msg, fixed_header_members = []):

spaces around = or no spaces? I don't really know myself but I'm used
to having no spaces.

> @@ -540,7 +555,7 @@ class YnlFamily(SpecFamily):
>                          print('Unexpected message: ' + repr(gm))
>                          continue
>  
> -                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name))
> +                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name) | gm.fixed_header_attrs)

nit: also line wrap?

Re: [PATCH net-next v4 6/7] docs: netlink: document struct support for genetlink-legacy

[Jakub Kicinski]

On Fri, 24 Mar 2023 19:18:59 +0000 Donald Hunter wrote:
>  Legacy families can define C structures both to be used as the contents
> -of an attribute and as a fixed message header. The plan is to define
> -the structs in ``definitions`` and link the appropriate attrs.
> +of an attribute and as a fixed message header. Structs are defined
> +in ``definitions`` and referenced in operations or attributes.

We should call out that the structs in YAML are implicitly "packed"
(in the C sense of the word), so struct { u8 a; u16 b; u8 c; } is 
4 bytes not 6 bytes.

Any padding must be explicitly, C-like languages should infer the need
for explicit packing from whether the members are naturally aligned.

> +.. code-block:: yaml
> +
> +  definitions:
> +    -
> +      name: message-header
> +      type: struct
> +      members:
> +        -
> +          name: a
> +          type: u32
> +        -
> +          name: b
> +          type: string

Maybe not the most fortunate example :) cause I think that for
string/binary we'll need an explicit length. Maybe not for
last one if it's a flexible array... but that's rare in NL.

The rest LGTM, thanks!

Re: [PATCH net-next v4 7/7] docs: netlink: document the sub-type attribute property

[Jakub Kicinski]

On Fri, 24 Mar 2023 19:19:00 +0000 Donald Hunter wrote:
> +sub-type
> +~~~~~~~~
> +
> +Attributes can have a ``sub-type`` that is interpreted in a ``type``
> +specific way. For example, an attribute with ``type: binary`` can have
> +``sub-type: u32`` which says to interpret the binary blob as an array of
> +``u32``. Binary types are described in more detail in
> +:doc:`genetlink-legacy`.

I think sub-type is only used for arrays? How about:

 Legacy families have special ways of expressing arrays. ``sub-type``
 can be used to define the type of array members in case array members
 are not fully defined as attributes (in a bona fide attribute space).
 For instance a C array of u32 values can be specified with 
 ``type: binary`` and ``sub-type: u32``. Binary types and legacy array
 formats are described in more detail in :doc:`genetlink-legacy`.

Re: [PATCH net-next v4 2/7] tools: ynl: Add C array attribute decoding to ynl

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 24 Mar 2023 19:18:55 +0000 Donald Hunter wrote:
>> Add support for decoding C arrays from binay blobs in genetlink-legacy
>> messages.
>> 
>> Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
>
> If Stan's byte-order support gets in first consider supporting
> byte-order in arrays as well:
> https://lore.kernel.org/r/20230324225656.3999785-2-sdf@google.com/

Ack. I'll watch for it. 

>
> Otherwise LGTM:
>
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next v4 4/7] tools: ynl: Add fixed-header support to ynl

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 24 Mar 2023 19:18:57 +0000 Donald Hunter wrote:
>> diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
>> index d50c78b9f42d..3b8984122383 100644
>> --- a/Documentation/netlink/genetlink-legacy.yaml
>> +++ b/Documentation/netlink/genetlink-legacy.yaml
>> @@ -261,6 +261,13 @@ properties:
>>        async-enum:
>>          description: Name for the enum type with notifications/events.
>>          type: string
>> +      # Start genetlink-legacy
>> +      fixed-header: &fixed-header
>> +        description: |
>> +          Name of the structure defininig the optional fixed-length protocol header. This header is
>
> Typo in 'defininig', could you also wrap at 80 chars?
> Old school kernel style.

Will do. The spec does spill beyond 100 chars tho.


>> +          placed in a message after the netlink and genetlink headers and before any attributes.
>> +        type: string
>> +      # End genetlink-legacy
>
>>  class GenlMsg:
>> -    def __init__(self, nl_msg):
>> +    def __init__(self, nl_msg, fixed_header_members = []):
>
> spaces around = or no spaces? I don't really know myself but I'm used
> to having no spaces.

Happy to go with existing convention in the codebase and will remove spaces.

>> @@ -540,7 +555,7 @@ class YnlFamily(SpecFamily):
>>                          print('Unexpected message: ' + repr(gm))
>>                          continue
>>  
>> -                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name))
>> +                rsp.append(self._decode(gm.raw_attrs, op.attr_set.name) | gm.fixed_header_attrs)
>
> nit: also line wrap?

Will do.

Re: [PATCH net-next v4 6/7] docs: netlink: document struct support for genetlink-legacy

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 24 Mar 2023 19:18:59 +0000 Donald Hunter wrote:
>>  Legacy families can define C structures both to be used as the contents
>> -of an attribute and as a fixed message header. The plan is to define
>> -the structs in ``definitions`` and link the appropriate attrs.
>> +of an attribute and as a fixed message header. Structs are defined
>> +in ``definitions`` and referenced in operations or attributes.
>
> We should call out that the structs in YAML are implicitly "packed"
> (in the C sense of the word), so struct { u8 a; u16 b; u8 c; } is 
> 4 bytes not 6 bytes.
>
> Any padding must be explicitly, C-like languages should infer the need
> for explicit packing from whether the members are naturally aligned.

I'll update the text to mention padding, with this example.

>> +.. code-block:: yaml
>> +
>> +  definitions:
>> +    -
>> +      name: message-header
>> +      type: struct
>> +      members:
>> +        -
>> +          name: a
>> +          type: u32
>> +        -
>> +          name: b
>> +          type: string
>
> Maybe not the most fortunate example :) cause I think that for
> string/binary we'll need an explicit length. Maybe not for
> last one if it's a flexible array... but that's rare in NL.

Ah, good point. I'll change the example to match the struct above.

> The rest LGTM, thanks!

Re: [PATCH net-next v4 7/7] docs: netlink: document the sub-type attribute property

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Fri, 24 Mar 2023 19:19:00 +0000 Donald Hunter wrote:
>> +sub-type
>> +~~~~~~~~
>> +
>> +Attributes can have a ``sub-type`` that is interpreted in a ``type``
>> +specific way. For example, an attribute with ``type: binary`` can have
>> +``sub-type: u32`` which says to interpret the binary blob as an array of
>> +``u32``. Binary types are described in more detail in
>> +:doc:`genetlink-legacy`.
>
> I think sub-type is only used for arrays? How about:
>
>  Legacy families have special ways of expressing arrays. ``sub-type``
>  can be used to define the type of array members in case array members
>  are not fully defined as attributes (in a bona fide attribute space).
>  For instance a C array of u32 values can be specified with 
>  ``type: binary`` and ``sub-type: u32``. Binary types and legacy array
>  formats are described in more detail in :doc:`genetlink-legacy`.

I'll use this text verbatim. Thanks.