mctpd: Add `mctp/links/<linkName>` D-Bus object #39

ThuBaNguyen · 2024-05-20T03:44:21Z

Create the MCTP Link D-Bus objects for the existing MCTP links at /xyz/openbmc_project/mctp/links/<linkName>. When the MCTP links is removed from/added to the system, the D-Bus object will also be removed/added.
Create the au.com.CodeConstruct.MCTP.Link D-Bus interface for MCTP Link D-Bus objects. The interface includes the Role property which reports BMC roles in the link. The possible value of Role are BusOwner, Endpoint and Unknown.
Because the BMC Role in the MCTP link is fixed. The Role property is changeable value but it can only be changed when the current configured value is Unknown.

Ex:

~# busctl tree xyz.openbmc_project.MCTP
`- /xyz
  `- /xyz/openbmc_project
    `- /xyz/openbmc_project/mctp
      |- /xyz/openbmc_project/mctp/1
      | `- /xyz/openbmc_project/mctp/1/8
      `- /xyz/openbmc_project/mctp/links
        |- /xyz/openbmc_project/mctp/links/lo
        `- /xyz/openbmc_project/mctp/links/mctpi2c3

~# busctl introspect xyz.openbmc_project.MCTP /xyz/openbmc_project/mctp/links
NAME                                TYPE      SIGNATURE RESULT/VALUE FLAGS
org.freedesktop.DBus.Introspectable interface -         -            -
.Introspect                         method    -         s            -
org.freedesktop.DBus.Peer           interface -         -            -
.GetMachineId                       method    -         s            -
.Ping                               method    -         -            -
org.freedesktop.DBus.Properties     interface -         -            -
.Get                                method    ss        v            -
.GetAll                             method    s         a{sv}        -
.Set                                method    ssv       -            -
.PropertiesChanged                  signal    sa{sv}as  -            -

~# busctl introspect xyz.openbmc_project.MCTP /xyz/openbmc_project/mctp/links/mctpi2c3
NAME                                TYPE      SIGNATURE RESULT/VALUE FLAGS
au.com.CodeConstruct.MCTP.Link      interface -         -            -
.Role                               property  s         "Unknown"    emits-change writable
org.freedesktop.DBus.Introspectable interface -         -            -
.Introspect                         method    -         s            -
org.freedesktop.DBus.Peer           interface -         -            -
.GetMachineId                       method    -         s            -
.Ping                               method    -         -            -
org.freedesktop.DBus.Properties     interface -         -            -
.Get                                method    ss        v            -
.GetAll                             method    s         a{sv}        -
.Set                                method    ssv       -            -
.PropertiesChanged                  signal    sa{sv}as  -            -

~# busctl set-property xyz.openbmc_project.MCTP /xyz/openbmc_project/mctp/links/mctpi2c3 au.com.CodeConstruct.MCTP.Link Role s BusOwner

~# busctl set-property xyz.openbmc_project.MCTP /xyz/openbmc_project/mctp/links/mctpi2c3 au.com.CodeConstruct.MCTP.Link Role
s "BusOwner"

Import a copy of the tomlc99 parser, somewhat temporarily, from upstream commit 5221b3d. We import the toml.[ch] sources, the LICENSE, and add a short readme for the upstream reference. Signed-off-by: Jeremy Kerr <[email protected]>

This change adds a basic configuration interface, using a simple toml syntax. This allows configuration of the existing mctpd parameters: bus-owner mode, the mctp timeout, and the UUID. We ship a small example configuration under conf/. Signed-off-by: Jeremy Kerr <[email protected]>

jk-ozlabs · 2024-05-20T03:48:35Z

Thanks!

We'll need to check whether this is compatible with the existing layout where the top-level /mctp/ can host a non-network child object. If not, we will have to move those to endpoints/.

jk-ozlabs · 2024-05-28T09:30:24Z

I started writing some notes on the interface changes, but that expanded beyond just this change, so they're now in #40.

