Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d91f8728-6a63-415d-577c-bd76e69ec7f6@oracle.com>
Date:   Fri, 28 Oct 2022 10:29:45 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
Subject: Re: [PATCH v12 7/7] x86/crash: Add x86 crash hotplug support
Content-Language: en-US
To:     Borislav Petkov <bp@alien8.de>
Cc:     Baoquan He <bhe@redhat.com>, david@redhat.com,
        Oscar Salvador <osalvador@suse.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        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, 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,
        sourabhjain@linux.ibm.com, linux-mm@kvack.org
References: <d6386653-eb71-188c-8a09-5db46b4e42d4@oracle.com>
 <YzcqE1RVtPcuLlxN@zn.tnic> <Y0Dh4ieUUZ4oXa1/@MiWiFi-R3L-srv>
 <Y0b9apyIs+RpSo1e@zn.tnic> <53aed03e-2eed-09b1-9532-fe4e497ea47d@oracle.com>
 <Y0cmaPTKQuWtwIRh@zn.tnic> <Y0d+mFivS+88+Chr@MiWiFi-R3L-srv>
 <Y1e7Hw0PRDI9NxU9@zn.tnic> <Y1lIoTBNvHrKX5K9@MiWiFi-R3L-srv>
 <35c98ca6-10f8-b248-78c5-99fce7e66c65@oracle.com> <Y1uspLb7fLdtnQq+@zn.tnic>
From:   Eric DeVolder <eric.devolder@oracle.com>
In-Reply-To: <Y1uspLb7fLdtnQq+@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?cCs3elVtVlp2a1REUTNMTHdkTzVUQi95Z3ZDblRZSy9iei9qWVpvNjVNT3hH?=
 =?utf-8?B?akczaHRSUWx0NElQUWkyYjV0UVNUQkFIRW43UDFuVGYycWpMeHlXNjAxMlgz?=
 =?utf-8?B?OVlYSHFRdTBjS3NINGxLanRhZEV6dUxiQmRDaEJYd1Q4K20za2Qxai9yRjh4?=
 =?utf-8?B?emx3V21Kck5EcTA0WEcyNmFJeEFqRjdWQmdaQ0FNN2RkUmxNcWZENWZDeGlu?=
 =?utf-8?B?bVFmRU9ITWpnMEdsMUFpQmFwUDlJSzVWaTdrWDJWVFBSSlZ3alFnSjJmaXRW?=
 =?utf-8?B?M0dCRFVvWjY4anZGRDJoWXNZbm5hWWI0a0w4dU4xNklQRmxnSExaZTRwSmJF?=
 =?utf-8?B?VDBQUHBmSzBDVXBYLzFKYzZqQW9IbW8vSnRJZHlmc3FyNENodldyUXoxT1pI?=
 =?utf-8?B?djVpOXpZSHB1T2lMR0VGcmpnQ1VQNFZCTnovSWJZaFZRK2dnelIvRHZmNmF2?=
 =?utf-8?B?ZHdETjNFbHQ1T1hESTNlWHd4ejZuckFsRzUvRVZlMVg4cHdoaVU4eTNDYi8z?=
 =?utf-8?B?b1ZjYkx0TVZZcUZhNEVVdlM0SEkwV2h0a2VrTks4UXJKZGZabFhBKy9Lb1pB?=
 =?utf-8?B?b05mY0RCdTk3MUhFc29UL2NISTRNZktRbUg1c29XYS9FVWRJT25mc0xpbGY0?=
 =?utf-8?B?dmczNWVSeVIyUTJvMENNR0JaTGV2cVdkaktQa1BWYUxWeHV5VmNNYVhTMTZa?=
 =?utf-8?B?OFpTbUU0Sk1ab08vdXp3MXI2b0tycGxkWnZiOWlRcUpIVk9UcjRRVEs1bFhu?=
 =?utf-8?B?MVF0T2Q1WmNWQkxiTHlwcFd5dVh3YkZGMUExaVFwL0psSnBtUHhDb0pjZzBL?=
 =?utf-8?B?QS90amJrQ1VLRnl3eWszZnNYeElTUE85ektweXUxQklpdytIalJtY2JnNjBu?=
 =?utf-8?B?ME9vQ1JlNFh0Q1NxTVVQR3h3QlhTaHAzMW9sSG1IR0JEZlEwb2h5eUVPNkpq?=
 =?utf-8?B?R1hTREIwN0VsQmZCbXNyalpydWZNekVEdTBKajQ1N0kwWE5nUzN5MGlQOGtB?=
 =?utf-8?B?ZmcvNE5pd3hEUkJvckQ1S2NaS2IxaVVSbC9xTzQwTmN4QVNFZXhFeHMwNGFT?=
 =?utf-8?B?SW9UOEFiOHY3U2NranF1czJQSFBoeEk2MERjTjIrY0pNMWpHT1BQS2NubW92?=
 =?utf-8?B?cFRmRURyOGhtTkdiWWFnTTUvWUxlWFpxM3l3MzN2VHpGNThSb0FDUDJhWWx1?=
 =?utf-8?B?U0FHVnBpaXdvTkg3Szd6RHdaNGUwWm91NTNmeHgxZXZFam5kbkN3MVQwZUpL?=
 =?utf-8?B?TThwMm8waWYvR1huWnYrUXE2OFZyQVYyanIycmdFdGIwYmZQZXhEaW8wcVlJ?=
 =?utf-8?B?TlFwZUtRT1R6NTd0SjJ6S1JEVnhxWDdzbkc4bVNPZnJUV2lzSkZ2N0gxNzVZ?=
 =?utf-8?B?T2YxUFluMnBHbnNxMmtDNExlUExydnh2WTJFdWVwY1Y1eUJkKzZNMkdqU1NT?=
 =?utf-8?B?VTJSUVUydFpEV2lNdDBqcCtwOXlBbGR6ajdLRWVJM1hZWWJhK1ZiWlVZN3pG?=
 =?utf-8?B?SFNiT1IrY1d5V3RINUhnWnpWRnZMN2o2R1grRVVvaEs2dDBDdG05QUh0Wnkw?=
 =?utf-8?B?aG1MalBPYzRaaHROZWsyUW1mTDBnRG9oRlRpblZPZVRlQ25UUmVZODJrbS80?=
 =?utf-8?B?RzZYNkk5cGk1dzlSendHRldTMWdYb1EvNFVZZnFqNXZqSTJ0ODBUQnpJLzVy?=
 =?utf-8?B?TVdGdjBRMldMeTMrcDdPZVVVTHNIU2F6SGVsbGtRR1B4REEreXR4MXpTcmVO?=
 =?utf-8?B?TjVVa015V2lmTFVNMmIxMU1leXJHMHZXemFHNWxVc1JwZVVDSjdCc2FzRUpr?=
 =?utf-8?B?bUVoek9JTnVqVkF0WjdUV3o0dWFpc0ZYWDRtMUdDR3J3VnFxajYwODJYWThS?=
 =?utf-8?B?bk9zK1BsRVFEMmxPckZiVExFbm1MYUF3SjFDdGpOZmxQREpwR2xRRTZ1S3ht?=
 =?utf-8?B?TllzMkYxMXJzSnl1Nk9hR1grcUNpYm15eXB6aWhRb0hVcGRRd1J2Y0ZCeUlk?=
 =?utf-8?B?OVhOSVFSVnJ4Qk1XcG40YW5zZzVTNGR2KzFrNWN3dFpRT2xGY0txNnBWd1NC?=
 =?utf-8?B?UlVUQmhVNXBaTzhvQVZrNms5bjdHVUVMNTE4eFlpdWtmYmd1WENtbDMzbUJu?=
 =?utf-8?B?WjVQRElsSTdtWWRZNTlUck5zQjdJMStLVG1OVjgva0lGWWdXMFplemNJY2dL?=
 =?utf-8?Q?496WeTHlv2eaXusEtCocKTY=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4280bd7f-c35a-411a-7263-08dab8f9423e
