Re: [PATCH] x86/kexec: do not update E820 kexec table for setup_data

From: Dave Young
Date: Tue Mar 19 2024 - 00:13:36 EST


On Tue, 5 Mar 2024 at 09:32, Dave Young <dyoung@xxxxxxxxxx> wrote:
>
> crashkernel reservation failed on a Thinkpad t440s laptop recently,
> Actually the memblock reservation succeeded, but later insert_resource()
> failed.
>
> Test step:
> kexec load ->
> kexec reboot ->
> check the crashkernel memory
> dmesg|grep "crashkernel reserved"; saw reserved suceeeded:
> 0x00000000d0000000 - 0x00000000da000000
> grep Crash /proc/iomem: got nothing
>
> The background story is like below:
> Currently E820 code reserves setup_data regions for both the current kernel
> and the kexec kernel, and it will also insert them into resources list.
> Before the kexec kernel reboot nobody passes the old setup_data, kexec only
> passes SETUP_EFI and SETUP_IMA if needed. Thus the old setup data memory
> are not used at all. But due to old kernel updated the kexec e820 table
> as well so kexec kernel see them as E820_TYPE_RESERVED_KERN regions, later
> the old setup_data regions will be inserted into resources list in kexec
> kernel by e820__reserve_resources().
>
> Note, due to no setup_data passed in for those old regions they are not
> early reserved (by function early_reserve_memory), crashkernel memblock
> reservation will just regard them as usable memory and it could reserve
> reserve crashkernel region overlaps with the old setup_data regions.
>
> Just like the bug I noticed here, kdump insert_resource failed because
> e820__reserve_resources added the overlapped chunks in /proc/iomem already.
>
> Finally, looking at the code, the old setup_data regions are not used
> at all as no setup_data passed in by the kexec boot loader. Although
> something like SETUP_PCI etc could be needed, kexec should pass
> the info as setup_data so that kexec kernel can take care of them.
> This should be taken care of in other separate patches if needed.
>
> Thus drop the useless buggy code here.
>
> Signed-off-by: Dave Young <dyoung@xxxxxxxxxx>
> ---
> arch/x86/kernel/e820.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> Index: linux/arch/x86/kernel/e820.c
> ===================================================================
> --- linux.orig/arch/x86/kernel/e820.c
> +++ linux/arch/x86/kernel/e820.c
> @@ -1015,16 +1015,6 @@ void __init e820__reserve_setup_data(voi
> pa_next = data->next;
>
> e820__range_update(pa_data, sizeof(*data)+data->len, E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> -
> - /*
> - * SETUP_EFI and SETUP_IMA are supplied by kexec and do not need
> - * to be reserved.
> - */
> - if (data->type != SETUP_EFI && data->type != SETUP_IMA)
> - e820__range_update_kexec(pa_data,
> - sizeof(*data) + data->len,
> - E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> -
> if (data->type == SETUP_INDIRECT) {
> len += data->len;
> early_memunmap(data, sizeof(*data));
> @@ -1036,12 +1026,9 @@ void __init e820__reserve_setup_data(voi
>
> indirect = (struct setup_indirect *)data->data;
>
> - if (indirect->type != SETUP_INDIRECT) {
> + if (indirect->type != SETUP_INDIRECT)
> e820__range_update(indirect->addr, indirect->len,
> E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> - e820__range_update_kexec(indirect->addr, indirect->len,
> - E820_TYPE_RAM, E820_TYPE_RESERVED_KERN);
> - }
> }
>
> pa_data = pa_next;
> @@ -1049,7 +1036,6 @@ void __init e820__reserve_setup_data(voi
> }
>
> e820__update_table(e820_table);
> - e820__update_table(e820_table_kexec);
>
> pr_info("extended physical RAM map:\n");
> e820__print_table("reserve setup_data");
>

Kindly ping for review.

Thanks!
Dave