Support `LocaEids` property in `au.com.codeconstruct.MCTP.Network1` #54

ThuBaNguyen · 2024-11-15T02:18:11Z

Add code to show the local EIDs in EIDs property in au.com.CodeConstruct.MCTP.Interface1 D-Bus interface.

busctl introspect au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/interfaces/mctpi2c3
NAME                                 TYPE      SIGNATURE RESULT/VALUE FLAGS
au.com.CodeConstruct.MCTP.Interface1 interface -         -            -
.EIDs                                property  ay        1 8          const

amboar · 2024-11-18T00:03:37Z

I'm not trying to block the change here, but I feel we might want a holistic treatment of Figure 9 from DSP0236 v1.3.3. I feel that requires a bit more thought about how EID associations are represented. I don't think the proposal is wrong as such, but it is incomplete.

jk-ozlabs · 2024-11-18T09:16:47Z

From some chatting on Discord, my suggested approach was that we have an EIDs property on the network object, where an application can always pick the first to acquire a suitable local EID that can route to the local stack. Since the host will accept any local destination EID, it will always be valid on any incoming interface.

(the extension of that concept is that we may as well assign the same local EID to all interfaces, but that's entirely optional)

We may have a scenario in future where an application may need to query a local EID that is bound to a specific interface, but I don't think that's what's happening here.

ThuBaNguyen · 2024-11-18T12:38:11Z

From some chatting on Discord, my suggested approach was that we have an EIDs property on the network object, where an application can always pick the first to acquire a suitable local EID that can route to the local stack.

It seems I missed your idea in the Discord Chat. I thought we will add the EIDs property to au.com.CodeConstruct.MCTP.Interface1 D-Bus interface of interface D-Bus object such as /au/com/codeconstruct/mctp1/interfaces/mctpi2c3

In case, we want to add the EIDs property to network object path such as /au/com/codeconstruct/mctp1/networks/1, I wonder which D-Bus interface should we use to add EIDs property?

root@mtmitchell-dcscm:~# busctl introspect au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/networks/1
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  -            -

Since the host will accept any local destination EID, it will always be valid on any incoming interface.

I'm agree with you that when the interfaces are belong to the same network, this will be true. Although, this will make the routing table in both BMC, the bridge between BMC and terminus (if have) and the terminus are more complicated.
Ampere team is still evaluating this solution. I will give more opinions in this solution later.

(the extension of that concept is that we may as well assign the same local EID to all interfaces, but that's entirely optional)

This option can only be applied when BMC is BO in all interfaces.

We may have a scenario in future where an application may need to query a local EID that is bound to a specific interface, but I don't think that's what's happening here.

ThuBaNguyen · 2024-12-02T23:52:41Z

From some chatting on Discord, my suggested approach was that we have an EIDs property on the network object, where an application can always pick the first to acquire a suitable local EID that can route to the local stack. Since the host will accept any local destination EID, it will always be valid on any incoming interface.

What do you think about adding the D-Bus interface "au.com.CodeConstruct.MCTP.Network1" in network object path as below

 busctl introspect au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/networks/1
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  -            -
au.com.CodeConstruct.MCTP.Network1  interface -         -                                      -
.EIDs                               property  ay        2 8 12                                    const

Where:

au.com.CodeConstruct.MCTP.Network1 is interface name. 1 is version.
There is one properties in the network D-Bus interfaces:
a) EIDs will be BMC local EIDs in the network 1 as we discuss before.

jk-ozlabs · 2024-12-04T05:30:32Z

Looks good, but maybe make it obvious that the EIDs are local to this endpoint? we could name it LocalEIDs, perhaps?

ThuBaNguyen · 2024-12-04T07:12:05Z

Looks good, but maybe make it obvious that the EIDs are local to this endpoint? we could name it LocalEIDs, perhaps?

Sure. LocalEIDs is more details property name.

jk-ozlabs · 2024-12-04T08:02:18Z

ok, super. Also, just check the capitalisation on the interface name.

src/mctpd.c

jk-ozlabs · 2024-12-20T04:50:39Z

src/mctpd.c

+	}
+
+	return ret_eids;
+}


You have essentially the same logic repeated here. Just return the count and array from the one function.

This has been re-introduced in the recent changes; does this iteration code need to appear twice?

Yes.

We need to know the number of localEIDs first, then allocate ret_eids with number of localEIDs.
In the commit 17adff2, I tried to change combine find_local_eids_count_by_net and find_local_eids_by_net but eids is null event num is 1 in sd_bus_message_append_array(reply, 'y', eids, num);. That why I back to use two functions.

Using two functions also makes the code is clearer.

Using two functions also makes the code is clearer.

I disagree. You already have divergence between the two implementations (code formatting), as well as a bug duplicated in both (&ctx->peers[x] can never be null)

The number of local EIDs is fairly well constrained; does this even need to be calculated in advance? just malloc the whole 256 bytes, and iterate once.

but eids is null event num is 1 in sd_bus_message_append_array(reply, 'y', eids, num);

not sure I understand this, can you rephrase?

The number of local EIDs is fairly well constrained; does this even need to be calculated in advance? just malloc the whole 256 bytes, and iterate once.

I updated the latest code by malloc the localEIDs with 256 bytes as your suggest.
edc3f5f
Only need one functions and iterate once.

src/mctpd.c

jk-ozlabs

Looks good. Just a wording change on the doc addition, but I can edit that as part of the merge (unless you happen to beat me to it!)

Thanks for the contribution!

