Re: [PATCH v2 1/4] kaslr: parse the extended movable_node=nn[KMG]@ss[KMG]

[Chao Fan <fanc.fnst@cn.fujitsu.com>]

On Mon, Nov 13, 2017 at 04:10:41PM +0800, Baoquan He wrote:
>Hi Chao,
Hi Baoquan,

Thanks for your reply.
>
>Please think more on your patches, better to discuss with your
>colleagues and ask them to help review before your post.
>
>On 11/01/17 at 07:32pm, Chao Fan wrote:
>> Extend the movable_node to movable_node=nn[KMG]@ss[KMG].
>
>Firstly, apparently we can't make use of movable_node kernel parameter
>and extend it to pass in the immovable_nodes. In fact we are passing in
>the memory ranges which can be used for kernel data, not sure if
>kernelcore= is OK, or we can make a new one.

OK, I will think more about this.

>
>Think more about this, or consult your colleagues, FJ has many experts
>on mem hotplugging. Choose an proper one or create a new one.
>
>> 
>> Since in current code, kaslr may choose the memory region in movable
>> nodes to extract kernel, which will make the nodes can't be hot-removed.
>> To solve it, we can specify the region in immovable node. Create
>> immovable_mem to store the regions in immovable_mem, where should be
>> chosen by kaslr.
>> 
>> Multiple regions can be specified, comma delimited.
>> Considering the usage of memory, only support for 4 regions.
>> 4 regions contains 2 nodes at least, enough for kernel to extract.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 76 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 75 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 17818ba6906f..0a591c0023f1 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -107,6 +107,15 @@ 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
>> +
>> +/* Store the memory regions in immovable node */
>> +static struct mem_vector immovable_mem[MAX_IMMOVABLE_MEM];
>> +
>> +/* The immovable regions user specify, not more than 4 */
>> +static int num_immovable_region;
>
>I am fine we support 4 immvoable_mem for now. Please discuss with yor
>colleagues, make sure if 4 is OK.

OK. I will ask more colleagues to decide this.
In my opinion, 4 can contain 2 nodes at least.
The memory region will be enough.
Thanks for your advice.

>
>> +
>>  static bool mem_overlaps(struct mem_vector *one, struct mem_vector *two)
>>  {
>>  	/* Item one is entirely before item two. */
>> @@ -167,6 +176,38 @@ 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;
>> +
>> +	/* We support nn[KMG]@ss[KMG] and nn[KMG]. */
>> +	switch (*p) {
>> +	case '@':
>> +		*start = memparse(p + 1, &p);
>> +		return 0;
>> +	default:
>> +		/*
>> +		 * If w/o offset, only size specified, movable_node=nn[KMG]
>> +		 * has the same behaviour as movable_node=nn[KMG]@0. It means
>> +		 * the region starts from 0.
>> +		 */
>> +		*start = 0;
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>>  static void mem_avoid_memmap(char *str)
>>  {
>>  	static int i;
>> @@ -206,6 +247,36 @@ static void mem_avoid_memmap(char *str)
>>  		memmap_too_large = true;
>>  }
>>  
>> +#ifdef CONFIG_MEMORY_HOTPLUG
>> +static void mem_mark_immovable(char *str)
>
>I know you try to imitate the function memblock_mark_hotplug(), but do
>you really think you are marking the immovable mem regions? In below

Well, this depends to what the users specify.
If users specify the immovable mem regions, the stored data will be
immovable.
If users specify the movable mem regions, it will be movable regions.

If this puzzle users, I will make a new name.

>code?
>
>Even if use parse_immovable_mem_regions(), please do not use
>mem_mark_immovable.

Thanks for your advice, I will think about it.
>
>> +{
>> +	static int i;
>> +
>> +	while (str && (i < MAX_IMMOVABLE_MEM)) {
>> +		int rc;
>> +		unsigned long long start, size;
>> +		char *k = strchr(str, ',');
>> +
>> +		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++;
>> +	}
>> +	num_immovable_region = i;
>> +}
>> +#else
>> +static inline void mem_mark_immovable(char *str)
>> +{
>> +}
>> +#endif
>> +
>>  static int handle_mem_memmap(void)
>
>Please think of a better function name when you add immovable memory
>regions parsing in. Clearly it's not a right name now.

OK, I will try to make a new one and change the code.
Thank you so much for the advice.

Thanks,
Chao Fan

>
>>  {
>>  	char *args = (char *)get_cmd_line_ptr();
>> @@ -214,7 +285,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 +311,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);
>>  		} else if (!strcmp(param, "mem")) {
>>  			char *p = val;
>>  
>> -- 
>> 2.13.6
>> 
>> 
>> 
>
>