| 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
| ||
|
Date: Fri, 6 Jan 2023 11:24:51 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Kai Huang <kai.huang@...el.com>, linux-kernel@...r.kernel.org,
kvm@...r.kernel.org
Cc: linux-mm@...ck.org, peterz@...radead.org, tglx@...utronix.de,
seanjc@...gle.com, pbonzini@...hat.com, dan.j.williams@...el.com,
rafael.j.wysocki@...el.com, kirill.shutemov@...ux.intel.com,
ying.huang@...el.com, reinette.chatre@...el.com,
len.brown@...el.com, tony.luck@...el.com, ak@...ux.intel.com,
isaku.yamahata@...el.com, chao.gao@...el.com,
sathyanarayanan.kuppuswamy@...ux.intel.com, bagasdotme@...il.com,
sagis@...gle.com, imammedo@...hat.com
Subject: Re: [PATCH v8 08/16] x86/virt/tdx: Add placeholder to construct TDMRs
to cover all TDX memory regions
> +struct tdmr_info_list {
> + struct tdmr_info *first_tdmr;
This is named badly. This is really a pointer to an array. While it
_does_ of course point to the first member of the array, the naming
should make it clear that there are multiple tdmr_infos here.
> + int tdmr_sz;
> + int max_tdmrs;
> + int nr_tdmrs; /* Actual number of TDMRs */
> +};
This 'tdmr_info_list's is declared in an unfortunate place. I thought
the tdmr_size_single() function below was related to it.
Also, tdmr_sz and max_tdmrs can both be derived from 'sysinfo'. Do they
really need to be stored here? If so, I think I'd probably do something
like this with the structure:
struct tdmr_info_list {
struct tdmr_info *tdmrs;
int nr_consumed_tdmrs; // How many @tdmrs are in use
/* Metadata for freeing this structure: */
int tdmr_sz; // Size of one 'tdmr_info' (has a flex array)
int max_tdmrs; // How many @tdmrs are allocated
};
Modulo whataver folks are doing for comments these days.
> +/* Calculate the actual TDMR size */
> +static int tdmr_size_single(u16 max_reserved_per_tdmr)
> +{
> + int tdmr_sz;
> +
> + /*
> + * The actual size of TDMR depends on the maximum
> + * number of reserved areas.
> + */
> + tdmr_sz = sizeof(struct tdmr_info);
> + tdmr_sz += sizeof(struct tdmr_reserved_area) * max_reserved_per_tdmr;
> +
> + return ALIGN(tdmr_sz, TDMR_INFO_ALIGNMENT);
> +}
> +
> +static int alloc_tdmr_list(struct tdmr_info_list *tdmr_list,
> + struct tdsysinfo_struct *sysinfo)
> +{
> + size_t tdmr_sz, tdmr_array_sz;
> + void *tdmr_array;
> +
> + tdmr_sz = tdmr_size_single(sysinfo->max_reserved_per_tdmr);
> + tdmr_array_sz = tdmr_sz * sysinfo->max_tdmrs;
> +
> + /*
> + * To keep things simple, allocate all TDMRs together.
> + * The buffer needs to be physically contiguous to make
> + * sure each TDMR is physically contiguous.
> + */
> + tdmr_array = alloc_pages_exact(tdmr_array_sz,
> + GFP_KERNEL | __GFP_ZERO);
> + if (!tdmr_array)
> + return -ENOMEM;
> +
> + tdmr_list->first_tdmr = tdmr_array;
> + /*
^ probably missing whitepsace before the comment
> + * Keep the size of TDMR to find the target TDMR
> + * at a given index in the TDMR list.
> + */
> + tdmr_list->tdmr_sz = tdmr_sz;
> + tdmr_list->max_tdmrs = sysinfo->max_tdmrs;
> + tdmr_list->nr_tdmrs = 0;
> +
> + return 0;
> +}
> +
> +static void free_tdmr_list(struct tdmr_info_list *tdmr_list)
> +{
> + free_pages_exact(tdmr_list->first_tdmr,
> + tdmr_list->max_tdmrs * tdmr_list->tdmr_sz);
> +}
> +
> +/*
> + * Construct a list of TDMRs on the preallocated space in @tdmr_list
> + * to cover all TDX memory regions in @tmb_list based on the TDX module
> + * information in @sysinfo.
> + */
> +static int construct_tdmrs(struct list_head *tmb_list,
> + struct tdmr_info_list *tdmr_list,
> + struct tdsysinfo_struct *sysinfo)
> +{
> + /*
> + * TODO:
> + *
> + * - Fill out TDMRs to cover all TDX memory regions.
> + * - Allocate and set up PAMTs for each TDMR.
> + * - Designate reserved areas for each TDMR.
> + *
> + * Return -EINVAL until constructing TDMRs is done
> + */
> + return -EINVAL;
> +}
> +
> static int init_tdx_module(void)
> {
> /*
> @@ -358,6 +439,7 @@ static int init_tdx_module(void)
> TDSYSINFO_STRUCT_SIZE, TDSYSINFO_STRUCT_ALIGNMENT);
> struct cmr_info cmr_array[MAX_CMRS] __aligned(CMR_INFO_ARRAY_ALIGNMENT);
> struct tdsysinfo_struct *sysinfo = &PADDED_STRUCT(tdsysinfo);
> + struct tdmr_info_list tdmr_list;
> int ret;
>
> ret = tdx_get_sysinfo(sysinfo, cmr_array);
> @@ -380,11 +462,19 @@ static int init_tdx_module(void)
> if (ret)
> goto out;
>
> + /* Allocate enough space for constructing TDMRs */
> + ret = alloc_tdmr_list(&tdmr_list, sysinfo);
> + if (ret)
> + goto out_free_tdx_mem;
> +
> + /* Cover all TDX-usable memory regions in TDMRs */
> + ret = construct_tdmrs(&tdx_memlist, &tdmr_list, sysinfo);
> + if (ret)
> + goto out_free_tdmrs;
> +
> /*
> * TODO:
> *
> - * - Construct a list of TDMRs to cover all TDX-usable memory
> - * regions.
> * - Pick up one TDX private KeyID as the global KeyID.
> * - Configure the TDMRs and the global KeyID to the TDX module.
> * - Configure the global KeyID on all packages.
> @@ -393,6 +483,16 @@ static int init_tdx_module(void)
> * Return error before all steps are done.
> */
> ret = -EINVAL;
> +out_free_tdmrs:
> + /*
> + * Free the space for the TDMRs no matter the initialization is
> + * successful or not. They are not needed anymore after the
> + * module initialization.
> + */
> + free_tdmr_list(&tdmr_list);
> +out_free_tdx_mem:
> + if (ret)
> + free_tdx_memlist(&tdx_memlist);
> out:
> /*
> * @tdx_memlist is written here and read at memory hotplug time.
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 6d32f62e4182..d0c762f1a94c 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -90,6 +90,29 @@ struct tdsysinfo_struct {
> DECLARE_FLEX_ARRAY(struct cpuid_config, cpuid_configs);
> } __packed;
>
> +struct tdmr_reserved_area {
> + u64 offset;
> + u64 size;
> +} __packed;
> +
> +#define TDMR_INFO_ALIGNMENT 512
> +
> +struct tdmr_info {
> + u64 base;
> + u64 size;
> + u64 pamt_1g_base;
> + u64 pamt_1g_size;
> + u64 pamt_2m_base;
> + u64 pamt_2m_size;
> + u64 pamt_4k_base;
> + u64 pamt_4k_size;
> + /*
> + * Actual number of reserved areas depends on
> + * 'struct tdsysinfo_struct'::max_reserved_per_tdmr.
> + */
> + DECLARE_FLEX_ARRAY(struct tdmr_reserved_area, reserved_areas);
> +} __packed __aligned(TDMR_INFO_ALIGNMENT);
> +
> /*
> * Do not put any hardware-defined TDX structure representations below
> * this comment!
Powered by blists - more mailing lists