Received-SPF: pass (google.com: domain of linux-kernel+bounces-106985-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <ZeZ2Kos-OOZNSrmO@darkstar.users.ipa.redhat.com>
In-Reply-To: <ZeZ2Kos-OOZNSrmO@darkstar.users.ipa.redhat.com>
From: Dave Young <dyoung@redhat.com>
Date: Tue, 19 Mar 2024 12:13:36 +0800
Message-ID: <CALu+AoQzrVBjkBjurv_3p9kCe9EfPszcf1Lzo1G5_QQGbv8B+Q@mail.gmail.com>
Subject: Re: [PATCH] x86/kexec: do not update E820 kexec table for setup_data
To: x86@kernel.org
Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, 
	Dave Hansen <dave.hansen@linux.intel.com>, "H. Peter Anvin" <hpa@zytor.com>, 
	linux-kernel@vger.kernel.org, kexec@lists.infradead.org, 
	Baoquan He <bhe@redhat.com>, Eric Biederman <ebiederm@xmission.com>
Content-Type: text/plain; charset="UTF-8"

On Tue, 5 Mar 2024 at 09:32, Dave Young <dyoung@redhat.com> 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@redhat.com>
> ---
>  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