Message-ID: <cab057d8-98d8-a72a-8b61-8a01a0f088e7@oracle.com>
Date:   Thu, 16 Mar 2023 09:44:57 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH v19 2/7] 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: <20230306162228.8277-1-eric.devolder@oracle.com>
 <20230306162228.8277-3-eric.devolder@oracle.com>
 <ZBLrTl3UHYa1FV2S@MiWiFi-R3L-srv>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <ZBLrTl3UHYa1FV2S@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?c3k1TUtNR3B5QTRxZTlnUjEyNWg4RlBYWFZDK3ZETmpOR0JJem4rTjBBVWdT?=
 =?utf-8?B?dUNibEd5QXpReHhrT0hZOE1iWFJmYit4NkltL1A3SnBORDREUGNyd0VDRnNF?=
 =?utf-8?B?Vk4ydlNIeE8wMkZrZUtoZnJNUlFWOHl6cmxuZEZoSHIxUUdQODlLc0t6VmhI?=
 =?utf-8?B?MjlST0RoUndQbVNOT0gyOWF1dzVRMWhPeGF6a1hReGF2eEJUOTBoSWhrYlEx?=
 =?utf-8?B?ZEtiTEdGaTQ0YjIrUUVxUmJXVkdjWjBBMHhXcUxUL3d4MTRHZHdxeVVkK25p?=
 =?utf-8?B?V0dOQ29VUDVwc0puNTNVK0tqRVBYV2JvUTNHdXRjTUx6SlluMmhXc05obmc1?=
 =?utf-8?B?eStiTUx3QjNuekpUYlNRUWYxRFl2K2xSMm85OENWNnNKR2txSlVzckdJNHRU?=
 =?utf-8?B?UjV6SDBld3d6V1I3TUcrNGcxWTZ4ZndMc1hackd2QnZoU1FpWHY2NWNWd1hN?=
 =?utf-8?B?R2NoOHcyVkNBVTJpK1FSeUkzakFYZGdFaDgyYzloOUhNZ016VXBxekJOWkRj?=
 =?utf-8?B?K20rUG52MGgweWlhSWlhbzR6VDVyUHYyOGJSUTU5TlpXMkpvQmZ5U1BkRzJZ?=
 =?utf-8?B?S082ZDdxYjBId1IyZ0REMzdwR1I4R1F2Z3pvZGpJek1wMGV2TTh2blE5MDdT?=
 =?utf-8?B?VHJBbENzaUJoZmNvNHpvc0UybktQR1J2TkxBRUxheHlneS9vVVhIcFVuK2ZQ?=
 =?utf-8?B?SHphM000d2F0Qi9oZXRsUEZobkhGMTMrL0xweEx4eTBaL0cyOCtOaTVxNTlq?=
 =?utf-8?B?OEFvaXlZNTZjazcwekQraEVZN2pQcnJCNmgzakkyYVNUM2FlQUVQMG9NakVS?=
 =?utf-8?B?NGRpTkxQQU0xS0hUTXlZYjc2REZHN0tXY29jMVNnMis5RFFqa1d1UkJ1YURr?=
 =?utf-8?B?b2hYMlRsOVZkVk1ZcGhpT01nZ01pK3I3d2FrUVRnNG4zbFJVVjhhdnZiYXZM?=
 =?utf-8?B?VkduQVFEMWpXYXp3a1VqMzhOMmdGZlI1ZG5GNkRVZ1ZTRVpvc3Vkcmh5d1hV?=
 =?utf-8?B?c1A1K0dodDNQaUM0VGZqRlZDZ200bFNPNUxzblNZVWZNVFVVOGQ1TlMrZnJ1?=
 =?utf-8?B?L0lZTXNpZFBPREk0Q1VsSnNzbGRyOVRtYkgzR25mMWRQNkhZZVQyQzFiN1N5?=
 =?utf-8?B?UXlnTHhrM3Yza25OU1R6dk9TM0pKL3NEWU5PdzdlTUw1Z3lFRlRnRUxUeGFB?=
 =?utf-8?B?dGhWSkE5cjUxYjlkRjF1S1lIYk9wMjREWTBQU1FXRDNsMDNRLy9GTVlSU0ds?=
 =?utf-8?B?bHpIL3pCb3dQWXVENEtHSUdlZ2RzSTRnUHZMWXhFR2xhaFE3c0pONVJIREdJ?=
 =?utf-8?B?SXZvZ3YrbXNQdFB6S2FGT25GcEdYSWVpNFNXSWIveVhwUmlQVHV5Y2kxbThi?=
 =?utf-8?B?RjFiTEpncnJRZ0hiUnV0NU9LTEpvT2RUSUljSXFyQzRXVlU4dTlDUkpJQVF0?=
 =?utf-8?B?RU5mNkNyZHhDeUJEb2RjWGd2REgzTFlkNzRjbGNOZUNxaFJtWGc2RnVFMUVJ?=
 =?utf-8?B?S09WSUhaS3RRc0duWXJLZzRpSkxMMDJ2UE5YTVdqRzVLRnpIU0pSWlJNVVJt?=
 =?utf-8?B?VThicXRJejlqMUNZbyt4SmRtWFRpdXJ5djJhYnJPM2RHQ0MvMmV1T2dTQ0Vi?=
 =?utf-8?B?LzUyRW5wcWd4V0lTME8vZEVWVCs3OUdDblRBcWZFOXZtNFBzMFRLREJMMW16?=
 =?utf-8?B?c0hVajlzbG4xZWNXQ3o1NGZXT2Vtd1ZYaWxRTFZURnBlUzdzV1d5V0JVb1kz?=
 =?utf-8?B?RXBDK0xyMG1GOHgwVGgvY1YzdmVZUzNMQzlKWHB4SHdyRzlHUldsaWJwVTRh?=
 =?utf-8?B?U2VVT2xwYi9yb1JLclluai8xdDYwQ2psWW9hU0l5cUFwZXd4VDlvRGdrR3Br?=
 =?utf-8?B?Wkc2YjVPNEF0cFU5d2lPbVMyVlNWYUp5SnY2c3BKTWx6YzNqRDAvMVVoYXY3?=
 =?utf-8?B?QjQ0ekFVMUxPWDBmdFdSN1lsMnM5VmFOclYvTnc0UzA5eGpZM1haazNmOVpK?=
 =?utf-8?B?ajN6U0tWRm9tbkhqL3pBbGhWMzYzaGFOakV6SXdQMUc3QUFia3psR04wd2M3?=
 =?utf-8?B?ZzFCaWpTMWs0Nzg2STh2c0VTRzg2VFNZZ2FoaytVK3lBQXoxNnlhcWYwYXl1?=
 =?utf-8?B?bnBydFAwTnlsL1FhYU12YW1ibGgyMnU1RHVXbi9tTnREL3F1M1ZNQmVId0lC?=
 =?utf-8?Q?VIevRWvbFht9fR+JsnpUawM=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?cVphK21TaVV6UktyL0k0ZVpzeE1HTHJEeFFYbURHN2x5N0FTcTMxWm9seUJt?=
 =?utf-8?B?ZTlvRldGMURCTjlFdlFuKzZTcEU3aEFuSjNybUdnUXFpNGpNdVNTY2w2ZVVH?=
 =?utf-8?B?RGVBMmhUNVE2NWxDZEtmTjMzZGxxWWJKNEgrcE92TnI2WnZYT21MZmMzWURt?=
 =?utf-8?B?RXlHU2xvejYvV0JObVl1a1I3N3pSczdDcWVVZFNyd2lINGFXUDVEanNrSVAz?=
 =?utf-8?B?MHY5T1NLUEZEQ1UvRVFtdGZtN2NIb0YvaUg1OHdNTGRkdGNYbjZJTk95TkZq?=
 =?utf-8?B?K2RCOURFYmpNUTU5S29kbFpYMk5ZZ1N2eVFUZnlydnFydmVaaGxPcjNyMVR2?=
 =?utf-8?B?ZlVpQmdqRVM0bU8rRXgzTjVEVzgySUhTU1JmU3EyS0lLeUt5Wnk0V3pvZjZI?=
 =?utf-8?B?b0xFd01vdGltT2dLazRCU0M0dk5NSWVHMWphejhGV3hLQkxnYjRwWG81K2M2?=
 =?utf-8?B?REZUVXJQaHorYlJXYWJKSmxLTjFzdFF0K1RMNDVXcnNoVWdlQnQ0Mmc5Nmxw?=
 =?utf-8?B?Nk5WYjFaU0lPNWc1cjBqZFAzSS9VUDBYb2ZtWWRYd3lDc1JITXh6UTNHb01G?=
 =?utf-8?B?Qi9jTm5jM0dZV1krVW1YcHBXRjZwdjltZEd5U0RZOEZIbjJtWEVxSjBSNjdW?=
 =?utf-8?B?dHdRajlJc0pMam9QTk44RlhQZDdxcVd5K2tRazdPKzBqUkd1dW50SC9UMG5E?=
 =?utf-8?B?UEVsMlBGYlNzTmdxczVCdHBnWUxnY25EZGp2ZHdhK3lmWHNmQlpROWs4T2pL?=
 =?utf-8?B?dGhqYkpaQTF6MTZsY3R2a054SEFBdHRTbjZaN1MzNUZrendnR0pNaFlmWHE1?=
 =?utf-8?B?WFE3aUZtc0ZGaUZvRHpHcVlMQ1dleSt2QmpDTDNGL3poQVFsdXh6L3BqbkVM?=
 =?utf-8?B?Tkgvb211cXhZcEZzY2FISVg2RkdYdTNDZ1NVTXlUcWs0QXlNck9NRGh6YXNj?=
 =?utf-8?B?NkdhYzZMbnhRSkVveU80ZkhaVThsanlFL3BSVkRhQm9oRWtHeEc0b2VoN2t5?=
 =?utf-8?B?eENOb1Z4UEw2dUtwTldlUHNqNWZOZk5NdTdpM1Y5TXIvTXhVUWlSdm14R2hx?=
 =?utf-8?B?Z3Byc3VtQk1vZ1ovK3VEa2Z4NjRxZFpxR3FtTkJDOXRnTWZxRkJwVStNcWVY?=
 =?utf-8?B?dFcwS3RpRXFON0t5S0ZpUDgvaGpoWDVZVmlnMWQrTVZOZkVsblRkUjc2NGpW?=
 =?utf-8?B?QTU3Sm1Za3JUQW5JenVwTC9ZbU5ZWXg1cmRrWkNsdWdaYnBLeVJYcU12L0hP?=
 =?utf-8?B?TkJMb0ZWMkZXYTRRUDBLUDdFeDVtd29HeXd1eUpWeFV2NXZjbHpzMG5aUHZm?=
 =?utf-8?B?cTV4Ykk2T3B4dXZTVFlVVUNadUpyUDQ1eWR2TUh6TUpDK3BsOHBCbUZqQXd6?=
 =?utf-8?B?UlBmUTdOL3hlSVJ2dUFkdGxGSGQwMW42akZCaUZiZ3FJM1doSXNlWHBDdThK?=
 =?utf-8?B?Ym4xaVVZYUJiQXFJNGlvWjVOZit3M21vb1Y1OS93MVRvT0VUck5JamJGc0Fy?=
 =?utf-8?B?TE5zcys0dlJ1SjRaVmo0NWZhL2lxWFFQRUdobWdOckl3RlVWcTJLMUpKVzhF?=
 =?utf-8?Q?9Z5r5S9m8Tc0nzluniCJeLFP0=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b375dd7c-5522-458d-244a-08db262d0bf2
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Mar 2023 14:45:12.3858
 (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: S9hj/1FESG9uvCA2+X61iAQFQ0MphSfDNDQPq6siyUsXFwEild9pPwh8ZZdzV1RfClJcptwDq84n3QZTYX6SLUVRTEcKPnBrYikyef7Bkhw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB4316
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-16_10,2023-03-16_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 suspectscore=0 mlxscore=0
 bulkscore=0 mlxlogscore=999 malwarescore=0 spamscore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303150002
 definitions=main-2303160119
X-Proofpoint-GUID: uHEyE3WgkhVI-xBu5eIJKm17xaoaV78j
X-Proofpoint-ORIG-GUID: uHEyE3WgkhVI-xBu5eIJKm17xaoaV78j
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/16/23 05:11, Baoquan He wrote:
> On 03/06/23 at 11:22am, Eric DeVolder wrote:
> ......
>> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> +	/* Obtain lock while changing crash information */
>> +	if (kexec_trylock()) {
>> +
>> +		/* Check kdump is loaded */
>> +		if (kexec_crash_image) {
>> +			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;
> 
> It's good to initialize the image->hp_action here, however where do
> you check it? Do you plan to add some check somewhere?

Hi Baoquan,
The hp_action member is initialized to 0 in do_image_alloc_init(). I've
mapped KEXEC_CRASH_HP_NONE onto 0 on purpose.

But the use of image->hp_action = KEXEC_CRASH_HP_NONE is to actually
delineate that a hotplug event handling has completed. You can see
imae->hp_action set to hp_action to capture what the triggering event
was, as passed into this function.

I will go ahead and set image->hp_action = KEXEC_CRASH_HP_NONE; explicitly
in do_kimage_alloc_init(), as that is done for the other crash hotplug members.

Thanks!
eric


> 
>> +
>> +			/* Change back to read-only */
>> +			arch_kexec_protect_crashkres();
>> +		}
>> +
>> +out:
>> +		/* Release lock now that update complete */
>> +		kexec_unlock();
>> +	}
>> +}
> ......
>