Message-ID: <12fa9836-4ddd-2961-7d32-4754506c84ae@oracle.com>
Date:   Mon, 20 Mar 2023 10:06:29 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH v20 2/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,
        sourabhjain@linux.ibm.com, konrad.wilk@oracle.com,
        boris.ostrovsky@oracle.com
References: <20230317212128.21424-1-eric.devolder@oracle.com>
 <20230317212128.21424-3-eric.devolder@oracle.com>
 <ZBgVjjZdiEA5YxQX@MiWiFi-R3L-srv>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <ZBgVjjZdiEA5YxQX@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?WTFGSEhLc2NHc1hHYm1sNE1hRFZ4djRSdWpxM09EOWQ4ZmlCekp6eFJFdGtw?=
 =?utf-8?B?N0g5Z3Q5R2VyOVNBK2pTb01qeE9welUwcHN5a25XTVVUZU9sWkg2c3l1S3Na?=
 =?utf-8?B?T2tzQktLRk1NREhHalVCUnZEVXF2S3c2cms3NXpYckZDUTFuYmRIS29YTW9s?=
 =?utf-8?B?RU5EQVBuN0xFMlNueEFUL3Z5WklxcmlTcTIxRmQ1K3lWNWhsOFRtZTJBUm00?=
 =?utf-8?B?RW51OUM0RGFmWGtzZnFyWW8zWWNmZFllbmtUM2NDQ1VUYkwxTnZSc1l4UHJi?=
 =?utf-8?B?OVpqK3NpWkxwa0JBSHM2a3VQODZuem5QcGVBYWVUbXRBanNuK1BBUDlyK1Ry?=
 =?utf-8?B?aUd6TGx0UERQQ2s5TlM0UUE4WWhCSk5qUUttUGtvTGxtcCtOdG12NFcrL1hW?=
 =?utf-8?B?all6K0pIeHNMTG9ZQ0RqYWhZdVJnSjQvQUlFOC9zcjB5Zmg3SDJkWkR5Y3Zo?=
 =?utf-8?B?Y1MzNWhMT0ZqVE0yU1JtTFA4U1pkeGI0cmc0cE1XcmFTbTZZMnVreW5IaE1C?=
 =?utf-8?B?VjBCaGI5QmlHdTdDOWFjR1l4RFBNckZmQ3c3TVB3ZC96bDA5VzlFQ2N2R0ZW?=
 =?utf-8?B?MkJVR1Mzd2ptL0xacFR4eWZZTEU1Y3ZiNzFkbW9jUFZ2ZnJ6UjBpdVQrYUFR?=
 =?utf-8?B?WllaZytGdXRWS1pvQTFTckQrTEhwNllZSzdUYi9QMkFld3NXK0VKdnYrKzZl?=
 =?utf-8?B?cmlmaGZGV3RRRmRyb29xWTFkblp3SHhsZHowcWsxV3BSbHNWYmRSbjBCK0pK?=
 =?utf-8?B?emlXaDlONUtzRTlNbElwQUMrQmJzUFpEWCsvRDJXc3lPUm1RYi9ZNTFiQ3dP?=
 =?utf-8?B?RWppMkNtb1ZIOUtiZE9zdnlVN0l0eHNyNWxQZUJ5SXJOOFJnVVFxNkZyTHNk?=
 =?utf-8?B?VlkzNWpYL1VxbmxGYk9rUW1KSGpYdkIwU2R3cFJ0M0Z0K2VkL05vS2hOOVBC?=
 =?utf-8?B?WFgybys4Y3VibkIxOHV1VVUwandVSFY2QzlHc0g5eTRMb25TUEQwYzM1TUU1?=
 =?utf-8?B?N1hZcDYvR1JZUm5LR0llbEZ4V1RNQlBYWittdWNxQzgvZFFPbDU5SHhMSDFu?=
 =?utf-8?B?ODIzdHFYWFppNWhmNVlaQkhOb3N2dWNZOWJTQk9IZE9xS05VNExaelVJeGlz?=
 =?utf-8?B?dStBajgyaUR5aDdQSU83OHBQaFFuNmRaVVJJNU5TWnFUODIrTjN5cTcvang4?=
 =?utf-8?B?a2wwUzFTVTJLQ1lvcGFVamtHRjJIYnUrOXZtcFoxSlNvQ2RlTk9pbXZiTTZO?=
 =?utf-8?B?YUJpY1NmblZadnpycXRzY0U1NHI4MTUrQ0JSd2dJRGV6LzFGeHl5MkE4Y21E?=
 =?utf-8?B?Tks4bHoyM25YaTE0SDV4M0ZiSERPNktSYWliMHRkWnh3dFEvQWV0Q3pLWkRO?=
 =?utf-8?B?amJDd0RVaFdaZ1M4NWFWYlA2UExoY0lwaGlOcGVuZnkyaG1oR2cySVk2bko1?=
 =?utf-8?B?c0NEeGxxcGt5YlF6bVdqVGMyWkI0aVViZld5RStSN1JSd05pZ1hPRjJONE5B?=
 =?utf-8?B?RUNqTWl3ak8zTlhEK0dLcG9uSmNicy9kUmZVWmhEWE9EcnI3eEt6U1NOZmRN?=
 =?utf-8?B?N2ljUDNsSnpLeUFDMFM4UlIrMU1MMW03VVBVLzlHOVVJUkhJMDVDd0cvajdt?=
 =?utf-8?B?Z3VlbDY3M1prSmZNSUwyUktobXRGdHdLZ2NvZ1pobTI1clpWOUFZODFqVnoy?=
 =?utf-8?B?MkJtbytRTVY0eXY0L2VsQ3laS0Q3aTJnczEvbkI0bTJ3UDF1UlozMjUzeWg0?=
 =?utf-8?B?K1NTSXMybkxMYUtmMzVyNVprTjZpTDB5cFR3UnBDUXEvWW40azcyOWQ2SXZF?=
 =?utf-8?B?Z3g0T0U4RlNib1Fyd2dwZW5Dc2wyV2VhZWhXaHpQZDBSWjdrUHVXMHZwbzNN?=
 =?utf-8?B?RVoxa2ZqQUl5Y2Q5SHp4NlRrTGhWaVB1S2NHa3cwbk9Pc1J3VHIyRDZrWW9X?=
 =?utf-8?B?aWZXcGgvV2ZBb28waFdSSmhMV2JPYXVPd2dHRjMvdFJZRHhNRWRMSEpEelYw?=
 =?utf-8?B?aENNNmovZ3lSbStaTklzMy9yaTF3RjhIRDF3WmVSdm5rY3cvU1A2Y0VEZGFa?=
 =?utf-8?B?eEJWcUlQdWRObHNLcVlibGRhaTcyK3lDRm5Zamo3dk83eUtmNmhKWC8rak1k?=
 =?utf-8?B?QzAxL2hWeFc5QWdha1FmM2lKckxRNFdIVFpLaFhaRjZCT1pjQ0l6Q2o1VWNU?=
 =?utf-8?Q?QVy5A7B9a7PMpfWcVmLSWCg=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?dXZJK3l3U0VzYW1jdUhyWlpicng0cFJmZnQ2amlab1d3NG5nSGJnbXpLL1lK?=
 =?utf-8?B?eTByNDNCY3JKTUV3SVFDbmFIU2JiZkltbk5uWjYxMVF4MWNCNVR0Q0l4Sm52?=
 =?utf-8?B?QjlZbVM4UTB4cm1URXp0alEwWDRtSllGeW5ZbHI5dDgwWWVyeGtkdE5LMG1t?=
 =?utf-8?B?MVdyNGNYOGE2UUJ6ZUlmVm1SNVM3VGNQd0JyMWRmTzdySEJOSnpST1ExMDlG?=
 =?utf-8?B?d1llNzF5Y3RDMmpqTE1mSmc5cjZTb3F2ZjBVanQyaW5idmNoaTM2MlhmbVR5?=
 =?utf-8?B?d3BkT1dvOU9YeHExeDBYYzFNb2FSZVgrL0pZaGxETGo4LzJHWnVjWEpNdzZU?=
 =?utf-8?B?QTE5SFhROUJia2RBOHNoVmJZOUNVMk1TeEV2elFUSDg5bWRrbDBjSng0dUR4?=
 =?utf-8?B?WWtJSkVDOUxSVnRHbVR4NmxYbkM3VFRsNURGeXN6dU93eXRWZEFQZTV3VFpw?=
 =?utf-8?B?WlJzckE3a2taTmxHcE5qZC9IWnpLbTFmdkxvdjB1R3llekJwbXppYi9BMndO?=
 =?utf-8?B?cGZJdmI2QVdvRlgyc2xRY216THZWNWRscDFHMUF6WWFGVEdzYWdCUWRoZUJh?=
 =?utf-8?B?MWY5WTFVME5MZVduYlJ6ZFlPbldFc3U5QUNXUmlJZUpoNkxnWFlnd2JDaXVa?=
 =?utf-8?B?cTZ4Q0RjaXk0RUo2M3FEc0VJbjhSUVNTZFJ1ZitUSjFNVll3bVZuZWREZUNa?=
 =?utf-8?B?YVJXcXpZOEdkd29Sd24wbzI3VTA0RGdCQVN3U3QzMU5YeXF4QmU0NEZnYjBs?=
 =?utf-8?B?WUFJZTJCWFBXbkVTT3REaEUrbjVEaEZEWXlGc0ZKVit1TlZrVHBvUWgxVmZW?=
 =?utf-8?B?TXowYVRBQUNWcVlDaUdMWXRNelJuenBiZEpFWGlXVFkzMXcxV294aUhqSUNy?=
 =?utf-8?B?YjRuUEc5TEhnd3c5TnFZRUdZbHZ2Z2lRMFIxS1FoQ1FtbmdIME42S0N1Q3F5?=
 =?utf-8?B?eHBrOWw0QkUvbEJicEZBOFd5ZXdxWVU4b0hmelBxbEVsSWg0VE41cURSRllB?=
 =?utf-8?B?RXZpSkcyZDdSUG1MRjB5T1BrOXdZUVBSbzBsLzFtcWV3RyttVkRhREk4VWov?=
 =?utf-8?B?Qm9BUW1CNjRkUjExQXNZQXpoRVRPSVI0YU84K0NyYS96SHpCNnZacG1obGw5?=
 =?utf-8?B?d1Z2Q3ZsWUtINDZBYTBDd25odE83eVVIYm1ZWTZTdDhSVGRNbWF4MlZzOGxD?=
 =?utf-8?B?WjkydC9weDJkV3daZWxIa0NBclNhSjhqQzFIbTlTTklKMytQR0pZSkVHcnhj?=
 =?utf-8?B?SjZSMVpXTmRnZTNObnJXUkJpMi9jSTN5Mi9RM1RjZ1BiQVZuZmUzTnpLckcz?=
 =?utf-8?B?TzhIamZKTk00dnRJNVM0S2ZTN050M2ZlcXU1Sjd4TkFiT3NqSTVRanFxWjYw?=
 =?utf-8?B?Q3B6eTVRVlRPd3lXTXpuSFdkcEZBVi9PYU8weXF5ZFJFbXA0ejJSeG9jS0E5?=
 =?utf-8?B?MXV6M3prejk1cW1VOHl4WUVldTBNVDJvQ1FoZGxjOWtJQ3VEL2tDZjd0Q2kz?=
 =?utf-8?B?azZ4bnFmV3FjM1NQdFl6NXVlSVVIbWpXL0FJeUpnVkRrZHJ2YW53MFZ6QVYw?=
 =?utf-8?Q?IN2iyXG2NgFPobGzgyKTvNYfk=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 28912a6e-2ffa-4aa6-0586-08db2954b002
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2023 15:06:31.3094
 (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: XhIUK/GoNFFMLsDs8U4OxqxUqGq1cPRTMspec6NpK9+U+Cg065Oc0IBgYO3DK0DRaznCgjiJST2Fpjd/LhOjzsWX7izKy+ji8betEWg1Ivo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB7107
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-03-20_10,2023-03-20_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0 spamscore=0
 suspectscore=0 mlxlogscore=999 mlxscore=0 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303150002
 definitions=main-2303200128
X-Proofpoint-GUID: BEnBDTAIHqaTn-6xssLDrUJb8R8dm5US
X-Proofpoint-ORIG-GUID: BEnBDTAIHqaTn-6xssLDrUJb8R8dm5US
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/20/23 03:13, Baoquan He wrote:
> On 03/17/23 at 05:21pm, Eric DeVolder wrote:
> ......
>> @@ -697,3 +700,137 @@ static int __init crash_save_vmcoreinfo_init(void)
>>   }
>>   
>>   subsys_initcall(crash_save_vmcoreinfo_init);
>> +
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> +#undef pr_fmt
>> +#define pr_fmt(fmt) "crash hp: " fmt
>> +/*
>> + * To accurately reflect hot un/plug changes of cpu and memory resources
>> + * (including onling and offlining of those resources), the elfcorehdr
>> + * (which is passed to the crash kernel via the elfcorehdr= parameter)
>> + * must be updated with the new list of CPUs and memories.
>> + *
>> + * In order to make changes to elfcorehdr, two conditions are needed:
>> + * First, the segment containing the elfcorehdr must be large enough
>> + * to permit a growing number of resources; the elfcorehdr memory size
>> + * is based on NR_CPUS_DEFAULT and CRASH_MAX_MEMORY_RANGES.
>> + * Second, purgatory must explicitly exclude the elfcorehdr from the
>> + * list of segments it checks (since the elfcorehdr changes and thus
>> + * would require an update to purgatory itself to update the digest).
>> + */
>> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> +	/* Obtain lock while changing crash information */
>> +	if (!kexec_trylock())
>> +		return;
>> +
>> +	/* Check kdump is loaded */
>> +	if (kexec_crash_image) {
> 
> Here, what I mean is:
> 
> 	/* Obtain lock while changing crash information */
> 	if (!kexec_trylock())
> 		return;
> 
> 	/*If kdump is not loaded*/
> 	if (!kexec_crash_image)
> 		goto out;	
> 
> Then we reduce one tab of indentation for the following code block, e.g
> the for loop block will have smaller pressure on breaking the 80 chars
> limitation.
> 

Ah, yes, ok. I'll make that change. Do you prefer I post that soon, or give this v20 some more time?
eric

>> +		struct kimage *image = kexec_crash_image;
>> +
>> +		if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
>> +			hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
>> +			pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
>> +		else
>> +			pr_debug("hp_action %u\n", hp_action);
>> +
>> +		/*
>> +		 * When the struct kimage is allocated, the elfcorehdr_index
>> +		 * is set to -1. Find the segment containing the elfcorehdr,
>> +		 * if not already found. This works for both the kexec_load
>> +		 * and kexec_file_load paths.
>> +		 */
>> +		if (image->elfcorehdr_index < 0) {
>> +			unsigned long mem;
>> +			unsigned char *ptr;
>> +			unsigned int n;
>> +
>> +			for (n = 0; n < image->nr_segments; n++) {
>> +				mem = image->segment[n].mem;
>> +				ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
>> +				if (ptr) {
>> +					/* The segment containing elfcorehdr */
>> +					if (memcmp(ptr, ELFMAG, SELFMAG) == 0)
>> +						image->elfcorehdr_index = (int)n;
>> +					kunmap_local(ptr);
>> +				}
>> +			}
>> +		}
>> +
>> +		if (image->elfcorehdr_index < 0) {
>> +			pr_err("unable to locate elfcorehdr segment");
>> +			goto out;
>> +		}
>> +
>> +		/* Needed in order for the segments to be updated */
>> +		arch_kexec_unprotect_crashkres();
>> +
>> +		/* Differentiate between normal load and hotplug update */
>> +		image->hp_action = hp_action;
>> +
>> +		/* Now invoke arch-specific update handler */
>> +		arch_crash_handle_hotplug_event(image);
>> +
>> +		/* No longer handling a hotplug event */
>> +		image->hp_action = KEXEC_CRASH_HP_NONE;
>> +		image->elfcorehdr_updated = true;
>> +
>> +		/* Change back to read-only */
>> +		arch_kexec_protect_crashkres();
>> +	}
>> +
>> +out:
>> +	/* Release lock now that update complete */
>> +	kexec_unlock();
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> +	switch (val) {
>> +	case MEM_ONLINE:
>> +		crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY,
>> +			KEXEC_CRASH_HP_INVALID_CPU);
>> +		break;
>> +
>> +	case MEM_OFFLINE:
>> +		crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY,
>> +			KEXEC_CRASH_HP_INVALID_CPU);
>> +		break;
>> +	}
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> +	.notifier_call = crash_memhp_notifier,
>> +	.priority = 0
>> +};
>> +
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> +	crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> +	crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> +	return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> +	int result = 0;
>> +
>> +	if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> +		register_memory_notifier(&crash_memhp_nb);
>> +
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
>> +		result = cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
>> +			"crash/cpuhp", crash_cpuhp_online, crash_cpuhp_offline);
>> +	}
>> +
>> +	return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index 3d578c6fefee..8296d019737c 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -277,6 +277,12 @@ struct kimage *do_kimage_alloc_init(void)
>>   	/* Initialize the list of unusable pages */
>>   	INIT_LIST_HEAD(&image->unusable_pages);
>>   
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> +	image->hp_action = KEXEC_CRASH_HP_NONE;
>> +	image->elfcorehdr_index = -1;
>> +	image->elfcorehdr_updated = false;
>> +#endif
>> +
>>   	return image;
>>   }
>>   
>> -- 
>> 2.31.1
>>
>