[PATCH 1/2] powerpc/fadump: allocate memory for additional parameters early

[Sourabh Jain]

Hello Michael and Hari,

On 05/11/24 13:46, Michael Ellerman wrote:
> Hi Sourabh,
>
> Sourabh Jain <sourabhjain at linux.ibm.com> writes:
>> From: Hari Bathini <hbathini at linux.ibm.com>
>>
>> Memory for passing additional parameters to fadump capture kernel
>> is allocated during subsys_initcall level, using memblock. But
>> as slab is already available by this time, allocation happens via
>> the buddy allocator. This may work for radix MMU but is likely to
> Does it even work for radix? If the memory has been given out the buddy
> allocator then it could be overwritten at any moment. Or vice-versa,
> fadump might overwrite memory used for something else.
>> fail in most cases for hash MMU as hash MMU needs this memory in
>> the first memory block for it to be accesible in real mode in the
>> capture kernel (second boot). So, allocate memory for additional
>> parameters area as soon as MMU mode is obvious.
>>
>> Fixes: 683eab94da75 ("powerpc/fadump: setup additional parameters for dump capture kernel")
>> Reported-by: Venkat Rao Bagalkote <venkat88 at linux.vnet.ibm.com>
>> Closes: https://lore.kernel.org/lkml/a70e4064-a040-447b-8556-1fd02f19383d@linux.vnet.ibm.com/T/#u
>> Cc: Mahesh Salgaonkar <mahesh at linux.ibm.com>
>> Cc: Michael Ellerman <mpe at ellerman.id.au>
>> Signed-off-by: Hari Bathini <hbathini at linux.ibm.com>
>> Signed-off-by: Sourabh Jain <sourabhjain at linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/fadump.h |  2 ++
>>   arch/powerpc/kernel/fadump.c      | 15 ++++++++++-----
>>   arch/powerpc/kernel/prom.c        |  3 +++
>>   3 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h
>> index ef40c9b6972a..978102c5129e 100644
>> --- a/arch/powerpc/include/asm/fadump.h
>> +++ b/arch/powerpc/include/asm/fadump.h
>> @@ -19,6 +19,7 @@ extern int is_fadump_active(void);
>>   extern int should_fadump_crash(void);
>>   extern void crash_fadump(struct pt_regs *, const char *);
>>   extern void fadump_cleanup(void);
>> +extern void fadump_setup_param_area(void);
>   
> You can drop extern on new declarations.

Sure I will update the function declaration.

>
>>   extern void fadump_append_bootargs(void);
>>   
>>   #else	/* CONFIG_FA_DUMP */
>> @@ -26,6 +27,7 @@ static inline int is_fadump_active(void) { return 0; }
>>   static inline int should_fadump_crash(void) { return 0; }
>>   static inline void crash_fadump(struct pt_regs *regs, const char *str) { }
>>   static inline void fadump_cleanup(void) { }
>> +static inline void fadump_setup_param_area(void) { }
>>   static inline void fadump_append_bootargs(void) { }
>>   #endif /* !CONFIG_FA_DUMP */
>>   
>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>> index a612e7513a4f..4a3f80f42118 100644
>> --- a/arch/powerpc/kernel/fadump.c
>> +++ b/arch/powerpc/kernel/fadump.c
>> @@ -1586,6 +1586,12 @@ static void __init fadump_init_files(void)
>>   		return;
>>   	}
>>   
>> +	if (fw_dump.param_area) {
>> +		rc = sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr);
>> +		if (rc)
>> +			pr_err("unable to create bootargs_append sysfs file (%d)\n", rc);
>> +	}
>> +
>>   	debugfs_create_file("fadump_region", 0444, arch_debugfs_dir, NULL,
>>   			    &fadump_region_fops);
>>   
>> @@ -1740,7 +1746,7 @@ static void __init fadump_process(void)
>>    * Reserve memory to store additional parameters to be passed
>>    * for fadump/capture kernel.
>>    */
>> -static void __init fadump_setup_param_area(void)
>> +void __init fadump_setup_param_area(void)
>>   {
>>   	phys_addr_t range_start, range_end;
>>   
>> @@ -1748,7 +1754,7 @@ static void __init fadump_setup_param_area(void)
>>   		return;
>>   
>>   	/* This memory can't be used by PFW or bootloader as it is shared across kernels */
>> -	if (radix_enabled()) {
>> +	if (early_radix_enabled()) {
>>   		/*
>>   		 * Anywhere in the upper half should be good enough as all memory
>>   		 * is accessible in real mode.
>> @@ -1776,12 +1782,12 @@ static void __init fadump_setup_param_area(void)
>>   						       COMMAND_LINE_SIZE,
>>   						       range_start,
>>   						       range_end);
>> -	if (!fw_dump.param_area || sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr)) {
>> +	if (!fw_dump.param_area) {
>>   		pr_warn("WARNING: Could not setup area to pass additional parameters!\n");
>>   		return;
>>   	}
>>   
>> -	memset(phys_to_virt(fw_dump.param_area), 0, COMMAND_LINE_SIZE);
>> +	memset(__va(fw_dump.param_area), 0, COMMAND_LINE_SIZE);
>   
> This will now be running with the MMU off, so I think it would be
> clearer to not use __va() here. Using __va() will work, but only because
> the CPU ignores the top bits of the address in real mode.

For my understanding, at what point during kernel boot the MMU is
turned on, and how can we check if it's on or off ?

>
> There are cases where it's correct to use __va() in real mode, ie. when
> you're storing a pointer for later use in virtual mode, but this is not
> one of those cases AFAICS.


Do we have any other way to reset the reserved memory that early in the 
boot?

Or should we move the memory reset for the parameter area to
fadump_setup()->fadump_init_files()  ??

@@ -1587,6 +1587,7 @@ static void __init fadump_init_files(void)
         }

         if (fw_dump.param_area) {
+              memset(__va(fw_dump.param_area), 0, COMMAND_LINE_SIZE);
                 rc = sysfs_create_file(fadump_kobj, 
&bootargs_append_attr.attr);
                 if (rc)
                         pr_err("unable to create bootargs_append sysfs 
file (%d)\n", rc);
@@ -1787,7 +1788,6 @@ void __init fadump_setup_param_area(void)
                 return;
         }

-       memset(__va(fw_dump.param_area), 0, COMMAND_LINE_SIZE);
  }


Thanks for the review Michael.

- Sourabh Jain