From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751549AbdJTDl2 (ORCPT ); Thu, 19 Oct 2017 23:41:28 -0400 Received: from mail.cn.fujitsu.com ([183.91.158.132]:32299 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750905AbdJTDl1 (ORCPT ); Thu, 19 Oct 2017 23:41:27 -0400 X-IronPort-AV: E=Sophos;i="5.43,368,1503331200"; d="scan'208";a="29401773" Subject: Re: [PATCH 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG] To: Chao Fan References: <20171019100243.25259-1-fanc.fnst@cn.fujitsu.com> <20171019100243.25259-2-fanc.fnst@cn.fujitsu.com> <5e20f27e-5658-5d0c-4cb2-41b39124a0d6@cn.fujitsu.com> <20171020032202.GA5635@localhost.localdomain> CC: , , , , , , , , From: Dou Liyang Message-ID: <0fe05bd2-1155-0016-9de7-688812f9dc00@cn.fujitsu.com> Date: Fri, 20 Oct 2017 11:41:19 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20171020032202.GA5635@localhost.localdomain> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.226.106] X-yoursite-MailScanner-ID: C3C0748029C1.AA408 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: douly.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At 10/20/2017 11:22 AM, Chao Fan wrote: > On Fri, Oct 20, 2017 at 11:04:42AM +0800, Dou Liyang wrote: >> Hi Chao, >> > Hi Dou-san, > >> At 10/19/2017 06:02 PM, Chao Fan wrote: >>> Extend the movable_node to movable_node=nn[KMG]@ss[KMG]. >>> Since in current code, kaslr may choose the memory region in hot-pluggable >>> nodes. So we can specific the region in immovable node. And store the >>> regions in immovable_mem. >>> >> >> I guess you may mean that: >> >> In current Linux with KASLR. Kernel may choose a memory region in >> movable node for extracting kernel code, which will make the node >> can't be hot-removed. >> >> Solve it by only specific the region in immovable node. So create >> immovable_mem to store the region of movable node, and only choose >> the memory in immovable_mem array. >> > > Thanks for the explaination. > >>> Multiple regions can be specified, comma delimited. >>> Considering the usage of memory, only support for 4 regions. >>> 4 regions contains 2 nodes at least, and is enough for kernel to >>> extract. >>> >>> Signed-off-by: Chao Fan >>> --- >>> arch/x86/boot/compressed/kaslr.c | 63 +++++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 62 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c >>> index 17818ba6906f..3c1f5204693b 100644 >>> --- a/arch/x86/boot/compressed/kaslr.c >>> +++ b/arch/x86/boot/compressed/kaslr.c >>> @@ -107,6 +107,12 @@ enum mem_avoid_index { >>> >>> static struct mem_vector mem_avoid[MEM_AVOID_MAX]; >>> >>> +/* Only supporting at most 4 immovable memory regions with kaslr */ >>> +#define MAX_IMMOVABLE_MEM 4 >>> + >>> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM]; >>> +static int num_immovable_region; >>> + >>> static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two) >>> { >>> /* Item one is entirely before item two. */ >>> @@ -167,6 +173,28 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size) >>> return -EINVAL; >>> } >>> >>> +static int parse_immovable_mem(char *p, >>> + unsigned long long *start, >>> + unsigned long long *size) >>> +{ >>> + char *oldp; >>> + >>> + if (!p) >>> + return -EINVAL; >>> + >>> + oldp = p; >>> + *size = memparse(p, &p); >>> + if (p == oldp) >>> + return -EINVAL; >>> + >>> + if (*p == '@') { >>> + *start = memparse(p + 1, &p); >>> + return 0; >>> + } >>> + >> >> Here you don't consider that if @ss[KMG] is omitted. > > Yes, will add. Many thanks. > >> >>> + return -EINVAL; >>> +} >>> + >>> static void mem_avoid_memmap(char *str) >>> { >>> static int i; >>> @@ -206,6 +234,36 @@ static void mem_avoid_memmap(char *str) >>> memmap_too_large = true; >>> } >>> >>> +#ifdef CONFIG_MEMORY_HOTPLUG >>> +static void mem_mark_immovable(char *str) >>> +{ >>> + int i = 0; >>> + >> >> you have use num_immovable_region, 'i' is useless. just remove it. > > Using num_immovable_region makes code too long. Using i will be > clear and make sure shoter than 80 characters. Oh, God, that's horrific. Did you find that your code is wrong? num_immovable_region will be reset each time. Thanks, dou. > >> >>> + while (str && (i < MAX_IMMOVABLE_MEM)) { >>> + int rc; >>> + unsigned long long start, size; >>> + char *k = strchr(str, ','); >>> + >> >> Why do you put this definition here? IMO, moving it out is better. >> >>> + if (k) >>> + *k++ = 0; >>> + >>> + rc = parse_immovable_mem(str, &start, &size); >>> + if (rc < 0) >>> + break; >>> + str = k; >>> + >>> + immovable_mem[i].start = start; >>> + immovable_mem[i].size = size; >>> + i++; >> >> Replace it with num_immovable_region > > ditto... > Why do you care this little variable so much. > >> >>> + } >>> + num_immovable_region = i; >> >> Just remove it. > > ditto. > >> >>> +} >>> +#else >>> +static inline void mem_mark_immovable(char *str) >>> +{ >>> +} >>> +#endif >>> + >>> static int handle_mem_memmap(void) >>> { >>> char *args = (char *)get_cmd_line_ptr(); >>> @@ -214,7 +272,8 @@ static int handle_mem_memmap(void) >>> char *param, *val; >>> u64 mem_size; >>> >>> - if (!strstr(args, "memmap=") && !strstr(args, "mem=")) >>> + if (!strstr(args, "memmap=") && !strstr(args, "mem=") && >>> + !strstr(args, "movable_node=")) >>> return 0; >>> >>> tmp_cmdline = malloc(len + 1); >>> @@ -239,6 +298,8 @@ static int handle_mem_memmap(void) >>> >>> if (!strcmp(param, "memmap")) { >>> mem_avoid_memmap(val); >>> + } else if (!strcmp(param, "movable_node")) { >>> + mem_mark_immovable(val); >> >> AFAIK, handle_mem_memmap() is invoked in mem_avoid_init(), which is used to >> avoid mem. But, here the value of immovable node is the memory >> you want to mark and use, it is better that we split it out. > > There is existing and useful code, so it's better to reuse but not > re-write. > >> >> BTW, Using movable_node to store the memory of immovable node is >> strange and make me confuse. How about adding a new command option. >> > > I have added the document, if you think there are problems, please let > me know. > > Thanks, > Chao Fan > >> Thanks, >> dou. >>> } else if (!strcmp(param, "mem")) { >>> char *p = val; >>> >>> >