Re: [PATCH v21 1/5] kdump: return -ENOENT if required cmdline option does not exist

2022-03-15 Thread Leizhen (ThunderTown)



On 2022/3/16 13:39, Baoquan He wrote:
> On 02/27/22 at 11:07am, Zhen Lei wrote:
>> The crashkernel=Y,low is an optional command-line option. When it doesn't
>> exist, kernel will try to allocate minimum required memory below 4G
>> automatically. Give it a unique error code to distinguish it from other
>> error scenarios.
> 
> This log is a little confusing. __parse_crashkernel() has three callers. 
>  - parse_crashkernel()
>  - parse_crashkernel_high()
>  - parse_crashkernel_low()
> 
> How about tuning the git log as below:

Sure. Your description is much clearer than mine.

> 
> ==
> According to the current crashkernel=Y,low support in other ARCHes, it's
> an optional command-line option. When it doesn't exist, kernel will try
> to allocate minimum required memory below 4G automatically. 
> 
> However, __parse_crashkernel() returns '-EINVAL' for all error cases. It
> can't distinguish the nonexistent option from invalid option. 
> 
> Change __parse_crashkernel() to return '-ENOENT' for the nonexistent option
> case. With this change, crashkernel,low memory will take the default
> value if crashkernel=,low is not specified; while crashkernel reservation
> will fail and bail out if an invalid option is specified.
> ==
> 
>>
>> Signed-off-by: Zhen Lei 
>> ---
>>  kernel/crash_core.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>>
>> Signed-off-by: Zhen Lei 
>> ---
>>  kernel/crash_core.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>> index 256cf6db573cd09..4d57c03714f4e13 100644
>> --- a/kernel/crash_core.c
>> +++ b/kernel/crash_core.c
>> @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline,
>>  *crash_base = 0;
>>  
>>  ck_cmdline = get_last_crashkernel(cmdline, name, suffix);
>> -
>>  if (!ck_cmdline)
>> -return -EINVAL;
>> +return -ENOENT;
>>  
>>  ck_cmdline += strlen(name);
>>  
>> -- 
>> 2.25.1
>>
> 
> .
> 

-- 
Regards,
  Zhen Lei

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


Re: [PATCH v21 1/5] kdump: return -ENOENT if required cmdline option does not exist

2022-03-15 Thread Baoquan He
On 02/27/22 at 11:07am, Zhen Lei wrote:
> The crashkernel=Y,low is an optional command-line option. When it doesn't
> exist, kernel will try to allocate minimum required memory below 4G
> automatically. Give it a unique error code to distinguish it from other
> error scenarios.

This log is a little confusing. __parse_crashkernel() has three callers. 
 - parse_crashkernel()
 - parse_crashkernel_high()
 - parse_crashkernel_low()

How about tuning the git log as below:

==
According to the current crashkernel=Y,low support in other ARCHes, it's
an optional command-line option. When it doesn't exist, kernel will try
to allocate minimum required memory below 4G automatically. 

However, __parse_crashkernel() returns '-EINVAL' for all error cases. It
can't distinguish the nonexistent option from invalid option. 

Change __parse_crashkernel() to return '-ENOENT' for the nonexistent option
case. With this change, crashkernel,low memory will take the default
value if crashkernel=,low is not specified; while crashkernel reservation
will fail and bail out if an invalid option is specified.
==

> 
> Signed-off-by: Zhen Lei 
> ---
>  kernel/crash_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> 
> Signed-off-by: Zhen Lei 
> ---
>  kernel/crash_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 256cf6db573cd09..4d57c03714f4e13 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline,
>   *crash_base = 0;
>  
>   ck_cmdline = get_last_crashkernel(cmdline, name, suffix);
> -
>   if (!ck_cmdline)
> - return -EINVAL;
> + return -ENOENT;
>  
>   ck_cmdline += strlen(name);
>  
> -- 
> 2.25.1
> 


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


Re: [PATCH v21 1/5] kdump: return -ENOENT if required cmdline option does not exist