However, I'd rather not drag out this change too long, so already have progress on implementing the changes in #40, which should be quick to adapt to most design changes that may arise there.

So, I'll add a couple of review comments on this, just for top-level API things for now; we can cover implementation details soon. It's my goal that we would then rebase this change on to the dbus api rework that comes out of #40.

jk-ozlabs

Mostly looks good, we'll just need to synchronise with the dbus API changes.

docs/mctpd.md

src/mctp-netlink.h

src/mctpd.c

Rather than requiring a -c FILE argument, allow a default configuration file of /etc/mctpd.conf. We don't error if this is absent, to allow running with the existing defaults. Signed-off-by: Jeremy Kerr <[email protected]>

jk-ozlabs

Mostly looks good, thanks!

I have a few comments inline, but a lot of those are more questions to improve my understanding of your approach here.

src/mctpd.c

src/mctp-netlink.h

src/mctpd.c

We have these repeated, move to a common location. Signed-off-by: Jeremy Kerr <[email protected]>

…ig values Rather than hard-coding the mode types in the config parsing, move these to an array of role descriptions, and parse from that. This will be used in an upcoming change where we parse roles from dbus properties too. Signed-off-by: Jeremy Kerr <[email protected]>

This change implements the dbus interface rework specified in CodeConstruct#40. Essentially: this uses more standard bus, object and path names, and moves away from the xyz.openbmc_project namespace, and use au.com.codeconstruct (all lowercase) there instead, as we're not specificially an OpenBMC project. We also put collections of things (networks and endpoints) under a specifically-named object path, so we can introduce new collections alongside (interfaces) without compatibility issues This means: - the bus owner name is now au.com.codeconstruct.MCTP1 - interfaces are namespaced and versioned: - au.com.codeconstruct.MCTP.Endpoint1 - au.com.codeconstruct.MCTP.BusOwner1 - the top-level entrypoint path is versioned, as `/au/com/codeconstruct/mctp1` - the endpoint object tree is structured as `/au/com/codeconstruct/mctp1/networks/<n>/endpoints/<e>` Signed-off-by: Jeremy Kerr <[email protected]>

We'll be modifying this for the new Role-based interfaces which exist on the link/interface objects, so use a temporary name for now. Signed-off-by: Jeremy Kerr <[email protected]>

src/mctpd.c

Enumerate the parent D-Bus object path `.../mctp1/networks` for the network paths `.../mctp1/networks/<NetID>` and `.../mctp1/networks/<NetID>/endpoints` for the endpoints path `.../mctp1/networks/<NetID>/endpoints/<EID>`. Tested: Check the MCTP D-Bus interface: ``` busctl tree au.com.codeconstruct.MCTP1 `- /au `- /au/com `- /au/com/codeconstruct `- /au/com/codeconstruct/mctp1 `- /au/com/codeconstruct/mctp1/networks `- /au/com/codeconstruct/mctp1/networks/1 `- /au/com/codeconstruct/mctp1/networks/1/endpoints `- /au/com/codeconstruct/mctp1/networks/1/endpoints/8 ``` Signed-off-by: Thu Nguyen <[email protected]>

For upcoming mctpd changes, we will need a facility for storing dbus-specific information on each link. This change adds a void * userdata pointer to the links, and accessors for callers to get/set this pointer. For convenience, we also pass this pointer on change events, allowing for a quick way to release/update the userdata on link delete/change. WIP: untested. Signed-off-by: Jeremy Kerr <[email protected]>

src/mctp-netlink.c

src/mctp-netlink.h

src/mctpd.c

jk-ozlabs

Looking good! A couple of really minor things, but I think we're almost ready to go here. I'll start some testing on my side too.

docs/mctpd.md

src/mctpd.c

jk-ozlabs · 2024-06-16T03:45:41Z

A couple of new warnings have been introduced:

