| Message ID | 101f6f252db860ad7a7433596006da0d210dd5cb.1727173372.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TDX host: metadata reading tweaks, bug fix and info dump | expand |
On 24.09.24 г. 14:28 ч., Kai Huang wrote: > The TDX module provides a set of "Global Metadata Fields". They report > things like TDX module version, supported features, and fields related > to create/run TDX guests and so on. TDX supports 8/16/32/64 bits > metadata field element sizes. For a given metadata field, the element > size is encoded in the metadata field ID. > > For now the kernel only reads "TD Memory Region" (TDMR) related metadata > fields and they are all 16-bit. Thus the kernel only has one primitive > __read_sys_metadata_field16() to read 16-bit metadata field and the > macro, read_sys_metadata_field16(), which does additional build-time > check of the field ID makes sure the field is indeed 16-bit. > > Future changes will need to read more metadata fields with different > element sizes. Choose to provide one primitive for each element size to > support that. Similar to the build_mmio_read() macro, reimplement the > body of __read_sys_metadata_field16() as a macro build_sysmd_read(_size) > in size-agnostic way, so it can be used to generate one primitive for > each element size: > > build_sysmd_read(8) > build_sysmd_read(16) > .. > > Also extend read_sys_metadata_field16() take the '_size' as argument > (and rename it to read_sys_metadata_field() to make it size-agnostic) to > allow the READ_SYS_INFO() macro to choose which primitive to use. > > Signed-off-by: Kai Huang <kai.huang@intel.com> <snip> > +#define build_sysmd_read(_size) \ > +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val) \ > +{ \ > + u64 tmp; \ > + int ret; \ > + \ > + ret = tdh_sys_rd(field_id, &tmp); \ > + if (ret) \ > + return ret; \ > + \ > + *val = tmp; \ > + \ > + return 0; \ > } > > -#define read_sys_metadata_field16(_field_id, _val) \ > +build_sysmd_read(16) nit: Generally the unwritten convention for this kind of macro definition is to capitalize them and be of the from: DEFINE_xxxxx - similar to how event classes are defined. perhaps naming this macro: DEFINE_TDX_METADATA_READER() ought to be more descriptive, also the "md" contraction of metadata also seems a bit quirky (at least to me). It's not a deal breaker but if there is going to be another posting this might be something to consider. <snip>
> > > +#define build_sysmd_read(_size) \ > > +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val) \ > > +{ \ > > + u64 tmp; \ > > + int ret; \ > > + \ > > + ret = tdh_sys_rd(field_id, &tmp); \ > > + if (ret) \ > > + return ret; \ > > + \ > > + *val = tmp; \ > > + \ > > + return 0; \ > > } > > > > -#define read_sys_metadata_field16(_field_id, _val) \ > > +build_sysmd_read(16) > > nit: Generally the unwritten convention for this kind of macro > definition is to capitalize them and be of the from: > > DEFINE_xxxxx - similar to how event classes are defined. > > perhaps naming this macro: > > DEFINE_TDX_METADATA_READER() ought to be more descriptive, also the > "md" contraction of metadata also seems a bit quirky (at least to me). > > It's not a deal breaker but if there is going to be another posting this > might be something to consider. > Thanks for the comments. I don't have opinion on this. Dan said we can do something like build_mmio_read() macro and this is where build_sysmd_read() came from. Dan used build_tdg_sys_rd() in his patch too: https://lore.kernel.org/all/172618717675.516322.6087817418162288917.stgit@dwillia2-xfh.jf.intel.com/ Btw I actually agree that your DEFINE_xx() is more formal thus is better when the macro is used by multiple C files. But here only tdx.c uses it, and IMHO it's also fine to have a informal but shorter name here. Anyway let's see whether other people have anything to say.
On 9/24/24 04:28, Kai Huang wrote: > +#define build_sysmd_read(_size) \ > +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val) \ > +{ \ > + u64 tmp; \ > + int ret; \ > + \ > + ret = tdh_sys_rd(field_id, &tmp); \ > + if (ret) \ > + return ret; \ > + \ > + *val = tmp; \ > + \ > + return 0; \ > } Why? What's so important about having the compiler do the copy? #define read_sys_metadata_field(id, val) \ __read_sys_metadata_field(id, val, sizeof (*(val))) static int __read_sys_metadata_field(u64 field_id, void *ptr, int size) { ... memcpy(ptr, &tmp, size); return 0; } There's one simple #define there so that users don't have to do the sizeof and can't screw it up.
On 27/09/2024 3:47 am, Hansen, Dave wrote: > On 9/24/24 04:28, Kai Huang wrote: >> +#define build_sysmd_read(_size) \ >> +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val) \ >> +{ \ >> + u64 tmp; \ >> + int ret; \ >> + \ >> + ret = tdh_sys_rd(field_id, &tmp); \ >> + if (ret) \ >> + return ret; \ >> + \ >> + *val = tmp; \ >> + \ >> + return 0; \ >> } > > Why? What's so important about having the compiler do the copy? > > > #define read_sys_metadata_field(id, val) \ > __read_sys_metadata_field(id, val, sizeof (*(val))) > > static int __read_sys_metadata_field(u64 field_id, void *ptr, int size) > { > ... > memcpy(ptr, &tmp, size); > > return 0; > } > > There's one simple #define there so that users don't have to do the > sizeof and can't screw it up. Yes we can do this. This is basically what I did in the previous version: https://lore.kernel.org/kvm/0403cdb142b40b9838feeb222eb75a4831f6b46d.1724741926.git.kai.huang@intel.com/ But Dan commented using typeless 'void *' and 'size' is kinda a step backwards and we should do something similar to build_mmio_read(): https://lore.kernel.org/kvm/66db75497a213_22a2294b@dwillia2-xfh.jf.intel.com.notmuch/ Hi Dan, I think what Dave suggested makes sense. If the concern is using __read_sys_metadata_field() directly isn't typesafe, we can add a comment to it saying callers should not use it directly and use read_sys_metadata_field() instead. Dave's approach also makes the LoC slightly shorter, and cleaner from the perspective that we don't need to explicitly specify the '16/32/64' in the READ_SYS_INFO() macro anymore as shown in here: https://lore.kernel.org/kvm/79c256b8978310803bb4de48cd81dd373330cbc2.1727173372.git.kai.huang@intel.com/ Please let me know your comment?
On 9/26/24 15:22, Huang, Kai wrote: > But Dan commented using typeless 'void *' and 'size' is kinda a step > backwards and we should do something similar to build_mmio_read(): Well, void* is typeless, but at least it knows the size in this case. It's not completely aimless. I was thinking of how things like get_user() work.
On 27/09/2024 10:26 am, Hansen, Dave wrote: > On 9/26/24 15:22, Huang, Kai wrote: >> But Dan commented using typeless 'void *' and 'size' is kinda a step >> backwards and we should do something similar to build_mmio_read(): > > Well, void* is typeless, but at least it knows the size in this case. > It's not completely aimless. I was thinking of how things like > get_user() work. get_user(x,ptr) only works with simple types: * @ptr must have pointer-to-simple-variable type, and the result of * dereferencing @ptr must be assignable to @x without a cast. The compiler knows the type of both @x and @(*ptr), so it knows type-safety and size to copy. I think we can eliminate the __read_sys_metadata_field() by implementing it as a macro directly and get rid of 'void *' and 'size': static int tdh_sys_rd(u64 field_id, u64 *val) {} /* @_valptr must be pointer to u8/u16/u32/u64 */ #define read_sys_metadata_field(_field_id, _valptr) \ ({ \ u64 ___tmp; \ int ___ret; \ \ BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != \ sizeof(*_valptr)); \ \ ___ret = tdh_sys_rd(_field_id, &___tmp); \ \ *_valptr = ___tmp; \ ___ret; }) It sets *_valptr unconditionally but we can also only do it when ___ret is 0. The caller will need to do: static int get_tdx_metadata_X_which_is_32bit(...) { u32 metadata_X; int ret; ret = read_sys_metadata_field(MD_FIELD_ID_X, &metadata_X); return ret; } I haven't compiled and tested but it seems feasible. Any comments?
Huang, Kai wrote: > > > On 27/09/2024 10:26 am, Hansen, Dave wrote: > > On 9/26/24 15:22, Huang, Kai wrote: > >> But Dan commented using typeless 'void *' and 'size' is kinda a step > >> backwards and we should do something similar to build_mmio_read(): > > > > Well, void* is typeless, but at least it knows the size in this case. > > It's not completely aimless. I was thinking of how things like > > get_user() work. > > get_user(x,ptr) only works with simple types: > > * @ptr must have pointer-to-simple-variable type, and the result of > * dereferencing @ptr must be assignable to @x without a cast. > > The compiler knows the type of both @x and @(*ptr), so it knows > type-safety and size to copy. > > I think we can eliminate the __read_sys_metadata_field() by implementing > it as a macro directly and get rid of 'void *' and 'size': > > static int tdh_sys_rd(u64 field_id, u64 *val) {} > > /* @_valptr must be pointer to u8/u16/u32/u64 */ > #define read_sys_metadata_field(_field_id, _valptr) \ > ({ \ > u64 ___tmp; \ > int ___ret; \ > \ > BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != \ > sizeof(*_valptr)); \ > \ > ___ret = tdh_sys_rd(_field_id, &___tmp); \ > \ > *_valptr = ___tmp; \ > ___ret; > }) > > It sets *_valptr unconditionally but we can also only do it when ___ret > is 0. > > The caller will need to do: > > static int get_tdx_metadata_X_which_is_32bit(...) > { > u32 metadata_X; > int ret; > > ret = read_sys_metadata_field(MD_FIELD_ID_X, &metadata_X); > > return ret; > } > > I haven't compiled and tested but it seems feasible. > > Any comments? If it works this approach addresses all the concerns I had with getting the compiler to validate field sizes. Should be straightforward to put this in a shared location so that it can optionally use tdg_sys_rd internally.
On Tue, 2024-10-01 at 00:56 -0700, Dan Williams wrote: > Huang, Kai wrote: > > > > > > On 27/09/2024 10:26 am, Hansen, Dave wrote: > > > On 9/26/24 15:22, Huang, Kai wrote: > > > > But Dan commented using typeless 'void *' and 'size' is kinda a step > > > > backwards and we should do something similar to build_mmio_read(): > > > > > > Well, void* is typeless, but at least it knows the size in this case. > > > It's not completely aimless. I was thinking of how things like > > > get_user() work. > > > > get_user(x,ptr) only works with simple types: > > > > * @ptr must have pointer-to-simple-variable type, and the result of > > * dereferencing @ptr must be assignable to @x without a cast. > > > > The compiler knows the type of both @x and @(*ptr), so it knows > > type-safety and size to copy. > > > > I think we can eliminate the __read_sys_metadata_field() by implementing > > it as a macro directly and get rid of 'void *' and 'size': > > > > static int tdh_sys_rd(u64 field_id, u64 *val) {} > > > > /* @_valptr must be pointer to u8/u16/u32/u64 */ > > #define read_sys_metadata_field(_field_id, _valptr) \ > > ({ \ > > u64 ___tmp; \ > > int ___ret; \ > > \ > > BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != \ > > sizeof(*_valptr)); \ > > \ > > ___ret = tdh_sys_rd(_field_id, &___tmp); \ > > \ > > *_valptr = ___tmp; \ > > ___ret; > > }) > > > > It sets *_valptr unconditionally but we can also only do it when ___ret > > is 0. > > > > The caller will need to do: > > > > static int get_tdx_metadata_X_which_is_32bit(...) > > { > > u32 metadata_X; > > int ret; > > > > ret = read_sys_metadata_field(MD_FIELD_ID_X, &metadata_X); > > > > return ret; > > } > > > > I haven't compiled and tested but it seems feasible. > > > > Any comments? > > If it works this approach addresses all the concerns I had with getting > the compiler to validate field sizes. Yes I just quickly tested on my box and it worked -- TDX module can be initialized successfully and all metadata fields (module version, CMRs etc) seem to be correct. Hi Dave, Please let me know if you have any concern? Otherwise I will go with this route. > > Should be straightforward to put this in a shared location so that it > can optionally use tdg_sys_rd internally. Yeah it's doable. As you also noticed guest and host use different calls: guest uses tdg_vm_rd() (which is in Kirill's not-yet-merged series[*]), and host uses tdh_sys_rd(). [*] https://lore.kernel.org/all/20240828093505.2359947-2-kirill.shutemov@linux.intel.com/ This can be resolved by adding a new argument to the read_sys_metadata_field() macro, e.g.,: /* @_valptr must be pointer to u8/u16/u32/u64 */ #define read_sys_metadata_field(_read_func, _field_id, _valptr) \ ({ \ u64 ___tmp; \ int ___ret; \ \ BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != \ sizeof(*_valptr)); \ \ ___ret = _read_func(_field_id, &___tmp); \ \ *_valptr = ___tmp; \ ___ret; \ }) We can put it in <asm/tdx.h> (together with the MD_FIELD_ELE_SIZE() macro) for guest and host to share. And in guest code (arch/x86/coco/tdx/tdx.c), we can have a wrapper: #define tdg_read_sys_metadata_field(_field_id, _valptr) \ read_sys_metadata_field(tdg_vm_rd, _field_id, _valptr) Similarly, in the host code (arch/x86/virt/vmx/tdx/tdx.c), we can have: #define tdh_read_sys_metadata_field(_field_id, _valptr) \ read_sys_metadata_field(tdh_sys_rd, _field_id, _valptr) We can start with this if you think it's better. But I would like to discuss this more: Once we start to share, it feels a little bit odd to share only the read_sys_metadata_field() macro, because we can probably share others too: 1) The metadata field ID definitions and bit definitions (this is obvious). 2) tdh_sys_rd() and tdg_vm_rd() are similar and can be shared too: /* Read TD-scoped metadata */ static inline u64 __maybe_unused tdg_vm_rd(u64 field, u64 *value) { struct tdx_module_args args = { .rdx = field, }; u64 ret; ret = __tdcall_ret(TDG_VM_RD, &args); *value = args.r8; return ret; } static int tdh_sys_rd(u64 field_id, u64 *data) { struct tdx_module_args args = {}; int ret; /* * TDH.SYS.RD -- reads one global metadata field * - RDX (in): the field to read * - R8 (out): the field data */ args.rdx = field_id; ret = seamcall_prerr_ret(TDH_SYS_RD, &args); if (ret) return ret; *data = args.r8; return 0; } There are minor differences, e.g., tdh_sys_rd() only sets *data when TDH_SYS_RD succeeded but tdg_vm_rd() unconditionally sets *value (spec says R8 is set to 0 if TDG_VM_RD or TDH_SYS_RD fails, and guest code kinda depends on this). Putting aside which is better, those differences are resolvable. And if we start to share, it appears we should share this too. And then the TDH_SYS_RD definition (and probably all TDH_SYS_xx SEAMCALL leaf definitions) should be moved to <asm/tdx.h> too. And then the header will grow bigger and bigger, which brings us a question on how to better organize all those definitions: 1) TDCALL/SEAMCALL leaf function definitions (e.g., TDH_SYS_RD); 2) TDCALL/SEAMCALL low level functions (__tdcall(), __seamcall()); 3) TDCALL/SEAMCALL leaf function wrappers; 4) sys metadata field ID definitions; 5) sys metadata read helper macros; So to me we can have a separate series to address how to better organize TDX code and how to share code between guest and host (I can start to work on it if it's better to be done sooner rather than later), thus I am not sure we want to start sharing read_sys_metadata_field() macro in _this_ series. But of course, if it is worth to share read_sys_metadata_field() in this series, I can add a patch as the last patch of this series to only share read_sys_metadata_field() in <asm/tdx.h> as mentioned above.
On 10/1/24 03:44, Huang, Kai wrote: > Please let me know if you have any concern? Otherwise I will go with > this route. I still see some long unwieldy #defines in the mail thread. That's my biggest worry.
On Tue, 2024-10-01 at 08:19 -0700, Dave Hansen wrote: > On 10/1/24 03:44, Huang, Kai wrote: > > Please let me know if you have any concern? Otherwise I will go with > > this route. > I still see some long unwieldy #defines in the mail thread. That's my > biggest worry. I suppose you mean the read_sys_metadata_field() macro? We can split that into two smaller macros by moving BUILD_BUG_ON() out: /* Don't use this directly, use read_sys_metadata_read() instead. */ #define __read_sys_metadata_field(_field_id, _valptr) \ ({ \ u64 ___tmp; \ int ___ret; \ \ ___ret = tdh_sys_rd(_field_id, &___tmp); \ *_valptr = ___tmp; \ \ ___ret; \ }) /* @_valptr must be pointer of u8/u16/u32/u64. */ #define read_sys_metadata_field(_field_id, _valptr) \ ({ \ BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != \ sizeof(*_valptr)); \ __read_sys_metadata_field(_field_id, _valptr); \ }) Does this look good to you?
On Tue, 2024-10-01 at 21:40 +0000, Huang, Kai wrote: > On Tue, 2024-10-01 at 08:19 -0700, Dave Hansen wrote: > > On 10/1/24 03:44, Huang, Kai wrote: > > > Please let me know if you have any concern? Otherwise I will go with > > > this route. > > I still see some long unwieldy #defines in the mail thread. That's my > > biggest worry. > > I suppose you mean the read_sys_metadata_field() macro? > > We can split that into two smaller macros by moving BUILD_BUG_ON() out: > > /* Don't use this directly, use read_sys_metadata_read() instead. */ > #define __read_sys_metadata_field(_field_id, _valptr) \ > ({ \ > u64 ___tmp; \ > int ___ret; \ > \ > ___ret = tdh_sys_rd(_field_id, &___tmp); \ > *_valptr = ___tmp; \ > \ > ___ret; \ > }) > > /* @_valptr must be pointer of u8/u16/u32/u64. */ > #define read_sys_metadata_field(_field_id, _valptr) \ > ({ \ > BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != \ > sizeof(*_valptr)); \ > __read_sys_metadata_field(_field_id, _valptr); \ > }) > > Does this look good to you? Hi Dave, Would you let me know are you OK with this? If not, I will revert back to what you suggested: https://lore.kernel.org/lkml/cover.1727173372.git.kai.huang@intel.com/T/#m3874080ef158a4704c4082259d3594aa0a322fc8
On 10/6/24 20:07, Huang, Kai wrote:
> Would you let me know are you OK with this?
It still looks unwieldy. Is there no other choice?
On Sun, 2024-10-06 at 20:44 -0700, Dave Hansen wrote: > On 10/6/24 20:07, Huang, Kai wrote: > > Would you let me know are you OK with this? > > It still looks unwieldy. Is there no other choice? Sorry I cannot figure out a better way. I would love to hear if you have anything in mind?
On Mon, 2024-10-07 at 06:53 +0000, Huang, Kai wrote: > On Sun, 2024-10-06 at 20:44 -0700, Dave Hansen wrote: > > On 10/6/24 20:07, Huang, Kai wrote: > > > Would you let me know are you OK with this? > > > > It still looks unwieldy. Is there no other choice? > > Sorry I cannot figure out a better way. I would love to hear if you have > anything in mind? Hi Dave, Since you have concern about the unwieldy macro, I'll switch back to what you suggested: https://lore.kernel.org/lkml/cover.1727173372.git.kai.huang@intel.com/T/#m3874080ef158a4704c4082259d3594aa0a322fc8 .. unless I hear something new from you. Thanks for the review.
diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 2f7e4abc1bb9..b5c8dde9caf0 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -250,7 +250,7 @@ static int build_tdx_memlist(struct list_head *tmb_list) return ret; } -static int read_sys_metadata_field(u64 field_id, u64 *data) +static int tdh_sys_rd(u64 field_id, u64 *data) { struct tdx_module_args args = {}; int ret; @@ -270,25 +270,29 @@ static int read_sys_metadata_field(u64 field_id, u64 *data) return 0; } -static int __read_sys_metadata_field16(u64 field_id, u16 *val) -{ - u64 tmp; - int ret; - - ret = read_sys_metadata_field(field_id, &tmp); - if (ret) - return ret; - - *val = tmp; - - return 0; +#define build_sysmd_read(_size) \ +static int __read_sys_metadata_field##_size(u64 field_id, u##_size *val) \ +{ \ + u64 tmp; \ + int ret; \ + \ + ret = tdh_sys_rd(field_id, &tmp); \ + if (ret) \ + return ret; \ + \ + *val = tmp; \ + \ + return 0; \ } -#define read_sys_metadata_field16(_field_id, _val) \ +build_sysmd_read(16) + +#define read_sys_metadata_field(_field_id, _val, _size) \ ({ \ BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(_field_id) != \ - MD_FIELD_ID_ELE_SIZE_16BIT); \ - __read_sys_metadata_field16(_field_id, _val); \ + MD_FIELD_ID_ELE_SIZE_##_size##BIT); \ + BUILD_BUG_ON(_size != sizeof(*_val) * 8); \ + __read_sys_metadata_field##_size(_field_id, _val); \ }) static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr) @@ -296,8 +300,8 @@ static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr) int ret = 0; #define READ_SYS_INFO(_field_id, _member) \ - ret = ret ?: read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \ - &sysinfo_tdmr->_member) + ret = ret ?: read_sys_metadata_field(MD_FIELD_ID_##_field_id, \ + &sysinfo_tdmr->_member, 16) READ_SYS_INFO(MAX_TDMRS, max_tdmrs); READ_SYS_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
The TDX module provides a set of "Global Metadata Fields". They report things like TDX module version, supported features, and fields related to create/run TDX guests and so on. TDX supports 8/16/32/64 bits metadata field element sizes. For a given metadata field, the element size is encoded in the metadata field ID. For now the kernel only reads "TD Memory Region" (TDMR) related metadata fields and they are all 16-bit. Thus the kernel only has one primitive __read_sys_metadata_field16() to read 16-bit metadata field and the macro, read_sys_metadata_field16(), which does additional build-time check of the field ID makes sure the field is indeed 16-bit. Future changes will need to read more metadata fields with different element sizes. Choose to provide one primitive for each element size to support that. Similar to the build_mmio_read() macro, reimplement the body of __read_sys_metadata_field16() as a macro build_sysmd_read(_size) in size-agnostic way, so it can be used to generate one primitive for each element size: build_sysmd_read(8) build_sysmd_read(16) .. Also extend read_sys_metadata_field16() take the '_size' as argument (and rename it to read_sys_metadata_field() to make it size-agnostic) to allow the READ_SYS_INFO() macro to choose which primitive to use. Signed-off-by: Kai Huang <kai.huang@intel.com> --- v3 -> v4: - Change to use one primitive for each element size, similar to build_mmio_read() macro - Dan. - Rewrite changelog based on the new code. - "global metadata fields" -> "Global Metadata Fields" - Ardian. v2 -> v3: - Rename read_sys_metadata_field() to tdh_sys_rd() so the former can be used as the high level wrapper. Get rid of "stbuf_" prefix since people don't like it. - Rewrite after removing 'struct field_mapping' and reimplementing TD_SYSINFO_MAP(). --- arch/x86/virt/vmx/tdx/tdx.c | 40 ++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 18 deletions(-)