* [PATCH 1/2] efi: Determine how much space is used by boot services-only variables [not found] <515150EC.7040203@redhat.com> @ 2013-04-01 15:13 ` Matthew Garrett 2013-04-01 15:14 ` [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space Matthew Garrett ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Matthew Garrett @ 2013-04-01 15:13 UTC (permalink / raw) To: matt.fleming Cc: ben, jwboyer, linux-efi, seth.forshee, linux-kernel, x86, Matthew Garrett EFI variables can be flagged as being accessible only within boot services. This makes it awkward for us to figure out how much space they use at runtime. In theory we could figure this out by simply comparing the results from QueryVariableInfo() to the space used by all of our variables, but that fails if the platform doesn't garbage collect on every boot. Thankfully, calling QueryVariableInfo() while still inside boot services gives a more reliable answer. This patch passes that information from the EFI boot stub up to the efivars code, letting us calculate a reasonably accurate value. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- arch/x86/boot/compressed/eboot.c | 47 +++++++++++++++++++++++++++++++++++ arch/x86/include/asm/efi.h | 10 ++++++++ arch/x86/include/uapi/asm/bootparam.h | 1 + arch/x86/platform/efi/efi.c | 21 ++++++++++++++++ drivers/firmware/efivars.c | 29 +++++++++++++++++++++ 5 files changed, 108 insertions(+) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index c205035..8615f75 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -251,6 +251,51 @@ 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->query_variable_info) + 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(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; @@ -1157,6 +1202,8 @@ 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 60c89f3..6c3a154 100644 --- a/arch/x86/include/asm/efi.h +++ b/arch/x86/include/asm/efi.h @@ -93,6 +93,9 @@ extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size, #endif /* CONFIG_X86_32 */ +extern u64 efi_var_store_size; +extern u64 efi_var_remaining_size; +extern u64 efi_var_max_var_size; extern int add_efi_memmap; extern unsigned long x86_efi_facility; extern void efi_set_executable(efi_memory_desc_t *md, bool executable); @@ -102,6 +105,13 @@ extern void efi_call_phys_epilog(void); extern void efi_unmap_memmap(void); extern void efi_memory_uc(u64 addr, unsigned long size); +struct efi_var_bootdata { + struct setup_data data; + u64 store_size; + u64 remaining_size; + u64 max_var_size; +}; + #ifdef CONFIG_EFI static inline bool efi_is_native(void) diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index c15ddaf..0874424 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -6,6 +6,7 @@ #define SETUP_E820_EXT 1 #define SETUP_DTB 2 #define SETUP_PCI 3 +#define SETUP_EFI_VARS 4 /* ram_size flags */ #define RAMDISK_IMAGE_START_MASK 0x07FF diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index 5f2ecaf..cd2f020 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -71,6 +71,10 @@ static efi_system_table_t efi_systab __initdata; unsigned long x86_efi_facility; +u64 efi_var_store_size; +u64 efi_var_remaining_size; +u64 efi_var_max_var_size; + /* * Returns 1 if 'facility' is enabled, 0 otherwise. */ @@ -682,6 +686,9 @@ void __init efi_init(void) char vendor[100] = "unknown"; int i = 0; void *tmp; + struct setup_data *data; + struct efi_var_bootdata *efi_var_data; + u64 pa_data; #ifdef CONFIG_X86_32 if (boot_params.efi_info.efi_systab_hi || @@ -699,6 +706,20 @@ void __init efi_init(void) if (efi_systab_init(efi_phys.systab)) return; + pa_data = boot_params.hdr.setup_data; + while (pa_data) { + data = early_ioremap(pa_data, sizeof(*efi_var_data)); + if (data->type == SETUP_EFI_VARS) { + efi_var_data = (struct efi_var_bootdata *)data; + + efi_var_store_size = efi_var_data->store_size; + efi_var_remaining_size = efi_var_data->remaining_size; + efi_var_max_var_size = efi_var_data->max_var_size; + } + pa_data = data->next; + early_iounmap(data, sizeof(*efi_var_data)); + } + set_bit(EFI_SYSTEM_TABLES, &x86_efi_facility); /* diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 7acafb8..60e7d8f 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -85,6 +85,7 @@ #include <linux/ramfs.h> #include <linux/pagemap.h> +#include <asm/efi.h> #include <asm/uaccess.h> #define EFIVARS_VERSION "0.08" @@ -139,6 +140,8 @@ struct efivar_attribute { static struct efivars __efivars; static struct efivar_operations ops; +static u64 boot_var_size; +static u64 boot_used_size; #define PSTORE_EFI_ATTRIBUTES \ (EFI_VARIABLE_NON_VOLATILE | \ @@ -2014,6 +2017,9 @@ int register_efivars(struct efivars *efivars, efi_char16_t *variable_name; unsigned long variable_name_size = 1024; int error = 0; + struct efivar_entry *entry; + struct efi_variable *var; + u64 active_size = 0; variable_name = kzalloc(variable_name_size, GFP_KERNEL); if (!variable_name) { @@ -2086,6 +2092,25 @@ int register_efivars(struct efivars *efivars, } } while (status != EFI_NOT_FOUND); + if (boot_used_size) { + list_for_each_entry(entry, &efivars->list, list) { + var = &entry->var; + status = get_var_data(efivars, var); + + if (status || + !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) + continue; + + active_size += var->DataSize; + active_size += utf16_strsize(var->VariableName, 1024); + } + + if (active_size < boot_used_size) + boot_var_size = boot_used_size - active_size; + else + printk(KERN_WARNING FW_BUG "efivars: Inconsistent initial sizes\n"); + } + error = create_efivars_bin_attributes(efivars); if (error) unregister_efivars(efivars); @@ -2133,6 +2158,10 @@ efivars_init(void) ops.get_next_variable = efi.get_next_variable; ops.query_variable_info = efi.query_variable_info; +#ifdef CONFIG_X86 + boot_used_size = efi_var_store_size - efi_var_remaining_size; +#endif + error = register_efivars(&__efivars, &ops, efi_kobj); if (error) goto err_put; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space 2013-04-01 15:13 ` [PATCH 1/2] efi: Determine how much space is used by boot services-only variables Matthew Garrett @ 2013-04-01 15:14 ` Matthew Garrett 2013-04-01 15:50 ` Ben Hutchings 2013-04-03 13:11 ` Matt Fleming 2013-04-01 15:42 ` [PATCH 1/2] efi: Determine how much space is used by boot services-only variables Ben Hutchings 2013-04-03 13:09 ` Matt Fleming 2 siblings, 2 replies; 10+ messages in thread From: Matthew Garrett @ 2013-04-01 15:14 UTC (permalink / raw) To: matt.fleming Cc: ben, jwboyer, linux-efi, seth.forshee, linux-kernel, x86, Matthew Garrett EFI implementations distinguish between space that is actively used by a variable and space that merely hasn't been garbage collected yet. Space that hasn't yet been garbage collected isn't available for use and so isn't counted in the remaining_space field returned by QueryVariableInfo(). Combined with commit 68d9298 this can cause problems. Some implementations don't garbage collect until the remaining space is smaller than the maximum variable size, and as a result check_var_size() will always fail once more than 50% of the variable store has been used even if most of that space is marked as available for garbage collection. The user is unable to create new variables, and deleting variables doesn't increase the remaining space. The problem that 68d9298 was attempting to avoid was one where certain platforms fail if the actively used space is greater than 50% of the available storage space. We should be able to calculate that by simply summing the size of each available variable and subtracting that from the total storage space. With luck this will fix the problem described in https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting damage to occur to the machines 68d9298 was attempting to fix. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c index 60e7d8f..a6d8a58 100644 --- a/drivers/firmware/efivars.c +++ b/drivers/firmware/efivars.c @@ -439,9 +439,19 @@ static efi_status_t check_var_size_locked(struct efivars *efivars, u32 attributes, unsigned long size) { - u64 storage_size, remaining_size, max_size; + u64 storage_size, remaining_size, max_size, active_available; + struct efivar_entry *entry; + struct efi_variable *var; efi_status_t status; const struct efivar_operations *fops = efivars->ops; + unsigned long active_size = 0; + + /* + * Any writes other than EFI_VARIABLE_NON_VOLATILE will only hit + * RAM, not flash, so ignore them. + */ + if (!(attributes & EFI_VARIABLE_NON_VOLATILE)) + return EFI_SUCCESS; if (!efivars->ops->query_variable_info) return EFI_UNSUPPORTED; @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes, if (status != EFI_SUCCESS) return status; - if (!storage_size || size > remaining_size || size > max_size || - (remaining_size - size) < (storage_size / 2)) + list_for_each_entry(entry, &efivars->list, list) { + var = &entry->var; + status = get_var_data_locked(efivars, var); + + if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) + continue; + + active_size += var->DataSize; + active_size += utf16_strsize(var->VariableName, 1024); + /* + * There's some additional metadata associated with each + * variable. Intel's reference implementation is 60 bytes - + * bump that to 128 to account for vendor additions and + * potential alignment constraints + */ + active_size += 128; + } + + /* + * Add on the size of any boot services-only variables + */ + active_size += boot_var_size; + + active_available = storage_size - active_size; + + if (!storage_size || size > remaining_size || + (active_available - size) < (storage_size / 2)) return EFI_OUT_OF_RESOURCES; return status; -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space 2013-04-01 15:14 ` [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space Matthew Garrett @ 2013-04-01 15:50 ` Ben Hutchings 2013-04-03 13:11 ` Matt Fleming 1 sibling, 0 replies; 10+ messages in thread From: Ben Hutchings @ 2013-04-01 15:50 UTC (permalink / raw) To: Matthew Garrett Cc: matt.fleming, jwboyer, linux-efi, seth.forshee, linux-kernel, x86 [-- Attachment #1: Type: text/plain, Size: 3520 bytes --] On Mon, 2013-04-01 at 11:14 -0400, Matthew Garrett wrote: > EFI implementations distinguish between space that is actively used by a > variable and space that merely hasn't been garbage collected yet. Space > that hasn't yet been garbage collected isn't available for use and so isn't > counted in the remaining_space field returned by QueryVariableInfo(). > > Combined with commit 68d9298 this can cause problems. Some implementations > don't garbage collect until the remaining space is smaller than the maximum > variable size, and as a result check_var_size() will always fail once more > than 50% of the variable store has been used even if most of that space is > marked as available for garbage collection. The user is unable to create > new variables, and deleting variables doesn't increase the remaining space. > > The problem that 68d9298 was attempting to avoid was one where certain > platforms fail if the actively used space is greater than 50% of the > available storage space. We should be able to calculate that by simply > summing the size of each available variable and subtracting that from > the total storage space. With luck this will fix the problem described in > https://bugzilla.kernel.org/show_bug.cgi?id=55471 without permitting > damage to occur to the machines 68d9298 was attempting to fix. > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > --- > drivers/firmware/efivars.c | 41 ++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 38 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c > index 60e7d8f..a6d8a58 100644 > --- a/drivers/firmware/efivars.c > +++ b/drivers/firmware/efivars.c [...] > @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes, > if (status != EFI_SUCCESS) > return status; > > - if (!storage_size || size > remaining_size || size > max_size || > - (remaining_size - size) < (storage_size / 2)) > + list_for_each_entry(entry, &efivars->list, list) { > + var = &entry->var; > + status = get_var_data_locked(efivars, var); > + > + if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) > + continue; > + > + active_size += var->DataSize; > + active_size += utf16_strsize(var->VariableName, 1024); > + /* > + * There's some additional metadata associated with each > + * variable. Intel's reference implementation is 60 bytes - > + * bump that to 128 to account for vendor additions and > + * potential alignment constraints > + */ > + active_size += 128; > + } > + > + /* > + * Add on the size of any boot services-only variables > + */ > + active_size += boot_var_size; > + > + active_available = storage_size - active_size; > + > + if (!storage_size || size > remaining_size || > + (active_available - size) < (storage_size / 2)) > return EFI_OUT_OF_RESOURCES; Both substractions here could quite possibly underflow due to over-estimation of active_size. We can rearrange the condition to be: active_size + size > storage_size / 2 and delete the active_available variable. But I think metadata for the new variable should be accounted as well: active_size + size + 128 > storage_size / 2 Since we'll now be using that magic number twice, it needs a name. Ben. > return status; -- Ben Hutchings DNRC Motto: I can please only one person per day. Today is not your day. Tomorrow isn't looking good either. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space 2013-04-01 15:14 ` [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-01 15:50 ` Ben Hutchings @ 2013-04-03 13:11 ` Matt Fleming 2013-04-03 13:48 ` Matthew Garrett 1 sibling, 1 reply; 10+ messages in thread From: Matt Fleming @ 2013-04-03 13:11 UTC (permalink / raw) To: Matthew Garrett Cc: matt.fleming, ben, jwboyer, linux-efi, seth.forshee, linux-kernel, x86 On 01/04/13 16:14, Matthew Garrett wrote: > @@ -452,8 +462,33 @@ check_var_size_locked(struct efivars *efivars, u32 attributes, > if (status != EFI_SUCCESS) > return status; > > - if (!storage_size || size > remaining_size || size > max_size || > - (remaining_size - size) < (storage_size / 2)) > + list_for_each_entry(entry, &efivars->list, list) { > + var = &entry->var; > + status = get_var_data_locked(efivars, var); > + > + if (status || !(var->Attributes & EFI_VARIABLE_NON_VOLATILE)) > + continue; > + > + active_size += var->DataSize; > + active_size += utf16_strsize(var->VariableName, 1024); > + /* > + * There's some additional metadata associated with each > + * variable. Intel's reference implementation is 60 bytes - > + * bump that to 128 to account for vendor additions and > + * potential alignment constraints > + */ > + active_size += 128; > + } This is the kind of thing I was referring to when I said, Hmm... I'm not convinced this will provide a long-term solution. Is there anything that mandates that the size of all variables has to correlate sensibly with the results from QueryVariableInfo()? Even if there is in theory, I doubt it'll be true everywhere in practice, and trying to workaround implementation bugs in our workarounds for other bugs is the path to madness. We can't continue this approach where we attempt to guess how the firmware implements variable storage, because as we've seen, there are various schemes out there. This looks like something that will differ between implementations, and the fact that it's appearing in our code is a sure sign that this isn't the way to go. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space 2013-04-03 13:11 ` Matt Fleming @ 2013-04-03 13:48 ` Matthew Garrett 2013-04-03 17:12 ` Matt Fleming 0 siblings, 1 reply; 10+ messages in thread From: Matthew Garrett @ 2013-04-03 13:48 UTC (permalink / raw) To: Matt Fleming Cc: matt.fleming@intel.com, ben@decadent.org.uk, jwboyer@redhat.com, linux-efi@vger.kernel.org, seth.forshee@canonical.com, linux-kernel@vger.kernel.org, x86@kernel.org [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 890 bytes --] On Wed, 2013-04-03 at 14:11 +0100, Matt Fleming wrote: > This looks like something that will differ between implementations, and the > fact that it's appearing in our code is a sure sign that this isn't the way to > go. Our choices right now are: 1) Break machines that don't garbage collect on every reboot 2) Leave Samsungs (and some Lenovos?) vulnerable to bricking 3) Maintain a whitelist or blacklist that will inevitably be inadequate, either breaking otherwise working machines or risking bricking of broken ones 4) Attempt to implement something that'll work in all cases Dealing with firmware is hard. This fixes (1) without leaving us with (2), which seems like a net win. -- Matthew Garrett | mjg59@srcf.ucam.org ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space 2013-04-03 13:48 ` Matthew Garrett @ 2013-04-03 17:12 ` Matt Fleming 2013-04-03 17:20 ` Matthew Garrett 0 siblings, 1 reply; 10+ messages in thread From: Matt Fleming @ 2013-04-03 17:12 UTC (permalink / raw) To: Matthew Garrett Cc: matt.fleming@intel.com, ben@decadent.org.uk, jwboyer@redhat.com, linux-efi@vger.kernel.org, seth.forshee@canonical.com, linux-kernel@vger.kernel.org, x86@kernel.org On 03/04/13 14:48, Matthew Garrett wrote: > On Wed, 2013-04-03 at 14:11 +0100, Matt Fleming wrote: > >> This looks like something that will differ between implementations, and the >> fact that it's appearing in our code is a sure sign that this isn't the way to >> go. > > Our choices right now are: > > 1) Break machines that don't garbage collect on every reboot > 2) Leave Samsungs (and some Lenovos?) vulnerable to bricking > 3) Maintain a whitelist or blacklist that will inevitably be inadequate, > either breaking otherwise working machines or risking bricking of broken > ones > 4) Attempt to implement something that'll work in all cases The solution you're proposing has the same downsides as 3) - we risk having to tweak things either way. The difference is that in the case of 3) the tweaking is adding entries to the whitelist, whereas tweaking your solution has more chance of introducing further unwanted regressions because you'd be tweaking an algorithm, an algorithm that relies on the internal implementation of the variable storage code. > Dealing with firmware is hard. This fixes (1) without leaving us with > (2), which seems like a net win. I'm not convinced that implementing 3) would inevitably lead to 2), provided that we apply a bit of common sense when adding entries. I'm not advocating some kind of flag day where we add umpteen machines to the whitelist. For reference, I just pushed two patches to the 'whitelist' branch at, git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git which should hopefully illustrate the kind of thing that I'm talking about. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space 2013-04-03 17:12 ` Matt Fleming @ 2013-04-03 17:20 ` Matthew Garrett 0 siblings, 0 replies; 10+ messages in thread From: Matthew Garrett @ 2013-04-03 17:20 UTC (permalink / raw) To: Matt Fleming Cc: matt.fleming@intel.com, ben@decadent.org.uk, jwboyer@redhat.com, linux-efi@vger.kernel.org, seth.forshee@canonical.com, linux-kernel@vger.kernel.org, x86@kernel.org [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1495 bytes --] On Wed, 2013-04-03 at 18:12 +0100, Matt Fleming wrote: > The solution you're proposing has the same downsides as 3) - we risk > having to tweak things either way. The difference is that in the case of > 3) the tweaking is adding entries to the whitelist, whereas tweaking > your solution has more chance of introducing further unwanted > regressions because you'd be tweaking an algorithm, an algorithm that > relies on the internal implementation of the variable storage code. We *risk* having to tweak things, and we fail on the side of safety. > > Dealing with firmware is hard. This fixes (1) without leaving us with > > (2), which seems like a net win. > > I'm not convinced that implementing 3) would inevitably lead to 2), > provided that we apply a bit of common sense when adding entries. I'm > not advocating some kind of flag day where we add umpteen machines to > the whitelist. > > For reference, I just pushed two patches to the 'whitelist' branch at, > > git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git > > which should hopefully illustrate the kind of thing that I'm talking about. I don't think that works. People are complaining that we broke some Thinkpads as well, but we also have reports that Thinkpads can be bricked if we use too much space. -- Matthew Garrett | mjg59@srcf.ucam.org ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables 2013-04-01 15:13 ` [PATCH 1/2] efi: Determine how much space is used by boot services-only variables Matthew Garrett 2013-04-01 15:14 ` [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space Matthew Garrett @ 2013-04-01 15:42 ` Ben Hutchings 2013-04-03 13:09 ` Matt Fleming 2 siblings, 0 replies; 10+ messages in thread From: Ben Hutchings @ 2013-04-01 15:42 UTC (permalink / raw) To: Matthew Garrett Cc: matt.fleming, jwboyer, linux-efi, seth.forshee, linux-kernel, x86 [-- Attachment #1: Type: text/plain, Size: 1704 bytes --] On Mon, 2013-04-01 at 11:13 -0400, Matthew Garrett wrote: > EFI variables can be flagged as being accessible only within boot services. > This makes it awkward for us to figure out how much space they use at > runtime. In theory we could figure this out by simply comparing the results > from QueryVariableInfo() to the space used by all of our variables, but > that fails if the platform doesn't garbage collect on every boot. Thankfully, > calling QueryVariableInfo() while still inside boot services gives a more > reliable answer. This patch passes that information from the EFI boot stub > up to the efivars code, letting us calculate a reasonably accurate value. Good thinking. > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > --- [...] > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c > @@ -71,6 +71,10 @@ static efi_system_table_t efi_systab __initdata; > > unsigned long x86_efi_facility; > > +u64 efi_var_store_size; > +u64 efi_var_remaining_size; > +u64 efi_var_max_var_size; [...] > --- a/drivers/firmware/efivars.c > +++ b/drivers/firmware/efivars.c [...] > @@ -2133,6 +2158,10 @@ efivars_init(void) > ops.get_next_variable = efi.get_next_variable; > ops.query_variable_info = efi.query_variable_info; > > +#ifdef CONFIG_X86 > + boot_used_size = efi_var_store_size - efi_var_remaining_size; > +#endif efivars can be built as a module, but these aren't exported. Ben. > error = register_efivars(&__efivars, &ops, efi_kobj); > if (error) > goto err_put; -- Ben Hutchings DNRC Motto: I can please only one person per day. Today is not your day. Tomorrow isn't looking good either. [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 828 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables 2013-04-01 15:13 ` [PATCH 1/2] efi: Determine how much space is used by boot services-only variables Matthew Garrett 2013-04-01 15:14 ` [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space Matthew Garrett 2013-04-01 15:42 ` [PATCH 1/2] efi: Determine how much space is used by boot services-only variables Ben Hutchings @ 2013-04-03 13:09 ` Matt Fleming 2013-04-03 13:42 ` Matthew Garrett 2 siblings, 1 reply; 10+ messages in thread From: Matt Fleming @ 2013-04-03 13:09 UTC (permalink / raw) To: Matthew Garrett Cc: matt.fleming, ben, jwboyer, linux-efi, seth.forshee, linux-kernel, x86 On 01/04/13 16:13, Matthew Garrett wrote: > EFI variables can be flagged as being accessible only within boot services. > This makes it awkward for us to figure out how much space they use at > runtime. In theory we could figure this out by simply comparing the results > from QueryVariableInfo() to the space used by all of our variables, but > that fails if the platform doesn't garbage collect on every boot. Thankfully, > calling QueryVariableInfo() while still inside boot services gives a more > reliable answer. This patch passes that information from the EFI boot stub > up to the efivars code, letting us calculate a reasonably accurate value. > > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > --- > arch/x86/boot/compressed/eboot.c | 47 +++++++++++++++++++++++++++++++++++ > arch/x86/include/asm/efi.h | 10 ++++++++ > arch/x86/include/uapi/asm/bootparam.h | 1 + > arch/x86/platform/efi/efi.c | 21 ++++++++++++++++ > drivers/firmware/efivars.c | 29 +++++++++++++++++++++ > 5 files changed, 108 insertions(+) We're fixing a regression in efivars.c, but only for those users that boot via the EFI boot stub? That seems likely to upset some people. Introducing new features via the EFI boot stub is fine, and working around firmware bugs so that we can use some feature is also cool, but we can't start fixing regressions from other subsystems in the EFI boot stub. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] efi: Determine how much space is used by boot services-only variables 2013-04-03 13:09 ` Matt Fleming @ 2013-04-03 13:42 ` Matthew Garrett 0 siblings, 0 replies; 10+ messages in thread From: Matthew Garrett @ 2013-04-03 13:42 UTC (permalink / raw) To: Matt Fleming Cc: matt.fleming@intel.com, ben@decadent.org.uk, jwboyer@redhat.com, linux-efi@vger.kernel.org, seth.forshee@canonical.com, linux-kernel@vger.kernel.org, x86@kernel.org [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 527 bytes --] On Wed, 2013-04-03 at 14:09 +0100, Matt Fleming wrote: > We're fixing a regression in efivars.c, but only for those users that > boot via the EFI boot stub? That seems likely to upset some people. Not really - it just makes the estimates more accurate. Applying (2) without (1) should still fix the functional regression. -- Matthew Garrett | mjg59@srcf.ucam.org ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-04-03 17:20 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <515150EC.7040203@redhat.com>
2013-04-01 15:13 ` [PATCH 1/2] efi: Determine how much space is used by boot services-only variables Matthew Garrett
2013-04-01 15:14 ` [PATCH 2/2] efi: Distinguish between "remaining space" and actually used space Matthew Garrett
2013-04-01 15:50 ` Ben Hutchings
2013-04-03 13:11 ` Matt Fleming
2013-04-03 13:48 ` Matthew Garrett
2013-04-03 17:12 ` Matt Fleming
2013-04-03 17:20 ` Matthew Garrett
2013-04-01 15:42 ` [PATCH 1/2] efi: Determine how much space is used by boot services-only variables Ben Hutchings
2013-04-03 13:09 ` Matt Fleming
2013-04-03 13:42 ` Matthew Garrett
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox