Re: [PATCH v2] x86/kexec: Add EFI config table identity mapping for kexec kernel

Baoquan He
2023-06-01T08:14:05Z

On 06/01/23 at 03:20pm, 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.
>
> This patch will make sure the EFI config table is always mapped.
>
> Signed-off-by: Tao Liu <ltao@redhat.com>
> ---
> Changes in v2:
> - Rephrase the change log based on Baoquan's suggestion.
> - Rename map_efi_sys_cfg_tab() to map_efi_tables().
> - Link to v1: https://lore.kernel.org/kexec/20230525094914.23420-1-ltao@redhat.com/
> ---
> 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..664aefa6e896 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_tables(struct x86_mapping_info *info, pgd_t *level4p)
> {
> #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

The code comment need be updated too?

* Prepare EFI tables and ACPI tables for kexec kernel since they are
* not covered by pfn_mapped.

Other than this nit, this patch looks good to me, thanks.

Acked-by: Baoquan He <bhe@redhat.com>

> + * since they are not covered by pfn_mapped.
> */
> - result = map_efi_systab(&info, level4p);
> + result = map_efi_tables(&info, level4p);
> if (result)
> return result;
>
> --
> 2.33.1
>