linux-kernel - Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other global metadata fields

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fce898e6-0296-4c5e-9e6a-6b5e3fc87b95@suse.com>
Date: Thu, 26 Sep 2024 09:27:00 +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, adrian.hunter@...el.com
Subject: Re: [PATCH v4 3/8] x86/virt/tdx: Prepare to support reading other
 global metadata fields



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