Re: [PATCH v7 3/8] powerpc/crash: update kimage_arch struct

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

Hello Laurent,

On 20/01/23 00:27, Laurent Dufour wrote:
> On 15/01/2023 16:02:01, Sourabh Jain wrote:
>> Add a new member "fdt_index" to kimage_arch struct to hold the index of
>> the FDT (Flattened Device Tree) segment in the kexec segment array.
>>
>> Having direct access to FDT segment will help arch crash hotplug handler
>> to avoid looping kexec segment array to identify the FDT segment index
>> for every FDT update on hotplug events.
>>
>> The fdt_index is initialized during the kexec load for both kexec_load and
>> kexec_file_load system call.
>>
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/kexec.h  |  7 +++++++
>>   arch/powerpc/kexec/core_64.c      | 27 +++++++++++++++++++++++++++
>>   arch/powerpc/kexec/elf_64.c       |  6 ++++++
>>   arch/powerpc/kexec/file_load_64.c |  5 +++++
>>   4 files changed, 45 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
>> index 8090ad7d97d9d..5a322c1737661 100644
>> --- a/arch/powerpc/include/asm/kexec.h
>> +++ b/arch/powerpc/include/asm/kexec.h
>> @@ -103,6 +103,10 @@ 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);
>> +#if defined(CONFIG_CRASH_HOTPLUG)
>> +int machine_kexec_post_load(struct kimage *image);
>> +#define machine_kexec_post_load machine_kexec_post_load
>> +#endif
>>   #endif
>>   
>>   #if defined(CONFIG_CRASH_DUMP) && defined(CONFIG_PPC_RTAS)
>> @@ -118,6 +122,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..3d4fe1aa6f761 100644
>> --- a/arch/powerpc/kexec/core_64.c
>> +++ b/arch/powerpc/kexec/core_64.c
>> @@ -77,6 +77,33 @@ int machine_kexec_prepare(struct kimage *image)
>>   	return 0;
>>   }
>>   
>> +#if defined(CONFIG_CRASH_HOTPLUG)
> I think you should add a small function header describing that this
> function is recording the index of the FDT segment for later use.

Yes good to have one,  I will add it in the next version.

>
>> +int machine_kexec_post_load(struct kimage *kimage)
>> +{
>> +	int i;
>> +	void *ptr;
>> +	unsigned long mem;
>> +
>> +	/* Mark fdt_index invalid */
>> +	kimage->arch.fdt_index = -1;
> Is that really needed?
> This is already done in arch_kexec_kernel_image_probe() called before this
> function, isn't it?

Oh I didn't realize machine_kexec_post load is called for both
kexec_load and kexec_file_load system call.

The intention was to initialize fdt_index for both system calls but since
machine_kexec_post_load is called for both system calls,
initializing fdt_index in arch_kernel_image_probe function is redundant.

Thanks for point it out. I will fix this in the next version by only 
initializing the
fdtindex in machine_kexec_post_load. With this fdt_index will be 
initialized
for both syscalls.

>> +
>> +	if (kimage->type != KEXEC_TYPE_CRASH)
>> +		return 0;
>> +
>> +	for (i = 0; i < kimage->nr_segments; i++) {
>> +		mem = kimage->segment[i].mem;
>> +		ptr = __va(mem);
>> +
>> +		if (ptr && fdt_magic(ptr) == FDT_MAGIC) {
>> +			kimage->arch.fdt_index = i;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>>   /* Called during kexec sequence with MMU off */
>>   static notrace void copy_segments(unsigned long ind)
>>   {
>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>> index eeb258002d1e0..2a17f171661f1 100644
>> --- a/arch/powerpc/kexec/elf_64.c
>> +++ b/arch/powerpc/kexec/elf_64.c
>> @@ -123,6 +123,12 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>   	kbuf.buf_align = PAGE_SIZE;
>>   	kbuf.top_down = true;
>>   	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> +
>> +#if defined(CONFIG_CRASH_HOTPLUG)
>> +	image->arch.fdt_index = image->nr_segments;
> I'm sorry, I'm not familliar with that code, could you explain why
> fdt_index has to be assigned here, and to that value?

Again I didn't realize machine_kexec_post_load is also called for 
kexec_file_load
system call too, which makes this assignment redundant.

Now why this value?

The image->nr-segments holds the count of total number of kexec segments and
when a new segment/buffer is added ( by kexec_add_buffer()) it is 
incremented by 1. With
this the index of newly added segment in the kexec segment array will be 
image->nr_segments - 1.

So instead of adding fdt segment first and then initializing fdt_index 
as image->nr_segments - 1,
the fdt_index is initialized with nr_segments before adding the fdt 
kexec segment/buffer to the
segment array.

Hope I answered you query.

Thanks for the review.

- Sourabh


>
>> +#endif
>> +	kbuf.memsz = fdt_totalsize(fdt);
>> +
>>   	ret = kexec_add_buffer(&kbuf);
>>   	if (ret)
>>   		goto out_free_fdt;
>> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
>> index 9bc70b4d8eafc..725f74d1b928c 100644
>> --- a/arch/powerpc/kexec/file_load_64.c
>> +++ b/arch/powerpc/kexec/file_load_64.c
>> @@ -1153,6 +1153,11 @@ int arch_kexec_kernel_image_probe(struct kimage *image, void *buf,
>>   		return ret;
>>   	}
>>   
>> +#if defined(CONFIG_CRASH_HOTPLUG)
>> +	/* Mark fdt_index invalid */
>> +	image->arch.fdt_index = -1;
>> +#endif
>> +
>>   	return kexec_image_probe_default(image, buf, buf_len);
>>   }
>>   

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec