Re: [PATCH v5 0/8] TDX host: metadata reading tweaks, bug fix and info dump

[Huang, Kai]

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@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxx.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.