expose PMBus device info from build-i2c codegen; set IS_PMBUS

[hawkw]

This branch consists of three separate commits which build upon each other, and which I would like to rebase-merge as three separate commits to master. Overall, the primary goal here is to start populating the IS_PMBUS bit in the control-plane-agent inventory's DeviceCapabilities, which was added in oxidecomputer/management-gateway-service#488. In addition, I also tried to do some prep for future changes that want to generate more code for all PMBus devices, such as what @jamesmunns is working on in #2463.

In particular, the three commits do the following:

1. Update the management-gateway-service dep to pick up the new DeviceCapabilities flags

2. Add code in build-i2c's public API that is exposed to other build scripts (the device_descriptions() API) to indicate which devices are PMBus devices. In particular, I2cDeviceDescription now includes an optional PmbusDeviceDescription field, which is Some for PMBus devices. The presence of this field can be used to determine when a device is a PMBus device, such as when determining if the IS_PMBUS capability should be advertised.

   I did this by adding a whole new optional struct rather than just sticking an is_pmbus bool on I2cDeviceDescription, because I felt like future code, such as what @jamesmunns is working on for want to read PMBus status registers via MGS #2463, will almost certainly want a way to get a PMBus device's rail and phase names in codegen that lives outside of build-i2c (i.e. in
   task-validate-api's build script). Currently, there isn't a nice way
   to get this stuff in codegen that lives outside of build-i2c, so this
   should help make that nicer.

   Then, I've modified the codegen in task-validate-api, which is what control-plane-agent ultimately consumes for its codegen inventory devices, to include an is_pmbus field so that we can use this to populate the DeviceCapabilities flag.

3. Finally, actually set the flag in control-plane-agent. :)

[jamesmunns]

nit: Might be worth hoisting this up to the top level, and doing one arm for "has phases" and one for "doesn't have phases"? Then we could do a single assert for "lengths match", and then zip together rails and phases_list, instead of doing .get/indexing by i.

[hawkw]

hmm, so that ends up looking like this:

diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs
index 3ca2e1487..a4201fe57 100644
--- a/build/i2c/src/lib.rs
+++ b/build/i2c/src/lib.rs
@@ -1827,35 +1827,29 @@ pub fn device_descriptions() -> impl Iterator<Item = I2cDeviceDescription> {
                     return None;
                 }

-                let rails = power
-                    .rails
-                    .iter()
-                    .flatten()
-                    .enumerate()
-                    .map(|(i, rail)| {
-                        let phases = if let Some(ref phases_list) = power.phases
-                        {
-                            match phases_list.get(i) {
-                                Some(phases) => phases.clone(),
-                                None => {
-                                    // The rails and phases arrays are expected to
-                                    // be the same length; if this is not the case,
-                                    // the config file is malformed!
-                                    panic!(
-                                        "PMBus device {device_id:?} is missing
-                                         phases for its {i}th rail ({rail})",
-                                    );
-                                }
-                            }
-                        } else {
-                            Vec::new()
-                        };
-                        PmbusRailDescription {
-                            name: rail.clone(),
-                            phases,
-                        }
-                    })
-                    .collect::<Vec<_>>();
+                let rails =  power.rails.as_ref().map(|rails| {
+                    if let Some(ref phases) = power.phases {
+                        assert_eq!(rails.len(), phases.len(),
+                            "PMBus device {device_id:?}'s `power.rails` and \
+                             `power.phases` lists are not the same length"
+                        );
+                        rails.iter()
+                            .cloned()
+                            .zip(phases.iter().cloned())
+                            .map(|(name, phases)| PmbusRailDescription {
+                                name,
+                                phases,
+                            })
+                            .collect()
+                    } else {
+                        rails.iter().cloned()
+                            .map(|name| PmbusRailDescription {
+                                name,
+                                phases: Vec::new(),
+                            })
+                            .collect()
+                    }
+                }).unwrap_or_default();
                 Some(PmbusDeviceDescription { rails })
             });

which, incidentally, rustfmt decided to kinda give up on lol. Do you think that's nicer? because I'm happy to change it if you do...

