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: <0aee5cd1-eb8c-f778-c6e5-7f5ca3624d15@oracle.com>
Date:   Tue, 26 Apr 2022 15:08:26 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH v7 2/8] x86/crash: Introduce new options to support cpu
 and memory hotplug
Content-Language: en-US
To:     Borislav Petkov <bp@alien8.de>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        kexec@lists.infradead.org, ebiederm@xmission.com,
        dyoung@redhat.com, bhe@redhat.com, vgoyal@redhat.com,
        tglx@linutronix.de, mingo@redhat.com, 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-3-eric.devolder@oracle.com> <Ylgot+LUDQl+G/5N@zn.tnic>
 <f3ea7ee0-1cb8-6928-d0af-4c2fc63a42a4@oracle.com> <Yl6PueNCZZeobMWr@zn.tnic>
 <cc7a6b5c-954c-54fe-be5f-324dc984ac51@oracle.com> <Ymb1tBJsch/TB2ph@zn.tnic>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <Ymb1tBJsch/TB2ph@zn.tnic>
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?ZXZSUUlZZWlWSXpibTNRak10WGIzWWJtVDFzQjdoN2JLTWV6R1ZTZWNlemNI?=
 =?utf-8?B?Szk5eFluTmdqanRwbUtqa25PdGV1T1VGeXFPRHQrbXNFTDJrL3V4alRENUZj?=
 =?utf-8?B?Z3VKTzZ1TkZSZm4yUU05SjRpbmN6WkNDU3ZFYkNudW1sWlVobHNDNmJPZjcr?=
 =?utf-8?B?Y3hDY20rbWRDdXcxUG5QWW5NMm44S1Rjc2JmWEF6eG1sTzZUSWxSZVpIdzZT?=
 =?utf-8?B?MG9zYUw3TXg1N1p5UURtYm5yeURsOVUreFdWbkE2WG10NnhLaU9KSUVpOFRl?=
 =?utf-8?B?MmtYRElyc0NTOGtmNWM0QjI1Mmx1dmp1TmVENThzRE9PQkcycmtmY2FGRUhj?=
 =?utf-8?B?SmlPNWRSZEk0dk5mTWh3Ujd1dGsxZUJaV2ErQWgvaXJMa254c2Y1ZUVremNm?=
 =?utf-8?B?OVBRV3Ewa0NBVXZKY21NWVp3VUJJRzlieU8zRzEzcmw5Ri94T0FUaFpHTTJP?=
 =?utf-8?B?NW9lRVJpc3ZId2hnNmpOQXZZRUFkU0NGRnBkSnZqQjBNdVhLS2ZaaU95TDJl?=
 =?utf-8?B?K3ptU2swZS9oT0NTYWE5cTgrVGJzdExUdFNUOGRoMmFaTWl4NzNKZG1uN04w?=
 =?utf-8?B?aitma1JVOXlnWU1SWkJqb1NaSytmbGQ0elhKTFoveXk4S3VyS09sYStnT1Fu?=
 =?utf-8?B?UlZ4RXI1a2RCd0drdTVRbHdxMzlrMk9EVFljOWdjdncrdkVPS09zNWxFNWVQ?=
 =?utf-8?B?eHE2VkpaN2pMTVIvM091blRPUjhkRkZLL2xZVzNMRllXK2JHWVRBS1p5V0Ew?=
 =?utf-8?B?KzE4L2t1aDVnbCtpSFBQUnVOL1VKYjlCY3FScmNNZzZoZGxtWkNBRDhWb2ty?=
 =?utf-8?B?TlNqd3RMbVJ2dEYrR1BJcWVWbnlPdVVVMzJzc1lySjVScFZzQmhoRUY4M0p0?=
 =?utf-8?B?ZU5uVm1nbmpGZW1KTVd3bms2eWd0Z1hCTllmVjdCeUVaVmZrelhNSkZlU3V6?=
 =?utf-8?B?T2krRmY2a3QvTjlSdkhtTVNBQW1MTWlOSWs1L3FnZjVObkZsZzZqbzllM3Nn?=
 =?utf-8?B?aXlzUmJORVA0YzIzV0lzQndVVGE4SE8yWEwxcXhkRWZud1ZtUkk4VGZjOWxO?=
 =?utf-8?B?U0pGS1RJelU4YUJMNFpabEpuUXRnbUlzY3c4QVRTcjVHZFp0RHRPd3Z5Zmdk?=
 =?utf-8?B?UFRPK2xNM0pxUmhpd3V1R3VTbktMY2VjdjJySG5DRHFWNlpxRTBCZ3VGa1Jm?=
 =?utf-8?B?ZnlxVFl2N09ONk9WOWxEdXpsWUZxZVZFZGlkL2o4VFdDc1hxd2RYMnJlRWZS?=
 =?utf-8?B?ZnVzOW9tRmw0TThuWUtQcFg0Z2FZMWJDdnRER2Z3ZkxFeFV6K29qSXlzdFJV?=
 =?utf-8?B?dFdydmtJbWtXOS9iZTlGVmtSSUg2QzlYd0Eyc0hpUFhjMFQxZWFRTGp5TDFD?=
 =?utf-8?B?TmtUdFJiOHNEUEdQcGRTTnF5eGJzU1A5eFpmQzhZYVZGSmJGS256bUFuck4z?=
 =?utf-8?B?ZVFiT0k0c0xXWW1ET25sSmlUY1RRTFA0VnZoRTViQWMvUS9zM0toQmFpTklY?=
 =?utf-8?B?RFlRN25rdmtoREFkTlBaa0VsUDM4TXhxTTNPK3lZNmx3YW5iUkVOMWRibC83?=
 =?utf-8?B?Y0dZTGwveklwbzF3TlZXMis1Qi9OaGc5MWQrM3ZhcGJyKzFnbUVVdHZSTnNC?=
 =?utf-8?B?QXZmL0xKdEsvU2t2aUFnbm9VOVY0WXNWMmt2aGNydGJFcmdoLys1QllpTEFy?=
 =?utf-8?B?d0t4RVhvdWx2V1Y5b1k0Qm5sQlpJK0FXMVE1dE4xQ0d3Y0xlS1M4Nnp0Z3pX?=
 =?utf-8?B?R0ZrNFVsdi9ieE9HOHpoUDIxVUE1YTFkQkxXUmlxaTBpKzR6TWV4dFBaNmg2?=
 =?utf-8?B?dU9pWDJPdjROVlUra2NrM2NKbFFnSDd0WERtdHU2TTV2YWUwOElvbVJpOW9s?=
 =?utf-8?B?bENhckMvNjRtN01vZ2UxN2VIVWo3ZGNhZmRkaGVQR1Z1R1Ayc1R6azRhWVc4?=
 =?utf-8?B?d0xTNDUwaFRFK3ZwcFUydXNXNlRvU0pkdnlTNDZNY3d6ckMxOU1UdW51ZnI3?=
 =?utf-8?B?TE5yVEtNdllwWTJvYnFOcDUrdnhGMVVEallodHZ3NHJwTUNiTUt5QU1abXl3?=
 =?utf-8?B?WHRZZHdOQ2Y4S1FmSHJuM1pFMDlrWGs4d1oySU1QWDhxYlhRbVpabjVDLzhY?=
 =?utf-8?B?a0RMVEUrRUxWb0ZRQlQ2U3c1dFRzOU1HcEQ4WHA3azhJejRXd1huWjdpd2pn?=
 =?utf-8?B?cE92bHZqdnBKMzU1R0cyWXRiQWF6a2N4dzFPQmQvUzBiQzFlTWk3cFRvNGNR?=
 =?utf-8?B?QVByOEJxVFRESFNIU3JaOW8xYmUwRzN5YmowcFVZRTZybUJHTmJkZksrSUpj?=
 =?utf-8?B?OEpGaGpMZzdmTDc4K0pXYWJWb2dOTFdnc1VTSkxUQ0dYd2VmS2lYQTJ0VVBx?=
 =?utf-8?Q?Q8PkMj5CdhQjjKhW1muxs7+kyoEeWA1fCC/iV?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 55bd69ba-0fd9-4eea-f9e0-08da27c08ad3
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Apr 2022 20:08:34.5509
 (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: wgXtsK8molFgCIf0WnjD+zHIfITA/FzMQZ+f//nniZCB+5V3XrfmyYmg5lxFXocNUSUQOE99SlXbTBnahM8QzRor0EMTQokT3AhrnvULr1c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR10MB1436
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-26_06:2022-04-26,2022-04-26 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=999 mlxscore=0
 suspectscore=0 malwarescore=0 spamscore=0 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2204260128
X-Proofpoint-ORIG-GUID: 1EsZaEBT2W-HgEv6pNVzM0AF_S1-FBIs
X-Proofpoint-GUID: 1EsZaEBT2W-HgEv6pNVzM0AF_S1-FBIs
X-Spam-Status: No, score=-3.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
	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

Boris,
Many thanks for the time taken to illustrate for me!
I've one question below.
Eric

On 4/25/22 14:25, Borislav Petkov wrote:
> On Tue, Apr 19, 2022 at 04:58:47PM -0500, Eric DeVolder wrote:
>> So in taking this concept and looking at, in particular, patch 4/8
>> "crash: add generic infrastructure for crash hotplug support", I'm not
>> exactly sure how to apply this technique.
> 
> So I took your patch 4 and maimed into what I think it should look like,
> see below.
> 
> Now there's a single ifdef there and the registration routines are
> wrapped in IS_ENABLED() so that you register a callback only when the
> respective stuff - HOTPLUG_CPU or MEMORY_HOTPLUG - is enabled. Otherwise
> the couple of functions are unused but that's not that big of a deal.
> 
> I've also fixed up some other issues I've encountered along the way.
> 
> Holler if there are questions.
> 
> HTH.
> 
> ---
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e..b3c32e04d3f0 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -84,4 +84,12 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>   int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>   		unsigned long long *crash_size, unsigned long long *crash_base);
>   
> +#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
> +
> +struct kimage;
> +void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
> +				     unsigned int cpu);
>   #endif /* LINUX_CRASH_CORE_H */
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 02daff1f47dd..d907a1f0d3da 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -307,13 +307,11 @@ 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
>   	/* Virtual address of IMA measurement buffer for kexec syscall */
> @@ -329,15 +327,6 @@ 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 ecf746243ab2..a6c4ee1d5c86 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -497,57 +497,50 @@ 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)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
> +					    unsigned int cpu)
>   {
> -	pr_warn("crash hp: %s not implemented", __func__);
> +	WARN(1, "crash hotplug handler not implemented");
>   }
>   
> -static void crash_hotplug_handler(unsigned int hp_action,
> -	unsigned int cpu)
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>   {
> -	/* Obtain lock while changing crash information */
> -	if (!mutex_trylock(&kexec_mutex))
> +	if (!kexec_crash_image)
>   		return;
Why is it safe to examine kexec_crash_image outside the mutex? As I understand it, there is still 
the (very rare) opportunity for a kdump load/unload initiated via userland and this check to 
collide. (Similarly, I believe the mutex entry is almost always assured/likely.)

>   
> -	/* Check kdump is loaded */
> -	if (kexec_crash_image) {
> -		pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> +	mutex_lock(&kexec_mutex);
>   
> -		/* Needed in order for the segments to be updated */
> -		arch_kexec_unprotect_crashkres();
> +	pr_debug("crash hotplug: hp_action %u, cpu %u", hp_action, cpu);
>   
> -		/* Flag to differentiate between normal load and hotplug */
> -		kexec_crash_image->hotplug_event = true;
> +	/* Needed in order for the segments to be updated */
> +	arch_kexec_unprotect_crashkres();
>   
> -		/* Now invoke arch-specific update handler */
> -		arch_crash_hotplug_handler(kexec_crash_image, hp_action, cpu);
> +	/* Flag to differentiate between normal load and hotplug */
> +	kexec_crash_image->hotplug_event = true;
>   
> -		/* No longer handling a hotplug event */
> -		kexec_crash_image->hotplug_event = false;
> +	/* Now invoke arch-specific update handler */
> +	arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>   
> -		/* Change back to read-only */
> -		arch_kexec_protect_crashkres();
> -	}
> +	/* 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)
> +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);
> +		handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, -1U);
>   		break;
>   
>   	case MEM_OFFLINE:
> -		crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_MEMORY, -1U);
> +		handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, -1U);
>   		break;
>   	}
>   	return NOTIFY_OK;
> @@ -557,38 +550,33 @@ 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);
> +	handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>   	return 0;
>   }
>   
>   static int crash_cpuhp_offline(unsigned int cpu)
>   {
> -	crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> +	handle_hotplug_event(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 (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> +		register_memory_notifier(&crash_memhp_nb);
>   
> -#if defined(CONFIG_HOTPLUG_CPU)
> -	result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> -				"crash/cpuhp",
> -				crash_cpuhp_online, crash_cpuhp_offline);
> -#endif
> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> +		result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> +						   "crash/cpuhp",
> +						   crash_cpuhp_online,
> +						   crash_cpuhp_offline);
>   
>   	return result;
>   }
> -
>   subsys_initcall(crash_hotplug_init);
> -#endif /* CONFIG_CRASH_HOTPLUG */
> +#endif
>