X-MS-Exchange-CrossTenant-AuthSource: CO1PR10MB4531.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Oct 2022 15:29:52.7070
 (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: 4wdwqZkHoRe2m2WDOl5AVmgDTPDTQKZRoJyzwJTeGXPUuKp/Kwl9not1j7i0SvzvRAeujW25oyg2wotSxAEvF/8EJAzXtsvh23X/nPWsnuE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB5900
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-28_07,2022-10-27_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 spamscore=0
 suspectscore=0 phishscore=0 adultscore=0 mlxscore=0 mlxlogscore=999
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2210280096
X-Proofpoint-ORIG-GUID: jlbVogpr_SuM8FGJL-DUDoiTddtuX9Jo
X-Proofpoint-GUID: jlbVogpr_SuM8FGJL-DUDoiTddtuX9Jo
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        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


On 10/28/22 05:19, Borislav Petkov wrote:
> On Thu, Oct 27, 2022 at 02:24:11PM -0500, Eric DeVolder wrote:
>> Be aware, in reality, that if the system was fully populated, it would not
>> actually consume all 8192 phdrs. Rather /proc/iomem would essentially show a
>> large contiguous address space which would require just a single phdr.
> 
> Then that from below:
> 
> 	pnum += CONFIG_CRASH_MAX_MEMORY_RANGES;
> 
> which then would end up allocating 8192 would be a total waste.
> 
> So why don't you make that number dynamic then?
> 
> You start with something sensible:
> 
> 	total_num_pheaders = num_online_cpus() + "some number of regions" + "some few others"
> 
> I.e., a number which is a good compromise on the majority of machines.
> 
> Then, on hotplug events you count how many new regions are coming in
> and when you reach the total_num_pheaders number, you double it (or some
> other increase stragegy), reallocate the ELF header buffers etc needed
> for kdump and you're good.
> 
> This way, you don't waste memory unnecessarily on the majority of
> systems and those who need more, get to allocate more.

This patch series sizes and allocates the memory buffer/segment for the elfcorehdr once, at kdump 
load time.

In order to dynamically resize the elcorehdr memory buffer/segment, that causes the following ripple 
effects:

  - Permitting resizing of the elfcorehdr requires a means to "allocate" a new size buffer from 
within the crash kernel reserved area. There is no allocator today; currently it is a kind of 
one-pass placement process that happens at load time. The critical side effect of allocating a new 
elfcorehdr buffer memory/segment is that it creates a new address for the elfcorehdr.

  - The elfcorehdr is passed to the crash kernel via the elfcorehdr= kernel cmdline option. As such, 
a dynamic change to the size of the elfcorehdr size necessarily invites a change of address of that 
buffer, and therefore a change to rewrite the crash kernel cmdline to reflect the new elfcorehdr 
buffer address.

  - A change to the cmdline, also invites a possible change of address of the buffer containing the 
cmdline, and thus a change to the x86 boot_params, which contains the cmdline pointer.

  - A change to the cmdline and/or boot_params, which are *not* excluded from the hash/digest, means 
that the purgatory hash/digest needs to be recomputed, and purgatory re-linked with the new 
hash/digest and replaced.

A fair amount of work, but I have had this working in the past, around the v1 patch series 
timeframe. However, it only worked for the kexec_file_load() syscall as all the needed pieces of 
information were available; but for kexec_load(), it isn't possible to relink purgatory as by that 
point purgatory is but a user-space binary blob.

It was feedback on the v1/v2 that pointed out that by excluding the elfcorehdr from the hash/digest, 
the "change of address" problem with the elfcorehdr buffer/segment goes away, and, in turn, negates 
the need to: introduce an allocator for the crash kernel reserved space, rewrite the crash kernel 
cmdline with a new elfcorehdr, update boot_params with a new cmdline and re-link and replace 
purgatory with the updated digest. And it enables this hotplug efforts to support kexec_load() 
syscall as well.

So it is with this in mind that I suggest we stay with the statically sized elfcorehdr buffer.

If that can be agreed upon, then it is "just a matter" of picking a useful elfcorehdr size. 
Currently that size is derived from the NR_DEFAULT_CPUS and CRASH_MAX_MEMORY_RANGES. So, there is 
still the CRASH_MAX_MEMORY_RANGES knob to help a dial in size, should there be some issue with the 
default value/size.

Or if there is desire to drop computing the size from NR_DEFAULT_CPUs and CRASH_MAX_MEMORY_RANGES 
and simply go with CRASH_HOTPLUG_ELFCOREHDR_SZ which simply specifies the buffer size, then I'm also 
good with that.

I still owe a much better explanation of how to size the elfcorehdr. I can use the comments and 
ideas from the discussion to provide the necessary insight when choosing this value, whether that be 
CRASH_MAX_MEMORY_RANGES or CRASH_HOTPLUG_ELFCOREHDR_SZ.


> 
>> I'd prefer keeping CRASH_MAX_MEMORY_RANGES as that allow the maximum phdr
>> number value to be reflective of CPUs and/or memory; not all systems support
>> both CPU and memory hotplug. For example, I have queued up this change to
>> reflect this:
>>
>>      if (IS_ENABLED(CONFIG_HOTPLUG_CPU) || IS_ENABLED(CONFIG_MEMORY_HOTPLUG)) {
> 
> If you're going to keep CRASH_MAX_MEMORY_RANGES, then you can test only
> that thing as it expresses the dependency on CONFIG_HOTPLUG_CPU and
> CONFIG_MEMORY_HOTPLUG already.
> 
> If you end up making the number dynamic, then you could make that a
> different Kconfig item which contains all that crash code as most of the
> people won't need it anyway.

It is my intention to correct the CRASH_MAX_MEMORY_RANGES (if we keep it) as such:

config CRASH_MAX_MEMORY_RANGES
     depends on CRASH_DUMP && KEXEC_FILE && MEMORY_HOTPLUG

CRASH_MAX_MEMORY_RANGES should have never had CPU_HOTPLUG as a dependency; that was a cut-n-paste 
error on my part.

> 
> Hmm?
> 

Thank you for the time and thought on this topic!
eric