[v19,2/7] crash: add generic infrastructure for crash hotplug support

  To support crash hotplug, a mechanism is needed to update the crash
elfcorehdr upon CPU or memory changes (eg. hot un/plug or off/
onlining). The crash elfcorehdr describes the CPUs and memory to
be written into the vmcore.

To track CPU changes, callbacks are registered with the cpuhp
mechanism via cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN). The
crash hotplug elfcorehdr update has no explicit ordering requirement
(relative to other cpuhp states), so meets the criteria for
utilizing CPUHP_BP_PREPARE_DYN. CPUHP_BP_PREPARE_DYN is a dynamic
state and avoids the need to introduce a new state for crash
hotplug. Also, this is the last state in the PREPARE group, just
prior to the STARTING group, which is very close to the CPU
starting up in a plug/online situation, or stopping in a unplug/
offline situation. This minimizes the window of time during an
actual plug/online or unplug/offline situation in which the
elfcorehdr would be inaccurate. Note that for a CPU being unplugged
or offlined, the CPU will still be present in the list of CPUs
generated by crash_prepare_elf64_headers(). However, there is no
need to explicitly omit the CPU, see the discussion in the patch
'crash: change crash_prepare_elf64_headers() to
for_each_possible_cpu()'.

To track memory changes, a notifier is registered to capture the
memblock MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().

The CPU callbacks and memory notifiers invoke crash_handle_hotplug_event()
which performs needed tasks and then dispatches the event to the
architecture specific arch_crash_handle_hotplug_event() to update the
elfcorehdr with the current state of CPUs and memory. During the
process, the kexec_lock is held.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
 include/linux/crash_core.h |   9 +++
 include/linux/kexec.h      |  11 +++
 kernel/crash_core.c        | 138 +++++++++++++++++++++++++++++++++++++
 kernel/kexec_core.c        |   5 ++
 4 files changed, 163 insertions(+)
  

On 03/06/23 at 11:22am, Eric DeVolder wrote:
......
> +#ifdef CONFIG_CRASH_HOTPLUG
> +#undef pr_fmt
> +#define pr_fmt(fmt) "crash hp: " fmt
> +/*
> + * To accurately reflect hot un/plug changes of cpu and memory resources
> + * (including onling and offlining of those resources), the elfcorehdr
> + * (which is passed to the crash kernel via the elfcorehdr= parameter)
> + * must be updated with the new list of CPUs and memories.
> + *
> + * In order to make changes to elfcorehdr, two conditions are needed:
> + * First, the segment containing the elfcorehdr must be large enough
> + * to permit a growing number of resources; the elfcorehdr memory size
> + * is based on NR_CPUS_DEFAULT and CRASH_MAX_MEMORY_RANGES.
> + * Second, purgatory must explicitly exclude the elfcorehdr from the
> + * list of segments it checks (since the elfcorehdr changes and thus
> + * would require an update to purgatory itself to update the digest).
> + */
> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> +	/* Obtain lock while changing crash information */
> +	if (kexec_trylock()) {
> +
> +		/* Check kdump is loaded */
> +		if (kexec_crash_image) {

If the above check failed, I would directly return or jump out becuase
one indentation can be reduced.

> +			struct kimage *image = kexec_crash_image;
> +
> +			if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
> +				hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
> +				pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
> +			else
> +				pr_debug("hp_action %u\n", hp_action);
> +
> +			/*
> +			 * When the struct kimage is allocated, the elfcorehdr_index
> +			 * is set to -1. Find the segment containing the elfcorehdr,
> +			 * if not already found. This works for both the kexec_load
> +			 * and kexec_file_load paths.
> +			 */
> +			if (image->elfcorehdr_index < 0) {
> +				unsigned long mem;
> +				unsigned char *ptr;
> +				unsigned int n;
> +
> +				for (n = 0; n < image->nr_segments; n++) {
> +					mem = image->segment[n].mem;
> +					ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
> +					if (ptr) {
> +						/* The segment containing elfcorehdr */
> +						if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
> +							image->elfcorehdr_index = (int)n;
> +						}
> +						kunmap_local(ptr);
> +					}
> +				}
> +			}
> +
> +			if (image->elfcorehdr_index < 0) {
> +				pr_err("unable to locate elfcorehdr segment");
> +				goto out;
> +			}
> +
> +			/* Needed in order for the segments to be updated */
> +			arch_kexec_unprotect_crashkres();
> +
> +			/* Differentiate between normal load and hotplug update */
> +			image->hp_action = hp_action;
> +
> +			/* Now invoke arch-specific update handler */
> +			arch_crash_handle_hotplug_event(image);
> +
> +			/* No longer handling a hotplug event */
> +			image->hp_action = KEXEC_CRASH_HP_NONE;
> +			image->elfcorehdr_updated = true;
> +
> +			/* Change back to read-only */
> +			arch_kexec_protect_crashkres();
> +		}
> +
> +out:
> +		/* Release lock now that update complete */
> +		kexec_unlock();
> +	}
> +}
> +
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> +{
> +	switch (val) {
> +	case MEM_ONLINE:
> +		crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY,
> +			KEXEC_CRASH_HP_INVALID_CPU);
> +		break;
> +
> +	case MEM_OFFLINE:
> +		crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY,
> +			KEXEC_CRASH_HP_INVALID_CPU);
> +		break;
> +	}
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block crash_memhp_nb = {
> +	.notifier_call = crash_memhp_notifier,
> +	.priority = 0
> +};
> +

