Re: [RFD] efi assisted kdump
On Sat, 24 Jan, at 09:26:37PM, Dave Young wrote: > > There's several things I need to ask for help from EFI gurus to see if it is > doable: > > * Make sure in case EFI warm reboot the memory of previous kernel can be > retained. The only way to reserve memory across a reboot is with EFI capsules. > * How to determine if kernel is boot with EFI warm reboot in stub The presence of a capsule would indicate whether we were invoked from a crash handler. > * What is the right way to pass informations from 1st kernel to 2nd kernel. > Is it ok for saving something with efi variables? EFI capsules would the be most natural mechanism, but you could conceivably do it with EFI variables (there's an upper limit on the size of variable data, though). > * Other possible problems what I missed Support for runtime EFI capsules was rather spotty last time I looked. Things may have moved on, but it's definitely something to watch out for. There have been patches on linux-efi in recent months for making use of EFI capsules, https://lkml.kernel.org/r/1412692886-25306-1-git-send-email-m...@console-pimps.org https://lkml.kernel.org/r/1414984030-13859-1-git-send-email-hock.leong.k...@intel.com I could take a look at resubmitting the first series above if that would be useful for doing kexec across a reset. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] x86/efi: skip bgrt init for kexec reboot
On Thu, 04 Feb, at 05:42:00AM, Dave Young wrote: > > On 01/27/16 at 07:20pm, Dave Young wrote: > > For kexec reboot the bgrt image address could contains random data because > > we have freed boot service areas in 1st kernel boot phase. One possible > > result is kmalloc fail in efi_bgrt_init due to large random image size. > > > > So change efi_late_init to avoid efi_bgrt_init in case kexec boot. > > > > Signed-off-by: Dave Young > > --- > > arch/x86/platform/efi/efi.c |3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > --- linux-x86.orig/arch/x86/platform/efi/efi.c > > +++ linux-x86/arch/x86/platform/efi/efi.c > > @@ -531,7 +531,8 @@ void __init efi_init(void) > > > > void __init efi_late_init(void) > > { > > - efi_bgrt_init(); > > + if (!efi_setup) > > + efi_bgrt_init(); > > } > > > > void __init efi_set_executable(efi_memory_desc_t *md, bool executable) > > Matt, opinions about this patch? Yeah, I'm not happy seeing efi_setup escaping into even more places, nor am I happy to see more code paths introduced where kexec boot is special-cased. I'll reply with more details tomorrow. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] x86/efi: skip bgrt init for kexec reboot
On Wed, 03 Feb, at 10:53:33PM, Matt Fleming wrote: > On Thu, 04 Feb, at 05:42:00AM, Dave Young wrote: > > > > On 01/27/16 at 07:20pm, Dave Young wrote: > > > For kexec reboot the bgrt image address could contains random data because > > > we have freed boot service areas in 1st kernel boot phase. One possible > > > result is kmalloc fail in efi_bgrt_init due to large random image size. > > > > > > So change efi_late_init to avoid efi_bgrt_init in case kexec boot. > > > > > > Signed-off-by: Dave Young > > > --- > > > arch/x86/platform/efi/efi.c |3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > --- linux-x86.orig/arch/x86/platform/efi/efi.c > > > +++ linux-x86/arch/x86/platform/efi/efi.c > > > @@ -531,7 +531,8 @@ void __init efi_init(void) > > > > > > void __init efi_late_init(void) > > > { > > > - efi_bgrt_init(); > > > + if (!efi_setup) > > > + efi_bgrt_init(); > > > } > > > > > > void __init efi_set_executable(efi_memory_desc_t *md, bool executable) > > > > Matt, opinions about this patch? > > Yeah, I'm not happy seeing efi_setup escaping into even more places, > nor am I happy to see more code paths introduced where kexec boot is > special-cased. > > I'll reply with more details tomorrow. OK, let me expand upon that rather terse feedback. This patch highlights a general problem I see in the EFI code which is that we're continuously increasing the number of execution paths through the boot code. This makes it increasingly difficult to modify the code without introducing bugs and regressions. I was bitten by this recently with the EFI separate page table rework, which led to commit 753b11ef8e92 ("x86/efi: Setup separate EFI page tables in kexec paths"), i.e I forgot to update the special kexec virtual mapping function. We should be reducing the use of 'efi_setup', not adding more uses. As an aside, I've always had a problem with using 'efi_setup' to indicate when we've been booted via kexec. If a developer with no prior knowledge reads those if conditions they are going to have zero clue what the code means. Now, specifically for the issue you've raised, would it not make more sense for kexec to build its own ACPI tables and omit those entries that are not valid, e.g. BGRT? I can imagine that the BGRT driver won't be the only driver with this problem. Let's re-use the existing error paths that handle missing/invalid tables. Fundamentally I don't think there should be a discernible difference between "Booted via kexec" and "That ACPI table does not exist". ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] x86/efi: skip bgrt init for kexec reboot
On Thu, 04 Feb, at 07:09:03PM, Dave Young wrote: > > Consider the original code path, maybe change it to efi_kexec_setup will > be better to remind people? Or something else like a wraper function with > similar name.. Possibly. I had considered adding a new efi_enabled() bit for KEXEC_BOOT, but I'm worried that'll just encourage more uses. The best approach is going to be to see whether we can reduce the uses of efi_setup and the associated special code. Once we've completed that exercise, we can think about the best name for this variable. > For building ACPI tables we need do it in kernel instead of kexec-tools > because of kexec_file_load for secure boot case so we still need a conditional > code path for kexec.. Note that it may not be necessary to build any ACPI tables at all, provided that things like acpi_get_table() fail gracefully for kexec. I'm assuming that's the problem that you discovered when writing this patch. And yes, I don't expect you can build the ACPI table from userspace, but it should at least be possible to do it in setup_boot_parameters() or so when you setup the EFI table pointers (efi.config_tables), etc. I think that would be a natural home for this feature. > Also I'm not sure how to rebuild ACPI tables, it is easy or hard. Let me > checking the detail and think more about it. Thanks. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] x86/efi: skip bgrt init for kexec reboot
On Fri, 05 Feb, at 08:41:15AM, Dave Young wrote: > On 02/04/16 at 11:56am, Matt Fleming wrote: > > On Thu, 04 Feb, at 07:09:03PM, Dave Young wrote: > > > > > > Consider the original code path, maybe change it to efi_kexec_setup will > > > be better to remind people? Or something else like a wraper function with > > > similar name.. > > > > Possibly. I had considered adding a new efi_enabled() bit for > > KEXEC_BOOT, but I'm worried that'll just encourage more uses. > > > > The best approach is going to be to see whether we can reduce the uses > > of efi_setup and the associated special code. Once we've completed > > that exercise, we can think about the best name for this variable. > > Ok, thanks. > > > > > > For building ACPI tables we need do it in kernel instead of kexec-tools > > > because of kexec_file_load for secure boot case so we still need a > > > conditional > > > code path for kexec.. > > > > Note that it may not be necessary to build any ACPI tables at all, > > provided that things like acpi_get_table() fail gracefully for kexec. > > I'm assuming that's the problem that you discovered when writing this > > patch. > > > > And yes, I don't expect you can build the ACPI table from userspace, > > but it should at least be possible to do it in setup_boot_parameters() > > or so when you setup the EFI table pointers (efi.config_tables), etc. > > I think that would be a natural home for this feature. > > Thing is we support both kexec_load and kexec_file_load, if we do something > in kernel loader we will need do same in userspace kexec-tools as well. Actually, on thinking about it a little bit more, any changes we make would have to be on the second kernel side, because otherwise you end up with incompatibilities between kernel versions. For example, changing the data we pass in the SETUP_EFI chain is a bad idea becuase you then have to care about exactly which kernel version you kexec loaded. > Another way is we probably can retain the boot service areas for kexec > boot, but yes it is another special handling for kexec :(. Is this way > better to you? Unfortunately retaining Boot Services areas is infeasible because they can be *really* large, i.e. multiple gigabytes in size. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] x86/efi: skip bgrt init for kexec reboot
On Fri, 12 Feb, at 08:45:42PM, Dave Young wrote: > > Yes, rethink about this issue, how about something like below: Let's put a pin this idea. I've got another approach that I want to explore a little first. I'll send the patches out soon. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 0/2] ACPI, x86/efi: Remove ACPI BGRT tables for kexec
Based on Dave's report that the BGRT image regions are being accessed on kexec reboot (which by that time contain garbage), https://lkml.kernel.org/r/20160127112044.ga2...@dhcp-128-65.nay.redhat.com the following patches simply delete the table when doing a kexec boot. This is part of a wider compaign to stop kexec-specific code from leaking into all parts of arch/x86. Instead of sprinkling "if (efi_setup)", kexec should be shaping the platform information to more accurately describe which features are still available, so that existing drivers work transparently without kexec quirks. Matt Fleming (2): ACPICA: Tables: Add function to remove ACPI tables x86/efi: Delete ACPI BGRT when booting via kexec arch/x86/include/asm/efi.h| 1 + arch/x86/kernel/setup.c | 4 +++- arch/x86/platform/efi/efi.c | 38 ++ drivers/acpi/acpica/tbxface.c | 54 +++ include/acpi/acpixf.h | 3 +++ 5 files changed, 99 insertions(+), 1 deletion(-) -- 2.6.2 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 2/2] x86/efi: Delete ACPI BGRT when booting via kexec
Dave reports that for kexec reboot the ACPI BGRT image region may contain garbage data, because the image lives in EFI Boot Services regions that were released and freed when the first kernel called efi_free_boot_services(). Since the EFI Boot Services regions can be large (multiple gigabytes) preserving them throughout the kernel's lifetime and across kexec reboot is not a viable solution. Instead we need to avoid accessing the BGRT image regions under kexec. Rather than dirtying the ACPI BGRT driver with conditionals that check whether we're booting via kexec, we can execute the existing code path that exits if the table cannot be found - by removing the BGRT table. It is unfortunate that this logic cannot be folded into the existing kexec-specific EFI code, but there are dependencies on having loaded the ACPI tables, which happens much later. Reported-by: Dave Young Cc: Rafael J. Wysocki Cc: Josh Triplett Cc: Borislav Petkov Cc: Matthew Garrett Cc: Vivek Goyal Cc: Signed-off-by: Matt Fleming --- arch/x86/include/asm/efi.h | 1 + arch/x86/kernel/setup.c | 4 +++- arch/x86/platform/efi/efi.c | 38 ++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 7bb206f73915..0e813da58be6 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -146,6 +146,7 @@ extern void __init efi_dump_pagetable(void); extern void __init efi_apply_memmap_quirks(void); extern int __init efi_reuse_config(u64 tables, int nr_tables); extern void efi_delete_dummy_variable(void); +extern void __init efi_kexec_remove_acpi_tables(void); struct efi_setup_data { u64 fw_vendor; diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index d3d80e6d42a2..8131962ce8c2 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -1249,8 +1249,10 @@ void __init setup_arch(char **cmdline_p) register_refined_jiffies(CLOCK_TICK_RATE); #ifdef CONFIG_EFI - if (efi_enabled(EFI_BOOT)) + if (efi_enabled(EFI_BOOT)) { efi_apply_memmap_quirks(); + efi_kexec_remove_acpi_tables(); + } #endif } diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 39748d228e34..d87f42697ad0 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -485,6 +485,44 @@ static int __init efi_kexec_override(void) return 0; } +/* + * The BGRT table, if present, refers to memory regions that are no + * longer reserved during kexec boot, having been released by the + * previous kernel. The BGRT image likely contains garbage. + * + * Delete the ACPI table, since it is useless for kexec, and so that + * it is impossible for the ACPI BGRT driver to distinguish between + * "Platform has no BGRT" and "We were booted via kexec". + * + * Note that this function must only be called after the ACPI tables + * have been initialised. + */ +void __init efi_kexec_remove_acpi_tables(void) +{ + struct acpi_table_header *header; + acpi_status status; + + if (!efi_setup) + return; + + status = acpi_get_table(ACPI_SIG_BGRT, 0, &header); + if (ACPI_FAILURE(status)) + return; + + status = acpi_remove_table(ACPI_SIG_BGRT, 0); + if (ACPI_FAILURE(status)) { + pr_err("Failed to remove ACPI BGRT table\n"); + return; + } + + /* +* Since we've probably already told the user that we have a +* BGRT when parsing the ACPI tables, inform them that we have +* intentionally removed it now. +*/ + pr_info("Removed ACPI BGRT table for kexec\n"); +} + void __init efi_init(void) { efi_char16_t *c16; -- 2.6.2 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables
There are existing internal functions that allow the removal of ACPI tables, but they're not exposed to the OS in any useful way. Introduce acpi_remove_table() which allows tables to be invalidated in the global table list, resulting in failure of subsequent calls to acpi_get_table() for those tables. The rationale for this change is the ability to remove the BGRT table during kexec boot. The BGRT table refers to memory regions that are no longer reserved by the firmware once the kexec kernel boots, having been released for general allocation by the previous kernel. Cc: Dave Young Cc: Rafael J. Wysocki Cc: Josh Triplett Cc: Borislav Petkov Cc: Matthew Garrett Cc: Vivek Goyal Cc: Signed-off-by: Matt Fleming --- drivers/acpi/acpica/tbxface.c | 54 +++ include/acpi/acpixf.h | 3 +++ 2 files changed, 57 insertions(+) diff --git a/drivers/acpi/acpica/tbxface.c b/drivers/acpi/acpica/tbxface.c index 326df65decef..999eecd89601 100644 --- a/drivers/acpi/acpica/tbxface.c +++ b/drivers/acpi/acpica/tbxface.c @@ -480,3 +480,57 @@ cleanup: } ACPI_EXPORT_SYMBOL(acpi_remove_table_handler) + +/*** + * + * FUNCTION:acpi_remove_table + * + * PARAMETERS: signature - ACPI signature of needed table + * instance- Which instance (for SSDTs) + * + * RETURN: Status + * + * DESCRIPTION: Finds and removes an ACPI table. + * + **/ +acpi_status acpi_remove_table(char *signature, u32 instance) +{ + struct acpi_table_desc *table_desc; + acpi_status status; + u32 i; + u32 j; + + /* Parameter validation */ + if (!signature) { + return (AE_BAD_PARAMETER); + } + + /* Walk the root table list */ + + for (i = 0, j = 0; i < acpi_gbl_root_table_list.current_table_count; +i++) { + if (!ACPI_COMPARE_NAME + (&(acpi_gbl_root_table_list.tables[i].signature), +signature)) { + continue; + } + + if (++j < instance) { + continue; + } + + table_desc = &acpi_gbl_root_table_list.tables[i]; + + status = acpi_tb_validate_table(table_desc); + if (ACPI_FAILURE(status)) { + return (status); + } + + acpi_tb_uninstall_table(table_desc); + return (AE_OK); + } + + return (AE_NOT_FOUND); +} + +ACPI_EXPORT_SYMBOL(acpi_remove_table); diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index c96621e87c19..47e51612293e 100644 --- a/include/acpi/acpixf.h +++ b/include/acpi/acpixf.h @@ -505,6 +505,9 @@ ACPI_EXTERNAL_RETURN_STATUS(acpi_status ACPI_EXTERNAL_RETURN_STATUS(acpi_status acpi_remove_table_handler(acpi_table_handler handler)) +ACPI_EXTERNAL_RETURN_STATUS(acpi_status +acpi_remove_table(acpi_string signature, + u32 instance)) /* * Namespace and name interfaces -- 2.6.2 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/2] ACPICA: Tables: Add function to remove ACPI tables
On Thu, 18 Feb, at 09:15:28PM, Rafael J. Wysocki wrote: > > Actually, the reason is that, as a rule, the process for ACPICA > patches is that they first go to upstream ACPICA and they are acquired > by Linux from there. > > While there are some exceptions from that process, there also are good > reasons for that process to be followed, including the licensing one > mentioned by Lv. > > All that said, Matt, if you agree that the patch can be applied under > the BSD license, I think we can offer help with converting it to the > upstream ACPICA coding conventions and applying it there. Lv, would > you be able to take care of that? I don't have any problem with that, but can we hold off on this patch for now? There's another approach to fixing the BGRT issue with kexec that's being discussed which would supersede this, https://lkml.kernel.org/r/20160218141544.gh2...@codeblueprint.co.uk Assuming this patch does get picked up again, I'm happy to respin it against upstream ACPICA, but how do I go about getting dependent patches merged, PATCH 2/2 in this case? ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: kexec regression since 4.9 caused by efi
On Thu, 09 Mar, at 12:53:36PM, Ard Biesheuvel wrote: > > Hi Omar, > > Thanks for tracking this down. > > I wonder if this is an unintended side effect of the way we repurpose > the EFI_MEMORY_RUNTIME attribute in efi_arch_mem_reserve(). AFAIUI, > splitting memory map entries should only be necessary for regions that > are not runtime memory regions to begin with, and so whether their > virtual mapping address makes sense or not should be irrelevant. > > Perhaps this only illustrates my lack of understanding of the x86 way > of doing this, so perhaps Matt can shed some light on this? Sorry for the delay. Yes, Ard is correct. It's not necessary to split/reserve memory regions that already have the EFI_MEMORY_RUNTIME attribute. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: kexec regression since 4.9 caused by efi
On Mon, 13 Mar, at 03:37:48PM, Dave Young wrote: > > Omar, could you try below patch? Looking at the efi_mem_desc_lookup, it is not > correct to be used in efi_arch_mem_reserve, if it passed your test, I > can rewrite patch log with more background and send it out: > > for_each_efi_memory_desc(md) { > [snip] > if (!(md->attribute & EFI_MEMORY_RUNTIME) && > md->type != EFI_BOOT_SERVICES_DATA && > md->type != EFI_RUNTIME_SERVICES_DATA) { > continue; > } > > In above code, it meant to get a md of EFI_MEMORY_RUNTIME of either boot > data or runtime data, this is wrong for efi_mem_reserve, because we are > reserving boot data which has no EFI_MEMORY_RUNTIME attribute at the > running time. Just is happened to work and we did not capture the error. Wouldn't something like this be simpler? --- diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c index 30031d5293c4..cdfe8c628959 100644 --- a/arch/x86/platform/efi/quirks.c +++ b/arch/x86/platform/efi/quirks.c @@ -201,6 +201,10 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size) return; } + /* No need to reserve regions that will never be freed. */ + if (md.attribute & EFI_MEMORY_RUNTIME) + return; + size += addr % EFI_PAGE_SIZE; size = round_up(size, EFI_PAGE_SIZE); addr = round_down(addr, EFI_PAGE_SIZE); ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: kexec regression since 4.9 caused by efi
On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote: > > Matt, I think it should be fine although I think the md type checking in > efi_mem_desc_lookup() is causing confusion and not easy to understand.. Could you make that a separate patch if you think of improvements there? > How about move the if chunk early like below because it seems no need > to sanity check the addr + size any more if the md is still RUNTIME? My original version did as you suggest, but I changed it because we *really* want to know if someone tries to reserve a range that spans regions. That would be totally unexpected and a warning about a potential bug/issue. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: kexec regression since 4.9 caused by efi
On Mon, 20 Mar, at 10:14:12AM, Dave Young wrote: > > Matt, I'm fine if you prefer to capture the range checking errors. > Would you like me to post it or just you send it out? Can you please send out the patch with the minimal change to efi_arch_mem_reserve() and we'll get it into urgent ASAP. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v5 17/32] x86/mm: Add support to access boot related data in the clear
On Mon, 15 May, at 08:35:17PM, Borislav Petkov wrote: > On Tue, Apr 18, 2017 at 04:19:21PM -0500, Tom Lendacky wrote: > > > + paddr = boot_params.efi_info.efi_memmap_hi; > > + paddr <<= 32; > > + paddr |= boot_params.efi_info.efi_memmap; > > + if (phys_addr == paddr) > > + return true; > > + > > + paddr = boot_params.efi_info.efi_systab_hi; > > + paddr <<= 32; > > + paddr |= boot_params.efi_info.efi_systab; > > So those two above look like could be two global vars which are > initialized somewhere in the EFI init path: > > efi_memmap_phys and efi_systab_phys or so. > > Matt ? > > And then you won't need to create that paddr each time on the fly. I > mean, it's not a lot of instructions but still... We should already have the physical memmap address available in 'efi.memmap.phys_map'. And the physical address of the system table should be in 'efi_phys.systab'. See efi_init(). ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 17/36] efi: Update efi_mem_type() to return an error rather than 0
On Fri, 16 Jun, at 01:53:06PM, Tom Lendacky wrote: > The efi_mem_type() function currently returns a 0, which maps to > EFI_RESERVED_TYPE, if the function is unable to find a memmap entry for > the supplied physical address. Returning EFI_RESERVED_TYPE implies that > a memmap entry exists, when it doesn't. Instead of returning 0, change > the function to return a negative error value when no memmap entry is > found. > > Reviewed-by: Borislav Petkov > Signed-off-by: Tom Lendacky > --- > arch/ia64/kernel/efi.c |4 ++-- > arch/x86/platform/efi/efi.c |6 +++--- > include/linux/efi.h |2 +- > 3 files changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Matt Fleming ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 16/36] efi: Add an EFI table address match function
On Fri, 16 Jun, at 01:52:53PM, Tom Lendacky wrote: > Add a function that will determine if a supplied physical address matches > the address of an EFI table. > > Reviewed-by: Borislav Petkov > Signed-off-by: Tom Lendacky > --- > drivers/firmware/efi/efi.c | 33 + > include/linux/efi.h|7 +++ > 2 files changed, 40 insertions(+) Reviewed-by: Matt Fleming ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 18/36] x86/efi: Update EFI pagetable creation to work with SME
On Fri, 16 Jun, at 01:53:17PM, Tom Lendacky wrote: > When SME is active, pagetable entries created for EFI need to have the > encryption mask set as necessary. > > When the new pagetable pages are allocated they are mapped encrypted. So, > update the efi_pgt value that will be used in cr3 to include the encryption > mask so that the PGD table can be read successfully. The pagetable mapping > as well as the kernel are also added to the pagetable mapping as encrypted. > All other EFI mappings are mapped decrypted (tables, etc.). > > Reviewed-by: Borislav Petkov > Signed-off-by: Tom Lendacky > --- > arch/x86/platform/efi/efi_64.c | 15 +++ > 1 file changed, 11 insertions(+), 4 deletions(-) Reviewed-by: Matt Fleming ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 19/36] x86/mm: Add support to access boot related data in the clear
On Fri, 16 Jun, at 01:53:26PM, Tom Lendacky wrote: > Boot data (such as EFI related data) is not encrypted when the system is > booted because UEFI/BIOS does not run with SME active. In order to access > this data properly it needs to be mapped decrypted. > > Update early_memremap() to provide an arch specific routine to modify the > pagetable protection attributes before they are applied to the new > mapping. This is used to remove the encryption mask for boot related data. > > Update memremap() to provide an arch specific routine to determine if RAM > remapping is allowed. RAM remapping will cause an encrypted mapping to be > generated. By preventing RAM remapping, ioremap_cache() will be used > instead, which will provide a decrypted mapping of the boot related data. > > Signed-off-by: Tom Lendacky > --- > arch/x86/include/asm/io.h |5 + > arch/x86/mm/ioremap.c | 179 > + > include/linux/io.h|2 + > kernel/memremap.c | 20 - > mm/early_ioremap.c| 18 - > 5 files changed, 217 insertions(+), 7 deletions(-) Reviewed-by: Matt Fleming ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [edk2] Corrupted EFI region
On Wed, 18 Sep, at 01:24:14PM, jerry.hoem...@hp.com wrote: > Matt, > > I conducted the following experiments on a 3.11 kernel: Jerry, could you paste your memory map from the kernel log? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 2/6] x86 efi: reserve boot service fix
On Sun, 27 Oct, at 11:50:09AM, Borislav Petkov wrote: > On Sun, Oct 27, 2013 at 11:47:15AM +0800, dyo...@redhat.com wrote: > > Current code check boot service region with kernel text region by: > > start+size >= __pa_symbol(_text) > > The end of the above region should be start + size - 1 instead. > > > > I see this problem in ovmf + Fedora 19 grub boot: > > text start: 100 md start: 80 md size: 80 > > > > Signed-off-by: Dave Young > > Acked-by: Borislav Petkov > > Btw, Matt, this being a bugfix and all, shouldn't it be tagged for > stable? Well that depends. Dave, am I correct in thinking that you only noticed this bug when writing kexec support? I'm inclined not to bother with a stable tag if no one has ever noticed any fallout from this bug until now. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 2/6] x86 efi: reserve boot service fix
On Mon, 28 Oct, at 09:44:41AM, Borislav Petkov wrote: > On Mon, Oct 28, 2013 at 09:18:24AM +0800, Dave Young wrote: > > There should be some people see below message with non-kexec kernel: > > "Could not reserve boot range ..." > > I can find one other report like that: https://lkml.org/lkml/2013/7/16/309 > > [0.00] efi: Could not reserve boot range [0x00-0x000fff] > > for > > efi: mem00: type=3, attr=0xf, range=[0x-0x1000) > (0MB) > > which is EFI_BOOT_SERVICES_CODE and > > efi: Could not reserve boot range [0x05f000-0x09] > > for > > efi: mem06: type=3, attr=0xf, range=[0x0005f000-0x000a) > (0MB) > > which is of the same type. But that doesn't look like the issue that the user was complaining about. Rather, it's that the higher EFI regions are unable to be mapped because of the user's mem= kernel parameter setting. However, even though this bug isn't the main focus of the above report, since people are hitting it (and since the fix is trivial), I agree it's worth tagging for stable. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 0/7 v2] kexec kernel efi runtime support
On Tue, 05 Nov, at 04:20:07PM, dyo...@redhat.com wrote: > Hi, > > Here is the V2 for supporting kexec kernel efi runtime. > Per pervious discussion I pass the 1st kernel efi runtime mapping > via setup_data to 2nd kernel. Besides of the runtime mapping > info I also pass the fw_vendor, runtime, config table, smbios > physical address in setup_data. EFI spec mentioned fw_vendor, > runtime, config table addresses will be converted to virt address > after entering virtual mode, but we will use it as physical address > in efi_init. For smbios EFI spec did not mention about the address > updating, but during my test on a HP workstation, the bios will > convert it to Virt addr, thus pass it in setup_data as well. I see this in the dmesg, [0.00] efi: skipping setup_data on EFI 32BIT! despite the fact that this is on an x86-64 box. Turns out it's because CONFIG_DEBUG_BOOT_PARAMS isn't set in my config. You may want to turn that on automatically. After doing that things work on my ASUS box (good work!) but the SATA controller craps out on my Tunnelmountain machine, but that's probably unrelated and I'll debug that separately. I see a bunch of section mismatch warnings, WARNING: arch/x86/platform/efi/built-in.o(.text+0xd1e): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed() The function efi_map_regions_fixed() references the function __init efi_map_region_fixed(). This is often because efi_map_regions_fixed lacks a __init annotation or the annotation of efi_map_region_fixed is wrong. WARNING: arch/x86/platform/efi/built-in.o(.text+0xd2a): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:efi_phys The function efi_map_regions_fixed() references the variable __initdata efi_phys. This is often because efi_map_regions_fixed lacks a __initdata annotation or the annotation of efi_phys is wrong. WARNING: arch/x86/platform/built-in.o(.text+0xd1e): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed() The function efi_map_regions_fixed() references the function __init efi_map_region_fixed(). This is often because efi_map_regions_fixed lacks a __init annotation or the annotation of efi_map_region_fixed is wrong. WARNING: arch/x86/platform/built-in.o(.text+0xd2a): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:efi_phys The function efi_map_regions_fixed() references the variable __initdata efi_phys. This is often because efi_map_regions_fixed lacks a __initdata annotation or the annotation of efi_phys is wrong. WARNING: arch/x86/built-in.o(.text+0x7c357): Section mismatch in reference from the function parse_efi_setup() to the function .init.text:early_memremap() The function parse_efi_setup() references the function __init early_memremap(). This is often because parse_efi_setup lacks a __init annotation or the annotation of early_memremap is wrong. WARNING: arch/x86/built-in.o(.text+0x7c390): Section mismatch in reference from the function parse_efi_setup() to the function .init.text:early_iounmap() The function parse_efi_setup() references the function __init early_iounmap(). This is often because parse_efi_setup lacks a __init annotation or the annotation of early_iounmap is wrong. WARNING: arch/x86/built-in.o(.text+0x7c5ce): Section mismatch in reference from the function efi_map_regions_fixed() to the function .init.text:efi_map_region_fixed() The function efi_map_regions_fixed() references the function __init efi_map_region_fixed(). This is often because efi_map_regions_fixed lacks a __init annotation or the annotation of efi_map_region_fixed is wrong. WARNING: arch/x86/built-in.o(.text+0x7c5da): Section mismatch in reference from the function efi_map_regions_fixed() to the variable .init.data:vdso32_sysenter_end The function efi_map_regions_fixed() references the variable __initdata vdso32_sysenter_end. This is often because efi_map_regions_fixed lacks a __initdata annotation or the annotation of vdso32_sysenter_end is wrong. Also, many of your patch descriptions are missing subsystem tags. Please fix this in your next submission. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 3/7 v2] Cleanup efi_enter_virtual_mode function
On Tue, 05 Nov, at 04:20:10PM, dyo...@redhat.com wrote: > Add two small functions: > efi_merge_regions and efi_map_regions, efi_enter_virtual_mode > calls them instead of embedding two long for loop. > > v1->v2: > refresh; coding style fixes. > > Signed-off-by: Dave Young > --- > arch/x86/platform/efi/efi.c | 107 > +--- > 1 file changed, 63 insertions(+), 44 deletions(-) [...] > @@ -860,17 +840,59 @@ void __init efi_enter_virtual_mode(void) > efi.systab = (efi_system_table_t *) (unsigned long) > systab; > } > > - new_memmap = krealloc(new_memmap, > - (count + 1) * memmap.desc_size, > - GFP_KERNEL); > - if (!new_memmap) > + *new_memmap = krealloc(*new_memmap, > + (*count + 1) * memmap.desc_size, > + GFP_KERNEL); > + if (!*new_memmap) > goto err_out; > > - memcpy(new_memmap + (count * memmap.desc_size), md, > + memcpy(*new_memmap + (*count * memmap.desc_size), md, > memmap.desc_size); > - count++; > + (*count)++; > } > > +err_out: > + pr_err("Error reallocating memory, EFI runtime non-functional!\n"); > +} You should really return an error value here so that we don't dereference 'new_memmap' below if it's NULL. In fact, just return the memmap... > + > + efi_merge_regions(); > + > + efi_map_regions(&new_memmap, &count); > + and make sure to check the return value here. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 7/7 v2] x86: add xloadflags bit for efi runtime support on kexec
On Tue, 05 Nov, at 04:20:14PM, dyo...@redhat.com wrote: > Old kexec-tools can not load new kernel. The reason is previously kexec-tools > do not fill efi_info in x86 setup header thus efi init fail and switch > to noefi boot. In new kexec-tools it will by default fill efi_info and > pass other efi required infomation to 2nd kernel so kexec kernel efi > initialization will success finally. > > To prevent from breaking userspace, add a new xloadflags bit so kexec-tools > will check the flag and switch to old logic. > > Signed-off-by: Dave Young > --- > arch/x86/boot/header.S|9 - > arch/x86/include/uapi/asm/bootparam.h |1 + > 2 files changed, 9 insertions(+), 1 deletion(-) > > --- linux-2.6.orig/arch/x86/boot/header.S > +++ linux-2.6/arch/x86/boot/header.S > @@ -391,7 +391,14 @@ xloadflags: > #else > # define XLF23 0 > #endif > - .word XLF0 | XLF1 | XLF23 > + > +#if defined(CONFIG_X86_64) && defined(CONFIG_EFI) && defined(CONFIG_KEXEC) ? > +# define XLF4 XLF_EFI_KEXEC > +#else > +# define XLF4 0 > +#endif > + -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 6/7 v2] passing kexec necessary efi data via setup_data
On Tue, 05 Nov, at 04:20:13PM, dyo...@redhat.com wrote: > Add a new setup_data type SETUP_EFI for kexec use. > Passing the saved fw_vendor, runtime, config tables and > efi runtime mappings. > > When entering virtual mode, directly mapping the efi > runtime ragions which we passed in previously. And skip > the step to call SetVirtualAddressMap. > > Specially for HP z420 workstation it need another variable > saving, it's the smbios physical address, the HP bios > also update the SMBIOS address after entering virtual mode > besides of the standard fw_vendor,runtime and config table. Does that mean that on some of the platforms you tested the SMBIOS address isn't updated? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 1/7 v2] Add function efi_remap_region for remapping to saved virt address
On Tue, 05 Nov, at 04:20:08PM, dyo...@redhat.com wrote: > Kexec kernel will use saved runtime virtual mapping, so add a > new function efi_remap_region to remapping it directly without > calculate the virt addr from efi_va. > > The md is passed in from 1st kernel, the virtual addr is > saved in md->virt_addr. > > Signed-off-by: Dave Young > --- > arch/x86/include/asm/efi.h |1 + > arch/x86/platform/efi/efi_32.c |3 +++ > arch/x86/platform/efi/efi_64.c | 19 +++ > 3 files changed, 23 insertions(+) [...] > @@ -203,6 +203,25 @@ void __init efi_map_region(efi_memory_de > md->virt_addr = efi_va; > } > > +/* > + * kexec kernel will use efi_map_region_fixed to map efi > + * runtime memory ranges. md->virt_addr is the original virtual > + * address which had been mapped in kexec 1st kernel. > + */ > +void __init efi_map_region_fixed(efi_memory_desc_t *md) > +{ > + pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd); > + unsigned long pf = 0; > + > + if (!(md->attribute & EFI_MEMORY_WB)) > + pf |= _PAGE_PCD; > + > + if (kernel_map_pages_in_pgd(pgd, md->phys_addr, md->virt_addr, > + md->num_pages, pf)) > + pr_warn("Error mapping PA 0x%llx -> VA 0x%llx!\n", > +md->phys_addr, md->virt_addr); > +} > + This function is almost identical to __map_region(). Please implement efi_map_region_fixed() in terms of __map_region(), e.g. void __init efi_map_region_fixed(efi_memory_desc_t *md) { __map_region(md, md->virt_addr); } > @@ -47,6 +47,9 @@ void __init efi_map_region(efi_memory_de > old_map_region(md); > } > > +void __init efi_map_region_fixed(efi_memory_desc_t *md) > +{} > + The braces should be on the same line as the rest of the function prototype. Look at how it's done elsewhere in this file. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 5/7 v2] export efi runtime memory mapping to sysfs
On Tue, 05 Nov, at 04:20:12PM, dyo...@redhat.com wrote: > kexec kernel will need exactly same mapping for > efi runtime memory ranges. Thus here export the > runtime ranges mapping to sysfs, kexec-tools > will assemble them and pass to 2nd kernel via > setup_data. > > Introducing a new directly /sys/firmware/efi/efi-runtime-map > Just like /sys/firmware/memmap. Containing below attribute > in each file of that directory: > attribute num_pages phys_addr type virt_addr > > It will not work for efi 32bit. Only x86_64 currently. > > Signed-off-by: Dave Young > --- > Documentation/ABI/testing/sysfs-efi-runtime-map | 45 ++ > arch/x86/include/asm/efi.h |3 > arch/x86/platform/efi/efi.c | 11 + > drivers/firmware/efi/Kconfig| 10 + > drivers/firmware/efi/Makefile |1 > drivers/firmware/efi/efi-runtime-map.c | 164 > > drivers/firmware/efi/efi.c |3 > include/linux/efi.h |6 > 8 files changed, 242 insertions(+), 1 deletion(-) [...] > @@ -852,6 +855,14 @@ static void efi_map_regions(void **new_m > > memcpy(*new_memmap + (*count * memmap.desc_size), md, > memmap.desc_size); > + if (md->type != EFI_BOOT_SERVICES_CODE && > + md->type != EFI_BOOT_SERVICES_DATA) { > + efi_runtime_map = krealloc(efi_runtime_map, > + (nr_efi_runtime_map + 1) * > + sizeof(efi_memory_desc_t), GFP_KERNEL); > + *(efi_runtime_map + nr_efi_runtime_map) = *md; > + nr_efi_runtime_map++; > + } > (*count)++; > } You really need to be using 'memmap.desc_size' here and not sizeof(efi_memory_desc_t) as the two may differ. Also, you should be checking for failure of krealloc() and using memcpy() instead of directly dereferencing 'md'. > --- linux-2.6.orig/drivers/firmware/efi/Makefile > +++ linux-2.6/drivers/firmware/efi/Makefile > @@ -4,3 +4,4 @@ > obj-y+= efi.o vars.o > obj-$(CONFIG_EFI_VARS) += efivars.o > obj-$(CONFIG_EFI_VARS_PSTORE)+= efi-pstore.o > +obj-$(CONFIG_EFI_RUNTIME_MAP)+= efi-runtime-map.o > --- /dev/null > +++ linux-2.6/drivers/firmware/efi/efi-runtime-map.c Small nit but I wouldn't bother prefixing the filename with "efi-", since you can't build this file as a module. > +/* > + * These are default attributes that are added for every memmap entry. > + */ > +static struct attribute *def_attrs[] = { > + &map_type_attr.attr, > + &map_phys_addr_attr.attr, > + &map_virt_addr_attr.attr, > + &map_num_pages_attr.attr, > + &map_attribute_attr.attr, > + NULL > +}; If the UEFI spec ever releases an update for the memory descriptor structure, and bumps 'memmap.desc_version', how are we going to signal the incompatibility to legacy versions of kexec tools? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [patch 5/7 v2] export efi runtime memory mapping to sysfs
On Mon, 18 Nov, at 10:16:41AM, Dave Young wrote: > Matt, desc_version is already in boot_params.efi_info, so kexec-tools > can get the version from there. I do not need to export it as another > file. OK, cool. > I think for now we do not need worry much about the compatibility > issue, do you think I need add version checking in kexec-tools > currently? like below? > > if (desc_version != 1) /* current version is 1? */ > error out it is not supported Yes, something like that. If you already have kexec-specific ways of checking a version number then that's fine. Just make sure that, if the EFI memory descriptor structure is extended in the future and more files are exported for each descriptor, kexec-tools say "I don't know how to build a data structure for from these files". -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 04/12] efi: cleanup efi_enter_virtual_mode function
On Tue, 26 Nov, at 01:57:49PM, Dave Young wrote: > Add two small functions: > efi_merge_regions and efi_map_regions, efi_enter_virtual_mode > calls them instead of embedding two long for loop. > > v1->v2: > refresh; coding style fixes. > > v2->v3: > Toshi Kani: > remove unused variable > Matt: check return value of krealloc. > v3->v4: > Boris: Stretch comment to 80 cols > > Signed-off-by: Dave Young > Acked-by: Borislav Petkov > --- > arch/x86/platform/efi/efi.c | 109 > +++- > 1 file changed, 66 insertions(+), 43 deletions(-) [...] > @@ -922,9 +948,6 @@ void __init efi_enter_virtual_mode(void) >0, NULL); > > return; > - > - err_out: > - pr_err("Error reallocating memory, EFI runtime non-functional!\n"); > } > > /* Teeny, tiny comment: You could now delete the above return statement. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 05/12] efi: export more efi table variable to sysfs
On Tue, 26 Nov, at 01:57:50PM, Dave Young wrote: > Export fw_vendor, runtime and config tables physical > addresses to /sys/firmware/efi/systab becaue kexec > kernel will need them. This commit log needs updating. > From EFI spec these 3 variables will be updated to > virtual address after entering virtual mode. But > kernel startup code will need the physical address. > > changelog: > Greg: add standalone sysfs files instead of add lines to systab > Document them as testing ABI > Greg: use group attrs and is_visible > Boris: align comments lines > > Signed-off-by: Dave Young > --- > Documentation/ABI/testing/sysfs-firmware-efi | 26 > arch/x86/platform/efi/efi.c | 4 +++ > drivers/firmware/efi/efi.c | 44 > > include/linux/efi.h | 3 ++ > 4 files changed, 77 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi > > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi > b/Documentation/ABI/testing/sysfs-firmware-efi > new file mode 100644 > index 000..9252a24 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-firmware-efi > @@ -0,0 +1,26 @@ > +What:/sys/firmware/efi/fw_vendor > +Date:Nov 2013 > +Contact: Dave Young redhat.org eh? Typo or legit? > +Description: > + Shows the physical address of firmware vendor in the EFI > + system table. It shows the physical address of the firmware vendor "entry" or "field" in the EFI system table. > +Users: > + Kexec Mailing List The "Kexec mailing list" isn't a user, kexec is. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs
On Tue, 26 Nov, at 01:57:51PM, Dave Young wrote: > kexec kernel will need exactly same mapping for > efi runtime memory ranges. Thus here export the > runtime ranges mapping to sysfs, kexec-tools > will assemble them and pass to 2nd kernel via > setup_data. > > Introducing a new directly /sys/firmware/efi/runtime-map I'm not sure why the word "directly" is used here? > Just like /sys/firmware/memmap. Containing below attribute > in each file of that directory: > attribute num_pages phys_addr type virt_addr > > It will not work for efi 32bit. Only x86_64 currently. Actually, exporting the tables does work for 32-bit, right? It's just that kexec doesn't work for 32-bit EFI? > Matt: s/efi-runtime-map.c/runtime-map.c > change dir name to runtime-map > update to use desc_size in efi_runtime_map > cleaup the code, add function efi_save_runtime_map > improve err handling > > Signed-off-by: Dave Young > --- > .../ABI/testing/sysfs-firmware-efi-runtime-map | 45 + > arch/x86/platform/efi/efi.c| 26 +++ > drivers/firmware/efi/Kconfig | 10 ++ > drivers/firmware/efi/Makefile | 1 + > drivers/firmware/efi/efi.c | 3 +- > drivers/firmware/efi/runtime-map.c | 199 > + > include/linux/efi.h| 6 + > 7 files changed, 289 insertions(+), 1 deletion(-) > create mode 100644 Documentation/ABI/testing/sysfs-firmware-efi-runtime-map > create mode 100644 drivers/firmware/efi/runtime-map.c > > diff --git a/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map > b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map > new file mode 100644 > index 000..dab8d41 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-firmware-efi-runtime-map > @@ -0,0 +1,45 @@ > +What:/sys/firmware/efi/runtime-map/ > +Date:Oct 2013 > +Contact: Dave Young > +Description: > + Switching efi runtime services to virtual mode requires > + that all efi memory ranges which has the runtime attribute > + bit set to be mapped to virtual addresses. > + > + In kexec kernel kernel can not entering virtual mode again > + because there's a limitation that SetVirtualAddressMap can > + only be called once for entering virtual mode. But kexec > + kernel still need maintain same physical address to virtual > + address mapping as the 1st kernel. The mappings are exported > + to sysfs so userspace tools can reassemble them and pass them > + into kexec kernel. How about, "The efi runtime services can only be switched to virtual mode once without rebooting. The kexec kernel must maintain the same physical to virtual address mappings as the first kernel. The mappings are exported to sysfs so userspace tools can reassemble them and pass them into the kexec kernel." ? > + /sys/firmware/efi/runtim-map/ is what kernel export for ^^ runtime-map/ -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data
ns which was passed via setup_data > + * the virt_addr is a fixed addr which was used in > + * 1st kernel of kexec boot. > + */ > +static void __init efi_map_regions_fixed(void) > +{ > + int i; > + unsigned long size; > + efi_memory_desc_t *md; > + u64 end, systab; > + void *p; > + > + efi_runtime_map = kzalloc(nr_efi_runtime_map * memmap.desc_size, > + GFP_KERNEL); > + if (!efi_runtime_map) > + pr_err("Out of memory, EFI runtime on nested kexec > non-functional!\n"); > + > + for (i = 0, p = efi_runtime_map; i < nr_efi_runtime_map; i++) { > + md = esdata->map + i; Heh, I read that as 'esdata->map + 1' the first few times. > + efi_map_region_fixed(md); > + size = md->num_pages << PAGE_SHIFT; > + end = md->phys_addr + size; > + > + systab = (u64) (unsigned long) efi_phys.systab; > + if (md->phys_addr <= systab && systab < end) { > + systab += md->virt_addr - md->phys_addr; > + efi.systab = > + (efi_system_table_t *) (unsigned long) systab; > + } > + if (efi_runtime_map) { > + memcpy(p, md, memmap.desc_size); > + p += memmap.desc_size; > + } Is this if () needed? Is it possible to enter the loop and have 'efi_runtime_map' be NULL? > + } > +} > + > +/* > * This function will switch the EFI runtime services to virtual mode. > * Essentially, we look through the EFI memmap and map every region that > * has the runtime attribute bit set in its memory descriptor into the > @@ -901,6 +1015,10 @@ ret: > * so that we're in a different address space when calling a runtime > * function. For function arguments passing we do copy the PGDs of the > * kernel page table into ->trampoline_pgd prior to each call. > + * > + * Specially for kexec boot efi runtime maps in previous kernel should > + * be passed in via setup_data. In that case runtime ranges will be mapped > + * to fixed virtual addresses exactly same as the ones in previous kernel. > */ > void __init efi_enter_virtual_mode(void) > { > @@ -919,12 +1037,15 @@ void __init efi_enter_virtual_mode(void) > return; > } > > - efi_merge_regions(); > - > - new_memmap = efi_map_regions(&count); > - if (!new_memmap) { > - pr_err("Error reallocating memory, EFI runtime > non-functional!\n"); > - return; > + if (esdata) > + efi_map_regions_fixed(); > + else { > + efi_merge_regions(); > + new_memmap = efi_map_regions(&count); > + if (!new_memmap) { > + pr_err("Error reallocating memory, EFI runtime > non-functional!\n"); > + return; > + } > } > > BUG_ON(!efi.systab); > @@ -932,11 +1053,17 @@ void __init efi_enter_virtual_mode(void) > efi_setup_page_tables(); > efi_sync_low_kernel_mappings(); > > - status = phys_efi_set_virtual_address_map( > - memmap.desc_size * count, > - memmap.desc_size, > - memmap.desc_version, > - (efi_memory_desc_t *)__pa(new_memmap)); > + if (esdata) { > + status = EFI_SUCCESS; > + early_iounmap(esdata, sizeof(struct efi_setup_data) + > + nr_efi_runtime_map * > + sizeof(efi_memory_desc_t)); > + } else > + status = phys_efi_set_virtual_address_map( > + memmap.desc_size * count, > + memmap.desc_size, > + memmap.desc_version, > + (efi_memory_desc_t *)__pa(new_memmap)); > > if (status != EFI_SUCCESS) { > pr_alert("Unable to switch EFI into virtual mode " Please fold the pr_alert() into the else branch so that you don't have to do the 'status = EFI_SUCCESS' dance. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 07/12] efi: passing kexec necessary efi data via setup_data
On Wed, 27 Nov, at 12:52:37PM, Dave Young wrote: > To make it more readable, I will change them like below: > > p = efi_runtime_map; > md = efi_setup->map; > for (i = 0; i < nr_efi_runtime_map; i++) { > [...] > md += 1; > } Actually, md++ is the canonical way to write this. > > > > > + efi_map_region_fixed(md); > > > + size = md->num_pages << PAGE_SHIFT; > > > + end = md->phys_addr + size; > > > + > > > + systab = (u64) (unsigned long) efi_phys.systab; > > > + if (md->phys_addr <= systab && systab < end) { > > > + systab += md->virt_addr - md->phys_addr; > > > + efi.systab = > > > + (efi_system_table_t *) (unsigned long) systab; > > > + } > > > + if (efi_runtime_map) { > > > + memcpy(p, md, memmap.desc_size); > > > + p += memmap.desc_size; > > > + } > > > > Is this if () needed? Is it possible to enter the loop and have > > 'efi_runtime_map' be NULL? > > Yes, it is needed. if efi_runtime_map kmalloc fails I only print error, do not > return so kernel can still boot, just kexec on efi will not work that has been > put in the error message. OK. On second thought, is there any way to turn the above code into a call to efi_save_runtime_map()? Because you've basically duplicated that code and I can definitely envisage the two code paths fragmenting over time, e.g. when someone makes changes to one but not the other. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 08/12] efi: only print saved efi runtime maps instead of all memmap ranges for kexec
On Tue, 26 Nov, at 01:57:53PM, Dave Young wrote: > For kexec/kdump kernel efi runtime mappings are saved, printing original whole > memmap ranges does not make sense anymore. So introduce a new function to only > print runtime maps in case kexec/kdump kernel is used. > > Signed-off-by: Dave Young > --- > arch/x86/platform/efi/efi.c | 23 ++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > index fafeb40..c65b0b8 100644 > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c > @@ -430,6 +430,24 @@ int __init efi_memblock_x86_reserve_range(void) > return 0; > } > > +/* for kexec kernel runtime maps are passed in setup_data */ > +static void __init print_saved_runtime_map(void) > +{ > +#ifdef EFI_DEBUG > + int i; > + efi_memory_desc_t *md; > + > + for (i = 0; i < nr_efi_runtime_map; i++) { > + md = esdata->map + i; > + pr_info("mem%02u: type=%u, attr=0x%llx, " > + "range=[0x%016llx-0x%016llx) (%lluMB)\n", > + i, md->type, md->attribute, md->phys_addr, > + md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT), > + (md->num_pages >> (20 - EFI_PAGE_SHIFT))); > + } > +#endif /* EFI_DEBUG */ > +} > + > static void __init print_efi_memmap(void) > { > #ifdef EFI_DEBUG > @@ -782,7 +800,10 @@ void __init efi_init(void) > x86_platform.set_wallclock = efi_set_rtc_mmss; > } > #endif > - print_efi_memmap(); > + if (esdata) > + print_saved_runtime_map(); > + else > + print_efi_memmap(); > } Heh, you can probably already guess what I'm going to say here... How about using a single function to dump the memory ranges irrespective of whether the memory map comes from 'memmap' or 'esdata'? e.g. something along the lines of, if (esdata) print_efi_memmap(esdata->map, nr_efi_runtime_map, sizeof(esdata->map[0])); else print_efi_memmap(memmap.map, memmap.nr_map, memmap.desc_size); ? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 10/12] x86: export x86 boot_params to sysfs
On Tue, 26 Nov, at 01:57:55PM, Dave Young wrote: > +Users: > + Kexec Mailing List "Kexec" please. > +static ssize_t version_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) This is pretty strange indentation. Usually we prefer to line up the arguments like you've done elsewhere in this file. > +{ > + return sprintf(buf, "0x%04x\n", boot_params.hdr.version); > +} > + > +static struct kobj_attribute boot_params_version_attr = __ATTR_RO(version); > + > +static ssize_t boot_params_data_read(struct file *fp, struct kobject *kobj, > + struct bin_attribute *bin_attr, > + char *buf, loff_t off, size_t count) > +{ > + memcpy(buf, (void *)&boot_params + off, count); > + return count; > +} Don't we need some checks here to ensure we don't read past the end of boot_params? > +static ssize_t type_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) More whacky indentation. > +static int __init create_setup_data_node(struct kobject *parent, > + struct kobject **kobjp, int nr) Funky indentation. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 00/12] kexec kernel efi runtime support
On Tue, 26 Nov, at 01:57:45PM, Dave Young wrote: > Hi, > > Here is the V4 resend for supporting kexec kernel efi runtime. > Per pervious discussion I pass the 1st kernel efi runtime mapping > via setup_data to 2nd kernel. Besides of the runtime mapping > info I also pass the fw_vendor, runtime, config table, smbios > physical address in setup_data. EFI spec mentioned fw_vendor, > runtime, config table addresses will be converted to virt address > after entering virtual mode, but we will use it as physical address > in efi_init. For smbios EFI spec did not mention about the address > updating, but during my test on a HP workstation, the bios will > convert it to Virt addr, thus pass it in setup_data as well. Dave, your commits introduce a bunch of new sparse warnings, /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15: warning: incorrect type in assignment (different address spaces) /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15:expected struct setup_data *sdata /home/build/git/efi/arch/x86/platform/efi/efi.c:130:15:got void [noderef] * /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23: warning: incorrect type in argument 1 (different address spaces) /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23:expected void [noderef] *addr /home/build/git/efi/arch/x86/platform/efi/efi.c:140:23:got struct setup_data *sdata /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16: warning: incorrect type in assignment (different address spaces) /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16:expected struct efi_setup_data *[addressable] [toplevel] esdata /home/build/git/efi/arch/x86/platform/efi/efi.c:143:16:got void [noderef] * /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20: warning: incorrect type in assignment (different address spaces) /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20:expected void *tablep /home/build/git/efi/arch/x86/platform/efi/efi.c:701:20:got void [noderef] * /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23: warning: incorrect type in argument 1 (different address spaces) /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23:expected void [noderef] *addr /home/build/git/efi/arch/x86/platform/efi/efi.c:722:23:got void *tablep /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31: warning: incorrect type in argument 1 (different address spaces) /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31:expected void [noderef] *addr /home/build/git/efi/arch/x86/platform/efi/efi.c:1079:31:got struct efi_setup_data *[addressable] [toplevel] esdata -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 06/12] efi: export efi runtime memory mapping to sysfs
On Fri, 29 Nov, at 12:50:29PM, Borislav Petkov wrote: > > I did not add KEXEC because I thought it can be used for debugging > > purpose. > > mfleming is thinking about it :) I finally finished my thought. The sysfs code should be automatically enabled only if CONFIG_KEXEC is enabled, and not on my default because it is a debug interface. While it's true that the code may be useful for debugging the memory mappings of EFI systems, it doesn't make sense to always turn it on just for CONFIG_X86 and CONFIG_EFI. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v5 01/14] x86/mm: sparse warning fix for early_memremap
On Tue, 10 Dec, at 10:12:21AM, Dave Young wrote: > On 12/09/13 at 04:05pm, Borislav Petkov wrote: > > On Mon, Dec 09, 2013 at 05:42:14PM +0800, Dave Young wrote: > > > There's a lot of sparse warnings for code like below: > > > void *a = early_memremap(phys_addr, size); > > > > > > early_memremap intend to map kernel memory with ioremap facility, the > > > return > > > pointer should be a kernel ram pointer instead of iomem one. > > > > > > For making the function clearer and supressing sparse warnings this patch > > > do below two things: > > > 1. cast to (__force void *) for the return value of early_memremap > > > > I'd guess this is to shut up the __iomem thing? And we're getting that > > because we're using ioremap, ... hohum... > > Yes, IMHO early_memremap really should not return __iomem pointer since it's > *memremap*... This needs reviewing by at least one of the x86 folks, but it certainly makes sense to me. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v5 02/14] efi: use early_memremap and early_memunmap
pr_err("Could not map the memory map!\n"); > @@ -656,14 +656,14 @@ void __init efi_init(void) > /* >* Show what we know for posterity >*/ > - c16 = tmp = early_ioremap(efi.systab->fw_vendor, 2); > + c16 = tmp = early_memremap(efi.systab->fw_vendor, 2); > if (c16) { > for (i = 0; i < sizeof(vendor) - 1 && *c16; ++i) > vendor[i] = *c16++; > vendor[i] = '\0'; > } else > pr_err("Could not map the firmware vendor!\n"); > - early_iounmap(tmp, 2); > + early_memunmap(tmp, 2); > > pr_info("EFI v%u.%.02u by %s\n", > efi.systab->hdr.revision >> 16, > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > index 2e2fbde..b716a66 100644 > --- a/drivers/firmware/efi/efi.c > +++ b/drivers/firmware/efi/efi.c > @@ -253,7 +253,7 @@ int __init efi_config_init(efi_config_table_type_t > *arch_tables) > if (table64 >> 32) { > pr_cont("\n"); > pr_err("Table located above 4GB, disabling > EFI.\n"); > - early_iounmap(config_tables, > + early_memunmap(config_tables, > efi.systab->nr_tables * sz); > return -EINVAL; > } > @@ -269,6 +269,6 @@ int __init efi_config_init(efi_config_table_type_t > *arch_tables) > tablep += sz; > } > pr_cont("\n"); > - early_iounmap(config_tables, efi.systab->nr_tables * sz); > + early_memunmap(config_tables, efi.systab->nr_tables * sz); > return 0; > } > -- > 1.8.3.1 > -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v5 02/14] efi: use early_memremap and early_memunmap
On Wed, 11 Dec, at 12:02:27PM, Leif Lindholm wrote: > On Wed, Dec 11, 2013 at 10:39:03AM +0000, Matt Fleming wrote: > > Leif, Mark, does this patch look OK for ARM? We'd need to introduce a > > new early_memunmap() function so that things still build, but that > > should be straight forward. You'd even be able to get rid of the > > asymmetry in uefi_init() where you map efi.systab with early_memremap() > > but unmap it with early_iounmap(). > > This patch looks splendid for ARM. As long as Mark can get the new > function into an update to the early_*remap() set, I can include this > in my next version - which is coming this week. Great! Oh and ia64 would also need early_memunmap(). Tony, the original patch is here, http://article.gmane.org/gmane.linux.kernel.kexec/10383 -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v5 09/14] efi: passing kexec necessary efi data via setup_data
On Mon, 09 Dec, at 05:42:22PM, Dave Young wrote: > Add a new setup_data type SETUP_EFI for kexec use. > Passing the saved fw_vendor, runtime, config tables and efi runtime mappings. > > When entering virtual mode, directly mapping the efi runtime ragions which > we passed in previously. And skip the step to call SetVirtualAddressMap. > > Specially for HP z420 workstation we need save the smbios physical address. > The kernel boot sequence proceeds in the following order. Step 2 > requires efi.smbios to be the physical address. However, I found that on > HP z420 EFI system table has a virtual address of SMBIOS in step 1. Hence, > we need set it back to the physical address with the smbios in > efi_setup_data. (When it is still the physical address, it simply sets > the same value.) > > 1. efi_init() - Set efi.smbios from EFI system table > 2. dmi_scan_machine() - Temporary map efi.smbios to access SMBIOS table > 3. efi_enter_virtual_mode() - Map EFI ranges > > Tested on ovmf+qemu, lenovo thinkpad, a dell laptop and an > HP z420 workstation. > > v2: refresh based on previous patch changes, code cleanup. > v3: use ioremap instead of phys_to_virt for efi_setup > v5: improve some code structure per comments from Matt > Boris: improve code structure, spell fix, etc. > Improve changelog from Toshi. > change the variable efi_setup to the physical address of efi setup_data > instead of the ioremapped virt address > > Signed-off-by: Dave Young > --- > arch/x86/include/asm/efi.h| 11 ++ > arch/x86/include/uapi/asm/bootparam.h | 1 + > arch/x86/kernel/setup.c | 3 + > arch/x86/platform/efi/efi.c | 195 > ++ > 4 files changed, 187 insertions(+), 23 deletions(-) [...] > @@ -115,6 +116,25 @@ static int __init setup_storage_paranoia(char *arg) > } > early_param("efi_no_storage_paranoia", setup_storage_paranoia); > > +void __init parse_efi_setup(u64 phys_addr) > +{ > + struct setup_data *sd; > + > + if (!efi_enabled(EFI_64BIT)) { > + pr_warn("SETUP_EFI not supported on 32-bit\n"); > + return; > + } > + > + sd = early_memremap(phys_addr, sizeof(struct setup_data)); > + if (!sd) { > + pr_warn("efi: early_memremap setup_data failed\n"); You shouldn't need the "efi:" prefix in the message. > @@ -676,6 +766,8 @@ void __init efi_init(void) > efi.systab->hdr.revision >> 16, > efi.systab->hdr.revision & 0x, vendor); > > + efi_reuse_config(efi.systab->tables, efi.systab->nr_tables); > + Please check the return value. > if (efi_config_init(arch_tables)) > return; > > @@ -886,6 +978,50 @@ out_krealloc: > } > > /* > + * Map efi regions which was passed via setup_data. The virt_addr is a fixed > + * addr which was used in first kernel in case kexec boot. > + */ > +static int __init map_regions_fixed(void) > +{ > + int i, s, ret = 0; > + u64 end, systab; > + unsigned long size; > + efi_memory_desc_t *md; > + struct efi_setup_data *data; > + > + s = sizeof(*data) + nr_efi_runtime_map * sizeof(data->map[0]); > + data = early_memremap(efi_setup, s); > + if (!data) { > + ret = -ENOMEM; > + goto out; > + } > + for (i = 0, md = data->map; i < nr_efi_runtime_map; i++, md++) { > + efi_map_region_fixed(md); /* FIXME: add error handling */ Oops. Please fix this ;-) -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v5 00/14] kexec kernel efi runtime support
On Tue, 10 Dec, at 03:38:01PM, H. Peter Anvin wrote: > My intent was to pick them into a separate branch of the tip tree either > directly or via Matt (since Matt feeds us anyway.) > > But yes, work is still underway, although it is getting there. Yeah, I've only found a few minor nits this time. I'll merge this soon - presumably after the next revision. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v5 08/14] efi: export efi runtime memory mapping to sysfs
t; } - -subsys_initcall(efi_runtime_map_init); diff --git a/include/linux/efi.h b/include/linux/efi.h index 4f1651d303b8..47e067044054 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -873,9 +873,8 @@ int efivars_sysfs_init(void); #endif /* CONFIG_EFI_VARS */ #ifdef CONFIG_EFI_RUNTIME_MAP -extern void *efi_runtime_map; -extern int nr_efi_runtime_map; -extern struct kobject *efi_kobj; +int efi_runtime_map_init(struct kobject *); +int efi_runtime_map_setup(void *, int, u32); #endif #endif /* _LINUX_EFI_H */ -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 08/14] efi: export efi runtime memory mapping to sysfs
On Mon, 16 Dec, at 05:30:29PM, Dave Young wrote: > @@ -899,6 +928,11 @@ void __init efi_enter_virtual_mode(void) > return; > } > > +#ifdef CONFIG_EFI_RUNTIME_MAP > + efi_runtime_map_setup(efi_runtime_map, nr_efi_runtime_map, > + boot_params.efi_info.efi_memdesc_size); > +#endif [...] > @@ -167,6 +167,12 @@ static int __init efisubsys_init(void) > goto err_unregister; > } > > +#ifdef CONFIG_EFI_RUNTIME_MAP > + error = efi_runtime_map_init(efi_kobj); > + if (error) > + goto err_remove_group; > +#endif [...] > @@ -876,4 +876,9 @@ int efivars_sysfs_init(void); > > #endif /* CONFIG_EFI_VARS */ > > +#ifdef CONFIG_EFI_RUNTIME_MAP > +int efi_runtime_map_init(struct kobject *); > +void efi_runtime_map_setup(void *, int, u32); > +#endif I was thinking more along the lines of... --- diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 120086820654..fd5a0aad1fff 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -928,10 +928,8 @@ void __init efi_enter_virtual_mode(void) return; } -#ifdef CONFIG_EFI_RUNTIME_MAP efi_runtime_map_setup(efi_runtime_map, nr_efi_runtime_map, boot_params.efi_info.efi_memdesc_size); -#endif BUG_ON(!efi.systab); diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 4109dca787dc..4753bac65279 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -167,11 +167,9 @@ static int __init efisubsys_init(void) goto err_unregister; } -#ifdef CONFIG_EFI_RUNTIME_MAP error = efi_runtime_map_init(efi_kobj); if (error) goto err_remove_group; -#endif /* and the standard mountpoint for efivarfs */ efivars_kobj = kobject_create_and_add("efivars", efi_kobj); diff --git a/include/linux/efi.h b/include/linux/efi.h index a98eb0621583..e64540746c63 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -875,6 +875,14 @@ int efivars_sysfs_init(void); #ifdef CONFIG_EFI_RUNTIME_MAP int efi_runtime_map_init(struct kobject *); void efi_runtime_map_setup(void *, int, u32); +#else +static inline int efi_runtime_map_init(struct kobject *kobj) +{ + return 0; +} + +static inline void +efi_runtime_map_setup(void *map, int nr_entries, u32 desc_size) {} #endif #endif /* _LINUX_EFI_H */ -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 14/14] x86: kdebugfs do not use __va for getting setup_data virt addr
On Mon, 16 Dec, at 05:30:35PM, Dave Young wrote: > kdump kernel will use memmap=exactmap kernel cmdline, but __va does not > work in case memmap=exactmap, so let's always use ioremap_cache. > > Signed-off-by: Dave Young > --- > arch/x86/kernel/kdebugfs.c | 35 +++ > 1 file changed, 11 insertions(+), 24 deletions(-) Dave, I've no idea why this change is necessary from the commit log. Is it required for kexec to function on EFI? Why does __va() not work in the memmap=exactmap case? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 14/14] x86: kdebugfs do not use __va for getting setup_data virt addr
On Tue, 17 Dec, at 02:53:46PM, Dave Young wrote: > On 12/17/13 at 02:24pm, Dave Young wrote: > > On 12/16/13 at 04:35pm, Matt Fleming wrote: > > > On Mon, 16 Dec, at 05:30:35PM, Dave Young wrote: > > > > kdump kernel will use memmap=exactmap kernel cmdline, but __va does not > > > > work in case memmap=exactmap, so let's always use ioremap_cache. > > > > > > > > Signed-off-by: Dave Young > > > > --- > > > > arch/x86/kernel/kdebugfs.c | 35 +++ > > > > 1 file changed, 11 insertions(+), 24 deletions(-) > > > > > > Dave, I've no idea why this change is necessary from the commit log. Is > > > it required for kexec to function on EFI? Why does __va() not work in > > > the memmap=exactmap case? > > > > > > > During previous kdump tests I saw panics while reading the setup_data in > > debugfs. > > I thought it is caused by some unmapped addresses. At that time I also > > reproduced > > it by booting the non-kexec kernel with memmap=exact. > > > > Since you are asking about this I'm testing it again but I seems can not > > reproduce this problem any more, it's weird. > > > > I should dug more about it and save the panic messages. > > > > So let's drop this patch for now, I will keep an eye on this and address it > > later > > if I can find the problem again. > > Reproduced it again in normal boot with memmap=exactmap, but I agree it's a > corner > case, I remember I saw this in kdump kernel, but I still did not see it in > today's > testing so I think move this issue out of this patch series is fine to me. Please include the following oops when you submit this patch again. > [0.359467] BUG: unable to handle kernel paging request at 8800d5592020 > [0.359472] IP: [] arch_kdebugfs_init+0x11a/0x201 > [0.359474] PGD 316b067 PUD 37031063 PMD 0 > [0.359476] Oops: [#1] PREEMPT SMP > [0.359477] Modules linked in: > [0.359480] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 3.13.0-rc3+ #65 > [0.359481] Hardware name: Hewlett-Packard HP Z420 Workstation/1589, BIOS > J61 v03.15 05/09/2013 > [0.359482] task: 880037022000 ti: 880037188000 task.ti: > 880037188000 > [0.359484] RIP: 0010:[] [] > arch_kdebugfs_init+0x11a/0x201 > [0.359485] RSP: :880037189e30 EFLAGS: 00010286 > [0.359486] RAX: 8800372e30e0 RBX: 8800372e30e0 RCX: > 0191 > [0.359486] RDX: RSI: 8168a39e RDI: > 880037189e50 > [0.359487] RBP: 880037189e90 R08: 8800372e30e0 R09: > > [0.359488] R10: R11: R12: > 88003640aea0 > [0.359489] R13: d5592018 R14: 88003640b200 R15: > 8800d5592018 > [0.359490] FS: () GS:88003748() > knlGS: > [0.359491] CS: 0010 DS: ES: CR0: 80050033 > [0.359491] CR2: 8800d5592020 CR3: 02716000 CR4: > 000407e0 > [0.359492] Stack: > [0.359494] 817da6d5 88003640b0e0 88003640afc0 > 0004 > [0.359496] 880037180033 8149d82d c9b42d58 > 817dbca2 > [0.359498] 00f3 > > [0.359499] Call Trace: > [0.359502] [] ? boot_params_ksysfs_init+0x1ff/0x244 > [0.359507] [] ? mutex_unlock+0x9/0xb > [0.359508] [] ? topology_init+0x36/0x36 > [0.359512] [] do_one_initcall+0xae/0x15b > [0.359515] [] ? parameq+0xb/0x1f > [0.359516] [] ? parse_args+0x25c/0x33a > [0.359519] [] kernel_init_freeable+0x115/0x19b > [0.359521] [] ? do_early_param+0x88/0x88 > [0.359523] [] ? rest_init+0xbd/0xbd > [0.359524] [] kernel_init+0x9/0xfa > [0.359527] [] ret_from_fork+0x7c/0xb0 > [0.359528] [] ? rest_init+0xbd/0xbd > [0.359548] Code: ff 48 85 c0 48 89 c3 0f 84 b9 00 00 00 49 bf 00 00 00 00 > 00 88 ff ff 4c 89 28 8b 55 bc 48 8d 7d c0 4d 01 ef 48 c > [0.359549] RIP [] arch_kdebugfs_init+0x11a/0x201 > [0.359550] RSP > [0.359550] CR2: 8800d5592020 > [0.359559] ---[ end trace d52b3fbe64a26115 ]--- > [0.621287] kworker/u8:0 (43) used greatest stack depth: 5232 bytes left > [0.628021] Kernel panic - not syncing: Attempted to kill init! > exitcode=0x0009 > [0.628021] -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 10/14] efi: only print saved efi runtime maps instead of all memmap ranges for kexec
On Mon, 16 Dec, at 05:30:31PM, Dave Young wrote: > For kexec/kdump kernel efi runtime mappings are saved, printing original whole > memmap ranges does not make sense anymore. So introduce a new function to only > print runtime maps in case kexec/kdump kernel is used. > > changelog: > Matt: use efi_setup instead of esdata > share function print_efi_memmap for both normal and kexec boot. > > Signed-off-by: Dave Young > --- > arch/x86/platform/efi/efi.c | 24 +--- > 1 file changed, 17 insertions(+), 7 deletions(-) Thinking about this a bit more, I'm not at all sure why this patch exists. Why do we not want to print the entire memory map range like we did in the first kernel? The e820 map is printed exactly like it was in the first kernel, why should the EFI memmap be special? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 09/12] efi: passing kexec necessary efi data via setup_data
ap((unsigned long)phys, sizeof(*systab64)); if (systab64 == NULL) { pr_err("Couldn't map the system table!\n"); + if (data) + early_iounmap(data, sizeof(*data)); return -ENOMEM; } efi_systab.hdr = systab64->hdr; - efi_systab.fw_vendor = systab64->fw_vendor; - tmp |= systab64->fw_vendor; + efi_systab.fw_vendor = data ? (unsigned long)data->fw_vendor : + systab64->fw_vendor; + tmp |= data ? data->fw_vendor : systab64->fw_vendor; efi_systab.fw_revision = systab64->fw_revision; efi_systab.con_in_handle = systab64->con_in_handle; tmp |= systab64->con_in_handle; @@ -519,15 +529,20 @@ static int __init efi_systab_init(void *phys) tmp |= systab64->stderr_handle; efi_systab.stderr = systab64->stderr; tmp |= systab64->stderr; - efi_systab.runtime = (void *)(unsigned long)systab64->runtime; - tmp |= systab64->runtime; + efi_systab.runtime = data ? +(void *)(unsigned long)data->runtime : +(void *)(unsigned long)systab64->runtime; + tmp |= data ? data->runtime : systab64->runtime; efi_systab.boottime = (void *)(unsigned long)systab64->boottime; tmp |= systab64->boottime; efi_systab.nr_tables = systab64->nr_tables; - efi_systab.tables = systab64->tables; - tmp |= systab64->tables; + efi_systab.tables = data ? (unsigned long)data->tables : + systab64->tables; + tmp |= data ? data->tables : systab64->tables; early_iounmap(systab64, sizeof(*systab64)); + if (data) + early_iounmap(data, sizeof(*data)); #ifdef CONFIG_X86_32 if (tmp >> 32) { pr_err("EFI data located above 4GB, disabling EFI.\n"); -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 09/12] efi: passing kexec necessary efi data via setup_data
On Fri, 20 Dec, at 06:02:19PM, Dave Young wrote: > @@ -133,6 +133,19 @@ extern void efi_sync_low_kernel_mappings(void); > extern void efi_setup_page_tables(void); > extern void __init old_map_region(efi_memory_desc_t *md); > > +struct efi_setup_data { > + u64 fw_vendor; > + u64 runtime; > + u64 tables; > + u64 smbios; > + u64 reserved[8]; > + efi_memory_desc_t map[0]; > +}; [...] > +static void get_nr_runtime_map(void) > +{ > + if (!efi_setup) > + return; > + > + nr_efi_runtime_map = (efi_data_len - sizeof(struct efi_setup_data)) / > + sizeof(efi_memory_desc_t); > +} Do we actually need the 'map' entry in efi_setup_data now that you're passing it via efi_info (which is much better approach!)? Also, we don't need the global nr_efi_runtime_map or efi_runtime_map variables now, right? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 00/12] kexec kernel efi runtime support
On Fri, 20 Dec, at 06:02:10PM, Dave Young wrote: > Here is the V7 patchset for supporting kexec kernel efi runtime. > Per pervious discussion I pass the 1st kernel efi runtime mapping > via setup_data to 2nd kernel. Besides of the runtime mapping > info I also pass the fw_vendor, runtime, config table, smbios > physical address in setup_data. EFI spec mentioned fw_vendor, > runtime, config table addresses will be converted to virt address > after entering virtual mode, but we will use it as physical address > in efi_init. For smbios EFI spec did not mention about the address > updating, but during my test on a HP workstation, the bios will > convert it to Virt addr, thus pass it in setup_data as well. OK Dave, I've pulled patches 3-12 into the 'kexec' branch at, git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git I plan on sending this branch to the tip folks this week for further testing. Please have a look and make sure I haven't messed anything up when dropping the first two patches (though they were very small and trival). Toshi, if you could grab that branch and give it a test, that'd be excellent. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 00/12] kexec kernel efi runtime support
On Sun, 22 Dec, at 02:27:01PM, Toshi Kani wrote: > > The kexec branch is missing the following change, which is required for > fast reboot with multi-cpus. > >commit 279f1df915c3a6ed3075d98a849705bf53851f99 >Author: Vivek Goyal >Date: Tue Nov 26 10:25:28 2013 +0800 > >kexec: migrate to reboot cpu > > With this change added, I confirmed that the branch kernel works fine. I can't find that commit in Linus' tree. Where is it from? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 09/12] efi: passing kexec necessary efi data via setup_data
On Mon, 23 Dec, at 10:09:58AM, Dave Young wrote: > On 12/21/13 at 04:06pm, Matt Fleming wrote: > > On Fri, 20 Dec, at 06:02:19PM, Dave Young wrote: > > > @@ -133,6 +133,19 @@ extern void efi_sync_low_kernel_mappings(void); > > > extern void efi_setup_page_tables(void); > > > extern void __init old_map_region(efi_memory_desc_t *md); > > > > > > +struct efi_setup_data { > > > + u64 fw_vendor; > > > + u64 runtime; > > > + u64 tables; > > > + u64 smbios; > > > + u64 reserved[8]; > > > + efi_memory_desc_t map[0]; > > > +}; > > > > [...] > > > > > +static void get_nr_runtime_map(void) > > > +{ > > > + if (!efi_setup) > > > + return; > > > + > > > + nr_efi_runtime_map = (efi_data_len - sizeof(struct efi_setup_data)) / > > > + sizeof(efi_memory_desc_t); > > > +} > > > > Do we actually need the 'map' entry in efi_setup_data now that you're > > passing it via efi_info (which is much better approach!)? Also, we don't > > need the global nr_efi_runtime_map or efi_runtime_map variables now, > > right? > > The map is still necessary because we need store the map somewhere and pass > the physicall address to kexec kernel. Passing them in setup_data is the > only better way currently... > > In efi_info there's only an entry for the map physical address, the original > map area is not valid any more. Where do you dereference efi_setup_data.map* in the kexec kernel? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 09/12] efi: passing kexec necessary efi data via setup_data
On Wed, 25 Dec, at 11:32:05AM, Dave Young wrote: > > Matt, if you want to remove the map[0] please fold below one line > patch? I have no strong opinion. > > Index: linux/arch/x86/include/asm/efi.h > === > --- linux.orig/arch/x86/include/asm/efi.h > +++ linux/arch/x86/include/asm/efi.h > @@ -139,7 +139,6 @@ struct efi_setup_data { > u64 tables; > u64 smbios; > u64 reserved[8]; > - efi_memory_desc_t map[0]; > }; > > extern u64 efi_setup; Thanks Dave, I will fold this in. I would suggest updating your kexec-tools patches so that the two structures are in sync. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v7 00/12] kexec kernel efi runtime support
On Thu, 02 Jan, at 10:42:56AM, Dave Young wrote: > > Hi, Matt > > randconfig build robot reports several problems: > 1. sparse warnings which should be fixed by the early_memremap patches Yeah, this will be fixed up when Mark's memremap patch series gets merged. > Here is the fix for 2. and 3, please take a look. I'm not sure if I > should resend the patches or leave them to you. Please send these as separate patches and include the compiler errors in the commit message. I'll pick them up and send them to Peter. > build fix: move parse_efi_setup to efi*.c, call it in efi_init instead in > setup.c Why have you moved the call site for parse_efi_setup()? What's the rationale? Parsing SETUP_* entries outside of parse_setup_data() seems to me to be a step backwards in terms of clarity. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH tip/efi-kexec] x86: setup.c build fix
On Fri, 03 Jan, at 11:56:49AM, Dave Young wrote: > In case without CONFIG_EFI, there will be below build error: >arch/x86/built-in.o: In function `setup_arch': > >> (.init.text+0x9dc): undefined reference to `parse_efi_setup' > > Thus fix it by adding blank inline function in asm/efi.h > Also remove an unused declaration for variable efi_data_len. > > Signed-off-by: Dave Young > --- > arch/x86/include/asm/efi.h |5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) Thanks Dave. I picked up both patches. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH tip/efi-kexec] x86: setup.c build fix
On Fri, 03 Jan, at 02:24:28PM, H. Peter Anvin wrote: > > Something I should be pulling? Yep, I'll be sending the pull request momentarily. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 4/4] x86: Pass memory range via E820 for kdump
On Fri, 14 Mar, at 10:47:26AM, Dave Young wrote: > > Can you test with matt's tree to see if it works? > If it still happens please post the full log. So that'd be the 'next' branch at, git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git which contains Borislav's fixes for the EFI memmap code. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 4/4] x86: Pass memory range via E820 for kdump
On Sat, 15 Mar, at 03:26:25PM, Borislav Petkov wrote: > > Btw, Matt, your whole efi/next stuff is already in tip, right? Because > if so, Linn could simply test latest tip/master. Yep, everything is in tip/master. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH 00/13][V3] kexec: A new system call to allow in kernel loading
On 6 June 2014 21:37, H. Peter Anvin wrote: > > OK... this is seriously problematic. > > #if defined(CONFIG_RELOCATABLE) && defined(CONFIG_X86_64) && \ > !defined(CONFIG_EFI_MIXED) >/* kernel/boot_param/ramdisk could be loaded above 4g */ > # define XLF1 XLF_CAN_BE_LOADED_ABOVE_4G > #else > # define XLF1 0 > #endif > > The fact that even compiling with CONFIG_EFI_MIXED disables > XLF_CAN_BE_LOADED_ABOVE_4G is really not going to fly. We should expect > CONFIG_EFI_MIXED to be the norm, but *also* should expect that there is > a legitimate need to load above 4G. > > Matt, could you explain why this is necessary? We need to figure out a > way around this. > > My thinking is that disabling this flag is unnecessary, since a 32-bit > EFI loader should not load above the 4G mark anyway, but if I'm confused > and there is a more fundamental requirement, then we need to consider > that more carefully. No, your comments are absolutely correct. I was the one who was confused. I found this in the git history, commit 7d453eee36ae Author: Matt Fleming Date: Fri Jan 10 18:52:06 2014 + x86/efi: Wire up CONFIG_EFI_MIXED Add the Kconfig option and bump the kernel header version so that boot loaders can check whether the handover code is available if they want. The xloadflags field in the bzImage header is also updated to reflect that the kernel supports both entry points by setting both of XLF_EFI_HANDOVER_32 and XLF_EFI_HANDOVER_64 when CONFIG_EFI_MIXED=y. XLF_CAN_BE_LOADED_ABOVE_4G is disabled so that the kernel text is guaranteed to be addressable with 32-bits. As you've pointed out above, a 32-bit loader is never going to load the kernel above 4G, so we don't need to disable it. What's the best way to fix this up? Just undo the change from the above commit? ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH 00/13][V3] kexec: A new system call to allow in kernel loading
On 6 June 2014 22:00, H. Peter Anvin wrote: > > Yes, presumably (as a separate patch since the actual commit is quite > large.) The patch needs to have a good description why the original > patch was wrong. Right. I'll take a look at this in the morning. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 15/15] kexec: Support kexec/kdump on EFI systems
On Thu, 26 Jun, at 04:33:44PM, Vivek Goyal wrote: > This patch does two thigns. It passes EFI run time mappings to second > kernel in bootparams efi_info. Second kernel parse this info and create > new mappings in second kernel. That means mappings in first and second > kernel will be same. This paves the way to enable EFI in kexec kernel. > > This patch also prepares and passes EFI setup data through bootparams. > This contains bunch of information about various tables and their > addresses. > > These information gathering and passing has been written along the lines > of what current kexec-tools is doing to make kexec work with UEFI. > > Signed-off-by: Vivek Goyal > CC: linux-...@vger.kernel.org > --- > arch/x86/kernel/kexec-bzimage64.c | 146 > ++--- > drivers/firmware/efi/runtime-map.c | 21 ++ > include/linux/efi.h| 19 + > 3 files changed, 174 insertions(+), 12 deletions(-) [...] > diff --git a/drivers/firmware/efi/runtime-map.c > b/drivers/firmware/efi/runtime-map.c > index 97cdd16..40f2213 100644 > --- a/drivers/firmware/efi/runtime-map.c > +++ b/drivers/firmware/efi/runtime-map.c > @@ -138,6 +138,27 @@ add_sysfs_runtime_map_entry(struct kobject *kobj, int nr) > return entry; > } > > +int get_efi_runtime_map_size(void) > +{ > + return nr_efi_runtime_map * efi_memdesc_size; > +} > + > +int get_efi_runtime_map_desc_size(void) > +{ > + return efi_memdesc_size; > +} > + > +int efi_runtime_map_copy(void *buf, size_t bufsz) > +{ > + size_t sz = get_efi_runtime_map_size(); > + > + if (sz > bufsz) > + sz = bufsz; > + > + memcpy(buf, efi_runtime_map, sz); > + return 0; > +} Could we prefix these with efi_, e.g. efi_get_runtime_map_size() ? -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 15/15] kexec: Support kexec/kdump on EFI systems
On Tue, 01 Jul, at 01:14:19PM, Andrew Morton wrote: > > This? > > From: Andrew Morton > Subject: kexec-support-kexec-kdump-on-efi-systems-fix > > s/get_efi/efi_get/g, per Matt > > Cc: Vivek Goyal > Cc: Matt Fleming > Signed-off-by: Andrew Morton Yep, looks spot on. Thanks Andrew. -- Matt Fleming, Intel Open Source Technology Center ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec