Re: [V2] powerpc/mm: Limit the max memory we can support

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

Michael Ellerman <mpe@ellerman.id.au> writes:

> On Fri, 2015-29-05 at 08:20:18 UTC, "Aneesh Kumar K.V" wrote:
>> We need to limit the max memory based on Linux page table format.
>> Add checks to limit memory based on pte size. Also limit the memory
>> based on MAX_PHSYSMEM_BITS.
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> Changes from V1:
>> * Update commit message. 4K can handle 64TB
>> * Also limit based on MAX_PHSYSMEM_BITS
>> 
>>  arch/powerpc/include/asm/mmu.h       |  8 ++++++++
>>  arch/powerpc/include/asm/sparsemem.h |  2 --
>>  arch/powerpc/kernel/prom.c           | 25 ++++++++++++++++++++++---
>>  3 files changed, 30 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
>> index 3d5abfe6ba67..d44d49093c8d 100644
>> --- a/arch/powerpc/include/asm/mmu.h
>> +++ b/arch/powerpc/include/asm/mmu.h
>> @@ -200,6 +200,14 @@ static inline void assert_pte_locked(struct mm_struct *mm, unsigned long addr)
>>  #  include <asm/mmu-8xx.h>
>>  #endif
>>  
>> +#ifdef CONFIG_PHYS_64BIT
>> +/*
>> + * Max supported memory on 64bit system is 64TB.
>
> Can you document in the comment where the limit comes from?

Will update the patch addressing all your feedback. But I would request
to drop this patch from the series for now. We need further fixes [1] in
this area and I will do a separate series addressing all the issues.

[1] Right now we ALIGN the total memory with PAGE_SIZE. That is not
really correct if we end up doing kernel linear mapping with 16MB size. 

>
>> + */
>> +#define MAX_PHYSMEM_BITS        46
>> +#else
>> +#define MAX_PHYSMEM_BITS        32
>> +#endif
>>  
>>  #endif /* __KERNEL__ */
>>  #endif /* _ASM_POWERPC_MMU_H_ */
>> diff --git a/arch/powerpc/include/asm/sparsemem.h b/arch/powerpc/include/asm/sparsemem.h
>> index f6fc0ee813d7..fc3808378893 100644
>> --- a/arch/powerpc/include/asm/sparsemem.h
>> +++ b/arch/powerpc/include/asm/sparsemem.h
>> @@ -11,8 +11,6 @@
>>  #define SECTION_SIZE_BITS       24
>>  
>>  #define MAX_PHYSADDR_BITS       46
>> -#define MAX_PHYSMEM_BITS        46
>
> Is there now no link between those two?
>
>>  #ifdef CONFIG_MEMORY_HOTPLUG
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index 308c5e15676b..c09315b32ca7 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -698,9 +698,28 @@ void __init early_init_devtree(void *params)
>>  #endif
>>  		reserve_crashkernel();
>>  	early_reserve_mem();
>> -
>> -	/* Ensure that total memory size is page-aligned. */
>> -	limit = ALIGN(memory_limit ?: memblock_phys_mem_size(), PAGE_SIZE);
>> +	/*
>> +	 * if not specified limit the memory based on the pfn count that
>> +	 * we can fit in pte_t. Also ensure that total memory size is
>> +	 * page-aligned.
>
> Shouldn't you do the logic below even if memory_limit is specified? Otherwise
> someone can specify a really large memory_limit which will then overflow.
>
>> +	 */
>> +	if (!memory_limit) {
>> +		int bit_count;
>> +		phys_addr_t pte_mem_limit;
>> +
>> +		limit = memblock_phys_mem_size();
>> +		if (limit >= (1ULL << MAX_PHYSMEM_BITS))
>> +			limit = (1ULL << MAX_PHYSMEM_BITS) - 1;
>> +
>> +		BUILD_BUG_ON(sizeof(pte_basic_t) > 8);
>> +		bit_count = (sizeof(pte_basic_t) * 8) - PTE_RPN_SHIFT + PAGE_SHIFT;
>> +		pte_mem_limit = ~0ULL >> (64 - bit_count);
>
> It's fairly obvious what you're doing here, but a bit of a comment wouldn't hurt.
>
>> +		if (limit > pte_mem_limit)
>> +			limit = pte_mem_limit;
>> +	} else
>> +		limit = memory_limit;
>> +
>> +	limit = ALIGN(limit, PAGE_SIZE);
>
> cheers