[jamesmunns]

Here's my take on it, and I actually think this makes one thing a bit more clear: what we do when EITHER rails or power vecs are missing (right now unless we have both we return Some(vec![]):

diff --git a/build/i2c/src/lib.rs b/build/i2c/src/lib.rs
index 3ca2e1487..f6dcd0b43 100644
--- a/build/i2c/src/lib.rs
+++ b/build/i2c/src/lib.rs
@@ -1827,35 +1827,31 @@ pub fn device_descriptions() -> impl Iterator<Item = I2cDeviceDescription> {
                     return None;
                 }
 
-                let rails = power
-                    .rails
+                // If we're missing rails OR phases, we have no rails.
+                //
+                // TODO: Do we want to match on this? Are we actually fine with 3/4 match arms just
+                // giving up and returning `Some(PmbusDeviceDescription { rails: vec![] })`?
+                let (Some(rails), Some(phases)) = (power.rails.as_ref(), power.phases.as_ref()) else {
+                    return Some(PmbusDeviceDescription { rails: vec![] })
+                };
+
+                // If we have rails and phases, they MUST match each other's lengths
+                assert_eq!(
+                    rails.len(),
+                    phases.len(),
+                    "PMBus device {device_id:?} has mismatched rails and phases",
+                );
+
+                // A phase for every rail, and every rail in its phase
+                let rails = rails
                     .iter()
-                    .flatten()
-                    .enumerate()
-                    .map(|(i, rail)| {
-                        let phases = if let Some(ref phases_list) = power.phases
-                        {
-                            match phases_list.get(i) {
-                                Some(phases) => phases.clone(),
-                                None => {
-                                    // The rails and phases arrays are expected to
-                                    // be the same length; if this is not the case,
-                                    // the config file is malformed!
-                                    panic!(
-                                        "PMBus device {device_id:?} is missing
-                                         phases for its {i}th rail ({rail})",
-                                    );
-                                }
-                            }
-                        } else {
-                            Vec::new()
-                        };
-                        PmbusRailDescription {
-                            name: rail.clone(),
-                            phases,
-                        }
+                    .zip(phases.iter())
+                    .map(|(r, p)| PmbusRailDescription {
+                        name: r.clone(),
+                        phases: p.clone(),
                     })
-                    .collect::<Vec<_>>();
+                    .collect();
+
                 Some(PmbusDeviceDescription { rails })
             });

Less nested:

// If we're missing rails OR phases, we have no rails.
//
// TODO: Do we want to match on this? Are we actually fine with 3/4 match arms just
// giving up and returning `Some(PmbusDeviceDescription { rails: vec![] })`?
let (Some(rails), Some(phases)) = (power.rails.as_ref(), power.phases.as_ref()) else {
    return Some(PmbusDeviceDescription { rails: vec![] })
};

// If we have rails and phases, they MUST match each other's lengths
assert_eq!(
    rails.len(),
    phases.len(),
    "PMBus device {device_id:?} has mismatched rails and phases",
);

// A phase for every rail, and every rail in its phase
let rails = rails
    .iter()
    .zip(phases.iter())
    .map(|(r, p)| PmbusRailDescription {
        name: r.clone(),
        phases: p.clone(),
    })
    .collect();

Some(PmbusDeviceDescription { rails })

[hawkw]

Hm, your code snippet implements different (and in this case, incorrect) behavior than what mine does. Not all devices have multiple phases, some devices have a list of rails and no corresponding list of phases. The requirement is that if there is a list of phases, it must be as long as the list of rails.

[mkeeter]

@hawkw This generally looks good; can you do a smoke test to make sure this didn't break humility validate or faux-mgs inventory?

[hawkw]

what the heck happened here: https://github.com/oxidecomputer/hubris/actions/runs/26239754200/job/77222643916?pr=2519#step:4:288

[jamesmunns]

what the heck happened here: https://github.com/oxidecomputer/hubris/actions/runs/26239754200/job/77222643916?pr=2519#step:4:288

I've been seeing these pretty often in other PRs. I'm not sure if it's some kind of cache failure, a build getting cancelled when it shouldn't, or just GitHub jank. It's starting to happen often enough to worry about imo.

[hawkw]

sigh...

[hawkw]

@mkeeter well, humility validate works fine, at least:

eliza@alfred ~ $ pfexec humility flash
humility: WARNING: archive in environment variable overriding archive in environment file
humility: attached to archive
humility: attaching with chip set to "STM32H753ZITx"
humility: attached to 1fc9:0143:XJACFXSEKTQJS via CMSIS-DAP
humility: flash/archive mismatch; reflashing
humility: skipping measurement token handoff
humility: erasing slot 12
humility: done
humility: skipping measurement token handoff
humility: resetting the SP, please wait...
humility: done with auxiliary flash
humility: flashing done
eliza@alfred ~ $ pfexec humility validate
humility: WARNING: archive in environment variable overriding archive in environment file
humility: attached to 1fc9:0143:XJACFXSEKTQJS via CMSIS-DAP
ID VALIDATION   C P  MUX ADDR DEVICE        DESCRIPTION
 0 present      1 B  -   0x70 oximux16      Front FPGA virtual mux
 1 validated    1 B  1:1 0x50 at24csw080    U.2 Sharkfin A VPD
 2 validated    1 B  1:1 0x38 max5970       U.2 Sharkfin A hot swap controller
 3 removed      1 B  1:1 0x6a nvme_bmc      U.2 A NVMe Basic Management Command
 4 validated    1 B  1:2 0x50 at24csw080    U.2 Sharkfin B VPD
 5 validated    1 B  1:2 0x38 max5970       U.2 Sharkfin B hot swap controller
 6 removed      1 B  1:2 0x6a nvme_bmc      U.2 B NVMe Basic Management Control
 7 validated    1 B  1:3 0x50 at24csw080    U.2 Sharkfin C VPD
 8 validated    1 B  1:3 0x38 max5970       U.2 Sharkfin C hot swap controller
 9 removed      1 B  1:3 0x6a nvme_bmc      U.2 C NVMe Basic Management Control
