linux-kernel - Re: [PATCH 5/5] x86/virt/tdx: Export global metadata read infrastructure

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: <1c8537b8-bb91-48ee-ae9a-5f54b828b49c@intel.com>
Date: Fri, 15 Mar 2024 13:24:29 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: Xiaoyao Li <xiaoyao.li@...el.com>, <linux-kernel@...r.kernel.org>,
	<kvm@...r.kernel.org>
CC: <x86@...nel.org>, <dave.hansen@...el.com>,
	<kirill.shutemov@...ux.intel.com>, <peterz@...radead.org>,
	<tglx@...utronix.de>, <bp@...en8.de>, <mingo@...hat.com>, <hpa@...or.com>,
	<seanjc@...gle.com>, <pbonzini@...hat.com>, <isaku.yamahata@...el.com>,
	<jgross@...e.com>
Subject: Re: [PATCH 5/5] x86/virt/tdx: Export global metadata read
 infrastructure



On 13/03/2024 4:44 pm, Xiaoyao Li wrote:
> On 3/1/2024 7:20 PM, Kai Huang wrote:
>> KVM will need to read a bunch of non-TDMR related metadata to create and
>> run TDX guests.  Export the metadata read infrastructure for KVM to use.
>>
>> Specifically, export two helpers:
>>
>> 1) The helper which reads multiple metadata fields to a buffer of a
>>     structure based on the "field ID -> structure member" mapping table.
>>
>> 2) The low level helper which just reads a given field ID.
> 
> How about introducing a helper to read a single metadata field comparing 
> to 1) instead of the low level helper.
> 
> The low level helper tdx_sys_metadata_field_read() requires the data buf 
> to be u64 *. So the caller needs to use a temporary variable and handle 
> the memcpy when the field is less than 8 bytes.
> 
> so why not expose a high level helper to read single field, e.g.,
> 
> +int tdx_sys_metadata_read_single(u64 field_id, int bytes, void *buf)
> +{
> +       return stbuf_read_sys_metadata_field(field_id, 0, bytes, buf);
> +}
> +EXPORT_SYMBOL_GPL(tdx_sys_metadata_read_single);

As replied here where these APIs are (supposedly) to be used:

https://lore.kernel.org/kvm/e88e5448-e354-4ec6-b7de-93dd8f7786b5@intel.com/

I don't see why we need to use a temporary 'u64'.  We can just use it 
directly or type cast to 'u16' when needed, which has the same result of 
doing explicit memory copy based on size.

So I am not convinced at this stage that we need the code as you 
suggested.  At least I believe the current APIs are sufficient for KVM 
to use.

However I'll put more background on how KVM is going to use into the 
changelog to justify the current APIs are enough.