Hi Baoquan,
On Fri, May 26, 2023 at 12:08 PM Baoquan He <bhe@xxxxxxxxxx> wrote:
>
> Hi Tao,
>
> On 05/25/23 at 05:49pm, Tao Liu wrote:
> > A kexec kernel bootup hang is observed on Intel Atom cpu due to unmapped
> > EFI config table.
> >
> > Currently EFI system table is identity-mapped for the kexec kernel, but EFI
> > config table is not mapped explicitly:
> >
> > commit 6bbeb276b71f ("x86/kexec: Add the EFI system tables and ACPI
> > tables to the ident map")
> >
> > Later in the following 2 commits, EFI config table will be accessed when
> > enabling sev at kernel startup. This may result in a page fault due to EFI
> > config table's unmapped address. Since the page fault occurs at an early
> > stage, it is unrecoverable and kernel hangs.
> >
> > commit ec1c66af3a30 ("x86/compressed/64: Detect/setup SEV/SME features
> > earlier during boot")
> > commit c01fce9cef84 ("x86/compressed: Add SEV-SNP feature
> > detection/setup")
> >
> > In addition, the issue doesn't appear on all systems, because the kexec
> > kernel uses Page Size Extension (PSE) for identity mapping. In most cases,
> > EFI config table can end up to be mapped into due to 1 GB page size.
> > However if nogbpages is set, or cpu doesn't support pdpe1gb feature
> > (e.g Intel Atom x6425RE cpu), EFI config table may not be mapped into
> > due to 2 MB page size, thus a page fault hang is more likely to happen.
> >
> > In this patch, we will make sure the EFI config table is always mapped.
>
> Nice work. While you may need to rephrase above sentence, x86
> maintainers don't like log with the 'this patch,' or 'we'. Please refer
> to 'Changelog' part of Documentation/process/maintainer-tip.rst and
> improve it.
OK, Thanks for the suggestion! I will get the sentence rephrased in v2.
>
> >
> > Signed-off-by: Tao Liu <ltao@xxxxxxxxxx>
> > ---
> > arch/x86/kernel/machine_kexec_64.c | 35 ++++++++++++++++++++++++++----
> > 1 file changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> > index 1a3e2c05a8a5..755aa12f583f 100644
> > --- a/arch/x86/kernel/machine_kexec_64.c
> > +++ b/arch/x86/kernel/machine_kexec_64.c
> > @@ -28,6 +28,7 @@
> > #include <asm/setup.h>
> > #include <asm/set_memory.h>
> > #include <asm/cpu.h>
> > +#include <asm/efi.h>
> >
> > #ifdef CONFIG_ACPI
> > /*
> > @@ -86,10 +87,12 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
> > #endif
> >
> > static int
> > -map_efi_systab(struct x86_mapping_info *info, pgd_t *level4p)
> > +map_efi_sys_cfg_tab(struct x86_mapping_info *info, pgd_t *level4p)
>
> Can we call the function map_efi_tables() since we will map efi system
> table, system config table. If you need add another table mapping here,
> what would you call it, map_efi_sys_cfg_xxx_tab()?
>
Yeah, map_efi_sys_cfg_xxx_tab() is surely a bad name. I agree with the
map_efi_tables() name.
Thanks,
Tao Liu
> Anyway, not very strong opinion as long as x86 maintainer likes it.
>
> > {
> > #ifdef CONFIG_EFI
> > unsigned long mstart, mend;
> > + void *kaddr;
> > + int ret;
> >
> > if (!efi_enabled(EFI_BOOT))
> > return 0;
> > @@ -105,6 +108,30 @@ map_efi_systab(struct x86_mapping_info *info, pgd_t *level4p)
> > if (!mstart)
> > return 0;
> >
> > + ret = kernel_ident_mapping_init(info, level4p, mstart, mend);
> > + if (ret)
> > + return ret;
> > +
> > + kaddr = memremap(mstart, mend - mstart, MEMREMAP_WB);
> > + if (!kaddr) {
> > + pr_err("Could not map UEFI system table\n");
> > + return -ENOMEM;
> > + }
> > +
> > + mstart = efi_config_table;
> > +
> > + if (efi_enabled(EFI_64BIT)) {
> > + efi_system_table_64_t *stbl = (efi_system_table_64_t *)kaddr;
> > +
> > + mend = mstart + sizeof(efi_config_table_64_t) * stbl->nr_tables;
> > + } else {
> > + efi_system_table_32_t *stbl = (efi_system_table_32_t *)kaddr;
> > +
> > + mend = mstart + sizeof(efi_config_table_32_t) * stbl->nr_tables;
> > + }
> > +
> > + memunmap(kaddr);
> > +
> > return kernel_ident_mapping_init(info, level4p, mstart, mend);
> > #endif
> > return 0;
> > @@ -244,10 +271,10 @@ static int init_pgtable(struct kimage *image, unsigned long start_pgtable)
> > }
> >
> > /*
> > - * Prepare EFI systab and ACPI tables for kexec kernel since they are
> > - * not covered by pfn_mapped.
> > + * Prepare EFI systab, config table and ACPI tables for kexec kernel
> > + * since they are not covered by pfn_mapped.
> > */
> > - result = map_efi_systab(&info, level4p);
> > + result = map_efi_sys_cfg_tab(&info, level4p);
> > if (result)
> > return result;
> >
> > --
> > 2.33.1
> >
>
