Re: [PATCH 06/12] x86: efi: Replace GFP_ATOMIC with GFP_KERNEL in efi_query_variable_store
* Ard Biesheuvel wrote: > From: Jia-Ju Bai > > The function kzalloc here is not called in atomic context. > If nonblocking in efi_query_variable_store is true, > namely it is in atomic context, efi_query_variable_store will return before > this kzalloc is called. > Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL. > > This is found by a static analysis tool named DCNS written by myself. Please fix typos/spelling in changelogs when applying patches. I improved the above to: efi_query_variable_store() does an atomic kzalloc() unnecessarily, because we can never get this far when called in an atomic context, namely when nonblocking == 1. Replace it with GFP_KERNEL. This was found by the DCNS static analysis tool written by myself. Thanks, Ingo -- 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 12/12] efi: make const array 'apple' static
On 9 March 2018 at 07:47, Ingo Molnar wrote:
>
> * Ard Biesheuvel wrote:
>
>> From: Colin Ian King
>>
>> Don't populate the const read-only array 'buf' on the stack but instead
>> make it static. Makes the object code smaller by 64 bytes:
>>
>> Before:
>>text data bss dec hex filename
>>9264 1 1692812441
>> arch/x86/boot/compressed/eboot.o
>>
>> After:
>>text data bss dec hex filename
>>9200 1 1692172401
>> arch/x86/boot/compressed/eboot.o
>>
>> (gcc version 7.2.0 x86_64)
>>
>> Signed-off-by: Colin Ian King
>> Signed-off-by: Ard Biesheuvel
>> ---
>> arch/x86/boot/compressed/eboot.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/boot/compressed/eboot.c
>> b/arch/x86/boot/compressed/eboot.c
>> index 886a9115af62..f2251c1c9853 100644
>> --- a/arch/x86/boot/compressed/eboot.c
>> +++ b/arch/x86/boot/compressed/eboot.c
>> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct
>> boot_params *boot_params)
>>
>> static void setup_quirks(struct boot_params *boot_params)
>> {
>> - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
>> efi_table_attr(efi_system_table, fw_vendor, sys_table);
>
> As a general policy, please don't put 'static' variables into the local scope,
> use file scope instead - right before setup_quirks() would be fine.
>
> This makes it abundantly clear that it's not on the stack.
>
Fair enough. I didn't know there was such a policy, but since these
have local scope by definition, it doesn't pollute the global
namespace so it's fine
> Also, would it make sense to rename it to something more descriptive like
> "apple_unicode_str[]" or so?
>
> Plus an unicode string literal initializer would be pretty descriptive as
> well,
> instead of the weird looking character array, i.e. something like:
>
> static efi_char16_t const apple_unicode_str[] = u"Apple";
>
> ... or so?
>
is u"xxx" the same as L"xxx"?
In any case, this is for historical reasons: at some point (and I
don't remember the exact details) we had a conflict at link time with
objects using 4 byte wchar_t, so we started using this notation to be
independent of the size of wchar_t. That issue no longer exists so we
should be able to get rid of this.
--
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 12/12] efi: make const array 'apple' static
* Ard Biesheuvel wrote:
> From: Colin Ian King
>
> Don't populate the const read-only array 'buf' on the stack but instead
> make it static. Makes the object code smaller by 64 bytes:
>
> Before:
>text data bss dec hex filename
>9264 1 1692812441 arch/x86/boot/compressed/eboot.o
>
> After:
>text data bss dec hex filename
>9200 1 1692172401 arch/x86/boot/compressed/eboot.o
>
> (gcc version 7.2.0 x86_64)
>
> Signed-off-by: Colin Ian King
> Signed-off-by: Ard Biesheuvel
> ---
> arch/x86/boot/compressed/eboot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index 886a9115af62..f2251c1c9853 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct
> boot_params *boot_params)
>
> static void setup_quirks(struct boot_params *boot_params)
> {
> - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
> efi_table_attr(efi_system_table, fw_vendor, sys_table);
As a general policy, please don't put 'static' variables into the local scope,
use file scope instead - right before setup_quirks() would be fine.
This makes it abundantly clear that it's not on the stack.
Also, would it make sense to rename it to something more descriptive like
"apple_unicode_str[]" or so?
Plus an unicode string literal initializer would be pretty descriptive as well,
instead of the weird looking character array, i.e. something like:
static efi_char16_t const apple_unicode_str[] = u"Apple";
... or so?
Thanks,
Ingo
--
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 12/12] efi: make const array 'apple' static
On 9 March 2018 at 07:43, Ard Biesheuvel wrote:
> On 8 March 2018 at 11:05, Joe Perches wrote:
>> On Thu, 2018-03-08 at 08:00 +, Ard Biesheuvel wrote:
>>> From: Colin Ian King
>>>
>>> Don't populate the const read-only array 'buf' on the stack but instead
>>> make it static. Makes the object code smaller by 64 bytes:
>>>
>>> Before:
>>>text data bss dec hex filename
>>>9264 1 1692812441
>>> arch/x86/boot/compressed/eboot.o
>>>
>>> After:
>>>text data bss dec hex filename
>>>9200 1 1692172401
>>> arch/x86/boot/compressed/eboot.o
>>>
>>> (gcc version 7.2.0 x86_64)
>>>
>>> Signed-off-by: Colin Ian King
>>> Signed-off-by: Ard Biesheuvel
>>> ---
>>> arch/x86/boot/compressed/eboot.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/boot/compressed/eboot.c
>>> b/arch/x86/boot/compressed/eboot.c
>>> index 886a9115af62..f2251c1c9853 100644
>>> --- a/arch/x86/boot/compressed/eboot.c
>>> +++ b/arch/x86/boot/compressed/eboot.c
>>> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct
>>> boot_params *boot_params)
>>>
>>> static void setup_quirks(struct boot_params *boot_params)
>>> {
>>> - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>>> + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>>
>> Perhaps
>>
>> static const efi_char16_t apple[] ...
>>
>> is better.
>>
>
> Why would that be any better? I have always found the 'const'
> placement after the type to be much clearer.
>
> const void *
> void const *
> void * const
>
> I.e., #2 and #3 are equivalent,
That would be #1 and #2, of course
> and so 'const' associates to the left
> not to the right, unless it is at the beginning.
>
> Personally, I don't mind either way, but saying it is 'better' is a stretch
> imo
--
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 12/12] efi: make const array 'apple' static
On 8 March 2018 at 11:05, Joe Perches wrote:
> On Thu, 2018-03-08 at 08:00 +, Ard Biesheuvel wrote:
>> From: Colin Ian King
>>
>> Don't populate the const read-only array 'buf' on the stack but instead
>> make it static. Makes the object code smaller by 64 bytes:
>>
>> Before:
>>text data bss dec hex filename
>>9264 1 1692812441
>> arch/x86/boot/compressed/eboot.o
>>
>> After:
>>text data bss dec hex filename
>>9200 1 1692172401
>> arch/x86/boot/compressed/eboot.o
>>
>> (gcc version 7.2.0 x86_64)
>>
>> Signed-off-by: Colin Ian King
>> Signed-off-by: Ard Biesheuvel
>> ---
>> arch/x86/boot/compressed/eboot.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/boot/compressed/eboot.c
>> b/arch/x86/boot/compressed/eboot.c
>> index 886a9115af62..f2251c1c9853 100644
>> --- a/arch/x86/boot/compressed/eboot.c
>> +++ b/arch/x86/boot/compressed/eboot.c
>> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct
>> boot_params *boot_params)
>>
>> static void setup_quirks(struct boot_params *boot_params)
>> {
>> - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>> + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
>
> Perhaps
>
> static const efi_char16_t apple[] ...
>
> is better.
>
Why would that be any better? I have always found the 'const'
placement after the type to be much clearer.
const void *
void const *
void * const
I.e., #2 and #3 are equivalent, and so 'const' associates to the left
not to the right, unless it is at the beginning.
Personally, I don't mind either way, but saying it is 'better' is a stretch imo
--
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 07/12] efi: Use efi_mm in x86 as well as ARM
* Ard Biesheuvel wrote:
> From: Sai Praneeth
>
> Presently, only ARM uses mm_struct to manage efi page tables and efi
> runtime region mappings. As this is the preferred approach, let's make
> this data structure common across architectures. Specially, for x86,
> using this data structure improves code maintainability and readability.
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 85f6ccb80b91..00f977ddd718 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -2,10 +2,14 @@
> #ifndef _ASM_X86_EFI_H
> #define _ASM_X86_EFI_H
>
> +#include
> +#include
> +
> #include
> #include
> #include
> #include
> +#include
>
> /*
> * We map the EFI regions needed for runtime services non-contiguously,
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index f5083aa72eae..f1b7d68ac460 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -966,6 +966,8 @@ extern struct efi {
> unsigned long flags;
> } efi;
>
> +extern struct mm_struct efi_mm;
> +
> static inline int
> efi_guidcmp (efi_guid_t left, efi_guid_t right)
> {
Ugh, I can see three problems with this patch:
1)
Why is the low level asm/efi.h header polluted with two of the biggest header
files in existence, to add a type to _another_ header (efi.h)?
2)
Why is included if what is being relied on is mm_struct?
3)
But even looks unnecessary in efi.h, a simple forward
declaration of mm_struct would do ...
The high level MM and sched headers should be added to the actual .c files that
make use of them.
Thanks,
Ingo
--
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 V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()
> > Another warning by checkpatch is "use of in_atomic() in drivers code" > > I'm assuming it warns because you're touching files in drivers/ but the efi > fun is > not really a driver... True! That makes sense :) > > But looking at patch 3, that thing looks like a real mess. Some of the things > - > pstore, it seems - do stuff in atomic context and yet you want to do efi > stuff in a > workqueue which doesn't stomach atomic context to begin with. > > So if you wanna do workqueue, you should make sure all efi stuff gets delayed > to process context and queued properly. For example, we log MCEs from atomic > context by putting them on a lockless buffer and then kicking irq_work to > queue > the work when we return to process context. > Can you do something like that? > I think we can do this, it's is a good idea. I looked at this approach and saw that in oops_end() function, part of arch/x86/kernel/dumpstack, between oops_exit() and panic() (here we are not in atomic context, so, I think we can use work queues) we could have something like efi_flush_buffer() which will flush the buffer and queue the work to efi_rts_wq. But, I guess, we have some downsides with this design: 1. We are doing this to have "no exceptions to use efi_rts_wq", but we will be making the common case complicated. i.e. When a user requests to write some efi variable, we will first write it to a buffer and then flush the buffer using efi_rts_wq. Instead, we could have written the variable directly. Maybe, you meant, we should use this buffer only while pstore and not during normal case (which sounds reasonably OK). 2. It doesn't look rational that, when we are already going down, we schedule (because we will be invoking efi_runtime_services() through work queue) to log some stuff. Not, sure if that's happening in other parts of kernel or if it's OK to do that. I will try the suggested approach and will keep this thread posted. > "Hence, pstore calls efi_runtime_services() without using efi_rts_wq" - that > doesn't sound like optimal design to me. I would try to shove them all through > the workqueue - not have exceptions. > Alternatively, instead of playing around with in_atomic(), we could have wrapper functions like efi_write_var_non_wq() which will only be used by pstore. This function will not use efi_rts_wq and directly invoke efi_runtime_service. Just an attempt to make the code not look messy. > Then this: > > > A potential issue could be, for instance, an NMI interrupt (like perf) > > trying to profile some user data while in efi_pgd. > > I can't understand. > > How did we handle this until now and why is it a problem all of a sudden? > > Because I don't recall being unable to run perf while efi runtime services are > happening. > That's true! AFAIK, we don't have any issues handling NMI while in efi_pgd. We might have issues only when, we are already in efi_pgd, NMI comes along and NMI handler tries to touch the regions that are not mapped in efi_pgd (Eg: User space part of process address space) and using kthread inherently means that we will not have any user space. Regards, Sai
Re: [PATCH 1/2] efi/esrt: fix unsupported version initialization failure
On 8 March 2018 at 18:15, Ard Biesheuvel wrote:
> On 8 March 2018 at 18:00, Ard Biesheuvel wrote:
>> On 8 March 2018 at 16:11, Tyler Baicar wrote:
>>> On 2/24/2018 2:20 AM, Dave Young wrote:
On 02/23/18 at 12:42pm, Tyler Baicar wrote:
>
> If ESRT initialization fails due to an unsupported version, the
> early_memremap allocation is never unmapped. This will cause an
> early ioremap leak. So, make sure to unmap the memory allocation
> before returning from efi_esrt_init().
>
> Signed-off-by: Tyler Baicar
> ---
> drivers/firmware/efi/esrt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index c47e0c6..504f3c3 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
> } else {
> pr_err("Unsupported ESRT version %lld.\n",
>tmpesrt.fw_resource_version);
> - return;
> + goto err_memunmap;
> }
> if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
> --
Reviewed-by: Dave Young
>>>
>>> Thank you Dave for your review here and input on the other patch.
>>>
>>> Ard,
>>>
>>> Can this patch be picked up? I understand patch 2 is not acceptable, but
>>> this one should
>>> be good to go I think.
>>>
>>
>> Yeah you're right. I'll pick it up as a bugfix.
>
> Actually, on second thought, could you respin this patch, and just
> move the memunamp() to right after the memcpy()? That way, we can get
> rid of all the 'goto err_memunmap's afaict
>
OK, now I'm confused. Does anyone have a clue why in efi_esrt_init()
the memremap() is done a second time? AFAICT it just reserves the
region, it does not actually access the second mapping at all.
--
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: Regression from efi: call get_event_log before ExitBootServices
On 03/08/2018 03:45 AM, Thiebaud Weksteen wrote: > Jeremy, Hans, could you both describe precisely how your boot is > configured? This feature is only triggered when booting the EFI stub of the > kernel so this may be not executed if you are using something else in > between. I put everything I know in the other sub-thread. > Jeremy, would you be able to modify the efi_retrieve_tpm2_eventlog_1_2 > function to add multiple efi_printk(sys_table_arg, "message\n"); to test: > if you get the output on your screen; and isolate which call might be the > cause of the hang? > I can forward a debug patch if that helps. Thanks for the patch, here's the output: Locating the TCG2Protocol Calling GetEventLog on TCG2Protocol Log returned log_location is not empty log_size != 0 Allocating memory for storing the logs Returned from memory allocation Copying log to new location At this point it hangs. Regards, Jeremy -- 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 1/2] efi/esrt: fix unsupported version initialization failure
On 8 March 2018 at 18:00, Ard Biesheuvel wrote:
> On 8 March 2018 at 16:11, Tyler Baicar wrote:
>> On 2/24/2018 2:20 AM, Dave Young wrote:
>>>
>>> On 02/23/18 at 12:42pm, Tyler Baicar wrote:
If ESRT initialization fails due to an unsupported version, the
early_memremap allocation is never unmapped. This will cause an
early ioremap leak. So, make sure to unmap the memory allocation
before returning from efi_esrt_init().
Signed-off-by: Tyler Baicar
---
drivers/firmware/efi/esrt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index c47e0c6..504f3c3 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
} else {
pr_err("Unsupported ESRT version %lld.\n",
tmpesrt.fw_resource_version);
- return;
+ goto err_memunmap;
}
if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
--
>>>
>>> Reviewed-by: Dave Young
>>
>> Thank you Dave for your review here and input on the other patch.
>>
>> Ard,
>>
>> Can this patch be picked up? I understand patch 2 is not acceptable, but
>> this one should
>> be good to go I think.
>>
>
> Yeah you're right. I'll pick it up as a bugfix.
Actually, on second thought, could you respin this patch, and just
move the memunamp() to right after the memcpy()? That way, we can get
rid of all the 'goto err_memunmap's afaict
Thanks,
Ard.
--
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 V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()
/*
> >> > +* Since we process only one efi_runtime_service() at a time, an
> >> > +* ordered workqueue (which creates only one execution context)
> >> > +* should suffice all our needs.
> >> > +*/
> >> > + efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue",
> >> > + 0);
> >>
> >> efi_rts_wq or efi_rts_workqueue?
> >>
> >> > + if (!efi_rts_wq) {
> >> > + pr_err("Failed to create efi_rts_workqueue, EFI runtime
> >> > services "
> >>
> >> Same here.
> >
> > Sure! I will make it consistent with "efi_rts_wq". Just tried to be
> > more verbose with names :)
> >
>
> It is not a big deal, but using the exact same name is better for the
> purposes of
> grepping and things like that :-)
Yes, that makes sense.
> By the way, check the commit title/message, there are some others there too.
Sure! I will make changes to commit/title messages too.
Regards,
Sai
RE: [PATCH V2 1/3] x86/efi: Call efi_delete_dummy_variable() during efi subsystem initialization
> > void __init efi_enter_virtual_mode(void) diff --git
> > a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index
> > cd42f66a7c85..838b8efe639c 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -328,6 +328,12 @@ static int __init efisubsys_init(void)
> > if (!efi_enabled(EFI_BOOT))
> > return 0;
> >
> > + /*
> > +* Clean DUMMY object calls EFI Runtime Service, set_variable(), so
> > +* it should be invoked only after efi_rts_workqueue is ready.
> > +*/
> > + efi_delete_dummy_variable();
> > +
>
> Is there any way to keep this local to arch/x86?
>
I think, we can definitely do that. I will retake a look at the possibilities
and will update this thread.
> > /* We register the efi directory at /sys/firmware/efi */
> > efi_kobj = kobject_create_and_add("efi", firmware_kobj);
> > if (!efi_kobj) {
> > diff --git a/include/linux/efi.h b/include/linux/efi.h index
> > f5083aa72eae..c4efb3ef0dfa 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -992,6 +992,7 @@ extern efi_status_t efi_query_variable_store(u32
Re: [PATCH 1/2] efi/esrt: fix unsupported version initialization failure
On 8 March 2018 at 16:11, Tyler Baicar wrote:
> On 2/24/2018 2:20 AM, Dave Young wrote:
>>
>> On 02/23/18 at 12:42pm, Tyler Baicar wrote:
>>>
>>> If ESRT initialization fails due to an unsupported version, the
>>> early_memremap allocation is never unmapped. This will cause an
>>> early ioremap leak. So, make sure to unmap the memory allocation
>>> before returning from efi_esrt_init().
>>>
>>> Signed-off-by: Tyler Baicar
>>> ---
>>> drivers/firmware/efi/esrt.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
>>> index c47e0c6..504f3c3 100644
>>> --- a/drivers/firmware/efi/esrt.c
>>> +++ b/drivers/firmware/efi/esrt.c
>>> @@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
>>> } else {
>>> pr_err("Unsupported ESRT version %lld.\n",
>>>tmpesrt.fw_resource_version);
>>> - return;
>>> + goto err_memunmap;
>>> }
>>> if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
>>> --
>>
>> Reviewed-by: Dave Young
>
> Thank you Dave for your review here and input on the other patch.
>
> Ard,
>
> Can this patch be picked up? I understand patch 2 is not acceptable, but
> this one should
> be good to go I think.
>
Yeah you're right. I'll pick it up as a bugfix.
--
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: Regression from efi: call get_event_log before ExitBootServices
On 03/08/2018 11:50 AM, Hans de Goede wrote: > added these now> > > Hi, > > On 07-03-18 12:34, Javier Martinez Canillas wrote: >> Are you also able to read the TPM event logs? >> >> $ hexdump /sys/kernel/security/tpm0/binary_bios_measurements > > Yes for me that outputs a lot of hex :) For me, /sys/kernel/security/tmp0 doesn't exist on 4.15.6 or 4.16 with the patch reverted. >> The UEFI firmware does some measurements and so does shim. So you should >> have some event logs. What version of shim are you using? And also would >> be good to know if it's the same shim version that Jeremy is using. > > That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, which is > the last version for F27 AFAICT. All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32. > > But Jeremy's tablet might very well be not using the shim at all, as > I manually installed Fedora 25 on the tablet he now has, before Fedora > supported > machines with 32 bit EFI. I then later did a "dnf distro-sync" to > Fedora-27. > > Jeremy might also very well still be booting using a grub binary I build > manually back then, without any shim being involved. > > Jeremy what does efibootmgr -v output on your device ? # efibootmgr -v BootCurrent: 0003 Timeout: 4 seconds BootOrder: 0003,,0001,2001,2002,2003 Boot* Android X64 OS HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\BOOT\bootx64.efi)RC Boot0001* Internal EFI Shell FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(c57ad6b7-0515-40a8-9d21-551652854e37)RCM&". Boot0003* Fedora HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\fedora\grubx64.efi) Boot2001* EFI USB DeviceRC Boot2002* EFI DVD/CDROM RC Boot2003* EFI Network RC Boot8087* Udm FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(9a9ab4c1-ee1b-488b-b300-24544a7bd418) I think you're right about it using the old grub binary. I'm embarrassingly unfamiliar with both UEFI and grub, but I'm guessing you set the location of grub.cfg at compile time? When I boot \EFI\fedora\grubx64.efi, it's pulling the grub.cfg from \EFI\redhat\grub.cfg. Regards, Jeremy -- 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 V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()
> "Hence, pstore calls efi_runtime_services() without using efi_rts_wq" - > that doesn't sound like optimal design to me. I would try to shove them > all through the workqueue - not have exceptions. But pstore is trying to save the last gasp dying words from a kernel that has paniced. There isn't any guarantee that work queue will run. I think it is reasonable to have some special case to make sure that we do save the information. But perhaps that special case should be to have pstore call directly into the guts of the EFI code and not worry about all these fancy switches of "mm" etc. This is true for the machine check logging case too, but the mitigation is that the details of the error persist in the machine check banks across the reset ... so all is not lost if the work queue isn't run here. -Tony
Re: Regression from efi: call get_event_log before ExitBootServices
Hi, On 07-03-18 12:34, Javier Martinez Canillas wrote: On 03/07/2018 12:10 PM, Hans de Goede wrote: Both according to the BIOS and to the /sys/class/tpm/tpm0/device/description file it is a TPM 2.0. I see, so you can choose enabling the TPM 1.2 or TPM 2.0 device? At least that's the case on my X1 Carbon laptop. I've both a hardware TPM 1.2 and a firmware TPM 2.0 that's implemented as an Intel ME application (AFAIU). This device only has the firmware TPM 2.0 implementation. I'm actually amazed that this machine has a TPM at all, a quick internet search shows that it is a software implemented TPM running as part of the TXE firmware. A quick search suggests that it comes with Windows 10? Yes, it comes with Windows 10. For start, can you please check if you can boot a v4.16-rcX kernel with the TPM device enabled? That way we will know that at least that it consistently fails on this machine and is not and isolated issue. I just tried and v4.16-rc3 boots fine for me, repeatedly. That's an interesting data point. I guess Jeremy's model may actually have something in the TPM log I don't think so. The UEFI firmware already does some measurements and also does shim. So you *should* have some logs. while my TPM log is empty... Is there anyway to make sure the TPM log has some info to retreive? Are you also able to read the TPM event logs? $ hexdump /sys/kernel/security/tpm0/binary_bios_measurements Yes for me that outputs a lot of hex :) The UEFI firmware does some measurements and so does shim. So you should have some event logs. What version of shim are you using? And also would be good to know if it's the same shim version that Jeremy is using. That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, which is the last version for F27 AFAICT. But Jeremy's tablet might very well be not using the shim at all, as I manually installed Fedora 25 on the tablet he now has, before Fedora supported machines with 32 bit EFI. I then later did a "dnf distro-sync" to Fedora-27. Jeremy might also very well still be booting using a grub binary I build manually back then, without any shim being involved. Jeremy what does efibootmgr -v output on your device ? Regards, Hans -- 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 1/2] efi/esrt: fix unsupported version initialization failure
On 2/24/2018 2:20 AM, Dave Young wrote:
On 02/23/18 at 12:42pm, Tyler Baicar wrote:
If ESRT initialization fails due to an unsupported version, the
early_memremap allocation is never unmapped. This will cause an
early ioremap leak. So, make sure to unmap the memory allocation
before returning from efi_esrt_init().
Signed-off-by: Tyler Baicar
---
drivers/firmware/efi/esrt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index c47e0c6..504f3c3 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -285,7 +285,7 @@ void __init efi_esrt_init(void)
} else {
pr_err("Unsupported ESRT version %lld.\n",
tmpesrt.fw_resource_version);
- return;
+ goto err_memunmap;
}
if (tmpesrt.fw_resource_count > 0 && max - size < entry_size) {
--
Reviewed-by: Dave Young
Thank you Dave for your review here and input on the other patch.
Ard,
Can this patch be picked up? I understand patch 2 is not acceptable, but this
one should
be good to go I think.
Thanks,
Tyler
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
--
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 V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()
On Thu, Mar 08, 2018 at 05:31:03AM +, Prakhya, Sai Praneeth wrote: > Another warning by checkpatch is "use of in_atomic() in drivers code" I'm assuming it warns because you're touching files in drivers/ but the efi fun is not really a driver... But looking at patch 3, that thing looks like a real mess. Some of the things - pstore, it seems - do stuff in atomic context and yet you want to do efi stuff in a workqueue which doesn't stomach atomic context to begin with. So if you wanna do workqueue, you should make sure all efi stuff gets delayed to process context and queued properly. For example, we log MCEs from atomic context by putting them on a lockless buffer and then kicking irq_work to queue the work when we return to process context. Can you do something like that? "Hence, pstore calls efi_runtime_services() without using efi_rts_wq" - that doesn't sound like optimal design to me. I would try to shove them all through the workqueue - not have exceptions. Then this: > A potential issue could be, for instance, an NMI interrupt (like perf) > trying to profile some user data while in efi_pgd. I can't understand. How did we handle this until now and why is it a problem all of a sudden? Because I don't recall being unable to run perf while efi runtime services are happening. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. -- 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 12/12] efi: make const array 'apple' static
On Thu, 2018-03-08 at 08:00 +, Ard Biesheuvel wrote:
> From: Colin Ian King
>
> Don't populate the const read-only array 'buf' on the stack but instead
> make it static. Makes the object code smaller by 64 bytes:
>
> Before:
>text data bss dec hex filename
>9264 1 1692812441 arch/x86/boot/compressed/eboot.o
>
> After:
>text data bss dec hex filename
>9200 1 1692172401 arch/x86/boot/compressed/eboot.o
>
> (gcc version 7.2.0 x86_64)
>
> Signed-off-by: Colin Ian King
> Signed-off-by: Ard Biesheuvel
> ---
> arch/x86/boot/compressed/eboot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index 886a9115af62..f2251c1c9853 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct
> boot_params *boot_params)
>
> static void setup_quirks(struct boot_params *boot_params)
> {
> - efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
> + static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
Perhaps
static const efi_char16_t apple[] ...
is better.
> efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
> efi_table_attr(efi_system_table, fw_vendor, sys_table);
>
--
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 0/2] ESRT fixes for relocatable kexec'd kernel
Ard, On Wed, Mar 07, 2018 at 07:55:34PM +, Ard Biesheuvel wrote: > (+ James) > > Hello Akashi, > > On 6 March 2018 at 09:00, AKASHI Takahiro wrote: > > Tyler, Jeffrey, > > > > On Fri, Mar 02, 2018 at 08:27:11AM -0500, Tyler Baicar wrote: > >> On 3/2/2018 12:53 AM, AKASHI Takahiro wrote: > >> >Tyler, Jeffrey, > >> > > >> >[Note: This issue takes place in kexec, not kdump. So to be precise, > >> >it is not the same phenomenon as what I addressed in [1],[2]: > >> > [1] > >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-February/557254.html > >> > [2] > >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-January/553098.html > >> >] > >> > > >> >On Thu, Mar 01, 2018 at 12:56:38PM -0500, Tyler Baicar wrote: > >> >>Hello, > >> >> > >> >>On 2/28/2018 9:50 PM, AKASHI Takahiro wrote: > >> >>>Hi, > >> >>> > >> >>>On Wed, Feb 28, 2018 at 08:39:42AM -0700, Jeffrey Hugo wrote: > >> On 2/27/2018 11:19 PM, AKASHI Takahiro wrote: > >> >Tyler, > >> > > >> ># I missed catching your patch as its subject doesn't contain arm64. > >> > > >> >On Fri, Feb 23, 2018 at 12:42:31PM -0700, Tyler Baicar wrote: > >> >>Currently on arm64 ESRT memory does not appear to be properly > >> >>blocked off. > >> >>Upon successful initialization, ESRT prints out the memory region > >> >>that it > >> >>exists in like: > >> >> > >> >>esrt: Reserving ESRT space from 0x0a4c1c18 to > >> >>0x0a4c1cf0. > >> >> > >> >>But then by dumping /proc/iomem this region appears as part of > >> >>System RAM > >> >>rather than being reserved: > >> >> > >> >>08f1-0dee : System RAM > >> >> > >> >>This causes issues when trying to kexec if the kernel is > >> >>relocatable. When > >> >>kexec tries to execute, this memory can be selected to relocate the > >> >>kernel to > >> >>which then overwrites all the ESRT information. Then when the > >> >>kexec'd kernel > >> >>tries to initialize ESRT, it doesn't recognize the ESRT version > >> >>number and > >> >>just returns from efi_esrt_init(). > >> >I'm not sure what is the root cause of your problem. > >> >Do you have good confidence that the kernel (2nd kernel image in this > >> >case?) > >> >really overwrite ESRT region? > >> According to my debug, yes. > >> Using JTAG, I was able to determine that the ESRT memory region was > >> getting > >> overwritten by the secondary kernel in > >> kernel/arch/arm64/kernel/relocate_kernel.S - specifically the > >> "copy_page" > >> line of arm64_relocate_new_kernel() > >> > >> >To my best knowledge, kexec is carefully designed not to do such a > >> >thing > >> >as it allocates a temporary buffer for kernel image and copies it to > >> >the > >> >final destination at the very end of the 1st kernel. > >> > > >> >My guess is that kexec, or rather kexec-tools, tries to load the > >> >kernel image > >> >at 0x8f8 (or 0x908?, not sure) in your case. It may or may > >> >not be > >> >overlapped with ESRT. > >> >(Try "-d" option when executing kexec command for confirmation.) > >> With -d, I see > >> > >> get_memory_ranges_iomem_cb: 09611000 - 0e5f : > >> System RAM > >> > >> That overlaps the ESRT reservation - > >> [ 0.00] esrt: Reserving ESRT space from 0x0b708718 to > >> 0x0b7087f0 > >> > >> >Are you using initrd with kexec? > >> Yes > >> >>>To make the things clear, can you show me, if possible, the followings: > >> >>I have attached all of these: > >> >Many thanks. > >> >According to the data, ESRT was overwritten by initrd, not the kernel > >> >image. > >> >It doesn't matter to you though :) > >> > > >> >The solution would be, as Ard suggested, that more information be > >> >added to /proc/iomem. > >> >I'm going to fix the issue as quickly as possible. > >> Great, thank you!! Please add us to the fix and we will gladly test it out. > > > > I have created a workaround patch, attached below, as a kind of PoC. > > Can you give it a go, please? > > You need another patch for kexec-tools, too. See > > https:/git.linaro.org/people/takahiro.akashi/kexecl-tools.git arm64/resv_mem > > > > Thanks for putting this together. Some questions below. > > > With this patch, extra entries for firmware-reserved memory resources, > > which means any regions that are already reserved before > > arm64_memblock_init(), > > or specifically efi/acpi tables in this case, are added to /proc/iomem. > > > > $ cat /proc/iomem (on my qemu+edk2 execution) > > ... > > 4000-5871 : System RAM > > 4008-40f1 : Kernel code > > 4104-411e9fff : Kernel data > > 5440-583f : Crash kernel > > 5859-585e : reserved > > 5870-5871 : reserve
Re: Regression from efi: call get_event_log before ExitBootServices
---
drivers/firmware/efi/libstub/tpm.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/firmware/efi/libstub/tpm.c
b/drivers/firmware/efi/libstub/tpm.c
index da661bf8cb96..773afcd6a37c 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -74,19 +74,23 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t
*sys_table_arg)
efi_bool_t truncated;
void *tcg2_protocol;
+ efi_printk(sys_table_arg, "Locating the TCG2Protocol\n");
status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
&tcg2_protocol);
if (status != EFI_SUCCESS)
return;
+ efi_printk(sys_table_arg, "Calling GetEventLog on TCG2Protocol\n");
status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol,
EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
&log_location, &log_last_entry, &truncated);
if (status != EFI_SUCCESS)
return;
+ efi_printk(sys_table_arg, "Log returned\n");
if (!log_location)
return;
+ efi_printk(sys_table_arg, "log_location is not empty\n");
first_entry_addr = (unsigned long) log_location;
/*
@@ -94,7 +98,9 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t
*sys_table_arg)
*/
if (!log_last_entry) {
log_size = 0;
+ efi_printk(sys_table_arg, "log_size = 0\n");
} else {
+ efi_printk(sys_table_arg, "log_size != 0\n");
last_entry_addr = (unsigned long) log_last_entry;
/*
* get_event_log only returns the address of the last entry.
@@ -106,26 +112,31 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t
*sys_table_arg)
log_size = log_last_entry - log_location + last_entry_size;
}
+ efi_printk(sys_table_arg, "Allocating memory for storing the logs\n");
/* Allocate space for the logs and copy them. */
status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
sizeof(*log_tbl) + log_size,
(void **) &log_tbl);
+ efi_printk(sys_table_arg, "Returned from memory allocation\n");
if (status != EFI_SUCCESS) {
efi_printk(sys_table_arg,
"Unable to allocate memory for event log\n");
return;
}
+ efi_printk(sys_table_arg, "Copying log to new location\n");
memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
log_tbl->size = log_size;
log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
+ efi_printk(sys_table_arg, "Installing the log into the configuration
table\n");
status = efi_call_early(install_configuration_table,
&linux_eventlog_guid, log_tbl);
if (status != EFI_SUCCESS)
goto err_free;
+ efi_printk(sys_table_arg, "Done\n");
return;
err_free:
--
2.16.2.395.g2e18187dfd-goog
--
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 V2 2/3] efi: Introduce efi_rts_workqueue and some infrastructure to invoke all efi_runtime_services()
On Thu, Mar 8, 2018 at 5:22 AM, Prakhya, Sai Praneeth
wrote:
>> > +struct workqueue_struct *efi_rts_wq;
>> > +
>> > static bool disable_runtime;
>> > static int __init setup_noefi(char *arg) { @@ -329,6 +331,19 @@
>> > static int __init efisubsys_init(void)
>> > return 0;
>> >
>> > /*
>> > +* Since we process only one efi_runtime_service() at a time, an
>> > +* ordered workqueue (which creates only one execution context)
>> > +* should suffice all our needs.
>> > +*/
>> > + efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue", 0);
>>
>> efi_rts_wq or efi_rts_workqueue?
>>
>> > + if (!efi_rts_wq) {
>> > + pr_err("Failed to create efi_rts_workqueue, EFI runtime
>> > services "
>>
>> Same here.
>
> Sure! I will make it consistent with "efi_rts_wq". Just tried to be more
> verbose
> with names :)
>
It is not a big deal, but using the exact same name is better for the
purposes of grepping and things like that :-) By the way, check the
commit title/message, there are some others there too.
> [...]
>
>> > +#define efi_queue_work(_rts, _arg1, _arg2, _arg3, _arg4, _arg5)
>> > \
>> > +({ \
>> > + struct efi_runtime_work efi_rts_work; \
>> > + \
>> > + INIT_WORK_ONSTACK(&efi_rts_work.work, efi_call_rts);\
>> > + efi_rts_work.func = _rts; \
>> > + efi_rts_work.arg1 = _arg1; \
>> > + efi_rts_work.arg2 = _arg2; \
>> > + efi_rts_work.arg3 = _arg3; \
>> > + efi_rts_work.arg4 = _arg4; \
>> > + efi_rts_work.arg5 = _arg5; \
>> > + /* \
>> > +* queue_work() returns 0 if work was already on queue, \
>> > +* _ideally_ this should never happen. \
>> > +*/ \
>> > + if (queue_work(efi_rts_wq, &efi_rts_work.work)) \
>> > + flush_work(&efi_rts_work.work); \
>> > + else\
>> > + BUG(); \
>>
>> Thanks for the change! One remark, I would just do:
>
> Sorry! but I am planning to remove BUG(). Looks like it could defeat the
> purpose
> of patch. Please see Boris comments on the other thread.
No problem. Let's see how it looks in v3 :-)
>
> [...]
>
>> > +/*
>> > + * efi_runtime_work: Details of EFI Runtime Service work
>> > + * @func: EFI Runtime Service function identifier
>> > + * @arg<1-5>: EFI Runtime Service function arguments
>> > + * @status:Status of executing EFI Runtime Service
>> > + */
>> > +struct efi_runtime_work {
>> > + u8 func;
>> > + void *arg1;
>> > + void *arg2;
>> > + void *arg3;
>> > + void *arg4;
>> > + void *arg5;
>> > + efi_status_t status;
>> > + struct work_struct work;
>> > +};
>>
>> Why is efi_runtime_work in the .h at all?
>>
>
> Thanks for the catch. I will move it to runtime-wrappers.c file and will make
> it
> static too. It isn't being used in any other place.
>
>> Please CC me for the next version! :-)
>
> Sure! Sorry for that. I should have done in V2.
Thanks!
Cheers,
Miguel
>
> Regards,
> Sai
--
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: Regression from efi: call get_event_log before ExitBootServices
On Wed, Mar 7, 2018 at 6:33 PM Jeremy Cline wrote: > On 03/07/2018 03:41 AM, Thiebaud Weksteen wrote: > > Hi, > > > > Thanks for testing and sending this report! This patch relies heavily on > > the functions exposed by the firmware. My first guess would be that some of > > these may not be implemented correctly by the manufacturer. > > > > Could you share more information on this specific device? > > Do you have any link to the manufacturer website (I found [1] but it is > > based on an ARM CPU)? > > Do you have the option to update your firmware? Is a copy of the firmware > > available from the manufacturer? > I couldn't find a copy of the firmware, unfortunately. No worries, thanks for looking that up. > > On your side, I assume no error message got displayed on the screen when > > booting. Would you be able to try to boot in an UEFI shell [2] and execute > > the command "dh -v"? > Yup, no errors on the screen. I've attached the output of dh -v from the > UEFI shell. Great, thanks for that. There is a module that exposes the EfiTcg2Protocol (TrEEDxe). So I'm going to assume this is properly located and then called. Unfortunately, this is so early in the boot that we can only rely on the EFI functions for logging/debugging. Jeremy, Hans, could you both describe precisely how your boot is configured? This feature is only triggered when booting the EFI stub of the kernel so this may be not executed if you are using something else in between. Jeremy, would you be able to modify the efi_retrieve_tpm2_eventlog_1_2 function to add multiple efi_printk(sys_table_arg, "message\n"); to test: if you get the output on your screen; and isolate which call might be the cause of the hang? I can forward a debug patch if that helps. Thanks > Regards, > Jeremy -- 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
[PATCH 02/12] efi/apple-properties: Device core takes care of empty properties
From: Andy Shevchenko
There is no need to artificially supply a property length and fake data
if property has type of boolean.
Remove redundant piece of data and code.
Signed-off-by: Andy Shevchenko
Reviewed-and-tested-by: Lukas Wunner
Signed-off-by: Ard Biesheuvel
---
drivers/firmware/efi/apple-properties.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/firmware/efi/apple-properties.c
b/drivers/firmware/efi/apple-properties.c
index 9f6bcf173b0e..b9602e0d7b50 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -52,8 +52,6 @@ struct properties_header {
struct dev_header dev_header[0];
};
-static u8 one __initdata = 1;
-
static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
struct device *dev, void *ptr,
struct property_entry entry[])
@@ -95,14 +93,9 @@ static void __init unmarshal_key_value_pairs(struct
dev_header *dev_header,
key_len - sizeof(key_len));
entry[i].name = key;
- entry[i].is_array = true;
entry[i].length = val_len - sizeof(val_len);
+ entry[i].is_array = !!entry[i].length;
entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
- if (!entry[i].length) {
- /* driver core doesn't accept empty properties */
- entry[i].length = 1;
- entry[i].pointer.raw_data = &one;
- }
if (dump_properties) {
dev_info(dev, "property: %s\n", entry[i].name);
--
2.15.1
--
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
[PATCH 01/12] efi/arm*: Only register page tables when they exist
From: Mark Rutland
Currently the arm/arm64 runtime code registers the runtime servies
pagetables with ptdump regardless of whether runtime services page
tables have been created.
As efi_mm.pgd is NULL in these cases, attempting to dump the efi page
tables results in a NULL pointer dereference in the ptdump code:
/sys/kernel/debug# cat efi_page_tables
[ 479.522600] Unable to handle kernel NULL pointer dereference at virtual
address
[ 479.522715] Mem abort info:
[ 479.522764] ESR = 0x9606
[ 479.522850] Exception class = DABT (current EL), IL = 32 bits
[ 479.522899] SET = 0, FnV = 0
[ 479.522937] EA = 0, S1PTW = 0
[ 479.528200] Data abort info:
[ 479.528230] ISV = 0, ISS = 0x0006
[ 479.528317] CM = 0, WnR = 0
[ 479.528317] user pgtable: 4k pages, 48-bit VAs, pgd = 64ab0cb0
[ 479.528449] [] *pgd=fbbe4003, *pud=fb66e003,
*pmd=
[ 479.528600] Internal error: Oops: 9606 [#1] PREEMPT SMP
[ 479.528664] Modules linked in:
[ 479.528699] CPU: 0 PID: 2457 Comm: cat Not tainted
4.15.0-rc3-00065-g2ad2ee7ecb5c-dirty #7
[ 479.528799] Hardware name: FVP Base (DT)
[ 479.528899] pstate: 0049 (nzcv daif +PAN -UAO)
[ 479.528941] pc : walk_pgd.isra.1+0x20/0x1d0
[ 479.529011] lr : ptdump_walk_pgd+0x30/0x50
[ 479.529105] sp : 0bf4bc20
[ 479.529185] x29: 0bf4bc20 x28: 9d22e000
[ 479.529271] x27: 0002 x26: 80007b4c63c0
[ 479.529358] x25: 014000c0 x24: 80007c098900
[ 479.529445] x23: 0bf4beb8 x22:
[ 479.529532] x21: 0bf4bd70 x20: 0001
[ 479.529618] x19: 0bf4bcb0 x18:
[ 479.529760] x17: 0041a1c8 x16: 082139d8
[ 479.529800] x15: 9d3c6030 x14: 9d2527f4
[ 479.529924] x13: 03f3 x12: 0038
[ 479.53] x11: 0003 x10: 0101010101010101
[ 479.530099] x9 : 17e94050 x8 : 003f
[ 479.530226] x7 : x6 :
[ 479.530313] x5 : 0001 x4 :
[ 479.530416] x3 : 09069fd8 x2 :
[ 479.530500] x1 : x0 :
[ 479.530599] Process cat (pid: 2457, stack limit = 0x5d1b0e6f)
[ 479.530660] Call trace:
[ 479.530746] walk_pgd.isra.1+0x20/0x1d0
[ 479.530833] ptdump_walk_pgd+0x30/0x50
[ 479.530907] ptdump_show+0x10/0x20
[ 479.530920] seq_read+0xc8/0x470
[ 479.531023] full_proxy_read+0x60/0x90
[ 479.531100] __vfs_read+0x18/0x100
[ 479.531180] vfs_read+0x88/0x160
[ 479.531267] SyS_read+0x48/0xb0
[ 479.531299] el0_svc_naked+0x20/0x24
[ 479.531400] Code: 91400420 f90033a0 a90707a2 f9403fa0 (f940)
[ 479.531499] ---[ end trace bfe8e28d8acb2b67 ]---
Segmentation fault
Let's avoid this problem by only registering the tables after their
successful creation, which is also less confusing when EFI runtime
services are not in use.
Reported-by: Will Deacon
Signed-off-by: Mark Rutland
Acked-by: Will Deacon
Signed-off-by: Ard Biesheuvel
---
drivers/firmware/efi/arm-runtime.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/firmware/efi/arm-runtime.c
b/drivers/firmware/efi/arm-runtime.c
index 1cc41c3d6315..86a1ad17a32e 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -54,6 +54,9 @@ static struct ptdump_info efi_ptdump_info = {
static int __init ptdump_init(void)
{
+ if (!efi_enabled(EFI_RUNTIME_SERVICES))
+ return 0;
+
return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables");
}
device_initcall(ptdump_init);
--
2.15.1
--
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
[PATCH 03/12] efi/arm*: Stop printing addresses of virtual mappings
With the recent %p -> %px changes, we now get something like this in
the kernel boot log on ARM/arm64 EFI systems:
Remapping and enabling EFI services.
EFI remap 0x0087fb83 => (ptrval)
EFI remap 0x0087fbdb => (ptrval)
EFI remap 0x0087fffc => (ptrval)
The physical addresses of the UEFI runtime regions will also be
printed when booting with the efi=debug command line option, and the
virtual addresses can be inspected via /sys/kernel/debug/efi_page_tables
(if enabled). So let's just remove the lines above.
Acked-by: Leif Lindholm
Signed-off-by: Ard Biesheuvel
---
drivers/firmware/efi/arm-runtime.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/firmware/efi/arm-runtime.c
b/drivers/firmware/efi/arm-runtime.c
index 86a1ad17a32e..13561aeb7396 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -83,10 +83,7 @@ static bool __init efi_virtmap_init(void)
return false;
ret = efi_create_mapping(&efi_mm, md);
- if (!ret) {
- pr_info(" EFI remap %pa => %p\n",
- &phys, (void *)(unsigned long)md->virt_addr);
- } else {
+ if (ret) {
pr_warn(" EFI remap %pa: failed to create mapping
(%d)\n",
&phys, ret);
return false;
--
2.15.1
--
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
[PATCH 05/12] efi: arm64: Check whether x18 is preserved by runtime services calls
Whether or not we will ever decide to start using x18 as a platform
register in Linux is uncertain, but by that time, we will need to
ensure that UEFI runtime services calls don't corrupt it. So let's
start issuing warnings now for this, and increase the likelihood that
these firmware images have all been replaced by that time.
This has been fixed on the EDK2 side in commit 6d73863b5464
("BaseTools/tools_def AARCH64: mark register x18 as reserved").,
dated July 13, 2017.
Acked-by: Will Deacon
Signed-off-by: Ard Biesheuvel
---
arch/arm64/include/asm/efi.h | 4 +++-
arch/arm64/kernel/Makefile | 3 ++-
arch/arm64/kernel/efi-rt-wrapper.S | 41 ++
arch/arm64/kernel/efi.c| 6 ++
4 files changed, 52 insertions(+), 2 deletions(-)
create mode 100644 arch/arm64/kernel/efi-rt-wrapper.S
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8389050328bb..192d791f1103 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -31,7 +31,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm,
efi_memory_desc_t *md);
({ \
efi_##f##_t *__f; \
__f = p->f; \
- __f(args); \
+ __efi_rt_asm_wrapper(__f, #f, args);\
})
#define arch_efi_call_virt_teardown() \
@@ -40,6 +40,8 @@ int efi_set_mapping_permissions(struct mm_struct *mm,
efi_memory_desc_t *md);
efi_virtmap_unload(); \
})
+efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);
+
#define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
/* arch specific definitions used by the stub code */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index b87541360f43..6a4bd80c75bd 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -38,7 +38,8 @@ arm64-obj-$(CONFIG_CPU_PM)+= sleep.o suspend.o
arm64-obj-$(CONFIG_CPU_IDLE) += cpuidle.o
arm64-obj-$(CONFIG_JUMP_LABEL) += jump_label.o
arm64-obj-$(CONFIG_KGDB) += kgdb.o
-arm64-obj-$(CONFIG_EFI)+= efi.o efi-entry.stub.o
+arm64-obj-$(CONFIG_EFI)+= efi.o efi-entry.stub.o
\
+ efi-rt-wrapper.o
arm64-obj-$(CONFIG_PCI)+= pci.o
arm64-obj-$(CONFIG_ARMV8_DEPRECATED) += armv8_deprecated.o
arm64-obj-$(CONFIG_ACPI) += acpi.o
diff --git a/arch/arm64/kernel/efi-rt-wrapper.S
b/arch/arm64/kernel/efi-rt-wrapper.S
new file mode 100644
index ..05235ebb336d
--- /dev/null
+++ b/arch/arm64/kernel/efi-rt-wrapper.S
@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 2018 Linaro Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+
+ENTRY(__efi_rt_asm_wrapper)
+ stp x29, x30, [sp, #-32]!
+ mov x29, sp
+
+ /*
+* Register x18 is designated as the 'platform' register by the AAPCS,
+* which means firmware running at the same exception level as the OS
+* (such as UEFI) should never touch it.
+*/
+ stp x1, x18, [sp, #16]
+
+ /*
+* We are lucky enough that no EFI runtime services take more than
+* 5 arguments, so all are passed in registers rather than via the
+* stack.
+*/
+ mov x8, x0
+ mov x0, x2
+ mov x1, x3
+ mov x2, x4
+ mov x3, x5
+ mov x4, x6
+ blr x8
+
+ ldp x1, x2, [sp, #16]
+ cmp x2, x18
+ ldp x29, x30, [sp], #32
+ b.ne0f
+ ret
+0: b efi_handle_corrupted_x18// tail call
+ENDPROC(__efi_rt_asm_wrapper)
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index f85ac58d08a3..fb5b3cd3a1c7 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -126,3 +126,9 @@ bool efi_poweroff_required(void)
{
return efi_enabled(EFI_RUNTIME_SERVICES);
}
+
+asmlinkage efi_status_t efi_handle_corrupted_x18(efi_status_t s, const char *f)
+{
+ pr_err_ratelimited(FW_BUG "register x18 corrupted by EFI %s\n", f);
+ return s;
+}
--
2.15.1
--
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
[PATCH 06/12] x86: efi: Replace GFP_ATOMIC with GFP_KERNEL in efi_query_variable_store
From: Jia-Ju Bai The function kzalloc here is not called in atomic context. If nonblocking in efi_query_variable_store is true, namely it is in atomic context, efi_query_variable_store will return before this kzalloc is called. Thus GFP_ATOMIC is not necessary, and it can be replaced with GFP_KERNEL. This is found by a static analysis tool named DCNS written by myself. Signed-off-by: Jia-Ju Bai Reviewed-by: Ingo Molnar Signed-off-by: Ard Biesheuvel --- arch/x86/platform/efi/quirks.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c index 5b513ccffde4..1ef11c26f79b 100644 --- a/arch/x86/platform/efi/quirks.c +++ b/arch/x86/platform/efi/quirks.c @@ -177,7 +177,7 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size, * that by attempting to use more space than is available. */ unsigned long dummy_size = remaining_size + 1024; - void *dummy = kzalloc(dummy_size, GFP_ATOMIC); + void *dummy = kzalloc(dummy_size, GFP_KERNEL); if (!dummy) return EFI_OUT_OF_RESOURCES; -- 2.15.1 -- 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
[PATCH 07/12] efi: Use efi_mm in x86 as well as ARM
From: Sai Praneeth
Presently, only ARM uses mm_struct to manage efi page tables and efi
runtime region mappings. As this is the preferred approach, let's make
this data structure common across architectures. Specially, for x86,
using this data structure improves code maintainability and readability.
Signed-off-by: Sai Praneeth Prakhya
Cc: "Lee, Chun-Yi"
Cc: Borislav Petkov
Cc: Tony Luck
Cc: Andy Lutomirski
Cc: Michael S. Tsirkin
Cc: Ricardo Neri
Cc: Ravi Shankar
Tested-by: Bhupesh Sharma
Reviewed-by: Matt Fleming
Signed-off-by: Ard Biesheuvel
---
arch/x86/include/asm/efi.h | 4
arch/x86/platform/efi/efi_64.c | 3 +++
drivers/firmware/efi/arm-runtime.c | 9 -
drivers/firmware/efi/efi.c | 9 +
include/linux/efi.h| 2 ++
5 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 85f6ccb80b91..00f977ddd718 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -2,10 +2,14 @@
#ifndef _ASM_X86_EFI_H
#define _ASM_X86_EFI_H
+#include
+#include
+
#include
#include
#include
#include
+#include
/*
* We map the EFI regions needed for runtime services non-contiguously,
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index c310a8284358..0045efe9947b 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -231,6 +231,9 @@ int __init efi_alloc_page_tables(void)
return -ENOMEM;
}
+ mm_init_cpumask(&efi_mm);
+ init_new_context(NULL, &efi_mm);
+
return 0;
}
diff --git a/drivers/firmware/efi/arm-runtime.c
b/drivers/firmware/efi/arm-runtime.c
index 13561aeb7396..5889cbea60b8 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -31,15 +31,6 @@
extern u64 efi_system_table;
-static struct mm_struct efi_mm = {
- .mm_rb = RB_ROOT,
- .mm_users = ATOMIC_INIT(2),
- .mm_count = ATOMIC_INIT(1),
- .mmap_sem = __RWSEM_INITIALIZER(efi_mm.mmap_sem),
- .page_table_lock= __SPIN_LOCK_UNLOCKED(efi_mm.page_table_lock),
- .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
-};
-
#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
#include
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index cd42f66a7c85..c0dda400d22a 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -75,6 +75,15 @@ static unsigned long *efi_tables[] = {
&efi.mem_attr_table,
};
+struct mm_struct efi_mm = {
+ .mm_rb = RB_ROOT,
+ .mm_users = ATOMIC_INIT(2),
+ .mm_count = ATOMIC_INIT(1),
+ .mmap_sem = __RWSEM_INITIALIZER(efi_mm.mmap_sem),
+ .page_table_lock= __SPIN_LOCK_UNLOCKED(efi_mm.page_table_lock),
+ .mmlist = LIST_HEAD_INIT(efi_mm.mmlist),
+};
+
static bool disable_runtime;
static int __init setup_noefi(char *arg)
{
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f5083aa72eae..f1b7d68ac460 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -966,6 +966,8 @@ extern struct efi {
unsigned long flags;
} efi;
+extern struct mm_struct efi_mm;
+
static inline int
efi_guidcmp (efi_guid_t left, efi_guid_t right)
{
--
2.15.1
--
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
[PATCH 09/12] x86/efi: Use efi_switch_mm() rather than manually twiddling with %cr3
From: Sai Praneeth
Use helper function (efi_switch_mm()) to switch to/from efi_mm. We
switch to efi_mm before calling
1. efi_set_virtual_address_map() and
2. Invoking any efi_runtime_service()
Likewise, we need to switch back to previous mm (mm context stolen by
efi_mm) after the above calls return successfully. We can use
efi_switch_mm() helper function only with x86_64 kernel and
"efi=old_map" disabled because, x86_32 and efi=old_map doesn't use
efi_pgd, rather they use swapper_pg_dir.
Signed-off-by: Sai Praneeth Prakhya
Cc: "Lee, Chun-Yi"
Cc: Borislav Petkov
Cc: Tony Luck
Cc: Andy Lutomirski
Cc: Michael S. Tsirkin
Cc: Bhupesh Sharma
Cc: Ricardo Neri
Cc: Ravi Shankar
Tested-by: Bhupesh Sharma
Reviewed-by: Matt Fleming
Signed-off-by: Ard Biesheuvel
---
arch/x86/include/asm/efi.h | 25 +-
arch/x86/platform/efi/efi_64.c | 40 +++-
arch/x86/platform/efi/efi_thunk_64.S | 2 +-
3 files changed, 32 insertions(+), 35 deletions(-)
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 00f977ddd718..cda9940bed7a 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -62,14 +62,13 @@ extern asmlinkage u64 efi_call(void *fp, ...);
#define efi_call_phys(f, args...) efi_call((f), args)
/*
- * Scratch space used for switching the pagetable in the EFI stub
+ * struct efi_scratch - Scratch space used while switching to/from efi_mm
+ * @phys_stack: stack used during EFI Mixed Mode
+ * @prev_mm:store/restore stolen mm_struct while switching to/from efi_mm
*/
struct efi_scratch {
- u64 r15;
- u64 prev_cr3;
- pgd_t *efi_pgt;
- booluse_pgd;
- u64 phys_stack;
+ u64 phys_stack;
+ struct mm_struct*prev_mm;
} __packed;
#define arch_efi_call_virt_setup() \
@@ -78,11 +77,8 @@ struct efi_scratch {
preempt_disable(); \
__kernel_fpu_begin(); \
\
- if (efi_scratch.use_pgd) { \
- efi_scratch.prev_cr3 = __read_cr3();\
- write_cr3((unsigned long)efi_scratch.efi_pgt); \
- __flush_tlb_all(); \
- } \
+ if (!efi_enabled(EFI_OLD_MEMMAP)) \
+ efi_switch_mm(&efi_mm); \
})
#define arch_efi_call_virt(p, f, args...) \
@@ -90,10 +86,8 @@ struct efi_scratch {
#define arch_efi_call_virt_teardown() \
({ \
- if (efi_scratch.use_pgd) { \
- write_cr3(efi_scratch.prev_cr3);\
- __flush_tlb_all(); \
- } \
+ if (!efi_enabled(EFI_OLD_MEMMAP)) \
+ efi_switch_mm(efi_scratch.prev_mm); \
\
__kernel_fpu_end(); \
preempt_enable(); \
@@ -135,6 +129,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 efi_switch_mm(struct mm_struct *mm);
struct efi_setup_data {
u64 fw_vendor;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 8881e601c32d..55c3f623ac49 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -81,9 +81,8 @@ pgd_t * __init efi_call_phys_prolog(void)
int n_pgds, i, j;
if (!efi_enabled(EFI_OLD_MEMMAP)) {
- save_pgd = (pgd_t *)__read_cr3();
- write_cr3((unsigned long)efi_scratch.efi_pgt);
- goto out;
+ efi_switch_mm(&efi_mm);
+ return NULL;
}
early_code_mapping_set_exec(1);
@@ -155,8 +154,7 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
pud_t *pud;
if (!efi_enabled(EFI_OLD_MEMMAP)) {
- write_cr3((unsigned long)save_pgd);
- __flush_tlb_all();
+ efi_switch_mm(efi_scratch.prev_mm);
return;
}
@@ -344,13 +342,6 @@ int __init efi_setup_page_tables(unsigned long pa_memmap,
unsigned num_pages)
if (efi_enabled
[PATCH 12/12] efi: make const array 'apple' static
From: Colin Ian King
Don't populate the const read-only array 'buf' on the stack but instead
make it static. Makes the object code smaller by 64 bytes:
Before:
textdata bss dec hex filename
9264 1 1692812441 arch/x86/boot/compressed/eboot.o
After:
textdata bss dec hex filename
9200 1 1692172401 arch/x86/boot/compressed/eboot.o
(gcc version 7.2.0 x86_64)
Signed-off-by: Colin Ian King
Signed-off-by: Ard Biesheuvel
---
arch/x86/boot/compressed/eboot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 886a9115af62..f2251c1c9853 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -423,7 +423,7 @@ static void retrieve_apple_device_properties(struct
boot_params *boot_params)
static void setup_quirks(struct boot_params *boot_params)
{
- efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
+ static efi_char16_t const apple[] = { 'A', 'p', 'p', 'l', 'e', 0 };
efi_char16_t *fw_vendor = (efi_char16_t *)(unsigned long)
efi_table_attr(efi_system_table, fw_vendor, sys_table);
--
2.15.1
--
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
[PATCH 08/12] x86/efi: Replace efi_pgd with efi_mm.pgd
From: Sai Praneeth
Since the previous patch added support for efi_mm, let's handle efi_pgd
through efi_mm and remove global variable efi_pgd.
Signed-off-by: Sai Praneeth Prakhya
Cc: "Lee, Chun-Yi"
Cc: Borislav Petkov
Cc: Tony Luck
Cc: Andy Lutomirski
Cc: Michael S. Tsirkin
Cc: Bhupesh Sharma
Cc: Ricardo Neri
Cc: Ravi Shankar
Tested-by: Bhupesh Sharma
Reviewed-by: Matt Fleming
Signed-off-by: Ard Biesheuvel
---
arch/x86/platform/efi/efi_64.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 0045efe9947b..8881e601c32d 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -190,8 +190,6 @@ void __init efi_call_phys_epilog(pgd_t *save_pgd)
early_code_mapping_set_exec(0);
}
-static pgd_t *efi_pgd;
-
/*
* We need our own copy of the higher levels of the page tables
* because we want to avoid inserting EFI region mappings (EFI_VA_END
@@ -203,7 +201,7 @@ static pgd_t *efi_pgd;
*/
int __init efi_alloc_page_tables(void)
{
- pgd_t *pgd;
+ pgd_t *pgd, *efi_pgd;
p4d_t *p4d;
pud_t *pud;
gfp_t gfp_mask;
@@ -231,6 +229,7 @@ int __init efi_alloc_page_tables(void)
return -ENOMEM;
}
+ efi_mm.pgd = efi_pgd;
mm_init_cpumask(&efi_mm);
init_new_context(NULL, &efi_mm);
@@ -246,6 +245,7 @@ void efi_sync_low_kernel_mappings(void)
pgd_t *pgd_k, *pgd_efi;
p4d_t *p4d_k, *p4d_efi;
pud_t *pud_k, *pud_efi;
+ pgd_t *efi_pgd = efi_mm.pgd;
if (efi_enabled(EFI_OLD_MEMMAP))
return;
@@ -339,7 +339,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap,
unsigned num_pages)
unsigned long pfn, text, pf;
struct page *page;
unsigned npages;
- pgd_t *pgd;
+ pgd_t *pgd = efi_mm.pgd;
if (efi_enabled(EFI_OLD_MEMMAP))
return 0;
@@ -349,8 +349,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap,
unsigned num_pages)
* this value is loaded into cr3 the PGD will be decrypted during
* the pagetable walk.
*/
- efi_scratch.efi_pgt = (pgd_t *)__sme_pa(efi_pgd);
- pgd = efi_pgd;
+ efi_scratch.efi_pgt = (pgd_t *)__sme_pa(pgd);
/*
* It can happen that the physical address of new_memmap lands in memory
@@ -420,7 +419,7 @@ static void __init __map_region(efi_memory_desc_t *md, u64
va)
{
unsigned long flags = _PAGE_RW;
unsigned long pfn;
- pgd_t *pgd = efi_pgd;
+ pgd_t *pgd = efi_mm.pgd;
if (!(md->attribute & EFI_MEMORY_WB))
flags |= _PAGE_PCD;
@@ -524,7 +523,7 @@ void __init parse_efi_setup(u64 phys_addr, u32 data_len)
static int __init efi_update_mappings(efi_memory_desc_t *md, unsigned long pf)
{
unsigned long pfn;
- pgd_t *pgd = efi_pgd;
+ pgd_t *pgd = efi_mm.pgd;
int err1, err2;
/* Update the 1:1 mapping */
@@ -621,7 +620,7 @@ void __init efi_dump_pagetable(void)
if (efi_enabled(EFI_OLD_MEMMAP))
ptdump_walk_pgd_level(NULL, swapper_pg_dir);
else
- ptdump_walk_pgd_level(NULL, efi_pgd);
+ ptdump_walk_pgd_level(NULL, efi_mm.pgd);
#endif
}
--
2.15.1
--
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
[PATCH 04/12] efi/x86: Fix trailing semicolons
From: Luis de Bethencourt The trailing semicolon is an empty statement that does no operation. Removing them since they don't do anything. Signed-off-by: Luis de Bethencourt Signed-off-by: Ard Biesheuvel --- arch/x86/boot/compressed/eboot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index 353e20c3f114..886a9115af62 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -439,7 +439,7 @@ setup_uga32(void **uga_handle, unsigned long size, u32 *width, u32 *height) struct efi_uga_draw_protocol *uga = NULL, *first_uga; efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID; unsigned long nr_ugas; - u32 *handles = (u32 *)uga_handle;; + u32 *handles = (u32 *)uga_handle; efi_status_t status = EFI_INVALID_PARAMETER; int i; @@ -484,7 +484,7 @@ setup_uga64(void **uga_handle, unsigned long size, u32 *width, u32 *height) struct efi_uga_draw_protocol *uga = NULL, *first_uga; efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID; unsigned long nr_ugas; - u64 *handles = (u64 *)uga_handle;; + u64 *handles = (u64 *)uga_handle; efi_status_t status = EFI_INVALID_PARAMETER; int i; -- 2.15.1 -- 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
[PATCH 11/12] efi/apple-properties: Use memremap() instead of ioremap()
From: Andy Shevchenko
The memory we are accessing through virtual address has no IO side
effects. Moreover, for IO memory we have to use special accessors,
which we don't use.
Due to above, convert the driver to use memremap() instead of ioremap().
Signed-off-by: Andy Shevchenko
Tested-by: Lukas Wunner
Signed-off-by: Ard Biesheuvel
---
drivers/firmware/efi/apple-properties.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/efi/apple-properties.c
b/drivers/firmware/efi/apple-properties.c
index b9602e0d7b50..adaa9a3714b9 100644
--- a/drivers/firmware/efi/apple-properties.c
+++ b/drivers/firmware/efi/apple-properties.c
@@ -19,6 +19,7 @@
#include
#include
+#include
#include
#include
#include
@@ -189,7 +190,7 @@ static int __init map_properties(void)
pa_data = boot_params.hdr.setup_data;
while (pa_data) {
- data = ioremap(pa_data, sizeof(*data));
+ data = memremap(pa_data, sizeof(*data), MEMREMAP_WB);
if (!data) {
pr_err("cannot map setup_data header\n");
return -ENOMEM;
@@ -197,14 +198,14 @@ static int __init map_properties(void)
if (data->type != SETUP_APPLE_PROPERTIES) {
pa_data = data->next;
- iounmap(data);
+ memunmap(data);
continue;
}
data_len = data->len;
- iounmap(data);
+ memunmap(data);
- data = ioremap(pa_data, sizeof(*data) + data_len);
+ data = memremap(pa_data, sizeof(*data) + data_len, MEMREMAP_WB);
if (!data) {
pr_err("cannot map setup_data payload\n");
return -ENOMEM;
@@ -229,7 +230,7 @@ static int __init map_properties(void)
* to avoid breaking the chain of ->next pointers.
*/
data->len = 0;
- iounmap(data);
+ memunmap(data);
free_bootmem_late(pa_data + sizeof(*data), data_len);
return ret;
--
2.15.1
--
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
[PATCH 10/12] efi: reorder pr_notice() with add_device_randomness() call
Currently, when we receive a random seed from the EFI stub, we call
add_device_randomness() to incorporate it into the entropy pool, and
issue a pr_notice() saying we are about to do that, e.g.,
[0.00] efi: RNG=0x87ff92cf18
[0.00] random: fast init done
[0.00] efi: seeding entropy pool
Let's reorder those calls to make the output look less confusing.
Signed-off-by: Ard Biesheuvel
---
drivers/firmware/efi/efi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index c0dda400d22a..232f4915223b 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -551,9 +551,9 @@ int __init efi_config_parse_tables(void *config_tables, int
count, int sz,
seed = early_memremap(efi.rng_seed,
sizeof(*seed) + size);
if (seed != NULL) {
+ pr_notice("seeding entropy pool\n");
add_device_randomness(seed->bits, seed->size);
early_memunmap(seed, sizeof(*seed) + size);
- pr_notice("seeding entropy pool\n");
} else {
pr_err("Could not map UEFI random seed!\n");
}
--
2.15.1
--
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
[GIT PULL 00/12] first batch of EFI changes for v4.17
The following changes since commit 7928b2cbe55b2a410a0f5c1f154610059c57b1b2: Linux 4.16-rc1 (2018-02-11 15:04:29 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git efi-next for you to fetch changes up to 67a2152c7362bd9ee3ba6d218e435a04c27c32aa: efi: make const array 'apple' static (2018-03-08 07:54:10 +) First batch of EFI changes for v4.17: - use efi_switch_mm() on x86 instead of manipulating %cr3 directly (Sai) - some fixes for the apple-properties code (Andy) - add WARN() on arm64 if UEFI Runtime Services corrupt the reserved x18 register (Ard) - other minor cleanups and bugfixes Andy Shevchenko (2): efi/apple-properties: Device core takes care of empty properties efi/apple-properties: Use memremap() instead of ioremap() Ard Biesheuvel (3): efi/arm*: Stop printing addresses of virtual mappings efi: arm64: Check whether x18 is preserved by runtime services calls efi: reorder pr_notice() with add_device_randomness() call Colin Ian King (1): efi: make const array 'apple' static Jia-Ju Bai (1): x86: efi: Replace GFP_ATOMIC with GFP_KERNEL in efi_query_variable_store Luis de Bethencourt (1): efi/x86: Fix trailing semicolons Mark Rutland (1): efi/arm*: Only register page tables when they exist Sai Praneeth (3): efi: Use efi_mm in x86 as well as ARM x86/efi: Replace efi_pgd with efi_mm.pgd x86/efi: Use efi_switch_mm() rather than manually twiddling with %cr3 arch/arm64/include/asm/efi.h| 4 ++- arch/arm64/kernel/Makefile | 3 +- arch/arm64/kernel/efi-rt-wrapper.S | 41 +++ arch/arm64/kernel/efi.c | 6 arch/x86/boot/compressed/eboot.c| 6 ++-- arch/x86/include/asm/efi.h | 29 - arch/x86/platform/efi/efi_64.c | 58 ++--- arch/x86/platform/efi/efi_thunk_64.S| 2 +- arch/x86/platform/efi/quirks.c | 2 +- drivers/firmware/efi/apple-properties.c | 20 drivers/firmware/efi/arm-runtime.c | 17 +++--- drivers/firmware/efi/efi.c | 11 ++- include/linux/efi.h | 2 ++ 13 files changed, 125 insertions(+), 76 deletions(-) create mode 100644 arch/arm64/kernel/efi-rt-wrapper.S -- 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
