Re: [PATCH v9 3/6] powerpc/crash: add a new member to the kimage_arch struct

[Sourabh Jain <sourabhjain@linux.ibm.com>]

On 13/03/23 21:55, Laurent Dufour wrote:
> On 12/03/2023 19:11:51, Sourabh Jain wrote:
>> A new member "fdt_index" is added to the kimage_arch struct to hold
>> the index of the FDT (Flattened Device Tree) segment from the kexec
>> the segment array.
>>
>> fdt_index will provide direct access to the FDT segment in the kexec
>> segment array after the kdump kernel is loaded.
>>
>> The new attribute will be used in the arch crash hotplug handler
>> (added in upcoming patches) on every CPU and memory hotplug event.
>>
>> The fdt_index is populated for both kexec_load and kexec_file_load
>> case.
>>
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/kexec.h |  5 +++++
>>   arch/powerpc/kexec/core_64.c     | 31 +++++++++++++++++++++++++++++++
>>   2 files changed, 36 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
>> index 8090ad7d97d9d..348eb96e8ca67 100644
>> --- a/arch/powerpc/include/asm/kexec.h
>> +++ b/arch/powerpc/include/asm/kexec.h
>> @@ -103,6 +103,8 @@ void kexec_copy_flush(struct kimage *image);
>>   struct crash_mem;
>>   int update_cpus_node(void *fdt);
>>   int get_crash_memory_ranges(struct crash_mem **mem_ranges);
>> +int machine_kexec_post_load(struct kimage *image);
>> +#define machine_kexec_post_load machine_kexec_post_load
> Minor comment, when CONFIG_CRASH_HOTPLUG is not set the function is simply
> returning 0, why not defining it has an inline in that case?

We can, but if the initialization of fdt_index is taken care during the 
allocation
of kimage struct, I will move the fdt_index discovery logic to arch crash
hotplug handler. More on fdt_index initialization in the next comment.


>>   #endif
>>   
>>   #if defined(CONFIG_CRASH_DUMP) && defined(CONFIG_PPC_RTAS)
>> @@ -118,6 +120,9 @@ extern const struct kexec_file_ops kexec_elf64_ops;
>>   struct kimage_arch {
>>   	struct crash_mem *exclude_ranges;
>>   
>> +#if defined(CONFIG_CRASH_HOTPLUG)
>> +	int fdt_index;
>> +#endif
>>   	unsigned long backup_start;
>>   	void *backup_buf;
>>   	void *fdt;
>> diff --git a/arch/powerpc/kexec/core_64.c b/arch/powerpc/kexec/core_64.c
>> index 0b292f93a74cc..531486c973988 100644
>> --- a/arch/powerpc/kexec/core_64.c
>> +++ b/arch/powerpc/kexec/core_64.c
>> @@ -77,6 +77,37 @@ int machine_kexec_prepare(struct kimage *image)
>>   	return 0;
>>   }
>>   
>> +int machine_kexec_post_load(struct kimage *kimage)
>> +{
>> +#if defined(CONFIG_CRASH_HOTPLUG)
>> +	int i;
>> +	void *ptr;
>> +	unsigned long mem;
>> +
>> +	/* Mark fdt_index invalid */
>> +	kimage->arch.fdt_index = -1;
> Why is that not done in the series introducing the generic
> crash hotplug update, in do_kimage_alloc_init() ?

do_kimage_alloc_init is a generic function where as fdt_index is a ppc 
specific
attribute. If fdt_index is initialized in do_kimage_alloc_init then 
other architectures
will have build issues.

> This way there is a guarantee that the field will not be used while set by
> default to 0.

I agree that until the kernel hits the machine_kexec_post_load function 
on the
kdump kernel load path there is no way to identify the fdt_index is holding
a valid index or not.

Since there is no consumer of kimage's fdt_index attribute from the 
point of its
allocation to until it is initialized, we don't have any impact of 
fdt_index holding
value 0 (which is not valid) for sometime.

But still we can do few things to allow fdt_index attribute consumers to 
check
the sanity of fdt_index.

1. Introduce arch specific function call to initialize kimage_arch 
struct (basically fdt_index for now).
and call it inside do_kimage_alloc_init and initialize the fdt_index 
with -1 there.

2. Add another attribute in kimage_arch struct to indicate the sanity of the
fdt_index attribute. For example fdt_index_valid, if this holds zero then
the fdt_index holds invalid index. (looks inefficient to me)

Not sure is it worth doing but please let me know your opinion.

Thanks,
Sourabh Jain