| 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
| ||
|
Message-ID: <8734k47pi0.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Wed, 06 Nov 2024 16:09:11 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: 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>, <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
Hi, Dave,
Thanks a lot for your detailed review!
Dave Hansen <dave.hansen@...el.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
Powered by blists - more mailing lists