Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output JSON for hardware configuration #1061

Merged
merged 1 commit into from
Aug 12, 2023

Conversation

Ivan-Velickovic
Copy link
Contributor

There is already existing YAML output for the hardware platform description, this adds JSON output as well.

My main motivation for this is that JSON is part of the standard library of Python and so doesn't need an external dependency to parse. Also, other generated config files such as gen_config have JSON and YAML versions so this is being consistent with that.

Example output:

{
  "devices": [
    {
      "start": 0,
      "end": 16777216
    },
    {
      "start": 83886080,
      "end": 87031808
    },
    {
      "start": 268435456,
      "end": 270532608
    },
    {
      "start": 2147483648,
      "end": 3291484160
    },
    {
      "start": 3291492352,
      "end": 3291496448
    },
    {
      "start": 3291500544,
      "end": 1099511627776
    }
  ],
  "memory": [
    {
      "start": 16777216,
      "end": 83886080
    },
    {
      "start": 87031808,
      "end": 268435456
    },
    {
      "start": 270532608,
      "end": 2147483648
    }
  ]
}

config.cmake Outdated
@@ -203,6 +207,7 @@ if(DEFINED KernelDTSList AND (NOT "${KernelDTSList}" STREQUAL ""))
"${device_dest}" --hardware-config "${config_file}" --hardware-schema
"${config_schema}" --yaml --yaml-out "${platform_yaml}" --sel4arch
"${KernelSel4Arch}" --addrspace-max "${KernelPaddrUserTop}"
--json --json-out "${platform_json}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if we could make CMake aware of the files this step creates, so it can track the dependencies and rebuild things automatically. However, since this seems missing for the YAML also, it's not be a blocker to merge this. Not sure if this can be done trivially anyway, since this does not even look like a CMake target, but runs at CMake's generation step?

@Ivan-Velickovic
Copy link
Contributor Author

Ivan-Velickovic commented Jun 20, 2023

Updated (formatted) output:

{
  "devices": [
    {
      "start": "0x0",
      "end": "0x8000000"
    },
    {
      "start": "0x8001000",
      "end": "0x8010000"
    },
    {
      "start": "0x8011000",
      "end": "0x8030000"
    },
    {
      "start": "0x8031000",
      "end": "0x60000000"
    },
    {
      "start": "0xc0000000",
      "end": "0x10000000000"
    }
  ],
  "memory": [
    {
      "start": "0x60000000",
      "end": "0xc0000000"
    }
  ]
}

Copy link
Member

@axel-h axel-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for me.

@Indanz
Copy link
Contributor

Indanz commented Jun 20, 2023

But it contradicts the intention of using JSON as machine processable standardized data interchange format, because you'd have to parse the string fields manually then.

I agree with @axel-h here, using hexadecimal as strings defeats the whole reason to spit out JSON in the first place. I thought the purpose was to easily integrate it with anything else that would need this info and supports JSON. Does the YAML output use decimals or hexadecimals?

@Ivan-Velickovic
Copy link
Contributor Author

But it contradicts the intention of using JSON as machine processable standardized data interchange format, because you'd have to parse the string fields manually then.

I agree with @axel-h here, using hexadecimal as strings defeats the whole reason to spit out JSON in the first place. I thought the purpose was to easily integrate it with anything else that would need this info and supports JSON. Does the YAML output use decimals or hexadecimals?

It uses hexadecimal as well.

@Indanz
Copy link
Contributor

Indanz commented Jun 20, 2023

It uses hexadecimal as well.

But YAML supports hexadecimals, so I assume it's not stuffed into a string, right?

People that want more readable data can read the YAML output, no need to also do hex in JSON. JSON is made for data exchange, if using it, do it the JSON way please. If you don't like the JSON way, don't use JSON, that's my opinion.

@Ivan-Velickovic
Copy link
Contributor Author

It uses hexadecimal as well.

But YAML supports hexadecimals, so I assume it's not stuffed into a string, right?