Because for_each_possible_cpu() is taken in
crash_prepare_elf64_headers(), x86 doesn't need to respond to cpu
hotplug or doesn't do anything with this patchset. This cpu part in
infrastructure is only for the later powerpc usage, right?

> +static int crash_cpuhp_online(unsigned int cpu)
> +{
> +	crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> +	return 0;
> +}
> +
> +static int crash_cpuhp_offline(unsigned int cpu)
> +{
> +	crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> +	return 0;
> +}
> +
> +static int __init crash_hotplug_init(void)
> +{
> +	int result = 0;
> +
> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> +		register_memory_notifier(&crash_memhp_nb);
> +
> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
> +		result = cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
> +			"crash/cpuhp", crash_cpuhp_online, crash_cpuhp_offline);
> +	}
> +
> +	return result;
> +}
> +
> +subsys_initcall(crash_hotplug_init);
> +#endif
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index 969e8f52f7da..f2f9d41ce5df 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -276,6 +276,11 @@ struct kimage *do_kimage_alloc_init(void)
>  	/* Initialize the list of unusable pages */
>  	INIT_LIST_HEAD(&image->unusable_pages);
>  
> +#ifdef CONFIG_CRASH_HOTPLUG
> +    image->elfcorehdr_index = -1;
> +    image->elfcorehdr_updated = false;
> +#endif
> +
>  	return image;
>  }
>  
> -- 
> 2.31.1
>
  

On 3/14/23 05:43, Baoquan He wrote:
> On 03/06/23 at 11:22am, Eric DeVolder wrote:
> ......
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> +#undef pr_fmt
>> +#define pr_fmt(fmt) "crash hp: " fmt
>> +/*
>> + * To accurately reflect hot un/plug changes of cpu and memory resources
>> + * (including onling and offlining of those resources), the elfcorehdr
>> + * (which is passed to the crash kernel via the elfcorehdr= parameter)
>> + * must be updated with the new list of CPUs and memories.
>> + *
>> + * In order to make changes to elfcorehdr, two conditions are needed:
>> + * First, the segment containing the elfcorehdr must be large enough
>> + * to permit a growing number of resources; the elfcorehdr memory size
>> + * is based on NR_CPUS_DEFAULT and CRASH_MAX_MEMORY_RANGES.
>> + * Second, purgatory must explicitly exclude the elfcorehdr from the
>> + * list of segments it checks (since the elfcorehdr changes and thus
>> + * would require an update to purgatory itself to update the digest).
>> + */
>> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> +	/* Obtain lock while changing crash information */
>> +	if (kexec_trylock()) {
>> +
>> +		/* Check kdump is loaded */
>> +		if (kexec_crash_image) {
> 
> If the above check failed, I would directly return or jump out becuase
> one indentation can be reduced.

Baoquan, ok, I'll change that in next version.

> 
>> +			struct kimage *image = kexec_crash_image;
>> +
>> +			if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
>> +				hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
>> +				pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
>> +			else
>> +				pr_debug("hp_action %u\n", hp_action);
>> +
>> +			/*
>> +			 * When the struct kimage is allocated, the elfcorehdr_index
>> +			 * is set to -1. Find the segment containing the elfcorehdr,
>> +			 * if not already found. This works for both the kexec_load
>> +			 * and kexec_file_load paths.
>> +			 */
>> +			if (image->elfcorehdr_index < 0) {
>> +				unsigned long mem;
>> +				unsigned char *ptr;
>> +				unsigned int n;
>> +
>> +				for (n = 0; n < image->nr_segments; n++) {
>> +					mem = image->segment[n].mem;
>> +					ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
>> +					if (ptr) {
>> +						/* The segment containing elfcorehdr */
>> +						if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
>> +							image->elfcorehdr_index = (int)n;
>> +						}
>> +						kunmap_local(ptr);
>> +					}
>> +				}
>> +			}
>> +
>> +			if (image->elfcorehdr_index < 0) {
>> +				pr_err("unable to locate elfcorehdr segment");
>> +				goto out;
>> +			}
>> +
>> +			/* Needed in order for the segments to be updated */
>> +			arch_kexec_unprotect_crashkres();
>> +
>> +			/* Differentiate between normal load and hotplug update */
>> +			image->hp_action = hp_action;
>> +
>> +			/* Now invoke arch-specific update handler */
>> +			arch_crash_handle_hotplug_event(image);
>> +
>> +			/* No longer handling a hotplug event */
>> +			image->hp_action = KEXEC_CRASH_HP_NONE;
>> +			image->elfcorehdr_updated = true;
>> +
>> +			/* Change back to read-only */
>> +			arch_kexec_protect_crashkres();
>> +		}
>> +
>> +out:
>> +		/* Release lock now that update complete */
>> +		kexec_unlock();
>> +	}
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> +	switch (val) {
>> +	case MEM_ONLINE:
>> +		crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY,
>> +			KEXEC_CRASH_HP_INVALID_CPU);
>> +		break;
>> +
>> +	case MEM_OFFLINE:
>> +		crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY,
>> +			KEXEC_CRASH_HP_INVALID_CPU);
>> +		break;
>> +	}
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> +	.notifier_call = crash_memhp_notifier,
>> +	.priority = 0
>> +};
>> +
> 
> Because for_each_possible_cpu() is taken in
> crash_prepare_elf64_headers(), x86 doesn't need to respond to cpu
> hotplug or doesn't do anything with this patchset. This cpu part in
> infrastructure is only for the later powerpc usage, right?

