Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <4eea2373-32f3-9960-cbec-21dc1a428807@oracle.com>
Date:   Thu, 14 Apr 2022 17:29:44 -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:     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>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <YleK3J/4HNuFioIh@MiWiFi-R3L-srv>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QmIydE1PKzdqTk1vM2wzeDRGRnZoQndJQjg2dWpMcEtsdUxjUnJkZzFRQzBG?=
 =?utf-8?B?UVZ2a0ErZHdwcjBxcWtYTVRnNDRTSzZhN0Z4MkU2dDdKc1Q5QU1ZQ25vM2N0?=
 =?utf-8?B?MUVXUHQ1WkNWRVY5aFZ2MUM2NTJtUC85ZUYzUWZlekhUK05oMkU0MkxIZzRW?=
 =?utf-8?B?ay9MR0cybU1ZWm5VdUxJNlFXRTAxcVAvVER3TmNvWUgycXFueTdTNnM4SDJG?=
 =?utf-8?B?WTF3aGc4OThBOWtpQmtxWUJNSCs0S1VaVHBHQkluOWNCWk1kbnd6Ty9SQm9B?=
 =?utf-8?B?ViswMDVFTDl6WWpmRXc3UG9IaTZ6cElvNDBFeDhxQTZ3R1hYZHFORFg2OVdD?=
 =?utf-8?B?Nk1vMXVMQ3VzZEh5ay9LY2NvTzAyZ01WQTBWZUt0dElRSGF3dU9FN3FFVVlC?=
 =?utf-8?B?OXdHM2RraUo2dHRtSkFCYllGNnRvSVZsR0l3eGZCY0ZSaUtZVFB2Um9iK25v?=
 =?utf-8?B?azFyT0ovNmJ2V2JLUGhSdXV5T1k3NmlpTzNjYXViYlBENVNveW4wYk1jT3ZT?=
 =?utf-8?B?S1FvUFpsaDZ1ZGk0MUJsOXg1bXJkYUJhYWVxYmRSTjJONHVyY0poZTBCR3A4?=
 =?utf-8?B?R0RER3h5M25IZjZZTXFQOW1uL0dPbloyRHVkSzdqYVlIS1RHSkd2NldpZVFJ?=
 =?utf-8?B?RHNlU3FCRlV6bm53cjdiVUlaTzVrN3lPSTRqYWJObWluK1JlZWVyNkpqaU9u?=
 =?utf-8?B?cFA1UTlRS1d1K1JqQzhRQkNkREtiS3NQTTEwUWVuSy9wY2xRbDY5dzZDNklM?=
 =?utf-8?B?dUxNYTNuZEttbmk0MmgwTitPZ3lvNkpKSzcrRElUNnB6SVpoRVZPSHJocmU0?=
 =?utf-8?B?S2dqMlJXQzd2ZktKRmxScFRubk9mdkhmR2k2dG1ZOVVTcktpem43NW14NitR?=
 =?utf-8?B?cWhIMWpJNWxPa2w5RUVFSFFFUnh2d25tVEpyRVNVbGh6L1p1bUlWQ2RtN3JX?=
 =?utf-8?B?MkJyNG8xOUhTRFdCcDlyNFBNK2h2RzRxNmE1ck9mcklFMzMvWHZXQURaSjRX?=
 =?utf-8?B?VXR2b1htR3VRajVTTlZRemVjek1POHFQNkhhU3NsUE04U0dLamJ2QVlQR1Nq?=
 =?utf-8?B?aGljbkwwTzQ5ZDRzY0NWWC9VMXhDQjQ4d0dRYlk0cHA0STNPTjZVTXdmckc5?=
 =?utf-8?B?VkdIV05nMEtuYlc5N05hZXliK0YxS0d5eUs4Qk5CVEpXUUhVcy9JYVpJNWtl?=
 =?utf-8?B?QWlXR04yVFVBU01EVy8wNUZkd21TQTlreXhJUHhURTFSbUh5WHBLdFRJNUFt?=
 =?utf-8?B?N3l4bGZ0MFJranNCcWEzMWUzZHdPWEJodEM3dEJaRUZ1NlIxRVk5dUEvcTZE?=
 =?utf-8?B?MXRreWxXTk1wT21PZXcwK1prZUJSd1FZeGRrcktjOUk1eDBENXB0NXBLZWVr?=
 =?utf-8?B?QVRXRDZuU0R1UUlMSDNYSW5zNEVYd3J5OStZWXNwTVJta2lNTU9zOVAvU05x?=
 =?utf-8?B?V05uWEF1MWhPSmplZHU3MWt5ZHNlaWNLMnA4ampYZStBSFZIa0xobFd1Qjl5?=
 =?utf-8?B?Q0hBbXZkbnpLcWFhU0NIVFR5UjFFZjllRkRDZ1BjMlNDUzdteFlHU0l2NW8x?=
 =?utf-8?B?WDVWK09nTmJrc0NYL1JWLy9BRXBtc2UyVUpVdG1VODhkdVFEUHp2UlRTaC9S?=
 =?utf-8?B?U1VoRHRoK2VqNmJoN3hFZ0VLaXFjTFpZd1Z4MjVBRGg3ZU04am9rY1ZuNlp3?=
 =?utf-8?B?UHVHQm5QSnRTSlhIRDdBRjRnQno1RXlvTXp2anVxWGE0NmhkMHZqbHFEUEV2?=
 =?utf-8?B?UzZKVHFiU3UycU5kNDhMdytZc2FFODByVnh3aUdkZGJHK3dCVktsRWNjbjlG?=
 =?utf-8?B?aEVrZ0h1L0dhM1pjbjd4QkFzUGxXMmRQNnBoM0VBQ09PR0tDQ0tOdlkxN2w0?=
 =?utf-8?B?S1Z5RFVqWmhwdGd0OC95MWIyeTNoY2JicEVmTWptTkJmWVlaalZXSlZiamtq?=
 =?utf-8?B?NEF6ZWlGTm5KMUtNVkFnejNLVUtlcUhFbVV6cWo2UDdMWUpnK00vWlhCOTcx?=
 =?utf-8?B?WmpRejRUU0NoWGd1VkExQWZsMENWKy9Ta0xrS3dtZDlpZVlQZWh5S21pbXN2?=
 =?utf-8?B?OVBiMXhaYjNKMU5waWJwcERNRXgzQktCaTVwS3F4d3pzbFdwY2hhMDFlZEJB?=
 =?utf-8?B?dzJPNEdONDhqRFdYREpyVVR2WHBmek1SWHNpRjRUOW1rbFQveHhWbTNMZ29T?=
 =?utf-8?B?UEVLMTBEN3RGK3paT3Y1OUZ0dmd3ek1Qcy84MGp5bjZxSDJYQkE5Y05pamVB?=
 =?utf-8?B?ZkM3cFIveDFESm90SUdmc2VjOFEzY2lvV1hiV3orcUZDMUhrME9nT3c0YkpC?=
 =?utf-8?B?OGZudnhhMnlnN0cwZzNDWER3Qzk0TTNTMmFScDdaYm84aXIyY2FYdXVyVTha?=
 =?utf-8?Q?7yVS1JEFRa9nhevUkvWrNOzETL19TRkVyU3E7?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f2d0bd28-bd94-4f76-363a-08da1e664908
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Apr 2022 22:29:48.9306
 (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: TdRCEP9M7K3fiM2IpSzdSNsnwlYE+zLFu9aDxakID1mBz7ikOM7Wm90ROWXXn3jJ6I8kZ3HKq4xDXKOfxslIVDBByrXYU5fdZklX9IeJX68=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4131
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-14_07:2022-04-14,2022-04-14 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 adultscore=0 mlxscore=0
 bulkscore=0 phishscore=0 spamscore=0 mlxlogscore=999 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2204140116
X-Proofpoint-GUID: 3fPDlLtyD_v0HOW2UvKMctrSiANHwTrT
X-Proofpoint-ORIG-GUID: 3fPDlLtyD_v0HOW2UvKMctrSiANHwTrT
X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable 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

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?

> And define a new macro to replace the magic number?
Yes, I will do this.

> 
> #define KEXEC_CRASH_HP_INVALID_CPU   -1U
>> +		break;
>> +
>> +	case MEM_OFFLINE:
>> +		crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_MEMORY, -1U);
>> +		break;
>> +	}
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> +	.notifier_call = crash_memhp_notifier,
>> +	.priority = 0
>> +};
>> +#endif
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU)
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> +	crash_hotplug_handler(KEXEC_CRASH_HP_ADD_CPU, cpu);
>   Should making the cpu as -1U?
Well, the cpu would be correct/valid cpu. Is there harm in reporting it?

>   	crash_hotplug_handler(KEXEC_CRASH_HP_ADD_CPU, -1U);
>> +	return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> +	crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> +	int result = 0;
>> +
>> +#if defined(CONFIG_MEMORY_HOTPLUG)
>> +	register_memory_notifier(&crash_memhp_nb);
>> +#endif
>> +
>> +#if defined(CONFIG_HOTPLUG_CPU)
>> +	result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
>> +				"crash/cpuhp",
>> +				crash_cpuhp_online, crash_cpuhp_offline);
>> +#endif
>> +
>> +	return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif /* CONFIG_CRASH_HOTPLUG */
>> -- 
>> 2.27.0
>>
>