Message-ID: <cae43a11-b270-a5d6-7464-d92651c67c4a@oracle.com>
Date:   Thu, 16 Mar 2023 10:47:00 -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
From:   Eric DeVolder <eric.devolder@oracle.com>
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>
 <cab057d8-98d8-a72a-8b61-8a01a0f088e7@oracle.com>
In-Reply-To: <cab057d8-98d8-a72a-8b61-8a01a0f088e7@oracle.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?TnpQSWtkRFB5amI4NTYwUTJvQTJPeVA2QnZBNHJ1U0dIMlpuWCs0bVVrR28y?=
 =?utf-8?B?YXArR0hJMVhGY1hIMHpoc1cwaFJYTmk5MXdCUStaamJGZm9mR0dsWm1DeURX?=
 =?utf-8?B?cU9rNE80cUlpQUxzQTQrUDVaWWFOOFA2WmhXdm1oWE92Wm9XMmcyeFk5bnFo?=
 =?utf-8?B?dVh1REpxTHczV1JHUzVmaklsdytoQ1U5MUpHOVFKTjBBTGJTSytyMUhDb3RZ?=
 =?utf-8?B?UjFnVUszdVl6Zy95L1dPZjNOWG1mbVQwbytPOWduZ1UvZEVUdDZrV2U4cmVM?=
 =?utf-8?B?V250bjRaZ1BmMW9lQnN1QVhmbjlKNTJEOXN4TlptVENEWUhKT1BITUxBTVlw?=
 =?utf-8?B?c1E2NzIyd2Z0K2xXbGVnaWdHQk1OYU1aSktHc01MYjE4em9YU1dBYkI3RnVE?=
 =?utf-8?B?Q3RoZVBRT3UvYTRxUjlMdWV4N3ZKdWxaQ1RhZFF2eW9Da2tqWFJpMjBCcS9l?=
 =?utf-8?B?TkV6RVptZlZFNjNjcnVoYmhYL2NiQmQrbWV2VnNPemtEZ2VjT1FYOE1zbkpu?=
 =?utf-8?B?V2JsL1lIcDBiQ0RQMzJSQkRzeThlMkI4aG5YSDF6L2I1TklEWWl1OGtZall6?=
 =?utf-8?B?TXJwYjhGZjBNUG9KYThBT3U0eWJqMkFwMC9mVk1PcCtkb2RkOUN2UzZCcTI3?=
 =?utf-8?B?SDhkL2VieWlDSHNmdjVtZUhkSk5mSHF2a04wYWtINnVmY0NhWlozMEhKVGNR?=
 =?utf-8?B?UW81ZERvZHd3eVY4UjNVMEdpRW91OTZNZXRubXJUdmI1MHRxNFJ1eW43TjdF?=
 =?utf-8?B?U09mWDlaYmFHd2dzWU9VdzkyUnZZa0lodEtha3JHQ1R6M29RMlZyWEFwR2d3?=
 =?utf-8?B?SlFrMDE2OXJuTW5vcjI3M0J2WE5INTljd0dxMFE5ZzlJd0RodXh3YUptZ290?=
 =?utf-8?B?dG5JclQ5M1hoMURMcU9oRXd0VU4zUFp0RlRXemlsSVdzVVkySnZDbjA4WWRl?=
 =?utf-8?B?UU01QWJJaFgzN213NXlTMmFWVENHa2ZZNFphcE1EV3pvTnd6VUg0MlN3eE95?=
 =?utf-8?B?OEU4cmM0M1BpTEZzRFBNeVd6RUxzMzN3NzN0Q1AvTW9HcHJ1dlBnM0p4R0xU?=
 =?utf-8?B?NUZtaU1lbVMzT1I1eGs0YU1HRlZUWmxyaDMxd3BlMlNyK3JWYjVMMmtuRWhj?=
 =?utf-8?B?TnBSYlJxbGNCWlR1alJJQXJ6WjZUcldWUGNLa1c0b3BXTnBLOG9wd1VIYXZG?=
 =?utf-8?B?TklkNkZxNkRVa1hOVnFLYXJYalM3UFpyd1dKTnhEa3FxTWFTWkJOdWFST1VE?=
 =?utf-8?B?em4yWXM5dHNiZVYzQlhaRlF6dWJoYVJMdVZ4eHlVRGVsT3lLM3dCa3pJOEMy?=
 =?utf-8?B?anZ5VW1IUlppZ3lvM2hkamppczN1V2VTcGVqSzFaZjRWMkdNK0Ixa0U4N2pN?=
 =?utf-8?B?SlZpVGNubmsxaUxKbVlZbVlwUUh2Q0RkUERLdXFKYlhmcVg5R1pvaWM4SCty?=
 =?utf-8?B?czE1ZC9mR1h5TW1VZnQ1djNIc3BiQVJRRW1FSmN3S1Jsb3lPekZydUxxc0tw?=
 =?utf-8?B?NWJjdmhkQy8xanZycmhLRUtneTBLcVRzNk1xckN2S0JzcHVESUVkL3lQc1Ni?=
 =?utf-8?B?dTQ2Z1lrNGpMVVlvdmxoQ0pNZUQ1Mlc4T0VwRXBOZXNrU0NSUkRMblh4NkF4?=
 =?utf-8?B?Q05xQmQyVEU3clpnWEpEK0RJZkZ0RG13bXV5MElvMHRpcUIxNWFYS1ovdUhM?=
 =?utf-8?B?YkFscll1YXRoNXExblZCNUNmQk1SdXRhK1g4TTdxeENYLzYxVjVtVE54a01h?=
 =?utf-8?B?YmRkeDQ5OXAvZFRXT3VkZ2NjWnBURVNFTUF1eG9HbzhzeDUrOUkrRFluUjBV?=
 =?utf-8?B?czl0Uk1kRE4zNSs2NG9tYWdycXlpUzJTYWh3V0FveTE5amRuOWo1ZUQvTjN4?=
 =?utf-8?B?ZFJaQ0M2dWFsK0ZKcHdBVE1GcmY2dE9KSjV3YjNlcENhRlRNOUVmM0N1a2NI?=
 =?utf-8?B?dFVpYmkrU2w4ZzJPRENpQVAvSjhqRlpza2hadUp4dHlJVGp2VVEvVzZ0NXp3?=
 =?utf-8?B?RFdlTlZSSWtUUDRrRnpiaEZTY0FsRVdGWTY3V1ZOcXYvbVhiT293d1hpaXZw?=
 =?utf-8?B?ejN6bStPWHh6N3F3WFdZSGhJYm82bWZKajJZeHNxYWgyN1E5VVNlY2RkS2s5?=
 =?utf-8?B?TC9jWkJHRlVTVkFOY21GdWlhdDdKS2N5VjlUS0Zld1hZYUROMXRxMzc2bDgx?=
 =?utf-8?Q?U+f7lPTJpILbMPvvQjb+54c=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?NTNxOEFtaHdyRm11Z010NlZEaVRkZmdHL2RPZXFVQ3RXeEk3YkNxMFRrbVRx?=
 =?utf-8?B?dnZqTFZuVER5am4vTzF6WjdQMDFydk5TbVZRL29Dd3ROZ1ZUSVphYlY5L2FT?=
 =?utf-8?B?UWljU1B1NG5IZEo1UHkvZjR1RE0rYXNkZ2ZOUEF3eTVkQzNrVjErNEJKVmVo?=
 =?utf-8?B?YmFsdWtKa2szbXh6OEVzazYyaWZZakdXSWJjOXZ1WEhpSFFHTnVaUUdiOC9T?=
 =?utf-8?B?Tm1Nc2dvaGZpSGhDbmU0U3B6bGtjaDl6Vmt6NGJFR01iNHgxd0JYV2lrNWlP?=
 =?utf-8?B?cWkxZjB5TGF0MnpjQ29ZanJqeGxDMVhsSW9vL2dsTG9tdEV6bnQyOXlId3Bz?=
 =?utf-8?B?SXd4VWFjdXFTcnlzdUJENTRYQk8vZW5RUzZ6VWo2QmNITUNBOWs1SVdrNHpy?=
 =?utf-8?B?N09tRWFvMGRSVG52UWpjMVJESUxkZldJTVFuZDRzSzEyNnA2ZzYwWjhZYVJ6?=
 =?utf-8?B?cjNGeEZLMWsyU1VzaGNkSVFINHU4bTJ4N0VBY3pkVGhZMUJYdW85KzMvdzla?=
 =?utf-8?B?b0ZHbnpoTlpLeGZmK1d6N2FEMloxVmN0Z1IyT2h3ODc2Mm1xUVpvSXYyY0xG?=
 =?utf-8?B?aDNEbENFUHNpbktPTVJVZDYvblZWMEwxaTZyeHo5VXd6RzNsY2dQY2hGNmdr?=
 =?utf-8?B?bmFtVTFKclQ0Z1VZWTBqVFdmZGdpc0lPc2J3ZUVyd1B3bHdFaXh1ZFhGNjVO?=
 =?utf-8?B?TW8zdnVLZ29hQnBFRnN6a2dFNDNpYTRxVDFzU1VXSWcrVTZ3ZUM4TXd4a2Fz?=
 =?utf-8?B?RmNnZXdlMTRTTEtBWXVjVVdqTDNmNU0rUElMc2FaZDBTeEF5R1ZiT2dhOUt2?=
 =?utf-8?B?TkpnKzZzRUIwMHVaZk8yY3UzalNkYmFSK1BOR3NyMmR6YzJkamhwcGxFeHdV?=
 =?utf-8?B?d0tNV3JLVGxpMTl6enFQZHB5OEIyZE54aHY3LzNJSUJhYXBmQ3ZjTFJqaDVQ?=
 =?utf-8?B?NGNPMDdkNkQ2bUpZWE9pRUtKK2lVeWh3ZVFvMlJvdllVS1ZObXB2a0xKbjVO?=
 =?utf-8?B?N0J6aVc3UjVmb3BiK01NTnpWMVdIYTlrR3YxWmdOU1pPQ2FmT3FJZTREcFdy?=
 =?utf-8?B?WG5PTHVTRi85Q0pMQTBpRjRpOFJWZkpzZXV5Qk9oM2xCZGUvMmRtMnRaQWVj?=
 =?utf-8?B?blpGMHlTcDhQT2pTZG5hOFMwQWZpaXd6YmI3dDhoUTJqVzZNTTg0MzFGeG9F?=
 =?utf-8?B?TWVOY3YybTliZGtwakRUY1dYaXg3Q2d6alZacm5wRVc0TnlFK2lFaUZBQk15?=
 =?utf-8?B?TE9DTU55NW5KdFZINS9BSEY4cFlMcXY0YW0xTzB6emJJdUk4VWZkYyt2Nmsw?=
 =?utf-8?B?ZnFrRGU4bzFLOVRSQkc3dDRYcy9GenhHcHhKdllwalZscHZscCs5L3RlWTRi?=
 =?utf-8?B?SEZKeC8yb1h2NXVYVTh4Ujc2bHlDZDdqOUVTUHhPSHNMdUplZVZzZlFESGFN?=
 =?utf-8?B?cmpoOVRSWkJZNkNiZGJ6OEJJNWJMN0gzVnJzWUhOMzk4UmZ0dG82dzh1M0ha?=
 =?utf-8?B?SjZzMjBGeVVsRzFqa3FCN1BzSmR3bWRibnVMT0gxNmhOUzZQNUVqTlZZT0cr?=
 =?utf-8?Q?3IkF3pfJz7UKvb7V4ScZ24GxA=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 474a7af3-8978-4755-332c-08db2635b286
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Mar 2023 15:47:07.6599
 (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: EH2B5qadg/lwHfDJYfQKMyT3JOYHAFNR1dPY3535fEUU041NSUZE9xrhDs0tmwHarBr3hFdEmN2cUKMMxsuT0gAbLdEmIr7AsLzaqfDwfAs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB5678
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 spamscore=0 phishscore=0
 mlxlogscore=999 malwarescore=0 mlxscore=0 adultscore=0 bulkscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303150002 definitions=main-2303160126
X-Proofpoint-ORIG-GUID: v3ZR2hcSNGUhgBB4PViSo1hg2D55MuVF
X-Proofpoint-GUID: v3ZR2hcSNGUhgBB4PViSo1hg2D55MuVF
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/16/23 09:44, Eric DeVolder wrote:
> 
> 
> 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

Baoquan, as you are currently working through v19, please let me know when I should put out v20 with 
this and Sourabh's feedback.
Thanks,
eric

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