docs/mctpd.md

ThuBaNguyen · 2025-01-07T06:34:33Z

The PULL is WIP

jk-ozlabs · 2025-01-07T06:36:57Z

OK, I'll wait on updates then!

jk-ozlabs · 2025-01-07T06:38:19Z

OK, if you're updating, then a couple of build warnings present. You may already be working on these, but just in case not:

[20/24] Compiling C object test-mctpd.p/src_mctpd.c.o
../src/mctpd.c: In function ‘bus_network_get_prop’:
../src/mctpd.c:3047:22: warning: ‘eids’ may be used uninitialized [-Wmaybe-uninitialized]
 3047 |                 rc = find_local_eids_by_net(ctx, netid, &num, eids);
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/mctpd.c:3039:21: note: ‘eids’ was declared here
 3039 |         mctp_eid_t *eids;
      |                     ^~~~
[21/24] Compiling C object mctpd.p/src_mctpd.c.o
../src/mctpd.c: In function ‘bus_network_get_prop’:
../src/mctpd.c:3047:22: warning: ‘eids’ may be used uninitialized [-Wmaybe-uninitialized]
 3047 |                 rc = find_local_eids_by_net(ctx, netid, &num, eids);
      |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/mctpd.c:3039:21: note: ‘eids’ was declared here
 3039 |         mctp_eid_t *eids;
      |                     ^~~~

ThuBaNguyen · 2025-01-07T08:27:38Z

The code is good now. I tested the code.

Add code to show the local EIDs in `LocalEIDs` property in `au.com.codeconstruct.MCTP.Network1` D-Bus interface of the D-Bus object path `/au/com/codeconstruct/mctp1/networks/<netId>`. ``` busctl introspect au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1/networks/1 NAME TYPE SIGNATURE RESULT/VALUE FLAGS au.com.codeconstruct.MCTP.Network1 interface - - - .LocalEIDs property ay 1 8 const org.freedesktop.DBus.Introspectable interface - - - .Introspect method - s - ``` Signed-off-by: Thu Nguyen <thu@os.amperecomputing.com>

jk-ozlabs

Looks good, thanks!

jk-ozlabs · 2025-01-08T04:29:05Z

Merged as 6470f9e , along with a set of tests for the dbus interface.

I had fixed up the commit title (LocaEIDs -> LocalEIDs), so GH isn't autoclosing this.

Thanks for the contribution!

ThuBaNguyen force-pushed the local-eids branch 2 times, most recently from cd541d6 to 41dd42a Compare December 8, 2024 07:17

ThuBaNguyen force-pushed the local-eids branch from 41dd42a to 7515c3f Compare December 17, 2024 07:56

ThuBaNguyen changed the title ~~mctpd: Add EIDs property to show local EIDs~~ Support LocaEids property in au.com.codeconstruct.MCTP.Network1 Dec 17, 2024

jk-ozlabs reviewed Dec 20, 2024

View reviewed changes

ThuBaNguyen force-pushed the local-eids branch 2 times, most recently from b8c4619 to bb6d0c9 Compare January 7, 2025 05:21

jk-ozlabs approved these changes Jan 7, 2025

View reviewed changes

docs/mctpd.md Outdated Show resolved Hide resolved

ThuBaNguyen marked this pull request as draft January 7, 2025 06:34

jk-ozlabs self-requested a review January 7, 2025 06:38

ThuBaNguyen force-pushed the local-eids branch 2 times, most recently from c0491a3 to 17adff2 Compare January 7, 2025 08:03

ThuBaNguyen marked this pull request as ready for review January 7, 2025 08:08

ThuBaNguyen force-pushed the local-eids branch 2 times, most recently from 2b795f6 to 47880d1 Compare January 7, 2025 08:23

ThuBaNguyen force-pushed the local-eids branch from 47880d1 to edc3f5f Compare January 7, 2025 09:39

jk-ozlabs approved these changes Jan 8, 2025

View reviewed changes

jk-ozlabs closed this Jan 8, 2025

Support LocaEids property in au.com.codeconstruct.MCTP.Network1 #54

Support LocaEids property in au.com.codeconstruct.MCTP.Network1 #54

Uh oh!

Conversation

ThuBaNguyen commented Nov 15, 2024

Uh oh!

amboar commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jk-ozlabs commented Nov 18, 2024

Uh oh!

ThuBaNguyen commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThuBaNguyen commented Dec 2, 2024

Uh oh!

jk-ozlabs commented Dec 4, 2024

Uh oh!

ThuBaNguyen commented Dec 4, 2024

Uh oh!

jk-ozlabs commented Dec 4, 2024

Uh oh!

Uh oh!

jk-ozlabs Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

jk-ozlabs Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

ThuBaNguyen Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

ThuBaNguyen Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

jk-ozlabs Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

ThuBaNguyen Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThuBaNguyen commented Jan 7, 2025

Uh oh!

jk-ozlabs commented Jan 7, 2025

Uh oh!

jk-ozlabs commented Jan 7, 2025

Uh oh!

ThuBaNguyen commented Jan 7, 2025

Uh oh!

jk-ozlabs left a comment

Choose a reason for hiding this comment

Uh oh!

jk-ozlabs commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Support `LocaEids` property in `au.com.codeconstruct.MCTP.Network1` #54

Support `LocaEids` property in `au.com.codeconstruct.MCTP.Network1` #54

amboar commented Nov 18, 2024 •

edited

Loading

ThuBaNguyen commented Nov 18, 2024 •

edited

Loading