LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Baoquan He <bhe@redhat.com>
To: Borislav Petkov <bp@alien8.de>
Cc: j-nomura@ce.jp.nec.com, kasong@redhat.com, dyoung@redhat.com,
	linux-kernel@vger.kernel.org, tglx@linutronix.de,
	fanc.fnst@cn.fujitsu.com, x86@kernel.org,
	kexec@lists.infradead.org, hpa@zytor.com
Subject: Re: [PATCH v5 1/2] x86/kexec: Build identity mapping for EFI systab and ACPI tables
Date: Sun, 28 Apr 2019 13:41:14 +0800	[thread overview]
Message-ID: <20190428054114.GS3584@localhost.localdomain> (raw)
In-Reply-To: <20190427161121.GC12360@zn.tnic>

On 04/27/19 at 06:11pm, Borislav Petkov wrote:
> On Wed, Apr 24, 2019 at 05:29:43PM +0800, Baoquan He wrote:
> > From: Kairui Song <kasong@redhat.com>
> > 
> > The current code only builds identity mapping for physical memory during
> > kexec-type loading. The regions reserved by firmware are not covered.
> > In the next patch, the boot decompressing code of kexec-ed kernel tries
> 
> There's no guarantee that when this patch gets applied, the next patch
> will be the one you mean. So explain what you mean here instead.

All agreed, will update all of them, thanks.

About this place, do you think below change is OK to you?

~~~
The current code only builds identity mapping for physical memory during
kexec-type loading. The regions reserved by firmware are not covered.
In the later code change, the boot decompressing code of kexec-ed kernel
will try to access EFI systab and ACPI tables, lacking identity mapping for
them will cause error and reset system to firmware.

Thanks
Baoquan

