Re: [PATCH] Modify UEFI anti-bricking code
於 四,2013-06-06 於 05:42 +,Matthew Garrett 提到: On Thu, 2013-06-06 at 13:05 +0800, joeyli wrote: + if (!(attributes EFI_VARIABLE_NON_VOLATILE)) + return EFI_OUT_OF_RESOURCES; I'd move this up to the top of the function, and just return 0 - there's no risk of the firmware causing problems if it's a volatile variable, so we should probably just pass it down to the firmware and return an error from there. OK, I moved volatile checking to the top of the function. New version, version 3 diff result like the following. Thanks a lot for reviewing Joey Lee diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index cc3cfe8..5ae2eb0 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -53,6 +53,8 @@ #define EFI_DEBUG 1 +#define EFI_MIN_RESERVE 5120 + #define EFI_DUMMY_GUID \ EFI_GUID(0x4424ac57, 0xbe4b, 0x47dd, 0x9e, 0x97, 0xed, 0x50, 0xf0, 0x9f, 0x92, 0xa9) @@ -988,7 +990,11 @@ void __init efi_enter_virtual_mode(void) kfree(new_memmap); /* clean DUMMY object */ - efi.set_variable(efi_dummy_name, EFI_DUMMY_GUID, 0, 0, NULL); + efi.set_variable(efi_dummy_name, EFI_DUMMY_GUID, +EFI_VARIABLE_NON_VOLATILE | +EFI_VARIABLE_BOOTSERVICE_ACCESS | +EFI_VARIABLE_RUNTIME_ACCESS, +0, NULL); } /* @@ -1040,6 +1046,9 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) efi_status_t status; u64 storage_size, remaining_size, max_size; + if (!(attributes EFI_VARIABLE_NON_VOLATILE)) + return 0; + status = efi.query_variable_info(attributes, storage_size, remaining_size, max_size); if (status != EFI_SUCCESS) @@ -1051,7 +1060,9 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) * write if permitting it would reduce the available space to under * 5KB. This figure was provided by Samsung, so should be safe. */ - if ((remaining_size - size 5120) !efi_no_storage_paranoia) { + if ((remaining_size - size EFI_MIN_RESERVE) + !efi_no_storage_paranoia) { + /* * Triggering garbage collection may require that the firmware * generate a real EFI_OUT_OF_RESOURCES error. We can force @@ -1061,7 +1072,10 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) void *dummy = kmalloc(dummy_size, GFP_ATOMIC); status = efi.set_variable(efi_dummy_name, EFI_DUMMY_GUID, - attributes, dummy_size, dummy); + EFI_VARIABLE_NON_VOLATILE | + EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS, + dummy_size, dummy); if (status == EFI_SUCCESS) { /* @@ -1069,7 +1083,10 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) * that we delete it... */ efi.set_variable(efi_dummy_name, EFI_DUMMY_GUID, -attributes, 0, dummy); +EFI_VARIABLE_NON_VOLATILE | +EFI_VARIABLE_BOOTSERVICE_ACCESS | +EFI_VARIABLE_RUNTIME_ACCESS, +0, dummy); } /* @@ -1085,7 +1102,7 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size) /* * There still isn't enough room, so return an error */ - if (remaining_size - size 5120) + if (remaining_size - size EFI_MIN_RESERVE) return EFI_OUT_OF_RESOURCES; } -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] x86, cpa: Map in an arbitrary pgd
On Sun, 02 Jun, at 02:56:08PM, Borislav Petkov wrote: From: Borislav Petkov b...@suse.de Add the ability to map pages in an arbitrary pgd. Signed-off-by: Borislav Petkov b...@suse.de --- arch/x86/include/asm/pgtable_types.h | 3 +- arch/x86/mm/pageattr.c | 80 2 files changed, 65 insertions(+), 18 deletions(-) diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h index e6423002c10b..0613e147f083 100644 --- a/arch/x86/include/asm/pgtable_types.h +++ b/arch/x86/include/asm/pgtable_types.h @@ -352,7 +352,8 @@ static inline void update_page_count(int level, unsigned long pages) { } */ extern pte_t *lookup_address(unsigned long address, unsigned int *level); extern phys_addr_t slow_virt_to_phys(void *__address); - +extern void kernel_map_pages_in_pgd(pgd_t *pgd, unsigned long address, + unsigned numpages, unsigned long page_flags); #endif /* !__ASSEMBLY__ */ #endif /* _ASM_X86_PGTABLE_DEFS_H */ diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c index bb32480c2d71..3d64e5fc2adc 100644 --- a/arch/x86/mm/pageattr.c +++ b/arch/x86/mm/pageattr.c @@ -30,6 +30,7 @@ */ struct cpa_data { unsigned long *vaddr; + pgd_t *pgd; pgprot_tmask_set; pgprot_tmask_clr; int numpages; [...] @@ -697,7 +714,10 @@ static int __change_page_attr(struct cpa_data *cpa, int primary) else address = *cpa-vaddr; repeat: - kpte = lookup_address(address, level); + if (cpa-pgd) + kpte = __lookup_address_in_pgd(cpa-pgd, address, level); + else + kpte = _lookup_address_cpa(cpa, address, level); Don't you also need to initialise .pgd in __set_pages_p() and __set_pages_np()? -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, 06 Jun, at 03:26:03PM, Borislav Petkov wrote: On Thu, Jun 06, 2013 at 11:42:24AM +0100, Matt Fleming wrote: On Sun, 02 Jun, at 02:56:09PM, Borislav Petkov wrote: + +static int __init parse_efi_cmdline(char *str) +{ + if (*str == '=') + str++; + if (!strncmp(str, 1:1_map, 7)) + efi_config |= EFI_CFG_MAP11; + + return 0; +} +early_param(efi, parse_efi_cmdline); This is fine for testing, but I genuinely think that this should be on by default once these patches are final. This would break the Macs, remember? We could make it be the default though and flip the logic so that users can fall back to the current ioremap functionality, i.e. boot with efi=no_1:1_map. Yeah, or white/blacklist them. We've already had enough of Oh, your machine used to work but now it's bust? Yeah, turn on this cmdline parameter to get things working again. But the point is that the funnies that have been seen on the Macs should be treated as the unusual case. The number of machines I've seen that require this 1:1 map is staggering, especially once you start poking at some of the other runtime services, like UpdateCapsule(). -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Modify UEFI anti-bricking code
On Thu, Jun 06, 2013 at 10:25:42AM +0100, Matt Fleming wrote: On Thu, 06 Jun, at 03:40:26PM, joeyli wrote: OK, I moved volatile checking to the top of the function. New version, version 3 diff result like the following. Thanks. This is what I've now got queued up. --- From 118428bf3b207d9b390a27f32dfef6dc2979078d Mon Sep 17 00:00:00 2001 From: Matthew Garrett matthew.garr...@nebula.com Date: Sat, 1 Jun 2013 16:06:20 -0400 Subject: [PATCH] Modify UEFI anti-bricking code This patch reworks the UEFI anti-bricking code, including an effective reversion of cc5a080c and 31ff2f20. It turns out that calling QueryVariableInfo() from boot services results in some firmware implementations jumping to physical addresses even after entering virtual mode, so until we have 1:1 mappings for UEFI runtime space this isn't going to work so well. Reverting these gets us back to the situation where we'd refuse to create variables on some systems because they classify deleted variables as used until the firmware triggers a garbage collection run, which they won't do until they reach a lower threshold. This results in it being impossible to install a bootloader, which is unhelpful. Feedback from Samsung indicates that the firmware doesn't need more than 5KB of storage space for its own purposes, so that seems like a reasonable threshold. However, there's still no guarantee that a platform will attempt garbage collection merely because it drops below this threshold. It seems that this is often only triggered if an attempt to write generates a genuine EFI_OUT_OF_RESOURCES error. We can force that by attempting to create a variable larger than the remaining space. This should fail, but if it somehow succeeds we can then immediately delete it. I've tested this on the UEFI machines I have available, but I don't have a Samsung and so can't verify that it avoids the bricking problem. Signed-off-by: Matthew Garrett matthew.garr...@nebula.com Signed-off-by: Lee, Chun-Y j...@suse.com [ dummy variable cleanup ] Signed-off-by: Matt Fleming matt.flem...@intel.com --- arch/x86/boot/compressed/eboot.c | 47 - arch/x86/include/asm/efi.h| 7 -- arch/x86/include/uapi/asm/bootparam.h | 1 - arch/x86/platform/efi/efi.c | 188 -- 4 files changed, 65 insertions(+), 178 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 35ee62f..c205035 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -251,51 +251,6 @@ static void find_bits(unsigned long mask, u8 *pos, u8 *size) *size = len; } -static efi_status_t setup_efi_vars(struct boot_params *params) -{ - struct setup_data *data; - struct efi_var_bootdata *efidata; - u64 store_size, remaining_size, var_size; - efi_status_t status; - - if (sys_table-runtime-hdr.revision EFI_2_00_SYSTEM_TABLE_REVISION) - return EFI_UNSUPPORTED; - - data = (struct setup_data *)(unsigned long)params-hdr.setup_data; - - while (data data-next) - data = (struct setup_data *)(unsigned long)data-next; - - status = efi_call_phys4((void *)sys_table-runtime-query_variable_info, - EFI_VARIABLE_NON_VOLATILE | - EFI_VARIABLE_BOOTSERVICE_ACCESS | - EFI_VARIABLE_RUNTIME_ACCESS, store_size, - remaining_size, var_size); - - if (status != EFI_SUCCESS) - return status; - - status = efi_call_phys3(sys_table-boottime-allocate_pool, - EFI_LOADER_DATA, sizeof(*efidata), efidata); - - if (status != EFI_SUCCESS) - return status; - - efidata-data.type = SETUP_EFI_VARS; - efidata-data.len = sizeof(struct efi_var_bootdata) - - sizeof(struct setup_data); - efidata-data.next = 0; - efidata-store_size = store_size; - efidata-remaining_size = remaining_size; - efidata-max_var_size = var_size; - - if (data) - data-next = (unsigned long)efidata; - else - params-hdr.setup_data = (unsigned long)efidata; - -} - static efi_status_t setup_efi_pci(struct boot_params *params) { efi_pci_io_protocol *pci; @@ -1202,8 +1157,6 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table, setup_graphics(boot_params); - setup_efi_vars(boot_params); - setup_efi_pci(boot_params); status = efi_call_phys3(sys_table-boottime-allocate_pool, diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h index 2fb5d58..60c89f3 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -102,13 +102,6 @@ extern void efi_call_phys_epilog(void); extern void efi_unmap_memmap(void); extern void efi_memory_uc(u64 addr,
Re: [PATCH] Modify UEFI anti-bricking code
On Thu, 06 Jun, at 09:48:46AM, Russ Anderson wrote: This looks like it will try to allocate more than the remaining size. Is that intended? Yes, the intention is to trigger garbage collection. -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Modify UEFI anti-bricking code
On Thu, Jun 06, 2013 at 04:00:39PM +0100, Matt Fleming wrote: On Thu, 06 Jun, at 09:48:46AM, Russ Anderson wrote: This looks like it will try to allocate more than the remaining size. Is that intended? Yes, the intention is to trigger garbage collection. OK, if that's what it takes. It just struck me as something that would fail initial parameter checking in the bios, before getting far enough to trigger garbage collection. Since I really want the rest of this patch to go in, I'm certainly not going to object. :-) Thanks, -- Russ Anderson, OS RAS/Partitioning Project Lead SGI - Silicon Graphics Inc r...@sgi.com -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 06:50:52PM +0100, Matthew Garrett wrote: On Thu, Jun 06, 2013 at 03:26:03PM +0200, Borislav Petkov wrote: This would break the Macs, remember? I think the Macs will be fine as long as we're passing the high mappings into SetVirtualAddressMap(). Right, on those we'll fall back to the current mappings and simply not have the 1:1 thing. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] x86, efi: Map runtime services 1:1
On 06/06/2013 08:58 AM, Borislav Petkov wrote: -- diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index 654be4ae3047..7a6129afdff1 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -1021,6 +1021,9 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code) /* Get the faulting address: */ address = read_cr2(); + if (read_cr3() == (unsigned long)real_mode_header-trampoline_pgd) + pr_err(%s: #PF addr: 0x%lx\n, __func__, address); + /* * Detect and handle instructions that would cause a page fault for * both a tracked kernel page and a userspace page. Or we could materialize mappings for this specific PGD. However, adding a read of %cr3 in __do_page_fault sounds expensive. -hpa -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 08:51:40PM +0200, Borislav Petkov wrote: On Thu, Jun 06, 2013 at 06:50:52PM +0100, Matthew Garrett wrote: On Thu, Jun 06, 2013 at 03:26:03PM +0200, Borislav Petkov wrote: This would break the Macs, remember? I think the Macs will be fine as long as we're passing the high mappings into SetVirtualAddressMap(). Right, on those we'll fall back to the current mappings and simply not have the 1:1 thing. No, I think that's the wrong thing to do. We should set up the current mappings and the 1:1 mappings, and pass the current mappings through SetVirtualAddressMap(). That matches the behaviour of Windows. -- Matthew Garrett | mj...@srcf.ucam.org -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] x86, efi: Map runtime services 1:1
On Thu, Jun 06, 2013 at 12:28:20PM -0700, H. Peter Anvin wrote: Or we could materialize mappings for this specific PGD. However, adding a read of %cr3 in __do_page_fault sounds expensive. Yes, I think we want to make sure all mappings are there when we do an EFI runtime call so that we never #PF while it executes. Matt mentioned on IRC that the it could be that his EFI runtime is referencing EFI_RESERVED area which we don't map. However, we need to confirm/disprove that first, as it is currently only a hunch. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 08:35:48PM +0100, Matthew Garrett wrote: No, I think that's the wrong thing to do. We should set up the current mappings and the 1:1 mappings, and pass the current mappings through SetVirtualAddressMap(). That matches the behaviour of Windows. And when do we use the 1:1 mappings and when the current mappings when doing runtime calls? Also, would the 1:1 mappings even work if not passed through SetVirtualAddressMap? I'm sensing a yes but I don't know... -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 08:54:50PM +0100, Matthew Garrett wrote: We want both to be available when we're making the call, but I think we should probably enter via the high addresses. The only reason we're doing this at all is that some systems don't update all of their pointers from physical mode, and we'd prefer them to work rather than fault... Actually, we do the 1:1 thing so that EFI runtime works in a kexec kernel too. Which won't work if we use the high addresses. However, if we can use the 1:1 map *after* SetVirtualAddressMap() has been called with the high mappings, then my issue is solved - we drop to using 1:1 in the kexec kernel only. But I don't think that is the case... -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 10:07:05PM +0200, Borislav Petkov wrote: On Thu, Jun 06, 2013 at 08:54:50PM +0100, Matthew Garrett wrote: We want both to be available when we're making the call, but I think we should probably enter via the high addresses. The only reason we're doing this at all is that some systems don't update all of their pointers from physical mode, and we'd prefer them to work rather than fault... Actually, we do the 1:1 thing so that EFI runtime works in a kexec kernel too. Which won't work if we use the high addresses. kexec seems like a lower priority than compatibility. Perhaps keep the efi argument for people who want to use kexec? hpa suggested allocating a fixed high area for UEFI mappings, which would also solve this. However, if we can use the 1:1 map *after* SetVirtualAddressMap() has been called with the high mappings, then my issue is solved - we drop to using 1:1 in the kexec kernel only. But I don't think that is the case... Yeah, calling SetVirtualAddressMap() with high addresses is going to result in the firmware having a bunch of pointers to high addresses. -- Matthew Garrett | mj...@srcf.ucam.org -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 09:18:28PM +0100, Matthew Garrett wrote: kexec seems like a lower priority than compatibility. Perhaps keep the efi argument for people who want to use kexec? This is what I currently have in the code: if you boot with efi=1:1_map, you get them. hpa suggested allocating a fixed high area for UEFI mappings, which would also solve this. I guess we can do that too. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 10:27:17PM +0200, Borislav Petkov wrote: On Thu, Jun 06, 2013 at 09:18:28PM +0100, Matthew Garrett wrote: kexec seems like a lower priority than compatibility. Perhaps keep the efi argument for people who want to use kexec? This is what I currently have in the code: if you boot with efi=1:1_map, you get them. Well, we want the 1:1 mappings to exist all the time. The only thing the option should change is whether they're passed to SetVirtualAddressMap() or not. -- Matthew Garrett | mj...@srcf.ucam.org -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 09:30:57PM +0100, Matthew Garrett wrote: Well, we want the 1:1 mappings to exist all the time. The only thing the option should change is whether they're passed to SetVirtualAddressMap() or not. But can you call them even if they haven't been passed through SetVirtualAddressMap, *after* SetVirtualAddressMap has been called? -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 09:50:57PM +0100, Matthew Garrett wrote: What do you mean by call them? I don't think we ever want to call by physical address, other than maybe in the kexec case. The only reason we really care about the physical addresses being mapped 1:1 is that some pointers may not have been updated. I want to be able to call the runtime services in the kexec kernel. Which means, the kexec kernel would simply map the runtime code/data regions 1:1 and then use the physical addresses to call the runtime services. Question is: would that work even if SetVirtualAddressMap has already run in the original kernel and with virtual addresses? -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86, efi: Add an efi= kernel command line parameter
On Thu, Jun 06, 2013 at 11:02:18PM +0200, Borislav Petkov wrote: On Thu, Jun 06, 2013 at 09:50:57PM +0100, Matthew Garrett wrote: What do you mean by call them? I don't think we ever want to call by physical address, other than maybe in the kexec case. The only reason we really care about the physical addresses being mapped 1:1 is that some pointers may not have been updated. I want to be able to call the runtime services in the kexec kernel. Which means, the kexec kernel would simply map the runtime code/data regions 1:1 and then use the physical addresses to call the runtime services. Question is: would that work even if SetVirtualAddressMap has already run in the original kernel and with virtual addresses? No. You'll need to have an option for that case. -- Matthew Garrett | mj...@srcf.ucam.org -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html