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

["Huang, Ying" <ying.huang@intel.com>]

Hi, Dave,

Thanks a lot for your detailed review!

Dave Hansen <dave.hansen@intel.com> writes:

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

Sure.  Will do that.

>> 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

Yes.  This looks better, will use this in the next version.

>
>> 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
>  */

This looks better, Thanks!  Will use it in the next version.

>> +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

Sure.  Will do that in the next version.

>>  	/*
>>  	 * 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_.

Sure.  Will do that in the next version.

>>  	/*
>>  	 * 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;
>>  }

--
Best Regards,
Huang, Ying