That is true, yes.

> 
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> +	crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> +	crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> +	int result = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> +		register_memory_notifier(&crash_memhp_nb);
>> +
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
>> +		result = cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
>> +			"crash/cpuhp", crash_cpuhp_online, crash_cpuhp_offline);
>> +	}
>> +
>> +	return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 969e8f52f7da..f2f9d41ce5df 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -276,6 +276,11 @@ struct kimage *do_kimage_alloc_init(void)
>>   	/* Initialize the list of unusable pages */
>>   	INIT_LIST_HEAD(&image->unusable_pages);
>>   
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> +    image->elfcorehdr_index = -1;
>> +    image->elfcorehdr_updated = false;
>> +#endif
>> +
>>   	return image;
>>   }
>>   
>> -- 
>> 2.31.1
>>
>
  

On 03/14/23 at 08:28am, Eric DeVolder wrote:
......
> > > +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> > > +{
> > > +	switch (val) {
> > > +	case MEM_ONLINE:
> > > +		crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY,
> > > +			KEXEC_CRASH_HP_INVALID_CPU);
> > > +		break;
> > > +
> > > +	case MEM_OFFLINE:
> > > +		crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY,
> > > +			KEXEC_CRASH_HP_INVALID_CPU);
> > > +		break;
> > > +	}
> > > +	return NOTIFY_OK;
> > > +}
> > > +
> > > +static struct notifier_block crash_memhp_nb = {
> > > +	.notifier_call = crash_memhp_notifier,
> > > +	.priority = 0
> > > +};
> > > +
> > 
> > Because for_each_possible_cpu() is taken in
> > crash_prepare_elf64_headers(), x86 doesn't need to respond to cpu
> > hotplug or doesn't do anything with this patchset. This cpu part in
> > infrastructure is only for the later powerpc usage, right?
> 
> That is true, yes.

Given this patchset is aimed at crash hotplug on x86, while obviously it
does't need to have the cpu hotplug support on x86 since the
for_each_possible_cpu() adjustment. People looking into this may be
confused if they don't follow the discussion thread of v18. Do we need
to mention this in cover letter or somewhere else? I could miss that
though it is has been told, please ignore this if yes.
  

