Re: [PATCH v7 4/8] crash: add generic infrastructure for crash hotplug support

[Baoquan He]

On 04/28/22 at 10:48am, Sourabh Jain wrote:
> Hi Baoquan,
> 
> On 26/04/22 10:52, Baoquan He wrote:
> > On 04/26/22 at 09:36am, Sourabh Jain wrote:
> > > On 15/04/22 03:59, Eric DeVolder wrote:
......

> > > > > > +#if defined(CONFIG_MEMORY_HOTPLUG)
> > > > > > +static int crash_memhp_notifier(struct notifier_block *nb,
> > > > > > +    unsigned long val, void *v)
> > > > > > +{
> > > > > > +    struct memory_notify *mhp = v;
> > > > > > +
> > > > > > +    switch (val) {
> > > > > > +    case MEM_ONLINE:
> > > > > > +        crash_hotplug_handler(KEXEC_CRASH_HP_ADD_MEMORY, -1U);
> > > > > We don't differentiate the memory add/remove, cpu add, except of cpu
> > > > > remove. Means the hp_action only differentiate cpu remove from the other
> > > > > action. Maybe only making two types?
> > > > > 
> > > > > #define KEXEC_CRASH_HP_REMOVE_CPU   0
> > > > > #define KEXEC_CRASH_HP_UPDATE_OTHER      1
> > > > > 
> > > > Sourabh Jain's work with PPC uses REMOVE_CPU, REMOVE_MEMORY, and
> > > > ADD_MEMORY.
> > > > Do you still want to consolidate these?
> > > On PowerPC different actions are needed for CPU add and memory add/remove.
> > > For CPU add case only FDT is updated whereas for the memory hotplug we will
> > > be
> > > updating FDT and elfcorehdr.
> > I don't understand. For elfcorehdr updating, we only need regenerate it.
> > Do you update them different for memory add/remove?
> 
> We have different actions for cpu remove, CPU add and memory add/remove
> case.
> 
> CPU remove: no action
> CPU add: update flattened device tree (FDT)
> memory add/remove: update FDT and regenerate/update elfcorehdr
> 
> Since memory add/remove action is same we can have common hp_action for
> them.

For memory hot add/remove, we need rengereate elfcorehdr, and add the
new elfcorehdr into fdt. Except of this, FDT need to know the hp_action
and the hot added/removed memory region, namely the start and end, e.g
[start, end]? 

I checked arm64 kexec code, seems we only need to know if mem hotplug
event happened, then regenerate elfcorehdr and embed the new elfcorehdr
into fdt. Then we don't know pass the [start, end] info into the
handler. Please tell if ppc is different or I missed anything.

If I am right, I would like the handler interface as Boris has made
in his draft patch.

void __weak arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
                                           unsigned int cpu)

static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> 
> > 
> > What I saw is the added action for memory hotplug is only for message
> > printing. Is this really needed? And memory hotplug is even not
> > supported. Please correct me if I missed anything.
> 
> I agree that currently memory hp_action is only used for printing warning
> message but
> eventually we will be handling memory hotplug case as well.
> 
> > +       /* crash update on memory hotplug is not support yet */
> > +       if (hp_action == KEXEC_CRASH_HP_REMOVE_MEMORY || hp_action == KEXEC_CRASH_HP_ADD_MEMORY) {
> > +               pr_info_once("crash hp: crash update is not supported with memory hotplug\n");
> > +               return;
> > +       }
> 
> Thanks,
> Sourabh Jain
>