Received-SPF: pass (google.com: domain of linux-kernel+bounces-103980-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <f492f669-4abb-406f-ad7b-2d134332644c@intel.com>
Date: Fri, 15 Mar 2024 10:10:56 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 5/5] x86/virt/tdx: Export global metadata read
 infrastructure
Content-Language: en-US
To: "Huang, Kai" <kai.huang@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
References: <cover.1709288433.git.kai.huang@intel.com>
 <ec9fc9f1d45348ddfc73362ddfb310cc5f129646.1709288433.git.kai.huang@intel.com>
 <bd61e29d-5842-4136-b30f-929b00bdf6f9@intel.com>
 <1c8537b8-bb91-48ee-ae9a-5f54b828b49c@intel.com>
From: Xiaoyao Li <xiaoyao.li@intel.com>
In-Reply-To: <1c8537b8-bb91-48ee-ae9a-5f54b828b49c@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 3/15/2024 8:24 AM, Huang, Kai wrote:
> 
> 
> 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.

The way to cast a u64 to u16 is based on the fact that the variable is 
u64 at first.

Given

	u16 feild_x;

We have to have a u64 tmp, passed to tdx_sys_metadata_field_read() to 
hold the output of metadata read, then

	filed_x = (u16) tmp;

If we pass field_x into tdx_sys_metadata_field_read(), the following 
(64-16) bits might be corrupted.

> 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.