On 3/14/23 09:22, Baoquan He wrote:
> On 03/14/23 at 08:28am, Eric DeVolder wrote:
> ......
>>>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>>>> +{
>>>> +	switch (val) {
>>>> +	case MEM_ONLINE:
>>>> +		crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY,
>>>> +			KEXEC_CRASH_HP_INVALID_CPU);
>>>> +		break;
>>>> +
>>>> +	case MEM_OFFLINE:
>>>> +		crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY,
>>>> +			KEXEC_CRASH_HP_INVALID_CPU);
>>>> +		break;
>>>> +	}
>>>> +	return NOTIFY_OK;
>>>> +}
>>>> +
>>>> +static struct notifier_block crash_memhp_nb = {
>>>> +	.notifier_call = crash_memhp_notifier,
>>>> +	.priority = 0
>>>> +};
>>>> +
>>>
>>> Because for_each_possible_cpu() is taken in
>>> crash_prepare_elf64_headers(), x86 doesn't need to respond to cpu
>>> hotplug or doesn't do anything with this patchset. This cpu part in
>>> infrastructure is only for the later powerpc usage, right?
>>
>> That is true, yes.
> 
> Given this patchset is aimed at crash hotplug on x86, while obviously it
> does't need to have the cpu hotplug support on x86 since the
> for_each_possible_cpu() adjustment. People looking into this may be
> confused if they don't follow the discussion thread of v18. Do we need
> to mention this in cover letter or somewhere else? I could miss that
> though it is has been told, please ignore this if yes.
> 
Good point, I'll update the cover letter to reflect this.
eric
  

On 03/06/23 at 11:22am, Eric DeVolder wrote:
......
> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> +{
> +	/* Obtain lock while changing crash information */
> +	if (kexec_trylock()) {
> +
> +		/* Check kdump is loaded */
> +		if (kexec_crash_image) {
> +			struct kimage *image = kexec_crash_image;
> +
> +			if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
> +				hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
> +				pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
> +			else
> +				pr_debug("hp_action %u\n", hp_action);
> +
> +			/*
> +			 * When the struct kimage is allocated, the elfcorehdr_index
> +			 * is set to -1. Find the segment containing the elfcorehdr,
> +			 * if not already found. This works for both the kexec_load
> +			 * and kexec_file_load paths.
> +			 */
> +			if (image->elfcorehdr_index < 0) {
> +				unsigned long mem;
> +				unsigned char *ptr;
> +				unsigned int n;
> +
> +				for (n = 0; n < image->nr_segments; n++) {
> +					mem = image->segment[n].mem;
> +					ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
> +					if (ptr) {
> +						/* The segment containing elfcorehdr */
> +						if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
> +							image->elfcorehdr_index = (int)n;
> +						}
> +						kunmap_local(ptr);
> +					}
> +				}
> +			}
> +
> +			if (image->elfcorehdr_index < 0) {
> +				pr_err("unable to locate elfcorehdr segment");
> +				goto out;
> +			}
> +
> +			/* Needed in order for the segments to be updated */
> +			arch_kexec_unprotect_crashkres();
> +
> +			/* Differentiate between normal load and hotplug update */
> +			image->hp_action = hp_action;
> +
> +			/* Now invoke arch-specific update handler */
> +			arch_crash_handle_hotplug_event(image);
> +
> +			/* No longer handling a hotplug event */
> +			image->hp_action = KEXEC_CRASH_HP_NONE;
> +			image->elfcorehdr_updated = true;

It's good to initialize the image->hp_action here, however where do
you check it? Do you plan to add some check somewhere?

> +
> +			/* Change back to read-only */
> +			arch_kexec_protect_crashkres();
> +		}
> +
> +out:
> +		/* Release lock now that update complete */
> +		kexec_unlock();
> +	}
> +}
......
  

On 3/16/23 05:11, Baoquan He wrote:
> On 03/06/23 at 11:22am, Eric DeVolder wrote:
> ......
>> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> +	/* Obtain lock while changing crash information */
>> +	if (kexec_trylock()) {
>> +
>> +		/* Check kdump is loaded */
>> +		if (kexec_crash_image) {
>> +			struct kimage *image = kexec_crash_image;
>> +
>> +			if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
>> +				hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
>> +				pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
>> +			else
>> +				pr_debug("hp_action %u\n", hp_action);
>> +
>> +			/*
>> +			 * When the struct kimage is allocated, the elfcorehdr_index
>> +			 * is set to -1. Find the segment containing the elfcorehdr,
>> +			 * if not already found. This works for both the kexec_load
>> +			 * and kexec_file_load paths.
>> +			 */
>> +			if (image->elfcorehdr_index < 0) {
>> +				unsigned long mem;
>> +				unsigned char *ptr;
>> +				unsigned int n;
>> +
>> +				for (n = 0; n < image->nr_segments; n++) {
>> +					mem = image->segment[n].mem;
>> +					ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
>> +					if (ptr) {
>> +						/* The segment containing elfcorehdr */
>> +						if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
>> +							image->elfcorehdr_index = (int)n;
>> +						}
>> +						kunmap_local(ptr);
>> +					}
>> +				}
>> +			}
>> +
>> +			if (image->elfcorehdr_index < 0) {
>> +				pr_err("unable to locate elfcorehdr segment");
>> +				goto out;
>> +			}
>> +
>> +			/* Needed in order for the segments to be updated */
>> +			arch_kexec_unprotect_crashkres();
>> +
>> +			/* Differentiate between normal load and hotplug update */
>> +			image->hp_action = hp_action;
>> +
>> +			/* Now invoke arch-specific update handler */
>> +			arch_crash_handle_hotplug_event(image);
>> +
>> +			/* No longer handling a hotplug event */
>> +			image->hp_action = KEXEC_CRASH_HP_NONE;
>> +			image->elfcorehdr_updated = true;
> 
> It's good to initialize the image->hp_action here, however where do
> you check it? Do you plan to add some check somewhere?

Hi Baoquan,
The hp_action member is initialized to 0 in do_image_alloc_init(). I've
mapped KEXEC_CRASH_HP_NONE onto 0 on purpose.

But the use of image->hp_action = KEXEC_CRASH_HP_NONE is to actually
delineate that a hotplug event handling has completed. You can see
imae->hp_action set to hp_action to capture what the triggering event
was, as passed into this function.

I will go ahead and set image->hp_action = KEXEC_CRASH_HP_NONE; explicitly
in do_kimage_alloc_init(), as that is done for the other crash hotplug members.

Thanks!
eric


> 
>> +
>> +			/* Change back to read-only */
>> +			arch_kexec_protect_crashkres();
>> +		}
>> +
>> +out:
>> +		/* Release lock now that update complete */
>> +		kexec_unlock();
>> +	}
>> +}
> ......
>
  

On 3/16/23 09:44, Eric DeVolder wrote:
> 
> 
> On 3/16/23 05:11, Baoquan He wrote:
>> On 03/06/23 at 11:22am, Eric DeVolder wrote:
>> ......
>>> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>>> +{
>>> +    /* Obtain lock while changing crash information */
>>> +    if (kexec_trylock()) {
>>> +
>>> +        /* Check kdump is loaded */
>>> +        if (kexec_crash_image) {
>>> +            struct kimage *image = kexec_crash_image;
>>> +
>>> +            if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
>>> +                hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
>>> +                pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
>>> +            else
>>> +                pr_debug("hp_action %u\n", hp_action);
>>> +
>>> +            /*
>>> +             * When the struct kimage is allocated, the elfcorehdr_index
>>> +             * is set to -1. Find the segment containing the elfcorehdr,
>>> +             * if not already found. This works for both the kexec_load
>>> +             * and kexec_file_load paths.
>>> +             */
>>> +            if (image->elfcorehdr_index < 0) {
>>> +                unsigned long mem;
>>> +                unsigned char *ptr;
>>> +                unsigned int n;
>>> +
>>> +                for (n = 0; n < image->nr_segments; n++) {
>>> +                    mem = image->segment[n].mem;
>>> +                    ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
>>> +                    if (ptr) {
>>> +                        /* The segment containing elfcorehdr */
>>> +                        if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
>>> +                            image->elfcorehdr_index = (int)n;
>>> +                        }
>>> +                        kunmap_local(ptr);
>>> +                    }
>>> +                }
>>> +            }
>>> +
>>> +            if (image->elfcorehdr_index < 0) {
>>> +                pr_err("unable to locate elfcorehdr segment");
>>> +                goto out;
>>> +            }
>>> +
>>> +            /* Needed in order for the segments to be updated */
>>> +            arch_kexec_unprotect_crashkres();
>>> +
>>> +            /* Differentiate between normal load and hotplug update */
>>> +            image->hp_action = hp_action;
>>> +
>>> +            /* Now invoke arch-specific update handler */
>>> +            arch_crash_handle_hotplug_event(image);
>>> +
>>> +            /* No longer handling a hotplug event */
>>> +            image->hp_action = KEXEC_CRASH_HP_NONE;
>>> +            image->elfcorehdr_updated = true;
>>
>> It's good to initialize the image->hp_action here, however where do
>> you check it? Do you plan to add some check somewhere?
> 
> Hi Baoquan,
> The hp_action member is initialized to 0 in do_image_alloc_init(). I've
> mapped KEXEC_CRASH_HP_NONE onto 0 on purpose.
> 
> But the use of image->hp_action = KEXEC_CRASH_HP_NONE is to actually
> delineate that a hotplug event handling has completed. You can see
> imae->hp_action set to hp_action to capture what the triggering event
> was, as passed into this function.
> 
> I will go ahead and set image->hp_action = KEXEC_CRASH_HP_NONE; explicitly
> in do_kimage_alloc_init(), as that is done for the other crash hotplug members.
> 
> Thanks!
> eric

Baoquan, as you are currently working through v19, please let me know when I should put out v20 with 
this and Sourabh's feedback.
Thanks,
eric

> 
> 
>>
>>> +
>>> +            /* Change back to read-only */
>>> +            arch_kexec_protect_crashkres();
>>> +        }
>>> +
>>> +out:
>>> +        /* Release lock now that update complete */
>>> +        kexec_unlock();
>>> +    }
>>> +}
>> ......
>>
  

