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

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(+)

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.

> +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?

> +
> +	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?

> +#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);
>  }
>

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);
>>   }
>>