Apcb::load signature is different depending on whether we are in no_std or not

[hanetzer]

Ok so I don't grok rust at all, but managed to cook up the following code:

use std::{env, fs, process};
use amd_apcb::{Apcb, ApcbIoOptions};

fn main() {
    let file = if let Some(file) = env::args().nth(1) {
        file
    } else {
        eprintln!("apcb-parse <file>");
        process::exit(1);
    };

    let mut buffer = fs::read(file).unwrap();
    let mut apcb = Apcb::load(&mut buffer[0..], &ApcbIoOptions::default()).unwrap();
    let yaml = serde_yaml::to_string(&apcb)?;
}

This seems to be the right direction so far, but I'm utterly unfamiliar with rust semantics and 'the right way to do things', let alone reading a big lib like this to figure out what to do.

[daym]

Hello!

As the error message that Rust prints says, please do this:

13   |     let mut apcb = Apcb::load(std::borrow::Cow::Borrowed(&mut buffer[0..]), &ApcbIoOptions::default()).unwrap();                                                             

See also https://doc.rust-lang.org/std/borrow/enum.Cow.html for what this is for.

With this change (note: you also need features = ["serde"] on the amd-apcb dependency--but I think you have that already), your program builds and I get:

cargo run -- /tmp/APCB_GN_D4.bin 
Finished dev [unoptimized + debuginfo] target(s) in 0.02s
 Running `target/debug/tst-apcb /tmp/APCB_GN_D4.bin`
Error: Error("serializing nested enums in YAML is not supported yet")

And that's an upstream serde_yaml bug (I'm not saying it's good like that; but we shouldn't work around their bug). Could you file a bug report in serde_yaml? They even have a test case to ascertain that it doesn't work: https://github.com/dtolnay/serde-yaml/blob/6875bc7c7dcb2565358ef9ee5e926cfd2dec2e7b/tests/test_error.rs#L176

In production, we are using json5 and/or serde_json--those work fine. So please just substitute serde_yaml by serde_json in your program.

We use nested enums a lot, for example:

                                                                    tokens: [
                                                                            {
                                                                                    Bool: {
                                                                                            MemLrdimmCapable: true
                                                                                    }
                                                                            },

Why this isn't supposed to work in YAML (if the test case is any indication), the kitchen sink of formats, is beyond me :)

[hanetzer]

Gotcha, and thanks. So something like, serde_json::to_string or so?

[daym]

Yes, although I would use serde_json::to_string_pretty because it adds newlines and indentation.

[hanetzer]

hrm. Unless I missed something you wrote, no joy.

use std::{env, fs, process};
use amd_apcb::{Apcb, ApcbIoOptions};

fn main() {
    let file = if let Some(file) = env::args().nth(1) {
        file
    } else {
        eprintln!("apcb-parse <file>");
        process::exit(1);
    };

    let mut buffer = fs::read(file).unwrap();
    let mut apcb = Apcb::load(std::borrow::Cow::Borrowed(&mut buffer[0..]), &ApcbIoOptions::default()).unwrap();
    let json = serde_json::to_string(&apcb);
    println!("{}", serde_json::to_string_pretty(&json).unwrap());
}

results in

error[E0277]: the trait bound `serde_json::Error: serde::ser::Serialize` is not satisfied
    --> src/main.rs:15:49
     |
15   |     println!("{}", serde_json::to_string_pretty(&json).unwrap());
     |                    ---------------------------- ^^^^^ the trait `serde::ser::Serialize` is not implemented for `serde_json::Error`
     |                    |
     |                    required by a bound introduced by this call
     |
     = help: the following other types implement trait `serde::ser::Serialize`:
               &'a T
               &'a mut T
               ()
               (T0, T1)
               (T0, T1, T2)
               (T0, T1, T2, T3)
               (T0, T1, T2, T3, T4)
               (T0, T1, T2, T3, T4, T5)
             and 312 others
     = note: required for `Result<std::string::String, serde_json::Error>` to implement `serde::ser::Serialize`
note: required by a bound in `to_string_pretty`
    --> ~/.cargo/registry/src/github.com-1ecc6299db9ec823/serde_json-1.0.103/src/ser.rs:2215:17
     |
2215 |     T: ?Sized + Serialize,
     |                 ^^^^^^^^^ required by this bound in `to_string_pretty`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `apcb-parse` due to previous error

[daym]

serde_json::to_string_pretty(&apcb) ;)

I'd use either serde_json::to_string_pretty(&apcb) or serde_json::to_string(&apcb), not both.

[hanetzer]

oh wait, I'm dumb. Note that I hit the apcb with to_string, and then try to hit that with to_string_pretty. works now.

[hanetzer]

On a non-rust related note: any kind of sharable documentation on the fields and meanings and such? Once my 3200g arrives sometime next week I'm going to be trying to port a consumer desktop board to coreboot (yeah I know its a different project/boot flow than what you guys use but I'm firmly entrenched in the gentoo linux world).

[daym]

works now.

Great!

Is there a text Unknown in the resulting JSON output? Then that means the library could not identify what that is.

This will probably happen when you are using non-Milan-server architectures. It would be nice for someone to collect those and also the information what CPU model it is exactly, in order to have a set of tokens used per CPU model. Paging @orangecms, if he is interested :)

[hanetzer]

yeah, there are 53 unknown fields; 52 of them are just

{
  "Unknown": [
    0
  ]
}

which I assume to just be padding of some form while there's one which seems more significant:

          {
            "custom_error_type": "Smu",
            "peak_map": 21845,
            "peak_attr": {
              "peak_count": 15,
              "pulse_width": 3,
              "repeat_count": 0,
              "_reserved_1": 0
            }
          },
          {
            "custom_error_type": "Unknown",
            "peak_map": 21845,
            "peak_attr": {
              "peak_count": 3,
              "pulse_width": 3,
              "repeat_count": 0,
              "_reserved_1": 0
            }
          }

[hanetzer]

(The above was produced from APCB_mandolin.bin in the coreboot blobs repo)

[daym]

Only? That's great and a lot better than I expected.

Yeah, AMD likes to add padding, sometimes, so that subsequent records are aligned to a 32 bit boundary.

[daym]

On a non-rust related note: any kind of sharable documentation on the fields and meanings and such?

I filed https://github.com/oxidecomputer/amd-apcb/issues/98 for the documentation request

[daym]

Closing this as won't-fix (I wouldn't know how, and Cow is really OK).

But if you have further questions, feel free to ask. (For now, just creating issues for those questions is OK, but I'll look into whether we want to enable github discussion forum or something. But really, when there are questions like this it means we should improve the documentation, so it IS an issue :) )