10 validated    1 B  1:4 0x50 at24csw080    U.2 Sharkfin D VPD
11 validated    1 B  1:4 0x38 max5970       U.2 Sharkfin D hot swap controller
12 removed      1 B  1:4 0x6a nvme_bmc      U.2 D NVMe Basic Management Control
13 validated    1 B  1:5 0x50 at24csw080    U.2 Sharkfin E VPD
14 validated    1 B  1:5 0x38 max5970       U.2 Sharkfin E hot swap controller
15 removed      1 B  1:5 0x6a nvme_bmc      U.2 E NVMe Basic Management Control
16 validated    1 B  1:6 0x50 at24csw080    U.2 Sharkfin F VPD
17 validated    1 B  1:6 0x38 max5970       U.2 Sharkfin F hot swap controller
18 removed      1 B  1:6 0x6a nvme_bmc      U.2 F NVMe Basic Management Control
19 validated    1 B  1:9 0x50 at24csw080    U.2 Sharkfin G VPD
20 validated    1 B  1:9 0x38 max5970       U.2 Sharkfin G hot swap controller
21 removed      1 B  1:9 0x6a nvme_bmc      U.2 G NVMe Basic Management Control
22 validated    1 B  1:10 0x50 at24csw080    U.2 Sharkfin H VPD
23 validated    1 B  1:10 0x38 max5970       U.2 Sharkfin H hot swap controller
24 removed      1 B  1:10 0x6a nvme_bmc      U.2 H NVMe Basic Management Control
25 validated    1 B  1:11 0x50 at24csw080    U.2 Sharkfin I VPD
26 validated    1 B  1:11 0x38 max5970       U.2 Sharkfin I hot swap controller
27 removed      1 B  1:11 0x6a nvme_bmc      U.2 I NVMe Basic Management Control
28 validated    1 B  1:12 0x50 at24csw080    U.2 Sharkfin J VPD
29 validated    1 B  1:12 0x38 max5970       U.2 Sharkfin J hot swap controller
30 removed      1 B  1:12 0x6a nvme_bmc      U.2 J NVMe Basic Management Control
31 validated    1 B  1:13 0x48 tmp117        Southwest temperature sensor
32 validated    1 B  1:13 0x49 tmp117        South temperature sensor
33 validated    1 B  1:13 0x4a tmp117        Southeast temperature sensor
34 present      2 F  -   0x70 oximux16      Main FPGA virtual mux
35 removed      2 F  1:1 0x6a nvme_bmc      M.2 A NVMe Basic Management Command
36 removed      2 F  1:2 0x6a nvme_bmc      M.2 B NVMe Basic Management Command
37 validated    2 F  1:4 0x3c sbrmi         CPU via SB-RMI
38 validated    2 F  1:4 0x4c sbtsi         CPU temperature sensor
39 validated    2 F  1:7 0x50 at24csw080    Fan VPD
40 validated    2 F  1:8 0x4c tmp451        T6 temperature sensor
41 validated    3 H  -   0x24 tps546b24a    A2 3.3V rail
42 validated    3 H  -   0x27 tps546b24a    A2 5V rail
43 validated    3 H  -   0x29 tps546b24a    A2 1.8V rail
44 validated    3 H  -   0x3a max5970       M.2 hot plug controller
45 validated    3 H  -   0x54 ltc4282       12V MCIO hot plug controller
46 validated    3 H  -   0x56 ltc4282       DIMM GHIJKL hot plug controller
47 validated    3 H  -   0x55 ltc4282       DIMM ABCDEF hot plug controller
48 validated    3 H  -   0x75 raa229620a    South power controller (Core 0, SOC)
49 validated    3 H  -   0x76 raa229620a    North power controller (Core 1, VDDIO)
50 validated    3 H  -   0x5c isl68224      SP5 power controller (V1P1, V1P8, V3P3)
51 validated    4 F  -   0x39 max5970       NIC hot swap
52 validated    4 F  -   0x25 tps546b24a    T6 power controller
53 validated    4 F  -   0x48 tmp117        Northwest temperature sensor
54 validated    4 F  -   0x49 tmp117        North temperature sensor
55 validated    4 F  -   0x4a tmp117        Northeast temperature sensor
56 validated    4 F  -   0x20 max31790      Fan controller
57 validated    4 F  -   0x67 bmr491        Intermediate bus converter
58 validated    4 F  -   0x50 at24csw080    Cosmo VPD
59 validated    4 F  -   0x11 lm5066i       Fan hot swap controller (east)
60 validated    4 F  -   0x12 lm5066i       Fan hot swap controller (central)
61 validated    4 F  -   0x13 lm5066i       Fan hot swap controller (west)
62 validated    4 F  -   0x14 adm127x       Sled hot swap controller
eliza@alfred ~ $

