Message ID | cover.1728903647.git.kai.huang@intel.com (mailing list archive) |
---|---|
Headers | show |
Series | TDX host: metadata reading tweaks, bug fix and info dump | expand |
I'm having one of those "I hate this all" moments. Look at what we say
in the code:
> * See the "global_metadata.json" in the "TDX 1.5 ABI definitions".
Basically step one in verifying that this is all right is: Hey, humans,
please go parse a machine-readable format. That's insanity. If Intel
wants to publish JSON as the canonical source of truth, that's fine.
It's great, actually. But let's stop playing human JSON parser and make
the computers do it for us, OK?
Let's just generate the code. Basically, as long as the generated C is
marginally readable, I'm OK with it. The most important things are:
1. Adding a field is dirt simple
2. Using the generated C is simple
In 99% of the cases, nobody ends up having to ever look at the generated
code.
Take a look at the attached python program and generated C file. I
think they qualify. We can check the script into tools/scripts/ and it
can get re-run when new json comes out or when a new field is needed.
You'd could call the generated code like this:
#include <generated.h>
read_gunk(&tgm);
and use it like this:
foo = tgm.BUILD_NUM;
bar = tgm.BUILD_DATE;
Any field you want to add is a single addition to the python list and
re-running the script. There's not even any need to do:
#define TDX_FOO_BAR_BUILD_DATE 0x8800000200000001
because it's unnecessary when you have:
ret |= read_...(0x8800000200000001, &tgm.BUILD_DATE);
that links the magic number and the "BUILD_DATE" so closely together
anyway. We also don't need type safety *here* at the "read" because
it's machine generated in the first place. If there's a type mismatch
between "0x8800000200000001" and "tgm.BUILD_DATE" we have bigger
problems on our hands.
All the type checking comes when the code consumes tgm.BUILD_DATE (or
whatever).
On Tue, Oct 15, 2024 at 5:30 PM Dave Hansen <dave.hansen@intel.com> wrote: > > I'm having one of those "I hate this all" moments. Look at what we say > in the code: > > > * See the "global_metadata.json" in the "TDX 1.5 ABI definitions". > > Basically step one in verifying that this is all right is: Hey, humans, > please go parse a machine-readable format. That's insanity. If Intel > wants to publish JSON as the canonical source of truth, that's fine. > It's great, actually. But let's stop playing human JSON parser and make > the computers do it for us, OK? > > Let's just generate the code. Basically, as long as the generated C is > marginally readable, I'm OK with it. The most important things are: > > 1. Adding a field is dirt simple > 2. Using the generated C is simple > > In 99% of the cases, nobody ends up having to ever look at the generated > code. > > Take a look at the attached python program and generated C file. I > think they qualify. We can check the script into tools/scripts/ and it > can get re-run when new json comes out or when a new field is needed. > You'd could call the generated code like this: Ok, so let's move this thing forward. Here is a more polished script and the output. Untested beyond compilation. Kai, feel free to include it in v6 with my Signed-off-by: Paolo Bonzini <pbonzini@redhat.om> I made an attempt at adding array support and using it with the CMR information; just to see if Intel is actually trying to make global_metadata.json accurate. The original code has for (i = 0; i < sysinfo_cmr->num_cmrs; i++) { READ_SYS_INFO(CMR_BASE + i, cmr_base[i]); READ_SYS_INFO(CMR_SIZE + i, cmr_size[i]); } The generated code instead always tries to read 32 fields and returns non-zero from get_tdx_sys_info_cmr if they are missing. If it fails to read the fields above NUM_CMRS, just remove that part of the tdx.py script and make sure that a comment in the code shames the TDX ABI documentation adequately. :) Thanks, Paolo
Paolo Bonzini wrote: > On Tue, Oct 15, 2024 at 5:30 PM Dave Hansen <dave.hansen@intel.com> wrote: > > > > I'm having one of those "I hate this all" moments. Look at what we say > > in the code: > > > > > * See the "global_metadata.json" in the "TDX 1.5 ABI definitions". > > > > Basically step one in verifying that this is all right is: Hey, humans, > > please go parse a machine-readable format. That's insanity. If Intel > > wants to publish JSON as the canonical source of truth, that's fine. > > It's great, actually. But let's stop playing human JSON parser and make > > the computers do it for us, OK? > > > > Let's just generate the code. Basically, as long as the generated C is > > marginally readable, I'm OK with it. The most important things are: > > > > 1. Adding a field is dirt simple > > 2. Using the generated C is simple > > > > In 99% of the cases, nobody ends up having to ever look at the generated > > code. > > > > Take a look at the attached python program and generated C file. I > > think they qualify. We can check the script into tools/scripts/ and it > > can get re-run when new json comes out or when a new field is needed. > > You'd could call the generated code like this: > > Ok, so let's move this thing forward. Here is a more polished script > and the output. Untested beyond compilation. > > Kai, feel free to include it in v6 with my > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.om> > > I made an attempt at adding array support and using it with the CMR > information; just to see if Intel is actually trying to make > global_metadata.json accurate. The original code has > > for (i = 0; i < sysinfo_cmr->num_cmrs; i++) { > READ_SYS_INFO(CMR_BASE + i, cmr_base[i]); > READ_SYS_INFO(CMR_SIZE + i, cmr_size[i]); > } > > The generated code instead always tries to read 32 fields and returns > non-zero from get_tdx_sys_info_cmr if they are missing. If it fails to > read the fields above NUM_CMRS, just remove that part of the tdx.py > script and make sure that a comment in the code shames the TDX ABI > documentation adequately. :) Thanks for doing this Paolo, I regret not pushing harder [1] / polishing up the bash+jq script I threw together to do the same. I took a look at your script and the autogenerated code and it looks good to me. Feel free to add my Reviewed-by on a patch that adds that collateral to the tools/ directory. [1]: http://lore.kernel.org/66b19beaadd28_4fc729410@dwillia2-xfh.jf.intel.com.notmuch
On 16/10/2024 8:04 am, Dan Williams wrote: > Paolo Bonzini wrote: >> On Tue, Oct 15, 2024 at 5:30 PM Dave Hansen <dave.hansen@intel.com> wrote: >>> >>> I'm having one of those "I hate this all" moments. Look at what we say >>> in the code: >>> >>>> * See the "global_metadata.json" in the "TDX 1.5 ABI definitions". >>> >>> Basically step one in verifying that this is all right is: Hey, humans, >>> please go parse a machine-readable format. That's insanity. If Intel >>> wants to publish JSON as the canonical source of truth, that's fine. >>> It's great, actually. But let's stop playing human JSON parser and make >>> the computers do it for us, OK? >>> >>> Let's just generate the code. Basically, as long as the generated C is >>> marginally readable, I'm OK with it. The most important things are: >>> >>> 1. Adding a field is dirt simple >>> 2. Using the generated C is simple >>> >>> In 99% of the cases, nobody ends up having to ever look at the generated >>> code. >>> >>> Take a look at the attached python program and generated C file. I >>> think they qualify. We can check the script into tools/scripts/ and it >>> can get re-run when new json comes out or when a new field is needed. >>> You'd could call the generated code like this: >> >> Ok, so let's move this thing forward. Here is a more polished script >> and the output. Untested beyond compilation. >> >> Kai, feel free to include it in v6 with my >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.om> >> >> I made an attempt at adding array support and using it with the CMR >> information; just to see if Intel is actually trying to make >> global_metadata.json accurate. The original code has >> >> for (i = 0; i < sysinfo_cmr->num_cmrs; i++) { >> READ_SYS_INFO(CMR_BASE + i, cmr_base[i]); >> READ_SYS_INFO(CMR_SIZE + i, cmr_size[i]); >> } >> >> The generated code instead always tries to read 32 fields and returns >> non-zero from get_tdx_sys_info_cmr if they are missing. If it fails to >> read the fields above NUM_CMRS, just remove that part of the tdx.py >> script and make sure that a comment in the code shames the TDX ABI >> documentation adequately. :) > > Thanks for doing this Paolo, I regret not pushing harder [1] / polishing > up the bash+jq script I threw together to do the same. > > I took a look at your script and the autogenerated code and it looks good > to me. > > Feel free to add my Reviewed-by on a patch that adds that collateral to > the tools/ directory. > > [1]: http://lore.kernel.org/66b19beaadd28_4fc729410@dwillia2-xfh.jf.intel.com.notmuch Hi Dave/Paolo/Dan, I'll go with this for the next version. Thanks for all your feedback and the code you shared. I appreciate.