On 03/16/23 at 09:44am, Eric DeVolder wrote:
> 
> 
> On 3/16/23 05:11, Baoquan He wrote:
> > On 03/06/23 at 11:22am, Eric DeVolder wrote:
> > ......
> > > +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> > > +{
> > > +	/* Obtain lock while changing crash information */
> > > +	if (kexec_trylock()) {
> > > +
> > > +		/* Check kdump is loaded */
> > > +		if (kexec_crash_image) {
> > > +			struct kimage *image = kexec_crash_image;
> > > +
> > > +			if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
> > > +				hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
> > > +				pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
> > > +			else
> > > +				pr_debug("hp_action %u\n", hp_action);
> > > +
> > > +			/*
> > > +			 * When the struct kimage is allocated, the elfcorehdr_index
> > > +			 * is set to -1. Find the segment containing the elfcorehdr,
> > > +			 * if not already found. This works for both the kexec_load
> > > +			 * and kexec_file_load paths.
> > > +			 */
> > > +			if (image->elfcorehdr_index < 0) {
> > > +				unsigned long mem;
> > > +				unsigned char *ptr;
> > > +				unsigned int n;
> > > +
> > > +				for (n = 0; n < image->nr_segments; n++) {
> > > +					mem = image->segment[n].mem;
> > > +					ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
> > > +					if (ptr) {
> > > +						/* The segment containing elfcorehdr */
> > > +						if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
> > > +							image->elfcorehdr_index = (int)n;
> > > +						}
> > > +						kunmap_local(ptr);
> > > +					}
> > > +				}
> > > +			}
> > > +
> > > +			if (image->elfcorehdr_index < 0) {
> > > +				pr_err("unable to locate elfcorehdr segment");
> > > +				goto out;
> > > +			}
> > > +
> > > +			/* Needed in order for the segments to be updated */
> > > +			arch_kexec_unprotect_crashkres();
> > > +
> > > +			/* Differentiate between normal load and hotplug update */
> > > +			image->hp_action = hp_action;
> > > +
> > > +			/* Now invoke arch-specific update handler */
> > > +			arch_crash_handle_hotplug_event(image);
> > > +
> > > +			/* No longer handling a hotplug event */
> > > +			image->hp_action = KEXEC_CRASH_HP_NONE;
> > > +			image->elfcorehdr_updated = true;
> > 
> > It's good to initialize the image->hp_action here, however where do
> > you check it? Do you plan to add some check somewhere?
> 
> Hi Baoquan,
> The hp_action member is initialized to 0 in do_image_alloc_init(). I've
> mapped KEXEC_CRASH_HP_NONE onto 0 on purpose.
> 
> But the use of image->hp_action = KEXEC_CRASH_HP_NONE is to actually
> delineate that a hotplug event handling has completed. You can see
> imae->hp_action set to hp_action to capture what the triggering event
> was, as passed into this function.
> 
> I will go ahead and set image->hp_action = KEXEC_CRASH_HP_NONE; explicitly
> in do_kimage_alloc_init(), as that is done for the other crash hotplug members.

Yeah, setting image->hp_action = KEXEC_CRASH_HP_NONE in
do_kimage_alloc_init() will make code clearer. While I am wondering if
we don't initialie image->hp_action to KEXEC_CRASH_HP_NONE, and don't
set image->hp_action to KEXEC_CRASH_HP_NONE to actually delineate that a
hotplug event handling has completed, what will happen?

I mean you set image->hp_action to KEXEC_CRASH_HP_NONE explicitly, where
do you check if it should not be KEXEC_CRASH_HP_NONE? In
crash_handle_hotplug_event(), we took __kexec_lock and assign the passed
hp_action anyway.
  

On 03/16/23 at 10:47am, Eric DeVolder wrote:
> 
> 
> On 3/16/23 09:44, Eric DeVolder wrote:
> > 
> > 
> > On 3/16/23 05:11, Baoquan He wrote:
> > > On 03/06/23 at 11:22am, Eric DeVolder wrote:
> > > ......
> > > > +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> > > > +{
> > > > +    /* Obtain lock while changing crash information */
> > > > +    if (kexec_trylock()) {
> > > > +
> > > > +        /* Check kdump is loaded */
> > > > +        if (kexec_crash_image) {
> > > > +            struct kimage *image = kexec_crash_image;
> > > > +
> > > > +            if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
> > > > +                hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
> > > > +                pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
> > > > +            else
> > > > +                pr_debug("hp_action %u\n", hp_action);
> > > > +
> > > > +            /*
> > > > +             * When the struct kimage is allocated, the elfcorehdr_index
> > > > +             * is set to -1. Find the segment containing the elfcorehdr,
> > > > +             * if not already found. This works for both the kexec_load
> > > > +             * and kexec_file_load paths.
> > > > +             */
> > > > +            if (image->elfcorehdr_index < 0) {
> > > > +                unsigned long mem;
> > > > +                unsigned char *ptr;
> > > > +                unsigned int n;
> > > > +
> > > > +                for (n = 0; n < image->nr_segments; n++) {
> > > > +                    mem = image->segment[n].mem;
> > > > +                    ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
> > > > +                    if (ptr) {
> > > > +                        /* The segment containing elfcorehdr */
> > > > +                        if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
> > > > +                            image->elfcorehdr_index = (int)n;
> > > > +                        }
> > > > +                        kunmap_local(ptr);
> > > > +                    }
> > > > +                }
> > > > +            }
> > > > +
> > > > +            if (image->elfcorehdr_index < 0) {
> > > > +                pr_err("unable to locate elfcorehdr segment");
> > > > +                goto out;
> > > > +            }
> > > > +
> > > > +            /* Needed in order for the segments to be updated */
> > > > +            arch_kexec_unprotect_crashkres();
> > > > +
> > > > +            /* Differentiate between normal load and hotplug update */
> > > > +            image->hp_action = hp_action;
> > > > +
> > > > +            /* Now invoke arch-specific update handler */
> > > > +            arch_crash_handle_hotplug_event(image);
> > > > +
> > > > +            /* No longer handling a hotplug event */
> > > > +            image->hp_action = KEXEC_CRASH_HP_NONE;
> > > > +            image->elfcorehdr_updated = true;
> > > 
> > > It's good to initialize the image->hp_action here, however where do
> > > you check it? Do you plan to add some check somewhere?
> > 
> > Hi Baoquan,
> > The hp_action member is initialized to 0 in do_image_alloc_init(). I've
> > mapped KEXEC_CRASH_HP_NONE onto 0 on purpose.
> > 
> > But the use of image->hp_action = KEXEC_CRASH_HP_NONE is to actually
> > delineate that a hotplug event handling has completed. You can see
> > imae->hp_action set to hp_action to capture what the triggering event
> > was, as passed into this function.
> > 
> > I will go ahead and set image->hp_action = KEXEC_CRASH_HP_NONE; explicitly
> > in do_kimage_alloc_init(), as that is done for the other crash hotplug members.
> > 
> > Thanks!
> > eric
> 
> Baoquan, as you are currently working through v19, please let me know when I
> should put out v20 with this and Sourabh's feedback.
> Thanks,
> eric

The overall serial looks good to me other than the nitpicks.
  

On 3/17/23 04:04, Baoquan He wrote:
> On 03/16/23 at 09:44am, Eric DeVolder wrote:
>>
>>
>> On 3/16/23 05:11, Baoquan He wrote:
>>> On 03/06/23 at 11:22am, Eric DeVolder wrote:
>>> ......
>>>> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>>>> +{
>>>> +	/* Obtain lock while changing crash information */
>>>> +	if (kexec_trylock()) {
>>>> +
>>>> +		/* Check kdump is loaded */
>>>> +		if (kexec_crash_image) {
>>>> +			struct kimage *image = kexec_crash_image;
>>>> +
>>>> +			if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
>>>> +				hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
>>>> +				pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
>>>> +			else
>>>> +				pr_debug("hp_action %u\n", hp_action);
>>>> +
>>>> +			/*
>>>> +			 * When the struct kimage is allocated, the elfcorehdr_index
>>>> +			 * is set to -1. Find the segment containing the elfcorehdr,
>>>> +			 * if not already found. This works for both the kexec_load
>>>> +			 * and kexec_file_load paths.
>>>> +			 */
>>>> +			if (image->elfcorehdr_index < 0) {
>>>> +				unsigned long mem;
>>>> +				unsigned char *ptr;
>>>> +				unsigned int n;
>>>> +
>>>> +				for (n = 0; n < image->nr_segments; n++) {
>>>> +					mem = image->segment[n].mem;
>>>> +					ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
>>>> +					if (ptr) {
>>>> +						/* The segment containing elfcorehdr */
>>>> +						if (memcmp(ptr, ELFMAG, SELFMAG) == 0) {
>>>> +							image->elfcorehdr_index = (int)n;
>>>> +						}
>>>> +						kunmap_local(ptr);
>>>> +					}
>>>> +				}
>>>> +			}
>>>> +
>>>> +			if (image->elfcorehdr_index < 0) {
>>>> +				pr_err("unable to locate elfcorehdr segment");
>>>> +				goto out;
>>>> +			}
>>>> +
>>>> +			/* Needed in order for the segments to be updated */
>>>> +			arch_kexec_unprotect_crashkres();
>>>> +
>>>> +			/* Differentiate between normal load and hotplug update */
>>>> +			image->hp_action = hp_action;
>>>> +
>>>> +			/* Now invoke arch-specific update handler */
>>>> +			arch_crash_handle_hotplug_event(image);
>>>> +
>>>> +			/* No longer handling a hotplug event */
>>>> +			image->hp_action = KEXEC_CRASH_HP_NONE;
>>>> +			image->elfcorehdr_updated = true;
>>>
>>> It's good to initialize the image->hp_action here, however where do
>>> you check it? Do you plan to add some check somewhere?
>>
>> Hi Baoquan,
>> The hp_action member is initialized to 0 in do_image_alloc_init(). I've
>> mapped KEXEC_CRASH_HP_NONE onto 0 on purpose.
>>
>> But the use of image->hp_action = KEXEC_CRASH_HP_NONE is to actually
>> delineate that a hotplug event handling has completed. You can see
>> imae->hp_action set to hp_action to capture what the triggering event
>> was, as passed into this function.
>>
>> I will go ahead and set image->hp_action = KEXEC_CRASH_HP_NONE; explicitly
>> in do_kimage_alloc_init(), as that is done for the other crash hotplug members.
> 
> Yeah, setting image->hp_action = KEXEC_CRASH_HP_NONE in
> do_kimage_alloc_init() will make code clearer. While I am wondering if
> we don't initialie image->hp_action to KEXEC_CRASH_HP_NONE, and don't
> set image->hp_action to KEXEC_CRASH_HP_NONE to actually delineate that a
> hotplug event handling has completed, what will happen?

Baoquan,
The KEXEC_CRASH_HP_NONE is the value 0, intentionally so that upon a alloc
of the kimage, the struct kimage is automatically zeroed and it was initialized
properly that way. I am explicitly setting hp_action to KEXEC_CRASH_HP_NONE now.

> 
> I mean you set image->hp_action to KEXEC_CRASH_HP_NONE explicitly, where
> do you check if it should not be KEXEC_CRASH_HP_NONE? In
> crash_handle_hotplug_event(), we took __kexec_lock and assign the passed
> hp_action anyway.
> 
The cpuhp callbacks and memory notifiers invoke crash_handle_hotplug_event()
with an appropriate hp_action. That hp_action is then stored in image->hp_action
within crash_handle_hotplug_event() for use by the arch-specific handler.

For x86, for example, the image->hp_action is used to short-circuit the arch-
specific handler if the event is a CPU plug/unplug (see patch x86/crash:
optimize cpu changes). For PPC, for example, the image->hp_action is used to
determine the appropriate actions for its FDT updates.

To summarize, the image->hp_action will be initalized to KEXEC_CRASH_HP_NONE
during do_kimage_alloc_init(). Then upon a cpu or memory plug/unplug/online/offline
event, the appropriate hp_action is stored in image->hp_action and then the
arch-specific handler called. Upon returning from the arch-specific handler,
the image->hp_action is reset back to KEXEC_CRASH_HP_NONE.

Hope this helps. I'll be posting v20 soon.
Thanks!
eric