I haven't yet gotten as far as testing that MGS doesn't get sad upon discovering the new bits (which I sure hope it wouldn't), since the only lab systems that have management network are currently busy doing other things again (cf oxidecomputer/meta#984). I'll see if I can get ahold of something to test against...

[jamesmunns]

Technically you want an extra newline before this, or you could change line 68 to a writeln!

DeviceDescription {
    device: "at24csw080",
    description: "U.2 Sharkfin A VPD",
    id: [74, 50, 48, 48, 47, 85, 50, 0, 0, 0, 0, 0, 0, 0, 0, 0],        is_pmbus: false,
    sensors: &[
    ],
},

[hawkw]

ew, whoops

[hawkw]

turns out this was, amusingly, a preexisting bug --- the bit that writes out the device ID was using write! rather than writeln!. before this PR it would have been sensors: &[ that would be jammed at the end of the line.

in any case, i fixed it.

[hawkw]

Okay, so it turns out that faux-mgs actually just prints DeviceCapabilities as an integer and doesn't actually try to interpret the bitflags. So, good news is, it works!

eliza@jeeves ~ $ faux-mgs --interface e1000g0 --discovery-addr '[fe80::aa40:25ff:fe04:11c5]:11111' inventory
May 26 17:04:06.849 INFO creating SP handle on interface e1000g0, component: faux-mgs
May 26 17:04:06.881 INFO initial discovery complete, addr: [fe80::aa40:25ff:fe04:11c5%2]:11111, interface: e1000g0, socket: control-plane-agent, component: faux-mgs
COMPONENT        STATUS       DEVICE           DESCRIPTION (CAPABILITIES)
sp               Present      sp               Service Processor (DeviceCapabilities(1))
sp-aux-flash     Present      sp-aux-flash     Service Processor auxiliary flash (DeviceCapabilities(0))
sp5-host-cpu     Present      sp5-host-cpu     Cosmo SP5 host cpu (DeviceCapabilities(4))
sp5-post-codes   Present      sp5-post-codes   Cosmo SP5 POST code buffer (DeviceCapabilities(0))
host-boot-flash  Present      host-boot-flash  Cosmo host boot flash (DeviceCapabilities(1))
host-boot-apob   Present      host-boot-apob   Cosmo host boot APOB region (DeviceCapabilities(0))
system-led       Present      system-led       System attention LED (DeviceCapabilities(8))
U31              Present      oximux16         Front FPGA virtual mux (DeviceCapabilities(0))
J200/U2          Present      at24csw080       U.2 Sharkfin A VPD (DeviceCapabilities(0))
J200/U1          Present      max5970          U.2 Sharkfin A hot swap controller (DeviceCapabilities(2))
J200/J2          NotPresent   nvme_bmc         U.2 A NVMe Basic Management Command (DeviceCapabilities(2))
J201/U2          Present      at24csw080       U.2 Sharkfin B VPD (DeviceCapabilities(0))
J201/U1          Present      max5970          U.2 Sharkfin B hot swap controller (DeviceCapabilities(2))
J201/J2          NotPresent   nvme_bmc         U.2 B NVMe Basic Management Control (DeviceCapabilities(2))
J202/U2          Present      at24csw080       U.2 Sharkfin C VPD (DeviceCapabilities(0))
J202/U1          Present      max5970          U.2 Sharkfin C hot swap controller (DeviceCapabilities(2))
J202/J2          NotPresent   nvme_bmc         U.2 C NVMe Basic Management Control (DeviceCapabilities(2))
J203/U2          Present      at24csw080       U.2 Sharkfin D VPD (DeviceCapabilities(0))
J203/U1          Present      max5970          U.2 Sharkfin D hot swap controller (DeviceCapabilities(2))
J203/J2          NotPresent   nvme_bmc         U.2 D NVMe Basic Management Control (DeviceCapabilities(2))
J204/U2          Present      at24csw080       U.2 Sharkfin E VPD (DeviceCapabilities(0))
J204/U1          Present      max5970          U.2 Sharkfin E hot swap controller (DeviceCapabilities(2))
J204/J2          NotPresent   nvme_bmc         U.2 E NVMe Basic Management Control (DeviceCapabilities(2))
J205/U2          Present      at24csw080       U.2 Sharkfin F VPD (DeviceCapabilities(0))
J205/U1          Present      max5970          U.2 Sharkfin F hot swap controller (DeviceCapabilities(2))
J205/J2          NotPresent   nvme_bmc         U.2 F NVMe Basic Management Control (DeviceCapabilities(2))
J206/U2          Present      at24csw080       U.2 Sharkfin G VPD (DeviceCapabilities(0))
J206/U1          Present      max5970          U.2 Sharkfin G hot swap controller (DeviceCapabilities(2))
J206/J2          NotPresent   nvme_bmc         U.2 G NVMe Basic Management Control (DeviceCapabilities(2))
J207/U2          Present      at24csw080       U.2 Sharkfin H VPD (DeviceCapabilities(0))
J207/U1          Present      max5970          U.2 Sharkfin H hot swap controller (DeviceCapabilities(2))
J207/J2          NotPresent   nvme_bmc         U.2 H NVMe Basic Management Control (DeviceCapabilities(2))
J208/U2          Present      at24csw080       U.2 Sharkfin I VPD (DeviceCapabilities(0))
J208/U1          Present      max5970          U.2 Sharkfin I hot swap controller (DeviceCapabilities(2))
J208/J2          NotPresent   nvme_bmc         U.2 I NVMe Basic Management Control (DeviceCapabilities(2))
J209/U2          Present      at24csw080       U.2 Sharkfin J VPD (DeviceCapabilities(0))
J209/U1          Present      max5970          U.2 Sharkfin J hot swap controller (DeviceCapabilities(2))
J209/J2          NotPresent   nvme_bmc         U.2 J NVMe Basic Management Control (DeviceCapabilities(2))
J44/U1           Present      tmp117           Southwest temperature sensor (DeviceCapabilities(2))
J45/U1           Present      tmp117           South temperature sensor (DeviceCapabilities(2))
J46/U1           Present      tmp117           Southeast temperature sensor (DeviceCapabilities(2))
U27/MUX          Present      oximux16         Main FPGA virtual mux (DeviceCapabilities(0))
J210             NotPresent   nvme_bmc         M.2 A NVMe Basic Management Command (DeviceCapabilities(2))
J211             NotPresent   nvme_bmc         M.2 B NVMe Basic Management Command (DeviceCapabilities(2))
U1/SBRMI         Present      sbrmi            CPU via SB-RMI (DeviceCapabilities(0))
U1/SBTSI         Present      sbtsi            CPU temperature sensor (DeviceCapabilities(2))
J34/U1           Present      at24csw080       Fan VPD (DeviceCapabilities(0))
U53              Present      tmp451           T6 temperature sensor (DeviceCapabilities(2))
U82              Present      tps546b24a       A2 3.3V rail (DeviceCapabilities(34))
U83              Present      tps546b24a       A2 5V rail (DeviceCapabilities(34))
U81              Present      tps546b24a       A2 1.8V rail (DeviceCapabilities(34))
U15              Present      max5970          M.2 hot plug controller (DeviceCapabilities(2))
U16              Present      ltc4282          12V MCIO hot plug controller (DeviceCapabilities(2))
U42              Present      ltc4282          DIMM GHIJKL hot plug controller (DeviceCapabilities(2))
U127             Present      ltc4282          DIMM ABCDEF hot plug controller (DeviceCapabilities(2))
U90              Present      raa229620a       South power controller (Core 0, SOC) (DeviceCapabilities(34))
U103             Present      raa229620a       North power controller (Core 1, VDDIO) (DeviceCapabilities(34))
U116             Present      isl68224         SP5 power controller (V1P1, V1P8, V3P3) (DeviceCapabilities(34))
U54              Present      max5970          NIC hot swap (DeviceCapabilities(2))
U123             Present      tps546b24a       T6 power controller (DeviceCapabilities(34))
J47/U1           Present      tmp117           Northwest temperature sensor (DeviceCapabilities(2))
J48/U1           Present      tmp117           North temperature sensor (DeviceCapabilities(2))
J49/U1           Present      tmp117           Northeast temperature sensor (DeviceCapabilities(2))
U58              Present      max31790         Fan controller (DeviceCapabilities(2))
U80              Present      bmr491           Intermediate bus converter (DeviceCapabilities(34))
U32              Present      at24csw080       Cosmo VPD (DeviceCapabilities(0))
U71              Present      lm5066i          Fan hot swap controller (east) (DeviceCapabilities(34))
U72              Present      lm5066i          Fan hot swap controller (central) (DeviceCapabilities(34))
U73              Present      lm5066i          Fan hot swap controller (west) (DeviceCapabilities(34))
U79              Present      adm127x          Sled hot swap controller (DeviceCapabilities(34))
eliza@jeeves ~ $

Somehow I had gotten it into my head that there was ... something ... that was printing the actual bits. I think the current behavior of the bitflags! macro is that it will deserialize even if it has unrecognized bits, so I think this is fine.

[hawkw]

And, we can see that the various PMBus devices here now have DeviceCapabilities(34); 0d34 == 0b100010, which means that they now correctly have bit 5 (IS_PMBUS) set, as well as bit 2 (HAS_MEASUREMENT_CHANNELS, which they had previously). So it works!