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

[Eric DeVolder]

On 5/5/22 06:04, Baoquan He wrote:
> On 05/05/22 at 03:29pm, Sourabh Jain wrote:
>>
>> On 05/05/22 11:15, Baoquan He wrote:
>>> 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.
>>
>> Yes we don't need start and end info as such but we expect arch
>> handler to have info about which hotplug action is performed.
>> It is just that I don't see an significant advantage of consolidation of
>> CPU ADD, memory ADD and Memory REMOVE in one hp_action as
>> KEXEC_CRASH_HP_UPDATE_OTHER.
> 
> I see. I don't oppose all those passed info, just worried the
> unnecessary info passed down to the handler.
> 
>>
>>> 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)
>> The above template works fine for PowerPC as long we have four different
>> hp_action
>> to indicate CPU add/remove and memory add/remove.
> 
> Cool. Then all things are clear. We can pass the needed hp_action and
> cpu number only.
> 
> Hi Eric,
> 
> The consensus is reached, please proceed when it's convenient.
> 
Excellent! I will post v8 soon!
Thanks!
eric