Re: [PATCH v3 1/2] arm64/mm: Allow __create_pgd_mapping() to propagate pgtable_alloc() errors

[Dev Jain <dev.jain@arm.com>]

On 15/10/25 9:35 pm, Kevin Brodsky wrote:
> On 15/10/2025 13:27, Linu Cherian wrote:
>> From: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
>>
>> arch_add_memory() is used to hotplug memory into a system but as a part
>> of its implementation it calls __create_pgd_mapping(), which uses
>> pgtable_alloc() in order to build intermediate page tables. As this path
>> was initally only used during early boot pgtable_alloc() is designed to
>> BUG_ON() on failure. However, in the event that memory hotplug is
>> attempted when the system's memory is extremely tight and the allocation
>> were to fail, it would lead to panicking the system, which is not
>> desirable. Hence update __create_pgd_mapping and all it's callers to be
>> non void and propagate -ENOMEM on allocation failure to allow system to
>> fail gracefully.
>>
>> But during early boot if there is an allocation failure, we want the
>> system to panic, hence create a wrapper around __create_pgd_mapping()
>> called early_create_pgd_mapping() which is designed to panic, if ret
>> is non zero value. All the init calls are updated to use this wrapper
>> rather than the modified __create_pgd_mapping() to restore
>> functionality.
>>
>> Fixes: 4ab215061554 ("arm64: Add memory hotplug support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
>> Signed-off-by: Linu Cherian <linu.cherian@arm.com>
> A couple more nits below (sorry I didn't catch them earlier), but otherwise:
>
> Reviewed-by: Kevin Brodsky <kevin.brodsky@arm.com>
>
>> ---
>> Changelog:
>>
>> v3:
>> * Fixed a maybe-uninitialized case in alloc_init_pud
>> * Added Fixes tag and CCed stable
>> * Few other trivial cleanups
>>
>>   arch/arm64/mm/mmu.c | 210 ++++++++++++++++++++++++++++----------------
>>   1 file changed, 132 insertions(+), 78 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index b8d37eb037fc..638cb4df31a9 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -49,6 +49,8 @@
>>   #define NO_CONT_MAPPINGS	BIT(1)
>>   #define NO_EXEC_MAPPINGS	BIT(2)	/* assumes FEAT_HPDS is not used */
>>   
>> +#define INVALID_PHYS_ADDR	(-1ULL)
>> +
>>   DEFINE_STATIC_KEY_FALSE(arm64_ptdump_lock_key);
>>   
>>   u64 kimage_voffset __ro_after_init;
>> @@ -194,11 +196,11 @@ static void init_pte(pte_t *ptep, unsigned long addr, unsigned long end,
>>   	} while (ptep++, addr += PAGE_SIZE, addr != end);
>>   }
>>   
>> -static void alloc_init_cont_pte(pmd_t *pmdp, unsigned long addr,
>> -				unsigned long end, phys_addr_t phys,
>> -				pgprot_t prot,
>> -				phys_addr_t (*pgtable_alloc)(enum pgtable_type),
>> -				int flags)
>> +static int alloc_init_cont_pte(pmd_t *pmdp, unsigned long addr,
>> +			       unsigned long end, phys_addr_t phys,
>> +			       pgprot_t prot,
>> +			       phys_addr_t (*pgtable_alloc)(enum pgtable_type),
>> +			       int flags)
>>   {
>>   	unsigned long next;
>>   	pmd_t pmd = READ_ONCE(*pmdp);
>> @@ -213,6 +215,8 @@ static void alloc_init_cont_pte(pmd_t *pmdp, unsigned long addr,
>>   			pmdval |= PMD_TABLE_PXN;
>>   		BUG_ON(!pgtable_alloc);
>>   		pte_phys = pgtable_alloc(TABLE_PTE);
>> +		if (pte_phys == INVALID_PHYS_ADDR)
>> +			return -ENOMEM;
>>   		ptep = pte_set_fixmap(pte_phys);
>>   		init_clear_pgtable(ptep);
>>   		ptep += pte_index(addr);
>> @@ -244,12 +248,15 @@ static void alloc_init_cont_pte(pmd_t *pmdp, unsigned long addr,
>>   	 * walker.
>>   	 */
>>   	pte_clear_fixmap();
>> +
>> +	return 0;
>>   }
>>   
>> -static void init_pmd(pmd_t *pmdp, unsigned long addr, unsigned long end,
>> -		     phys_addr_t phys, pgprot_t prot,
>> -		     phys_addr_t (*pgtable_alloc)(enum pgtable_type), int flags)
>> +static int init_pmd(pmd_t *pmdp, unsigned long addr, unsigned long end,
>> +		    phys_addr_t phys, pgprot_t prot,
>> +		    phys_addr_t (*pgtable_alloc)(enum pgtable_type), int flags)
>>   {
>> +	int ret;
> Nit: that could be added to the else block instead (makes it clearer
> it's not used for the final return, that got me confused when re-reading
> this patch).

+1.

Reviewed-by: Dev Jain <dev.jain@arm.com>

>
>
> [...]