[19/23] Compiling C object mctpd.p/src_mctpd.c.o
../src/mctpd.c: In function ‘mctpd_dbus_enumerate’:
../src/mctpd.c:3363:40: warning: ‘rc’ may be used uninitialized [-Wmaybe-uninitialized]
 3363 |         for (size_t i = 0; i < num_ifs && rc == 0; i++) {
      |                            ~~~~~~~~~~~~^~~~~~~~~~
../src/mctpd.c: In function ‘emit_interface_removed’:
../src/mctpd.c:3301:12: warning: ‘rc’ may be used uninitialized [-Wmaybe-uninitialized]
 3301 |         if (rc < 0) {
      |            ^

but those should be straightforward fixes

ThuBaNguyen · 2024-06-16T04:50:55Z

A couple of new warnings have been introduced:

[19/23] Compiling C object mctpd.p/src_mctpd.c.o
../src/mctpd.c: In function ‘mctpd_dbus_enumerate’:
../src/mctpd.c:3363:40: warning: ‘rc’ may be used uninitialized [-Wmaybe-uninitialized]
 3363 |         for (size_t i = 0; i < num_ifs && rc == 0; i++) {
      |                            ~~~~~~~~~~~~^~~~~~~~~~
../src/mctpd.c: In function ‘emit_interface_removed’:
../src/mctpd.c:3301:12: warning: ‘rc’ may be used uninitialized [-Wmaybe-uninitialized]
 3301 |         if (rc < 0) {
      |            ^

but those should be straightforward fixes

I fixed it.

jk-ozlabs · 2024-06-17T01:43:59Z

Seems to be one more remaining though:

./src/mctpd.c: In function ‘emit_interface_removed’:
../src/mctpd.c:3299:12: warning: ‘rc’ may be used uninitialized [-Wmaybe-uninitialized]
 3299 |         if (rc < 0) {
      |            ^

1. Create the MCTP interfaces D-Bus objects for the existing MCTP links at `/xyz/openbmc_project/mctp1/interfaces/<name>`. When the MCTP links is removed from/added to the system, the D-Bus object will also be removed/added. 2. Create the au.com.CodeConstruct.MCTP.Link1 D-Bus interface for MCTP interface D-Bus objects. The D-Bus interface includes the `Role` property which reports BMC roles in the MCTP link. The possible value of `Role` are `BusOwner`, `Endpoint` and `Unknown`. 3. Because the BMC `Role` in the MCTP interface is fixed. The `Role` property is changeable value but it can only be changed when the current configured value is `Unknown`. Ex: ``` busctl tree au.com.codeconstruct.MCTP1 `- /au `- /au/com `- /au/com/codeconstruct `- /au/com/codeconstruct/mctp1 |- /au/com/codeconstruct/mctp1/interfaces | |- /au/com/codeconstruct/mctp1/interfaces/lo | `- /au/com/codeconstruct/mctp1/interfaces/mctpi2c3 `- /au/com/codeconstruct/mctp1/networks `- /au/com/codeconstruct/mctp1/networks/1 `- /au/com/codeconstruct/mctp1/networks/1/endpoints `- /au/com/codeconstruct/mctp1/networks/1/endpoints/8 busctl introspect au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/interfaces/mctpi2c3 NAME TYPE SIGNATURE RESULT/VALUE FLAGS au.com.CodeConstruct.MCTP.Link1 interface - - - .Role property s "Endpoint" emits-change writable org.freedesktop.DBus.Introspectable interface - - - .Introspect method - s - org.freedesktop.DBus.Peer interface - - - .GetMachineId method - s - .Ping method - - - org.freedesktop.DBus.Properties interface - - - .Get method ss v - .GetAll method s a{sv} - .Set method ssv - - .PropertiesChanged signal sa{sv}as - - busctl set-property au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/interfaces/mctpi2c3 au.com.CodeConstruct.MCTP.Link1 Role s BusOwner busctl get-property au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/interfaces/mctpi2c3 au.com.CodeConstruct.MCTP.Link1 Role s "BusOwner" ``` Signed-off-by: Thu Nguyen <[email protected]>

ThuBaNguyen · 2024-06-17T02:48:22Z

Seems to be one more remaining though:

./src/mctpd.c: In function ‘emit_interface_removed’:
../src/mctpd.c:3299:12: warning: ‘rc’ may be used uninitialized [-Wmaybe-uninitialized]
 3299 |         if (rc < 0) {
      |            ^

I updated code.

jk-ozlabs · 2024-06-29T10:25:48Z

So this PR got auto-closed on merge of the pre-req dbus-rework branch, but we still do want the change. I have rebased the remaining change and created #44 to continue the interface objects work.

jk-ozlabs · 2024-06-29T10:26:54Z

Ah, seems like the auto-close was because the dbus-rework branch was the target for this PR. We want your branch merged into main :)

jk-ozlabs added 2 commits February 6, 2024 17:03

lib/tomlc99: Import tomlc99 TOML parser

a833b71

Import a copy of the tomlc99 parser, somewhat temporarily, from upstream commit 5221b3d. We import the toml.[ch] sources, the LICENSE, and add a short readme for the upstream reference. Signed-off-by: Jeremy Kerr <[email protected]>

jk-ozlabs added the enhancement New feature or request label May 20, 2024

jk-ozlabs mentioned this pull request May 28, 2024

dbus interface redesign #40

Closed

jk-ozlabs reviewed May 28, 2024

View reviewed changes

docs/mctpd.md Outdated Show resolved Hide resolved

src/mctp-netlink.h Outdated Show resolved Hide resolved

src/mctpd.c Outdated Show resolved Hide resolved

src/mctpd.c Outdated Show resolved Hide resolved

mctpd: attempt a default configuration from /etc/mctpd.conf

947205f

Rather than requiring a -c FILE argument, allow a default configuration file of /etc/mctpd.conf. We don't error if this is absent, to allow running with the existing defaults. Signed-off-by: Jeremy Kerr <[email protected]>

ThuBaNguyen force-pushed the add-mctp-link branch from e280eed to 3f01e54 Compare June 4, 2024 13:54

ThuBaNguyen changed the base branch from main to dev/dbus-rework June 4, 2024 13:56

ThuBaNguyen force-pushed the add-mctp-link branch from 3f01e54 to b9b32f6 Compare June 4, 2024 14:00

jk-ozlabs reviewed Jun 6, 2024

View reviewed changes

jk-ozlabs added 4 commits June 6, 2024 13:16

mctp-util: move ARRAY_SIZE, min() and max() to utils header

e01d74d

We have these repeated, move to a common location. Signed-off-by: Jeremy Kerr <[email protected]>

mctpd: use .DRAFT suffix on BusOwner1 interface

9ed35ed

We'll be modifying this for the new Role-based interfaces which exist on the link/interface objects, so use a temporary name for now. Signed-off-by: Jeremy Kerr <[email protected]>

jk-ozlabs force-pushed the dev/dbus-rework branch from 99b84c3 to 9ed35ed Compare June 6, 2024 05:21

ThuBaNguyen force-pushed the add-mctp-link branch 2 times, most recently from fd3dd47 to 671d0cb Compare June 7, 2024 04:35

jk-ozlabs reviewed Jun 7, 2024

View reviewed changes

src/mctpd.c Outdated Show resolved Hide resolved

jk-ozlabs reviewed Jun 7, 2024

View reviewed changes

src/mctpd.c Outdated Show resolved Hide resolved

ThuBaNguyen force-pushed the add-mctp-link branch 2 times, most recently from 9901b83 to ee3ea15 Compare June 11, 2024 12:42

ThuBaNguyen and others added 2 commits June 12, 2024 17:03

ThuBaNguyen force-pushed the add-mctp-link branch from ee3ea15 to 2bc1292 Compare June 12, 2024 17:04