linux-kernel - Re: [PATCH -V3 RESEND] x86, tdx, memory hotplug: Check whole hot-adding memory range for TDX

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: <702d9dbd-fb7d-44d0-a352-e78adf92254e@intel.com>
Date: Tue, 5 Nov 2024 10:34:59 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Huang Ying <ying.huang@...el.com>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
 Dave Hansen <dave.hansen@...ux.intel.com>,
 "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>
Cc: x86@...nel.org, linux-coco@...ts.linux.dev, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, Dan Williams <dan.j.williams@...el.com>,
 Kai Huang <kai.huang@...el.com>, David Hildenbrand <david@...hat.com>,
 Oscar Salvador <osalvador@...e.de>, "H. Peter Anvin" <hpa@...or.com>,
 Andy Lutomirski <luto@...nel.org>
Subject: Re: [PATCH -V3 RESEND] x86, tdx, memory hotplug: Check whole
 hot-adding memory range for TDX

First and foremost, this touches x86 and core mm code, but it seem to
solidly lean on being an x86 thing.  If anyone thinks this isn't x86
tree material, please speak up.

On 10/31/24 01:51, Huang Ying wrote:
> Therefore, this patch checks the TDX compatibility of the whole

Please zap the "this patch" nomenclature.  It showed up in a couple of
places.  ChatGPT is actually pretty good at this kind of stuff and using
imperative voice.

> hot-adding memory range through a newly added architecture specific
> function (arch_check_hotplug_memory_range()).  If this patch rejects
> the memory hot-adding for TDX compatibility, it will output a kernel
> log message like below,
> 
>   virt/tdx: Reject hot-adding memory range: 0xXXXXXXXX-0xXXXXXXXX for TDX compatibility.

I think this is more clear and much more succinct:

  virt/tdx: Rejecting incompatible memory range: 0xXXXXXXXX-0xXXXXXXXX


> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index eba178996d84..6db5da34e4ba 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -116,11 +116,13 @@ static inline u64 sc_retry(sc_func_t func, u64 fn,
>  int tdx_cpu_enable(void);
>  int tdx_enable(void);
>  const char *tdx_dump_mce_info(struct mce *m);
> +int tdx_check_hotplug_memory_range(u64 start, u64 size);
>  #else
>  static inline void tdx_init(void) { }
>  static inline int tdx_cpu_enable(void) { return -ENODEV; }
>  static inline int tdx_enable(void)  { return -ENODEV; }
>  static inline const char *tdx_dump_mce_info(struct mce *m) { return NULL; }
> +static inline int tdx_check_hotplug_memory_range(u64 start, u64 size) { return 0; }
>  #endif	/* CONFIG_INTEL_TDX_HOST */
>  
>  #endif /* !__ASSEMBLY__ */
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index ff253648706f..30a4ad4272ce 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -55,6 +55,7 @@
>  #include <asm/uv/uv.h>
>  #include <asm/setup.h>
>  #include <asm/ftrace.h>
> +#include <asm/tdx.h>
>  
>  #include "mm_internal.h"
>  
> @@ -974,6 +975,11 @@ int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages,
>  	return ret;
>  }
>  
> +int arch_check_hotplug_memory_range(u64 start, u64 size)
> +{
> +	return tdx_check_hotplug_memory_range(start, size);
> +}
> +
>  int arch_add_memory(int nid, u64 start, u64 size,
>  		    struct mhp_params *params)
>  {
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 4e2b2e2ac9f9..f70b4ebe7cc5 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1388,36 +1388,37 @@ static bool is_tdx_memory(unsigned long start_pfn, unsigned long end_pfn)
>  	return false;
>  }
>  
> -static int tdx_memory_notifier(struct notifier_block *nb, unsigned long action,
> -			       void *v)
> +/*
> + * We don't allow mixture of TDX and !TDX memory in the buddy so we
> + * won't run into trouble when launching encrypted VMs that really
> + * need TDX-capable memory.
> + */

No "we's" please.

I'd probably explain it like this:

/*
 * By convention, all RAM in the buddy must be TDX compatible whenever
 * TDX is enabled. This avoids having to do extra work later to find
 * TDX compatible memory to run VMs. Enforce that convention and reject
 * attempted hot-adds of any TDX-incompatible ranges.
 *
 * Returns 0 to pass the checks and allow the hot-add
 * Returns -ERRNO to fail the checks and reject the hot-add
 */

> +int tdx_check_hotplug_memory_range(u64 start, u64 size)
>  {
> -	struct memory_notify *mn = v;
> -
> -	if (action != MEM_GOING_ONLINE)
> -		return NOTIFY_OK;
> +	u64 start_pfn = PHYS_PFN(start);
> +	u64 end_pfn = PHYS_PFN(start + size);

Nit:                ^ please vertically align those

>  	/*
>  	 * Empty list means TDX isn't enabled.  Allow any memory
> -	 * to go online.
> +	 * to be hot-added.
>  	 */
>  	if (list_empty(&tdx_memlist))
> -		return NOTIFY_OK;
> +		return 0;

The changelog also needs _some_ discussion of why the locking context is
the same between the old and new uses of this function and why this
doesn't need any locking _here_.

>  	/*
>  	 * The TDX memory configuration is static and can not be
> -	 * changed.  Reject onlining any memory which is outside of
> +	 * changed.  Reject hot-adding any memory which is outside of
>  	 * the static configuration whether it supports TDX or not.
>  	 */
> -	if (is_tdx_memory(mn->start_pfn, mn->start_pfn + mn->nr_pages))
> -		return NOTIFY_OK;
> +	if (is_tdx_memory(start_pfn, end_pfn))
> +		return 0;
>  
> -	return NOTIFY_BAD;
> +	pr_info("Reject hot-adding memory range: %#llx-%#llx for TDX compatibility.\n",
> +		start, start + size);
> +
> +	return -EINVAL;
>  }