Messages in this thread | | | Date | Fri, 15 Mar 2024 10:10:56 +0800 | Subject | Re: [PATCH 5/5] x86/virt/tdx: Export global metadata read infrastructure | From | Xiaoyao Li <> |
| |
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.
| |