Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6af5bf76-065c-9167-a73e-21908f4b5ef3@oracle.com>
Date:   Wed, 4 May 2022 13:11:04 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH v7 4/8] crash: add generic infrastructure for crash
 hotplug support
Content-Language: en-US
To:     Sourabh Jain <sourabhjain@linux.ibm.com>,
        Baoquan He <bhe@redhat.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        kexec@lists.infradead.org, ebiederm@xmission.com,
        dyoung@redhat.com, vgoyal@redhat.com, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        hpa@zytor.com, nramas@linux.microsoft.com, thomas.lendacky@amd.com,
        robh@kernel.org, efault@gmx.de, rppt@kernel.org, david@redhat.com,
        konrad.wilk@oracle.com, boris.ostrovsky@oracle.com
References: <20220413164237.20845-1-eric.devolder@oracle.com>
 <20220413164237.20845-5-eric.devolder@oracle.com>
 <YleK3J/4HNuFioIh@MiWiFi-R3L-srv>
 <4eea2373-32f3-9960-cbec-21dc1a428807@oracle.com>
 <6f3a6cbb-0ac4-f178-fc17-18f9594da319@linux.ibm.com>
 <YmeBjrChOHsIYG3e@MiWiFi-R3L-srv>
 <f2dbd4fe-6201-b5c4-2725-dec9c3dbf13e@linux.ibm.com>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <f2dbd4fe-6201-b5c4-2725-dec9c3dbf13e@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eE5tN0k2VkltbGJkcVhoTDRUelZnY25ScEpDaWJGMGowQWlmSzVuTFhTeHc0?=
 =?utf-8?B?cTFHWmtsZXRRRWt2RThBTUhpN0FOZzVrN2FCaFJLTHRIYi9NT1pQb2xKVDJB?=
 =?utf-8?B?dUJBTWdpRllaNFNGbGZQSTY3ZU1tTElESEZCNnBrWVhRencxUlNJQzhmbDZ4?=
 =?utf-8?B?Q0ZyZjJlOUhxYWJvT1JjVE14MHVFWnhpZjIrajA2UUFBYWdGd1EySU5GUmdy?=
 =?utf-8?B?aUVCMWhITjNKVkE0RW8vSGRmUDZGakRLUTVKTVZCbFVreTFvYnUxL1ZNTUpw?=
 =?utf-8?B?amN6S0dwenBwS1ZUR05SQUdBdTJqdFg4ZXZRVFAwODhpRUx5MVFnTmFFZ2sy?=
 =?utf-8?B?c3ZEL3RQeGhsVkdETnU2RXRjOE1ienlsRkNIREo3UmFyNjBWOWtkN3Rwbi9X?=
 =?utf-8?B?bU44b1BSYlJzMnRaVTEyMG9MZ2JnN0JyUmsycUlmbzM2OFFjd2NOdmlxakZI?=
 =?utf-8?B?am1vaG94QjBkendMN2hPYWNzRWRXZlE4eHRKeEQzUUQ0bkhNOWw4eEhFU3d3?=
 =?utf-8?B?cTdzazdlUDFqa1pxVC9vUkJFM003UXBOZGI2cWZPU1Z3dDVTZ0J6ejVsZXo4?=
 =?utf-8?B?VkVyVkFhMlVGU3hoV2ViUy9kalg0MnhoWFo0QWRzSUU5NlZyQ0toMzNpdUM1?=
 =?utf-8?B?bTgyOU5TcWU0YUgva1NDLzdncFM1clpOb0VhUmVnb3FnOVdpU3ZkTkpabjg5?=
 =?utf-8?B?bXdKamFJdVg0YmQxSVRpUllNS1FoQjBzeEk3RkNHVDhpU0Q4a2lVN3R4U2Vz?=
 =?utf-8?B?STFiSEZmdG00REl5T1FNdlgwUFZVK0FNWUpYaTRJQkdndmVOSWdaL1ZnZ21h?=
 =?utf-8?B?R3hXSFAxSnY2WndJOVVPN0k5dWdLZ2E2V2hlUlIwaDR4TWhTTWJjeTJmbG5t?=
 =?utf-8?B?SytuNjRoR3hPbHVMY0VpSVpmQXcxc2wrVXJoaHR6d2dlSHV1TW9SdGF2Ty9j?=
 =?utf-8?B?dXA3eXVSQUxTSlEySzIycHBtTzRtYzlDeVdDMG0zeWpNTlRjamd5YjRkYjBH?=
 =?utf-8?B?QTM1R3BBVnRxVTRWZW9CdEthNnBnV2hZZE5zVThmYUtLU3cxaHJmcVphMExs?=
 =?utf-8?B?M2hPTFNkU21oaG1LenRwTHN0QTJBVmFNcCs2ZVR2QnRnVEJzWnRnc3Fsek9H?=
 =?utf-8?B?YWNGMTd5NXRVQWg2ZGs5b0czMUJFamwyaDdURlFvUHBqWUpzdFlaVGd1MkJr?=
 =?utf-8?B?Z3dqZkRIUGlxeDhtTVFIWDJUa20wdllDQlZWVEExV2ZBR2orLzUzaVc0Vmxj?=
 =?utf-8?B?YmR3Q1VoU05zcVQvUlRuUi9vb3dCczB6ekdvbkd2cmdUNXh4UCs2VW55ZlRJ?=
 =?utf-8?B?aXN2akRZWllkM3V1eVd3bGc1MEJFZENnVmxndDJzbExFK0VOUFFMd28wdTg3?=
 =?utf-8?B?SjVsMVRtYzA4MHIwNFQ5WUhRL1NrRnJJUTFkSUN3ZGdtSHpXcndSTmlZZjVK?=
 =?utf-8?B?QnRKQnlpbm1IMGFLUnd6TmFlQ1RvbzNOS1ZlaTRlemVRcXFSaWZHQXJJa3dr?=
 =?utf-8?B?L3JlUmNmdmhaaUxNcitoSXhIak5yU0ttcnJsdFQ1d1BxUzJNb1dmUHFTa3Bk?=
 =?utf-8?B?VTMrbWhkZ2oraS84UEx5TDRTOEtiaklLckxwYzlhQWhoRDBKMzdKcDZPZGRW?=
 =?utf-8?B?N3Z0QStRdmUvU0w2cGpsczlPeno2S2x5M0EvRHRlaVc2ODZHSmRUN3dMclFr?=
 =?utf-8?B?WjVMMm5hd0x5cFZKZmI5cGN2WXpFek9JVis1cVNkYm5aZitlbWFzTzF3Ykdm?=
 =?utf-8?B?Y3MxbzBlc0RCYkdMV1d3N25aWU1VYTJ3MWtSSG9BNldoLzc2SmNXUzVscFpx?=
 =?utf-8?B?cnNqNGg5ZUJaMEV2WG13Q25SUmhEL1d5dWQ2S2FvN2FNcWptNlhmUTNySExn?=
 =?utf-8?B?Zy9QRUR3NUthNEppbjZTSUc2STlzU1BhakRYOFpHUXFHWEJQNG1LNjBQYVFF?=
 =?utf-8?B?M1ZxTWc3c08rT2RQLzZJeEprclUzT25sRlVzYUdLNDN3YW9iOUQxS1VaeTFm?=
 =?utf-8?B?d0dhOGJpY3BDRkthVTRLemR6UGpCci9SUmVJQUJ0bmc1ODJqUmQ5NVUyZkZ4?=
 =?utf-8?B?SW5RclkvdmhmL0gyV1FRelh0clZsL2pVS1RuRkNlRDBLNktkaGUra1hqenVF?=
 =?utf-8?B?ZncwN3BmVFVZcXMyNTFUTmxydmJiWlJpdVIxQ2JuZTdmaVh1d1FKRWZCdEpT?=
 =?utf-8?B?V3M1bldsK0hPcFd1ZDFNeUtzaFBiT2VkcDRKQzVkZnl5OFpJZ015b3IrcUhv?=
 =?utf-8?B?MEo2L1h4cXVQRVVZZUswZEZQMStVSDFsbW9QT2kyaFJWTm44QWVKRnRFakYw?=
 =?utf-8?B?RXJRaFNCN2F0b01weCt1TXg0YVpYbUkzSzdsc2duQ014Qkx3OWhyenFSbS9a?=
 =?utf-8?Q?6U/PgyGd6mMEf1WRmPf8G5Idu2rcqUxNNTfVy?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3e4dd7dc-098b-4e94-1ed6-08da2df977d9
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 May 2022 18:11:11.0764
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rdRIrXmutXGnOsQ8COkNm1UUuXX2gRkbGi4wJ4QCyGwEIVkx4WD/AJ22kvNFwo7WkJOiftkA70tUb0TYqiXnDrtgEe7UE0Q9lzutsIwwF20=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB3811
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-05-04_04:2022-05-04,2022-05-04 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 malwarescore=0
 spamscore=0 mlxlogscore=999 mlxscore=0 suspectscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2205040108
