Hi, Dave,
Thanks a lot for your detailed review!
Dave Hansen <dave.hansen@xxxxxxxxx> 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