2022-03-15 Thread Baoquan He
On 03/15/22 at 09:32pm, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/3/15 20:21, Baoquan He wrote:
> > On 03/15/22 at 07:57pm, Baoquan He wrote:
> >> On 02/27/22 at 11:07am, Zhen Lei wrote:
> >>> The crashkernel=Y,low is an optional command-line option. When it doesn't
> >>> exist, kernel will try to allocate minimum required memory below 4G
> >>> automatically. Give it a unique error code to distinguish it from other
> >>> error scenarios.
> >>>
> >>> Signed-off-by: Zhen Lei 
> >>> ---
> >>>  kernel/crash_core.c | 3 +--
> >>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>
> >>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> >>> index 256cf6db573cd09..4d57c03714f4e13 100644
> >>> --- a/kernel/crash_core.c
> >>> +++ b/kernel/crash_core.c
> >>> @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline,
> >>>   *crash_base = 0;
> >>>  
> >>>   ck_cmdline = get_last_crashkernel(cmdline, name, suffix);
> >>> -
> >>>   if (!ck_cmdline)
> >>> - return -EINVAL;
> >>> + return -ENOENT;
> >>
> >> Firstly, I am not sure if '-ENOENT' is a right value to return. From the
> >> code comment of ENOENT, it's used for file or dir?
> >> #define ENOENT   2  /* No such file or directory */
> 
> This error code does not return to user mode, so there is no problem.
> There are a lot of places in the kernel that are used this way. For example:
> 
> int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg)
> {
>   if (!cpu_stop_queue_work(cpu, &work))
>   return -ENOENT;

OK, it's fine to me. Thanks for the investigation.


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


[PATCH] Drivers: hv: vmbus: Fix potential crash on module unload

2022-03-15 Thread Guilherme G. Piccoli
The vmbus driver relies on the panic notifier infrastructure to perform
some operations when a panic event is detected. Since vmbus can be built
as module, it is required that the driver handles both registering and
unregistering such panic notifier callback.

After commit 74347a99e73a ("x86/Hyper-V: Unload vmbus channel in hv panic 
callback")
though, the panic notifier registration is done unconditionally in the module
initialization routine whereas the unregistering procedure is conditionally
guarded and executes only if HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE capability
is set.

This patch fixes that by unconditionally unregistering the panic notifier
in the module's exit routine as well.

Fixes: 74347a99e73a ("x86/Hyper-V: Unload vmbus channel in hv panic callback")
Signed-off-by: Guilherme G. Piccoli 
---


Hi folks, thanks in advance for any reviews! This was build-tested
with Debian config, on 5.17-rc7.

This patch is a result of code analysis - I didn't experience this
issue but seems a valid/feasible case.

Also, this is part of an ongoing effort of clearing/refactoring the panic
notifiers, more will be done soon, but I prefer to send the simple bug
fixes quickly, or else it might take a while since the next steps are more
complex and subject to many iterations I expect.

Cheers,

Guilherme


 drivers/hv/vmbus_drv.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 12a2b37e87f3..12585324cc4a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2780,10 +2780,15 @@ static void __exit vmbus_exit(void)
if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
kmsg_dump_unregister(&hv_kmsg_dumper);
unregister_die_notifier(&hyperv_die_block);
-   atomic_notifier_chain_unregister(&panic_notifier_list,
-&hyperv_panic_block);
}
 
+   /*
+* The panic notifier is always registered, hence we should
+* also unconditionally unregister it here as well.
+*/
+   atomic_notifier_chain_unregister(&panic_notifier_list,
+&hyperv_panic_block);
+
free_page((unsigned long)hv_panic_page);
unregister_sysctl_table(hv_ctl_table_hdr);
hv_ctl_table_hdr = NULL;
-- 
2.35.1


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


Re: [PATCH v5 4/8] crash: generic crash hotplug support infrastructure

2022-03-15 Thread Eric DeVolder



On 3/15/22 07:08, Sourabh Jain wrote:

Hello Eric,

On 03/03/22 21:57, Eric DeVolder wrote:

This patch introduces a generic crash hot plug/unplug infrastructure
for CPU and memory changes. Upon CPU and memory changes, a generic
crash_hotplug_handler() obtains the appropriate lock, does some
important house keeping and then dispatches the hot plug/unplug event
to the architecture specific arch_crash_hotplug_handler(), and when
that handler returns, the lock is released.

This patch modifies crash_core.c to implement a subsys_initcall()
function that installs handlers for hot plug/unplug events. If CPU
hotplug is enabled, then cpuhp_setup_state() is invoked to register a
handler for CPU changes. Similarly, if memory hotplug is enabled, then
register_memory_notifier() is invoked to install a handler for memory
changes. These handlers in turn invoke the common generic handler
crash_hotplug_handler().

On the CPU side, cpuhp_setup_state_nocalls() is invoked with parameter
CPUHP_AP_ONLINE_DYN. While this works, when a CPU is being unplugged,
the CPU still shows up in foreach_present_cpu() during the regeneration
of the new CPU list, thus the need to explicitly check and exclude the
soon-to-be offlined CPU in crash_prepare_elf64_headers().

On the memory side, each un/plugged memory block passes through the
handler. For example, if a 1GiB DIMM is hotplugged, that generate 8
memory events, one for each 128MiB memblock.

Signed-off-by: Eric DeVolder 
---
  include/linux/kexec.h |  16 +++
  kernel/crash_core.c   | 108 ++
  2 files changed, 124 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index d7b59248441b..b11d75a6b2bc 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -300,6 +300,13 @@ struct kimage {
  /* Information for loading purgatory */
  struct purgatory_info purgatory_info;
+
+#ifdef CONFIG_CRASH_HOTPLUG
+    bool hotplug_event;
+    int offlinecpu;
+    bool elf_index_valid;
+    int elf_index;


How about keeping an array to track all kexec segment index need to be updated 
in
crash hotplug handler.

struct hp_segment {
    name;
    index;
    is_valid;
  }

It will be helpful if architecture need to updated multiple kexec segments  for 
a hotplug event.

For example, on PowerPC, we might need to update FDT and elfcorehdr on memory 
hot plug/unplug.

Thanks,
Sourabh Jain


Sourabh,
I'm OK with that. Another idea might be if there are just two, and one of them is elfcorehdr, then 
perhaps in addition to elf_index and elf_index_valid, maybe we add an arch_index and 
arch_index_valid? In the case of PPC, the FDT would be stored in arch_index?


Either way.
Thanks!
eric





+#endif
  #endif
  #ifdef CONFIG_IMA_KEXEC
@@ -316,6 +323,15 @@ struct kimage {
  unsigned long elf_load_addr;
  };
+#ifdef CONFIG_CRASH_HOTPLUG
+void arch_crash_hotplug_handler(struct kimage *image,
+    unsigned int hp_action, unsigned long a, unsigned long b);
+#define KEXEC_CRASH_HP_REMOVE_CPU   0
+#define KEXEC_CRASH_HP_ADD_CPU  1
+#define KEXEC_CRASH_HP_REMOVE_MEMORY 2
+#define KEXEC_CRASH_HP_ADD_MEMORY   3
+#endif /* CONFIG_CRASH_HOTPLUG */
+
  /* kexec interface functions */
  extern void machine_kexec(struct kimage *image);
  extern int machine_kexec_prepare(struct kimage *image);
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 256cf6db573c..76959d440f71 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -9,12 +9,17 @@
  #include 
  #include 
  #include 
+#include 
+#include 
+#include 
  #include 
  #include 
  #include 
+#include "kexec_internal.h"
+
  /* vmcoreinfo stuff */
  unsigned char *vmcoreinfo_data;
  size_t vmcoreinfo_size;
@@ -491,3 +496,106 @@ static int __init crash_save_vmcoreinfo_init(void)
  }
  subsys_initcall(crash_save_vmcoreinfo_init);
+
+#ifdef CONFIG_CRASH_HOTPLUG
+void __weak arch_crash_hotplug_handler(struct kimage *image,
+    unsigned int hp_action, unsigned long a, unsigned long b)
+{
+    pr_warn("crash hp: %s not implemented", __func__);
+}
+
+static void crash_hotplug_handler(unsigned int hp_action,
+    unsigned long a, unsigned long b)
+{
+    /* Obtain lock while changing crash information */
+    if (!mutex_trylock(&kexec_mutex))
+    return;
+
+    /* Check kdump is loaded */
+    if (kexec_crash_image) {
+    pr_debug("crash hp: hp_action %u, a %lu, b %lu", hp_action,
+    a, b);
+
+    /* Needed in order for the segments to be updated */
+    arch_kexec_unprotect_crashkres();
+
+    /* Flag to differentiate between normal load and hotplug */
+    kexec_crash_image->hotplug_event = true;
+
+    /* Now invoke arch-specific update handler */
+    arch_crash_hotplug_handler(kexec_crash_image, hp_action, a, b);
+
+    /* No longer handling a hotplug event */
+    kexec_crash_image->hotplug_event = false;
+
+    /* Change back to read-only */
+    arch_kexec_protect_crashkres();
+    }
+
+    /* Release lock no

Re: [PATCH v21 1/5] kdump: return -ENOENT if required cmdline option does not exist

2022-03-15 Thread Leizhen (ThunderTown)



On 2022/3/15 20:21, Baoquan He wrote:
> On 03/15/22 at 07:57pm, Baoquan He wrote:
>> On 02/27/22 at 11:07am, Zhen Lei wrote:
>>> The crashkernel=Y,low is an optional command-line option. When it doesn't
>>> exist, kernel will try to allocate minimum required memory below 4G
>>> automatically. Give it a unique error code to distinguish it from other
>>> error scenarios.
>>>
>>> Signed-off-by: Zhen Lei 
>>> ---
>>>  kernel/crash_core.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>>> index 256cf6db573cd09..4d57c03714f4e13 100644
>>> --- a/kernel/crash_core.c
>>> +++ b/kernel/crash_core.c
>>> @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline,
>>> *crash_base = 0;
>>>  
>>> ck_cmdline = get_last_crashkernel(cmdline, name, suffix);
>>> -
>>> if (!ck_cmdline)
>>> -   return -EINVAL;
>>> +   return -ENOENT;
>>
>> Firstly, I am not sure if '-ENOENT' is a right value to return. From the
>> code comment of ENOENT, it's used for file or dir?
>> #define ENOENT   2  /* No such file or directory */

This error code does not return to user mode, so there is no problem.
There are a lot of places in the kernel that are used this way. For example:

int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg)
{
if (!cpu_stop_queue_work(cpu, &work))
return -ENOENT;

>>
>> Secondly, we ever discussed the case including
>>  - no crashkernel=,low is provided;
>>  - messy code is provied, e.g crashkernel=aa,low
> 
> Checking the 3rd pach, this is handled. Take back my below words,
> continue reviewing.

Yes.

> 
>>
>> The 2nd one is not handled in this patchset. How about taking the
>> handling into another round of patches. This patchset just adds
>> crashkernel=,high purely.
>>
>>>  
>>> ck_cmdline += strlen(name);
>>>  
>>> -- 
>>> 2.25.1
>>>
>>
> 
> .
> 

-- 
Regards,
  Zhen Lei

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


Re: [PATCH v21 1/5] kdump: return -ENOENT if required cmdline option does not exist

2022-03-15 Thread Baoquan He
On 03/15/22 at 07:57pm, Baoquan He wrote:
> On 02/27/22 at 11:07am, Zhen Lei wrote:
> > The crashkernel=Y,low is an optional command-line option. When it doesn't
> > exist, kernel will try to allocate minimum required memory below 4G
> > automatically. Give it a unique error code to distinguish it from other
> > error scenarios.
> > 
> > Signed-off-by: Zhen Lei 
> > ---
> >  kernel/crash_core.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > index 256cf6db573cd09..4d57c03714f4e13 100644
> > --- a/kernel/crash_core.c
> > +++ b/kernel/crash_core.c
> > @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline,
> > *crash_base = 0;
> >  
> > ck_cmdline = get_last_crashkernel(cmdline, name, suffix);
> > -
> > if (!ck_cmdline)
> > -   return -EINVAL;
> > +   return -ENOENT;
> 
> Firstly, I am not sure if '-ENOENT' is a right value to return. From the
> code comment of ENOENT, it's used for file or dir?
> #define ENOENT   2  /* No such file or directory */
> 
> Secondly, we ever discussed the case including
>  - no crashkernel=,low is provided;
>  - messy code is provied, e.g crashkernel=aa,low

Checking the 3rd pach, this is handled. Take back my below words,
continue reviewing.

> 
> The 2nd one is not handled in this patchset. How about taking the
> handling into another round of patches. This patchset just adds
> crashkernel=,high purely.
> 
> >  
> > ck_cmdline += strlen(name);
> >  
> > -- 
> > 2.25.1
> > 
> 


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


Re: [PATCH v5 4/8] crash: generic crash hotplug support infrastructure

2022-03-15 Thread Sourabh Jain

Hello Eric,

On 03/03/22 21:57, Eric DeVolder wrote:

This patch introduces a generic crash hot plug/unplug infrastructure
for CPU and memory changes. Upon CPU and memory changes, a generic
crash_hotplug_handler() obtains the appropriate lock, does some
important house keeping and then dispatches the hot plug/unplug event
to the architecture specific arch_crash_hotplug_handler(), and when
that handler returns, the lock is released.

This patch modifies crash_core.c to implement a subsys_initcall()
function that installs handlers for hot plug/unplug events. If CPU
hotplug is enabled, then cpuhp_setup_state() is invoked to register a
handler for CPU changes. Similarly, if memory hotplug is enabled, then
register_memory_notifier() is invoked to install a handler for memory
changes. These handlers in turn invoke the common generic handler
crash_hotplug_handler().

On the CPU side, cpuhp_setup_state_nocalls() is invoked with parameter
CPUHP_AP_ONLINE_DYN. While this works, when a CPU is being unplugged,
the CPU still shows up in foreach_present_cpu() during the regeneration
of the new CPU list, thus the need to explicitly check and exclude the
soon-to-be offlined CPU in crash_prepare_elf64_headers().

On the memory side, each un/plugged memory block passes through the
handler. For example, if a 1GiB DIMM is hotplugged, that generate 8
memory events, one for each 128MiB memblock.

Signed-off-by: Eric DeVolder 
---
  include/linux/kexec.h |  16 +++
  kernel/crash_core.c   | 108 ++
  2 files changed, 124 insertions(+)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index d7b59248441b..b11d75a6b2bc 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -300,6 +300,13 @@ struct kimage {
  
  	/* Information for loading purgatory */

struct purgatory_info purgatory_info;
+
+#ifdef CONFIG_CRASH_HOTPLUG
+   bool hotplug_event;
+   int offlinecpu;
+   bool elf_index_valid;
+   int elf_index;


How about keeping an array to track all kexec segment index need to be 
updated in

crash hotplug handler.

struct hp_segment {
   name;
   index;
   is_valid;
 }

It will be helpful if architecture need to updated multiple kexec 
segments  for a hotplug event.


For example, on PowerPC, we might need to update FDT and elfcorehdr on 
memory hot plug/unplug.


Thanks,
Sourabh Jain



+#endif
  #endif
  
  #ifdef CONFIG_IMA_KEXEC

@@ -316,6 +323,15 @@ struct kimage {
unsigned long elf_load_addr;
  };
  
+#ifdef CONFIG_CRASH_HOTPLUG

+void arch_crash_hotplug_handler(struct kimage *image,
+   unsigned int hp_action, unsigned long a, unsigned long b);
+#define KEXEC_CRASH_HP_REMOVE_CPU   0
+#define KEXEC_CRASH_HP_ADD_CPU  1
+#define KEXEC_CRASH_HP_REMOVE_MEMORY 2
+#define KEXEC_CRASH_HP_ADD_MEMORY   3
+#endif /* CONFIG_CRASH_HOTPLUG */
+
  /* kexec interface functions */
  extern void machine_kexec(struct kimage *image);
  extern int machine_kexec_prepare(struct kimage *image);
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 256cf6db573c..76959d440f71 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -9,12 +9,17 @@
  #include 
  #include 
  #include 
+#include 
+#include 
+#include 
  
  #include 

  #include 
  
  #include 
  
+#include "kexec_internal.h"

+
  /* vmcoreinfo stuff */
  unsigned char *vmcoreinfo_data;
  size_t vmcoreinfo_size;
@@ -491,3 +496,106 @@ static int __init crash_save_vmcoreinfo_init(void)
  }
  
  subsys_initcall(crash_save_vmcoreinfo_init);

+
+#ifdef CONFIG_CRASH_HOTPLUG
+void __weak arch_crash_hotplug_handler(struct kimage *image,
+   unsigned int hp_action, unsigned long a, unsigned long b)
+{
+   pr_warn("crash hp: %s not implemented", __func__);
+}
+
+static void crash_hotplug_handler(unsigned int hp_action,
+   unsigned long a, unsigned long b)
+{
+   /* Obtain lock while changing crash information */
+   if (!mutex_trylock(&kexec_mutex))
+   return;
+
+   /* Check kdump is loaded */
+   if (kexec_crash_image) {
+   pr_debug("crash hp: hp_action %u, a %lu, b %lu", hp_action,
+   a, b);
+
+   /* Needed in order for the segments to be updated */
+   arch_kexec_unprotect_crashkres();
+
+   /* Flag to differentiate between normal load and hotplug */
+   kexec_crash_image->hotplug_event = true;
+
+   /* Now invoke arch-specific update handler */
+   arch_crash_hotplug_handler(kexec_crash_image, hp_action, a, b);
+
+   /* No longer handling a hotplug event */
+   kexec_crash_image->hotplug_event = false;
+
+   /* Change back to read-only */
+   arch_kexec_protect_crashkres();
+   }
+
+   /* Release lock now that update complete */
+   mutex_unlock(&kexec_mutex);
+}
+
+#if defined(CONFIG_MEMORY_HOTPLUG)
+static int crash_memhp_notifier(struct notifier_block *nb,
+   unsigned 

Re: [PATCH v21 5/5] docs: kdump: Update the crashkernel description for arm64

2022-03-15 Thread Baoquan He
On 02/27/22 at 11:07am, Zhen Lei wrote:
> Now arm64 has added support for "crashkernel=X,high" and
> "crashkernel=Y,low", and implements "crashkernel=X[@offset]" in the
> same way as x86. So update the Documentation.
> 
> Signed-off-by: Zhen Lei 

Looks good to me, thx.

Acked-by: Baoquan He 

> ---
>  Documentation/admin-guide/kernel-parameters.txt | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> b/Documentation/admin-guide/kernel-parameters.txt
> index f5a27f067db9ed9..63098786c93828c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -789,7 +789,7 @@
>   memory region [offset, offset + size] for that kernel
>   image. If '@offset' is omitted, then a suitable offset
>   is selected automatically.
> - [KNL, X86-64] Select a region under 4G first, and
> + [KNL, X86-64, ARM64] Select a region under 4G first, and
>   fall back to reserve region above 4G when '@offset'
>   hasn't been specified.
>   See Documentation/admin-guide/kdump/kdump.rst for 
> further details.
> @@ -802,20 +802,20 @@
>   Documentation/admin-guide/kdump/kdump.rst for an 
> example.
>  
>   crashkernel=size[KMG],high
> - [KNL, X86-64] range could be above 4G. Allow kernel
> + [KNL, X86-64, ARM64] range could be above 4G. Allow 
> kernel
>   to allocate physical memory region from top, so could
>   be above 4G if system have more than 4G ram installed.
>   Otherwise memory region will be allocated below 4G, if
>   available.
>   It will be ignored if crashkernel=X is specified.
>   crashkernel=size[KMG],low
> - [KNL, X86-64] range under 4G. When crashkernel=X,high
> + [KNL, X86-64, ARM64] range under 4G. When 
> crashkernel=X,high
>   is passed, kernel could allocate physical memory region
>   above 4G, that cause second kernel crash on system
>   that require some amount of low memory, e.g. swiotlb
>   requires at least 64M+32K low memory, also enough extra
>   low memory is needed to make sure DMA buffers for 32-bit
> - devices won't run out. Kernel would try to allocate at
> + devices won't run out. Kernel would try to allocate
>   at least 256M below 4G automatically.
>   This one let user to specify own low range under 4G
>   for second kernel instead.
> -- 
> 2.25.1
> 


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


Re: [PATCH v21 1/5] kdump: return -ENOENT if required cmdline option does not exist

2022-03-15 Thread Baoquan He
On 02/27/22 at 11:07am, Zhen Lei wrote:
> The crashkernel=Y,low is an optional command-line option. When it doesn't
> exist, kernel will try to allocate minimum required memory below 4G
> automatically. Give it a unique error code to distinguish it from other
> error scenarios.
> 
> Signed-off-by: Zhen Lei 
> ---
>  kernel/crash_core.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 256cf6db573cd09..4d57c03714f4e13 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline,
>   *crash_base = 0;
>  
>   ck_cmdline = get_last_crashkernel(cmdline, name, suffix);
> -
>   if (!ck_cmdline)
> - return -EINVAL;
> + return -ENOENT;

Firstly, I am not sure if '-ENOENT' is a right value to return. From the
code comment of ENOENT, it's used for file or dir?
#define ENOENT   2  /* No such file or directory */

Secondly, we ever discussed the case including
 - no crashkernel=,low is provided;
 - messy code is provied, e.g crashkernel=aa,low

The 2nd one is not handled in this patchset. How about taking the
handling into another round of patches. This patchset just adds
crashkernel=,high purely.

>  
>   ck_cmdline += strlen(name);
>  
> -- 
> 2.25.1
> 


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