X-Proofpoint-GUID: Bl9zTsz75vQ8QVHi1thOfSTJcevxauaK
X-Proofpoint-ORIG-GUID: Bl9zTsz75vQ8QVHi1thOfSTJcevxauaK
X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 4/28/22 00:18, 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:
>>>> Hi Baoquan,
>>>> Inline comments below.
>>>> Thanks!
>>>> eric
>>>>
>>>> On 4/13/22 21:45, Baoquan He wrote:
>>>>> On 04/13/22 at 12:42pm, Eric DeVolder wrote:
>>>>>> Upon CPU and memory changes, a generic crash_hotplug_handler()
>>>>>> dispatches the hot plug/unplug event to the architecture specific
>>>>>> arch_crash_hotplug_handler(). During the process, the kexec_mutex
>>>>>> is held.
>>>>>>
>>>>>> To support cpu hotplug, a callback is registered to capture the
>>>>>> CPUHP_AP_ONLINE_DYN online and ofline events via
>>>>>> cpuhp_setup_state_nocalls().
>>>>>>
>>>>>> To support memory hotplug, a notifier is registered to capture the
>>>>>> MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>>>>>>
>>>>>> The cpu callback and memory notifier then call crash_hotplug_handler()
>>>>>> to handle the hot plug/unplug event.
>>>>>>
>>>>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>>>>>> ---
>>>>>>    include/linux/kexec.h |  16 +++++++
>>>>>>    kernel/crash_core.c   | 101
>>>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>>>>    2 files changed, 117 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>>>>>> index f93f2591fc1e..02daff1f47dd 100644
>>>>>> --- a/include/linux/kexec.h
>>>>>> +++ b/include/linux/kexec.h
>>>>>> @@ -306,6 +306,13 @@ struct kimage {
>>>>>>          /* Information for loading purgatory */
>>>>>>        struct purgatory_info purgatory_info;
>>>>>> +
>>>>>> +#ifdef CONFIG_CRASH_HOTPLUG
>>>>>> +    bool hotplug_event;
>>>>>> +    unsigned int offlinecpu;
>>>>>> +    bool elfcorehdr_index_valid;
>>>>>> +    int elfcorehdr_index;
>>>>>> +#endif
>>>>>>    #endif
>>>>>>      #ifdef CONFIG_IMA_KEXEC
>>>>>> @@ -322,6 +329,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 int cpu);
>>>>>> +#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..ecf746243ab2 100644
>>>>>> --- a/kernel/crash_core.c
>>>>>> +++ b/kernel/crash_core.c
>>>>>> @@ -9,12 +9,17 @@
>>>>>>    #include <linux/init.h>
>>>>>>    #include <linux/utsname.h>
>>>>>>    #include <linux/vmalloc.h>
>>>>>> +#include <linux/highmem.h>
>>>>>> +#include <linux/memory.h>
>>>>>> +#include <linux/cpuhotplug.h>
>>>>>>      #include <asm/page.h>
>>>>>>    #include <asm/sections.h>
>>>>>>      #include <crypto/sha1.h>
>>>>>>    +#include "kexec_internal.h"
>>>>>> +
>>>>>>    /* vmcoreinfo stuff */
>>>>>>    unsigned char *vmcoreinfo_data;
>>>>>>    size_t vmcoreinfo_size;
>>>>>> @@ -491,3 +496,99 @@ 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 int cpu)
>>>>>> +{
>>>>>> +    pr_warn("crash hp: %s not implemented", __func__);
>>>>>> +}
>>>>>> +
>>>>>> +static void crash_hotplug_handler(unsigned int hp_action,
>>>>>> +    unsigned int cpu)
>>>>>> +{
>>>>>> +    /* 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, cpu %u", hp_action, cpu);
>>>>>> +
>>>>>> +        /* 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, cpu);
>>>>>> +
>>>>>> +        /* 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 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.
> 
>>
>> 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.

Baoquan,
It appears the straight forward thing to do here is just to keep the 4 cpu/mem add/remove 
combinations. It appears there is value in keeping them as currently defined. However, please 
indicate if you agree or not.
Thanks!
Eric

> 
>> +       /* 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
>