People that want more readable data can read the YAML output, no need to also do hex in JSON. JSON is made for data exchange, if using it, do it the JSON way please. If you don't like the JSON way, don't use JSON, that's my opinion.

Sure, but then we should change gen_config.json as well right? Since that also will dump numbers as strings. In my opinion it's nicer to have the output be human-readable, but given that it's an auto-generated file I guess that does not have that much value.

@Indanz
Copy link
Contributor

Indanz commented Jun 20, 2023

Sure, but then we should change gen_config.json as well right? Since that also will dump numbers as strings.

I don't know what the purpose of that file is and with what it interacts. If it interacts with cmake and cmake only supports strings, then it makes sense, for instance.

In my opinion it's nicer to have the output be human-readable, but given that it's an auto-generated file I guess that does not have that much value.

Of course human-readable is nicer, but my impression was that the YAML and JSON outputs are for machines, not humans.

@Ivan-Velickovic
Copy link
Contributor Author

Sure, but then we should change gen_config.json as well right? Since that also will dump numbers as strings.

I don't know what the purpose of that file is and with what it interacts. If it interacts with cmake and cmake only supports strings, then it makes sense, for instance.

My bad, should have explained it more. The gen_config.* files are all the configuration options for seL4 such as CONFIG_MAX_NUM_NODES etc that we usually see in both kernel and user-space code. This used to be just a header file (gen_config.h) but for seL4CP and for Nick's Rust work (as well as for l4v) it's useful to have other machine-readable formats. When the YAML/JSON versions of the config file got added the numbers were just dumped as strings. I don't think there's anything stopping them being dumped as JSON numbers as well? I can't remember if there was a justification for this or it was just done this way (@nspin do you remember?). I wanted this machine readable configuration file to be consistent with gen_config so that's why I wrapped the addresses in a string.

@ratmice
Copy link
Contributor

ratmice commented Jun 20, 2023

FWIW, it doesn't look like it is too difficult to deal with hex-in-string numbers within cmake itself, primarily through the math(EXPR ..), and it looks like you can convert back into hex with string(HEX), I believe you'd need to then prepend with "0x" though).
So I really don't have much of a preference anymore, even though initially I was a bit skeptical of the hex in string option.

foo.json:

{
   "start": "0x0",
   "end": "0x8000000"
}

foo.cmake (cmake 2.19 at least):

file(READ foo.json JSON_VAL)
string(JSON hexval GET ${JSON_VAL} "end")
math(EXPR decval "${hexval}" OUTPUT_FORMAT DECIMAL)
message(STATUS "Hex: ${hexval} => decimal: ${decval}")

@Indanz
Copy link
Contributor

Indanz commented Jun 20, 2023

Well, it is as it is now, if it is used already then changing the format makes life probably worse for everyone. I don't recommend changing formats once they're there, as it will just unnecessarily break existing software.

@kent-mcleod
Copy link
Member

I agree that the platform memory spec when encoded in JSON should use the native number type instead of hex strings.

I don't know what the purpose of that file is and with what it interacts. If it interacts with cmake and cmake only supports strings, then it makes sense, for instance.

gen_config.json is trying to encode config options from CMake for generating into C header files (or other language config files). Because there's an embedding here, to support the translation of CMake hex string values into c header hex token values its encoded as a string.

@Ivan-Velickovic
Copy link
Contributor Author

Okay I will revert the patch to what I had initially.

@Ivan-Velickovic
Copy link
Contributor Author

Can this be merged?

@Ivan-Velickovic
Copy link
Contributor Author

Is there anything else I need to change here?

@kent-mcleod
Copy link
Member

Is there anything else I need to change here?

There's a style error that needs to be resolved.

@Ivan-Velickovic
Copy link
Contributor Author

There's a style error that needs to be resolved.

Done

Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>
@Ivan-Velickovic
Copy link
Contributor Author

Rebased on master now.

@kent-mcleod kent-mcleod merged commit e1bdd80 into seL4:master Aug 12, 2023
35 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants