| Message ID | 20230312181154.278900-4-sourabhjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PowerPC: in kernel handling of CPU hotplug events for crash kernel | expand |
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? > #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() ? This way there is a guarantee that the field will not be used while set by default to 0. > + > + /* fdt_index remains invalid if it is not a crash kernel load */ > + if (kimage->type != KEXEC_TYPE_CRASH) > + return 0; > + /* > + * Find the FDT segment index in kexec segment array and > + * assign it to kimage's member fdt_index to enable direct > + * access to FDT segment later on. > + */ > + 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; > + } > + } > +#endif > + return 0; > +} > + > /* Called during kexec sequence with MMU off */ > static notrace void copy_segments(unsigned long ind) > {
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
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 #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; + + /* fdt_index remains invalid if it is not a crash kernel load */ + if (kimage->type != KEXEC_TYPE_CRASH) + return 0; + /* + * Find the FDT segment index in kexec segment array and + * assign it to kimage's member fdt_index to enable direct + * access to FDT segment later on. + */ + 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; + } + } +#endif + return 0; +} + /* Called during kexec sequence with MMU off */ static notrace void copy_segments(unsigned long ind) {
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(+)