Re: [PATCH v3 7/8] x86/virt/tdx: Reduce TDMR's reserved areas by using CMRs to find memory holes

[Nikolay Borisov]

On 27.08.24 г. 10:14 ч., Kai Huang wrote:
<snip>

> [2] Convertible Memory Regions of the problematic platform:
>
>    virt/tdx: CMR: [0x100000, 0x6f800000)
>    virt/tdx: CMR: [0x100000000, 0x107a000000)
>    virt/tdx: CMR: [0x1080000000, 0x207c000000)
>    virt/tdx: CMR: [0x2080000000, 0x307c000000)
>    virt/tdx: CMR: [0x3080000000, 0x407c000000)
>
> Fixes: dde3b60d572c9 ("x86/virt/tdx: Designate reserved areas for all TDMRs")
> Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
> ---

<snpi>

> ---
>   arch/x86/virt/vmx/tdx/tdx.c | 105 ++++++++++++++++++++++++++++++------
>   arch/x86/virt/vmx/tdx/tdx.h |  13 +++++
>   2 files changed, 102 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 0fb673dd43ed..fa335ab1ae92 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -331,6 +331,72 @@ static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version
>   	return ret;
>   }
>   
> +/* Update the @sysinfo_cmr->num_cmrs to trim tail empty CMRs */
> +static void trim_empty_tail_cmrs(struct tdx_sys_info_cmr *sysinfo_cmr)
> +{
> +	int i;
> +
> +	for (i = 0; i < sysinfo_cmr->num_cmrs; i++) {
> +		u64 cmr_base = sysinfo_cmr->cmr_base[i];
> +		u64 cmr_size = sysinfo_cmr->cmr_size[i];
> +
> +		if (!cmr_size) {
> +			WARN_ON_ONCE(cmr_base);
> +			break;
> +		}
> +
> +		/* TDX architecture: CMR must be 4KB aligned */
> +		WARN_ON_ONCE(!PAGE_ALIGNED(cmr_base) ||
> +				!PAGE_ALIGNED(cmr_size));
> +	}
> +
> +	sysinfo_cmr->num_cmrs = i;
> +}
> +
> +static int get_tdx_sys_info_cmr(struct tdx_sys_info_cmr *sysinfo_cmr)
> +{
> +	int i, ret = 0;
> +
> +#define TD_SYSINFO_MAP_CMR_INFO(_field_id, _member)	\
> +	TD_SYSINFO_MAP(_field_id, sysinfo_cmr, _member)
> +
> +	TD_SYSINFO_MAP_CMR_INFO(NUM_CMRS, num_cmrs);
> +
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < sysinfo_cmr->num_cmrs; i++) {
> +		TD_SYSINFO_MAP_CMR_INFO(CMR_BASE(i), cmr_base[i]);

Yeah, this is just golden, not only are you going through 3 levels of 
indirection to expand the TD_SYSINFO_MAP but you also top it with one 
final one MD_FIELD_ID_CMR_BASE(i). I'm fine with the CMR_BASE/SIZE 
macros, but the way you introduce 3 levels of macros just to avoid typing

TD_SYSINFO_MAP(foo, bar, zoo)

makes no sense.

<snip>