> > 
> > This error doesn't happen on all systems. Because kexec enables gbpages
> > to build identity mapping, the EFI systab and ACPI tables could have been
> > covered if they share the same 1 GB area with physical memory. To make
> > sure, we should map them always.
> > 
> > So here add mapping for them.
> > 
> > Signed-off-by: Kairui Song <kasong@redhat.com>
> 
> When you send someone else's patch, you need to add your SOB. Lemme
> point you to
> 
>   Documentation/process/submitting-patches.rst
> 
> again. Please have a deeper look.
> 
> > ---
> >  arch/x86/kernel/machine_kexec_64.c | 86 ++++++++++++++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> > index ceba408ea982..77b40c3e28d7 100644
> > --- a/arch/x86/kernel/machine_kexec_64.c
> > +++ b/arch/x86/kernel/machine_kexec_64.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/io.h>
> >  #include <linux/suspend.h>
> >  #include <linux/vmalloc.h>
> > +#include <linux/efi.h>
> >  
> >  #include <asm/init.h>
> >  #include <asm/pgtable.h>
> > @@ -29,6 +30,48 @@
> >  #include <asm/setup.h>
> >  #include <asm/set_memory.h>
> >  
> > +#ifdef CONFIG_ACPI
> > +/**
> 
> Two stars '**' are kernel-doc style but this comment is implementation
> detail and is irrelevant for kernel-doc ouput.
> 
> > + * Used while adding mapping for ACPI tables.
> > + * Can be reused when other iomem regions need be mapped
> > + */
> > +struct init_pgtable_data {
> > +	struct x86_mapping_info *info;
> > +	pgd_t *level4p;
> > +};
> > +
> > +static int mem_region_callback(struct resource *res, void *arg)
> > +{
> > +	struct init_pgtable_data *data = arg;
> > +	unsigned long mstart, mend;
> > +
> > +	mstart = res->start;
> > +	mend = mstart + resource_size(res) - 1;
> > +
> > +	return kernel_ident_mapping_init(data->info,
> > +			data->level4p, mstart, mend);
> 
> Do not break that line.
> 
> > +}
> > +
> > +static int init_acpi_pgtable(struct x86_mapping_info *info,
> > +				   pgd_t *level4p)
> 
> static int
> map_acpi_tables(...)
> 
> > +{
> > +	unsigned long flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> > +	struct init_pgtable_data data;
> > +
> > +	data.info = info;
> > +	data.level4p = level4p;
> > +	flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> > +	return walk_iomem_res_desc(IORES_DESC_ACPI_TABLES, flags, 0, -1,
> > +				   &data, mem_region_callback);
> > +}
> > +#else
> > +static int init_acpi_pgtable(struct x86_mapping_info *info,
> > +				   pgd_t *level4p)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  #ifdef CONFIG_KEXEC_FILE
> >  const struct kexec_file_ops * const kexec_file_loaders[] = {
> >  		&kexec_bzImage64_ops,
> > @@ -36,6 +79,37 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
> >  };
> >  #endif
> >  
> > +#ifdef CONFIG_EFI
> > +static int init_efi_systab_pgtable(struct x86_mapping_info *info,
> > +				   pgd_t *level4p)
> 
> This function's name is wrong. Make it like this:
> 
> static int
> map_efi_systab(struct x86_mapping_info *info, pgd_t *level4p)
> {
> #ifdef CONFIG_EFI
> 
> 	...
> 
> #endif
> 
> 	return 0;
> }
> 
> and drop the #else ifdeffery.
> 
> 
> > +{
> > +	unsigned long mstart, mend;
> > +
> > +	if (!efi_enabled(EFI_BOOT))
> > +		return 0;
> > +
> > +	mstart = (boot_params.efi_info.efi_systab |
> > +			((u64)boot_params.efi_info.efi_systab_hi<<32));
> > +
> > +	if (efi_enabled(EFI_64BIT))
> > +		mend = mstart + sizeof(efi_system_table_64_t);
> > +	else
> > +		mend = mstart + sizeof(efi_system_table_32_t);
> > +
> > +	if (mstart)
> > +		return kernel_ident_mapping_init(info,
> > +				level4p, mstart, mend);
> 
> Flip that logic:
> 
> 	if (!mstart)
> 		return 0;
> 
> 	return kernel_ident_mapping_init(info, level4p, mstart, mend);
> 
> and let the function stick out.
> 
> > +
> > +	return 0;
> > +}
> > +#else
> > +static inline int init_efi_systab_pgtable(struct x86_mapping_info *info,
> > +					  pgd_t *level4p)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +
> >  static void free_transition_pgtable(struct kimage *image)
> >  {
> >  	free_page((unsigned long)image->arch.p4d);
> > @@ -159,6 +233,18 @@ static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
> >  			return result;
> >  	}
> >  
> > +	/**
> 
> Two stars '**' are kernel-doc style comments above function names but
> not here.
> 
> > +	 * Prepare EFI systab and ACPI table mapping for kexec kernel,
> > +	 * since they are not covered by pfn_mapped.
> > +	 */
> > +	result = init_efi_systab_pgtable(&info, level4p);
> > +	if (result)
> > +		return result;
> > +
> > +	result = init_acpi_pgtable(&info, level4p);
> > +	if (result)
> > +		return result;
> > +
> >  	return init_transition_pgtable(image, level4p);
> >  }
> >  
> > -- 
> > 2.17.2
> > 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

  reply	other threads:[~2019-04-28  5:43 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-24  9:29 [PATCH v5 0/2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel Baoquan He
2019-04-24  9:29 ` [PATCH v5 1/2] x86/kexec: Build identity mapping for EFI systab and ACPI tables Baoquan He
2019-04-27 16:11   ` Borislav Petkov
2019-04-28  5:41     ` Baoquan He [this message]
2019-04-29 12:50       ` Borislav Petkov
2019-04-29  0:23   ` [PATCH v6 " Baoquan He
2019-04-29 13:55     ` Borislav Petkov
2019-04-29 14:16       ` Baoquan He
2019-05-13  1:43       ` Baoquan He
2019-05-13  7:07         ` Borislav Petkov
2019-05-13  7:32           ` Baoquan He
2019-05-13  7:50             ` Borislav Petkov
2019-05-13  8:02               ` Baoquan He
2019-05-15  5:17                 ` Junichi Nomura
2019-05-15  6:58                   ` Borislav Petkov
2019-05-15  7:09                     ` Junichi Nomura
2019-05-21  9:02                       ` Kairui Song
2019-05-21 10:43                         ` Junichi Nomura
2019-05-21 18:09                         ` Borislav Petkov
2019-05-28  2:49                           ` Kairui Song
2019-06-06 19:20                             ` Borislav Petkov
2019-05-13  8:06               ` Baoquan He
2019-05-14  3:22                 ` Dave Young
2019-05-14  3:33                   ` Baoquan He
2019-05-21 21:53                     ` Dirk van der Merwe
2019-05-21 23:04                       ` Borislav Petkov
2019-05-14  8:48                   ` Dave Young
2019-05-14 11:18                     ` Kairui Song
2019-05-14 11:38                     ` Peter Zijlstra
2019-05-14 12:58                       ` Dave Young
2019-05-14 13:54                         ` Peter Zijlstra
2019-05-14 14:09                         ` Ingo Molnar
2019-05-15  1:08                           ` Dave Young
2019-05-15  6:43                             ` Junichi Nomura
2019-05-17 13:41                   ` Borislav Petkov
2019-05-17 13:50                     ` [PATCH] x86/boot: Call get_rsdp_addr() after console_init() Borislav Petkov
2019-05-21  9:28                       ` Baoquan He
2019-06-06 19:22     ` [tip:x86/boot] x86/kexec: Add the EFI system tables and ACPI tables to the ident map tip-bot for Kairui Song
2019-04-24  9:29 ` [PATCH v5 2/2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernels Baoquan He
2019-04-24  9:33   ` Baoquan He
2019-04-24  9:38 ` [PATCH v5 0/2] x86/boot: Use efi_setup_data for searching RSDP on kexec-ed kernel Borislav Petkov
2019-04-24 10:00   ` Baoquan He

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190428054114.GS3584@localhost.localdomain \
    --to=bhe@redhat.com \
    --cc=bp@alien8.de \
    --cc=dyoung@redhat.com \
    --cc=fanc.fnst@cn.fujitsu.com \
    --cc=hpa@zytor.com \
    --cc=j-nomura@ce.jp.nec.com \
    --cc=kasong@redhat.com \
    --cc=kexec@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v5 1/2] x86/kexec: Build identity mapping for EFI systab and ACPI tables' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).