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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c9fb4377-5daf-4c22-b273-df12aa316d52@intel.com>
Date: Wed, 16 Oct 2024 10:11:52 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: Dan Williams <dan.j.williams@...el.com>, Paolo Bonzini
	<pbonzini@...hat.com>, Dave Hansen <dave.hansen@...el.com>
CC: <kirill.shutemov@...ux.intel.com>, <tglx@...utronix.de>, <bp@...en8.de>,
	<peterz@...radead.org>, <mingo@...hat.com>, <hpa@...or.com>,
	<seanjc@...gle.com>, <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<kvm@...r.kernel.org>, <rick.p.edgecombe@...el.com>,
	<isaku.yamahata@...el.com>, <adrian.hunter@...el.com>, <nik.borisov@...e.com>
Subject: Re: [PATCH v5 0/8] TDX host: metadata reading tweaks, bug fix and
 info dump



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@...el.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@...hat.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.