From: ebiederm@xmission.com (Eric W. Biederman)
To: Xunlei Pang <xlpang@redhat.com>
Cc: linux-kernel@vger.kernel.org, kexec@lists.infradead.org,
        akpm@linux-foundation.org, Dave Young <dyoung@redhat.com>,
        Baoquan He <bhe@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
        Ingo Molnar <mingo@redhat.com>, x86@kernel.org
References: <1489746150-28364-1-git-send-email-xlpang@redhat.com>
Date: Fri, 17 Mar 2017 12:38:49 -0500
In-Reply-To: <1489746150-28364-1-git-send-email-xlpang@redhat.com> (Xunlei
        Pang's message of "Fri, 17 Mar 2017 18:22:30 +0800")
Message-ID: <8737ebg5w6.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] x86_64, kexec: Avoid unnecessary identity mappings for kdump
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3591
Lines: 96

Xunlei Pang <xlpang@redhat.com> writes:

> kexec setups identity mappings for all the memory mapped in 1st kernel,
> this is not necessary for the kdump case. Actually it can cause extra
> memory consumption for paging structures, which is quite considerable
> on modern machines with huge memory.
>
> E.g. On our 24TB machine, it will waste around 96MB (around 4MB/TB)
> from the reserved memory range if setting all the identity mappings.
>
> It also causes some trouble for distributions that use an intelligent
> policy to evaluate the proper "crashkernel=X" for users.
>
> To solve it, in case of kdump, we only setup identity mappings for the
> crash memory and the ISA memory(may be needed by purgatory/kdump
> boot).

How about instead we detect the presence of 1GiB pages and use them
if they are available.  We already use 2MiB pages.  If we can do that
we will only need about 192K for page tables in the case you have
described and this all becomes a non-issue.

I strongly suspect that the presence of 24TiB of memory in an x86 system
strongly correlates to the presence of 1GiB pages.

In principle we certainly can use a less extensive mapping but that
should not be something that differs between the two kexec cases.

I can see forcing the low 1MiB range in.  But calling it ISA range is
very wrong and misleading.  The reasons that range are special during
boot-up have nothing to do with ISA.  But have everything to do with
where legacy page tables are mapped, and where we need identity pages to
start other cpus.  I think the only user that actually cares is
purgatory where it plays swapping games with the low 1MiB because we
can't preload what we need to down there or it would mess up the running
kernel.  So saying anything about the old ISA bus is wrong and
misleading.  At the very very least we need accurate comments.

Eric


>
> Signed-off-by: Xunlei Pang <xlpang@redhat.com>
> ---
>  arch/x86/kernel/machine_kexec_64.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 857cdbd..db77a76 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -112,14 +112,40 @@ static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
>  
>  	level4p = (pgd_t *)__va(start_pgtable);
>  	clear_page(level4p);
> -	for (i = 0; i < nr_pfn_mapped; i++) {
> -		mstart = pfn_mapped[i].start << PAGE_SHIFT;
> -		mend   = pfn_mapped[i].end << PAGE_SHIFT;
>  
> +	if (image->type == KEXEC_TYPE_CRASH) {
> +		/* Always map the ISA range */
>  		result = kernel_ident_mapping_init(&info,
> -						 level4p, mstart, mend);
> +				level4p, 0, ISA_END_ADDRESS);
>  		if (result)
>  			return result;
> +
> +		/* crashk_low_res may not be initialized when reaching here */
> +		if (crashk_low_res.end) {
> +			mstart = crashk_low_res.start;
> +			mend = crashk_low_res.end + 1;
> +			result = kernel_ident_mapping_init(&info,
> +					level4p, mstart, mend);
> +			if (result)
> +				return result;
> +		}
> +
> +		mstart = crashk_res.start;
> +		mend = crashk_res.end + 1;
> +		result = kernel_ident_mapping_init(&info,
> +				level4p, mstart, mend);
> +		if (result)
> +			return result;
> +	} else {
> +		for (i = 0; i < nr_pfn_mapped; i++) {
> +			mstart = pfn_mapped[i].start << PAGE_SHIFT;
> +			mend   = pfn_mapped[i].end << PAGE_SHIFT;
> +
> +			result = kernel_ident_mapping_init(&info,
> +					 level4p, mstart, mend);
> +			if (result)
> +				return result;
> +		}
>  	}
>  
>  	/*