| 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
| ||
|
Message-ID: <de4e1842-1ca2-44aa-b028-359008b591fd@suse.com>
Date: Fri, 30 Aug 2024 12:05:13 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: Kai Huang <kai.huang@...el.com>, dave.hansen@...el.com,
kirill.shutemov@...ux.intel.com, tglx@...utronix.de, bp@...en8.de,
peterz@...radead.org, mingo@...hat.com, hpa@...or.com,
dan.j.williams@...el.com, seanjc@...gle.com, pbonzini@...hat.com
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
rick.p.edgecombe@...el.com, isaku.yamahata@...el.com, chao.gao@...el.com,
binbin.wu@...ux.intel.com, adrian.hunter@...el.com
Subject: Re: [PATCH v3 3/8] x86/virt/tdx: Prepare to support reading other
global metadata fields
On 27.08.24 г. 10:14 ч., 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.
>
> For now the kernel only reads "TD Memory Region" (TDMR) related fields
> for module initialization. There are both immediate needs to read more
> fields for module initialization and near-future needs for other kernel
> components like KVM to run TDX guests.
> will be organized in different structures depending on their meanings.
>
> For now the kernel only reads TDMR related fields. The TD_SYSINFO_MAP()
> macro hard-codes the 'struct tdx_sys_info_tdmr' instance name. To make
> it work with different instances of different structures, extend it to
> take the structure instance name as an argument.
>
> This also means the current code which uses TD_SYSINFO_MAP() must type
> 'struct tdx_sys_info_tdmr' instance name explicitly for each use. To
> make the code easier to read, add a wrapper TD_SYSINFO_MAP_TDMR_INFO()
> which hides the instance name.
>
> TDX also support 8/16/32/64 bits metadata field element sizes. For now
> all TDMR related fields are 16-bit long thus the kernel only has one
> helper:
>
> static int read_sys_metadata_field16(u64 field_id, u16 *val) {}
>
> Future changes will need to read more metadata fields with different
> element sizes. To make the code short, extend the helper to take a
> 'void *' buffer and the buffer size so it can work with all element
> sizes.
>
> Note in this way the helper loses the type safety and the build-time
> check inside the helper cannot work anymore because the compiler cannot
> determine the exact value of the buffer size.
>
> To resolve those, add a wrapper of the helper which only works with
> u8/u16/u32/u64 directly and do build-time check, where the compiler
> can easily know both the element size (from field ID) and the buffer
> size(using sizeof()), before calling the helper.
>
> An alternative option is to provide one helper for each element size:
>
> static int read_sys_metadata_field8(u64 field_id, u8 *val) {}
> static int read_sys_metadata_field16(u64 field_id, u16 *val) {}
> ...
>
> But this will result in duplicated code given those helpers will look
> exactly the same except for the type of buffer pointer. It's feasible
> to define a macro for the body of the helper and define one entry for
> each element size to reduce the code, but it is a little bit
> over-engineering.
>
> Signed-off-by: Kai Huang <kai.huang@...el.com>
> ---
>
> 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 | 45 +++++++++++++++++++++----------------
> arch/x86/virt/vmx/tdx/tdx.h | 3 ++-
> 2 files changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 7e75c1b10838..1cd9035c783f 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,43 +270,50 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
> return 0;
> }
>
> -static int read_sys_metadata_field16(u64 field_id, u16 *val)
> +static int __read_sys_metadata_field(u64 field_id, void *val, int size)
The type of 'size' should be size_t.
> {
> u64 tmp;
> int ret;
>
> - BUILD_BUG_ON(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
> - MD_FIELD_ID_ELE_SIZE_16BIT);
> -
> - ret = read_sys_metadata_field(field_id, &tmp);
> + ret = tdh_sys_rd(field_id, &tmp);
> if (ret)
> return ret;
>
> - *val = tmp;
> + memcpy(val, &tmp, size);
>
> return 0;
> }
>
> +/* Wrapper to read one global metadata to u8/u16/u32/u64 */
> +#define read_sys_metadata_field(_field_id, _val) \
> + ({ \
> + BUILD_BUG_ON(MD_FIELD_ELE_SIZE(_field_id) != sizeof(typeof(*(_val)))); \
> + __read_sys_metadata_field(_field_id, _val, sizeof(typeof(*(_val)))); \
> + })
> +
> /*
> - * Assumes locally defined @ret and @sysinfo_tdmr to convey the error
> - * code and the 'struct tdx_sys_info_tdmr' instance to fill out.
> + * Read one global metadata field to a member of a structure instance,
> + * assuming locally defined @ret to convey the error code.
> */
> -#define TD_SYSINFO_MAP(_field_id, _member) \
> - ({ \
> - if (!ret) \
> - ret = read_sys_metadata_field16(MD_FIELD_ID_##_field_id, \
> - &sysinfo_tdmr->_member); \
> +#define TD_SYSINFO_MAP(_field_id, _stbuf, _member) \
> + ({ \
> + if (!ret) \
> + ret = read_sys_metadata_field(MD_FIELD_ID_##_field_id, \
> + &_stbuf->_member); \
> })
>
> static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
> {
> int ret = 0;
>
> - TD_SYSINFO_MAP(MAX_TDMRS, max_tdmrs);
> - TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> - TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]);
> - TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]);
> - TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]);
> +#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member) \
> + TD_SYSINFO_MAP(_field_id, sysinfo_tdmr, _member)
nit: I guess its a personal preference but honestly I think the amount
of macro indirection (3 levels) here is crazy, despite each being rather
simple. Just use TD_SYSINFO_MAP directly, saving the typing of
"sysinfo_tdmr" doesn't seem like a big deal.
You can probably take it even a bit further and simply opencode
read_sys_metadata_field macro inside TD_SYSINFO_MAP and be left with
just it, no ? No other patch in this series uses read_sys_metadata_field
stand alone, if anything factoring it out could be deferred until the
first users gets introduced.
> +
> + TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS, max_tdmrs);
> + TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr);
> + TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE, pamt_entry_size[TDX_PS_4K]);
> + TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE, pamt_entry_size[TDX_PS_2M]);
> + TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE, pamt_entry_size[TDX_PS_1G]);
>
> return ret;
> }
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 148f9b4d1140..7458f6717873 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -53,7 +53,8 @@
> #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id) \
> (((_field_id) & GENMASK_ULL(33, 32)) >> 32)
>
> -#define MD_FIELD_ID_ELE_SIZE_16BIT 1
> +#define MD_FIELD_ELE_SIZE(_field_id) \
That ELE seems a bit ambiguous, ELEM seems more natural and is in line
with other macros in the kernel.
> + (1 << MD_FIELD_ID_ELE_SIZE_CODE(_field_id))
>
> struct tdmr_reserved_area {
> u64 offset;
Powered by blists - more mailing lists