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

From: "Huang, Kai" <kai.huang@intel.com>
To: Xiaoyao Li <xiaoyao.li@intel.com>, <linux-kernel@vger.kernel.org>,
	<kvm@vger.kernel.org>
Cc: <x86@kernel.org>, <dave.hansen@intel.com>,
	<kirill.shutemov@linux.intel.com>, <peterz@infradead.org>,
	<tglx@linutronix.de>, <bp@alien8.de>, <mingo@redhat.com>,
	<hpa@zytor.com>, <seanjc@google.com>, <pbonzini@redhat.com>,
	<isaku.yamahata@intel.com>, <jgross@suse.com>
Subject: Re: [PATCH 5/5] x86/virt/tdx: Export global metadata read infrastructure
Date: Fri, 15 Mar 2024 13:24:29 +1300	[thread overview]
Message-ID: <1c8537b8-bb91-48ee-ae9a-5f54b828b49c@intel.com> (raw)
In-Reply-To: <bd61e29d-5842-4136-b30f-929b00bdf6f9@intel.com>

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.