[GIT PULL] EFI changes for v3.18

[GIT PULL] EFI changes for v3.18

[Matt Fleming]

Hi guys,

This pull request is coming later than I had intended, sorry about that.
It unfortunately got pushed out by a number of things, chasing bugs,
releases for other projects, etc.

Merging this tag into Linus' tree causes some conflicts, which I've
fixed up and pushed out as the efi-next-merge branch, in case you or
Linus want to take a look at it. The conflicts are fairly straight
forward to resolve.

The following changes since commit 99a5603e2a1f146ac0c6414d8a3669aa749ccff8:

  efi/arm64: Handle missing virtual mapping for UEFI System Table (2014-07-18 21:24:04 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git tags/efi-next

for you to fetch changes up to 1eb56e1adab89d36f071db4277a3c4610970215f:

  x86/efi: Adding efi_printks on memory allocationa and pci.reads (2014-09-28 17:43:21 +0100)

----------------------------------------------------------------
 * Implement new EFI runtime lock which is required by the UEFI
   specification - Ard Biesheuvel

 * Add new efi= x86 EFI boot stub parameter which mimics the way that
   the regular kernel parameter works - Matt Fleming

 * Improve error handling in efi-bgrt driver - Josh Triplett

 * Extend the "noefi" kernel parameter to arm64, add an efi=noruntime
   synonym - Dave Young

 * Beautify the EFI memory map logs - Laszlo Ersek

 * Fix compiler shadow warnings and improve variable names - Mark Rustad

 * Remove unused efi_call() macros, update comments, tag initialization
   functions with __init - Mathias Krause

 * Add more informative efi_printk()s to the x86 EFI boot stub at the
   firmware call-sites - Andre Müller

----------------------------------------------------------------
Andre Müller (1):
      x86/efi: Adding efi_printks on memory allocationa and pci.reads

Ard Biesheuvel (1):
      efi: Implement mandatory locking for UEFI Runtime Services

Dave Young (6):
      efi: Move noefi early param code out of x86 arch code
      lib: Add a generic cmdline parse function parse_option_str
      efi: Add kernel param efi=noruntime
      arm64/efi: uefi_init error handling fix
      arm64/efi: Do not enter virtual mode if booting with efi=noruntime or noefi
      x86/efi: Clear EFI_RUNTIME_SERVICES if failing to enter virtual mode

Josh Triplett (1):
      efi-bgrt: Add error handling; inform the user when ignoring the BGRT

Laszlo Ersek (5):
      efi: Add macro for EFI_MEMORY_UCE memory attribute
      efi: Introduce efi_md_typeattr_format()
      x86: efi: Format EFI memory type & attrs with efi_md_typeattr_format()
      ia64: efi: Format EFI memory type & attrs with efi_md_typeattr_format()
      arm64: efi: Format EFI memory type & attrs with efi_md_typeattr_format()

Mark Rustad (1):
      efi: Resolve some shadow warnings

Mathias Krause (4):
      x86/efi: Remove unused efi_call* macros
      x86/efi: Unexport add_efi_memmap variable
      x86/efi: Update comment regarding required phys mapped EFI services
      x86/efi: Mark initialization code as such

Matt Fleming (1):
      efi: Add efi= parameter parsing to the EFI boot stub

 Documentation/kernel-parameters.txt            |   8 +-
 arch/arm64/kernel/efi.c                        |  44 +++----
 arch/ia64/kernel/efi.c                         |   6 +-
 arch/x86/boot/compressed/eboot.c               |  32 +++--
 arch/x86/include/asm/efi.h                     |  33 ++----
 arch/x86/platform/efi/efi-bgrt.c               |  36 +++++-
 arch/x86/platform/efi/efi.c                    |  52 ++++-----
 arch/x86/platform/efi/efi_32.c                 |  12 +-
 arch/x86/platform/efi/efi_64.c                 |   6 +-
 arch/x86/platform/efi/efi_stub_32.S            |   4 +-
 drivers/firmware/efi/efi.c                     |  79 +++++++++++++
 drivers/firmware/efi/libstub/arm-stub.c        |   4 +
 drivers/firmware/efi/libstub/efi-stub-helper.c |  62 +++++++++-
 drivers/firmware/efi/runtime-wrappers.c        | 154 +++++++++++++++++++++++--
 drivers/firmware/efi/vars.c                    |  14 +--
 include/linux/efi.h                            |  11 ++
 include/linux/kernel.h                         |   1 +
 lib/cmdline.c                                  |  29 +++++
 18 files changed, 465 insertions(+), 122 deletions(-)

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [GIT PULL] EFI changes for v3.18

[Ingo Molnar]

* Matt Fleming <matt@console-pimps.org> wrote:

>  * Implement new EFI runtime lock which is required by the UEFI
>    specification - Ard Biesheuvel

Firstly, under what circumstances can EFI call parallelism happen 
currently? Most of the EFI code runs during early bootup, which 
is serialized.

Secondly, this locking pattern looks pretty disgusting:

@@ -94,7 +187,17 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
                                          unsigned long data_size,
                                          void *data)
 {
-       return efi_call_virt(set_variable, name, vendor, attr, data_size, data);
+       unsigned long flags;
+       efi_status_t status;
+       bool __in_nmi = efi_in_nmi();
+
+       if (!__in_nmi)
+               spin_lock_irqsave(&efi_runtime_lock, flags);
+       status = efi_call_virt(set_variable, name, vendor, attr, data_size,
+                              data);
+       if (!__in_nmi)
+               spin_unlock_irqrestore(&efi_runtime_lock, flags);
+       return status;
 }

and that's repeated in virt_efi_query_variable_info() as well.

and that's the explanation given:

+/*
+ * Some runtime services calls can be reentrant under NMI, even if the table
+ * above says they are not. (source: UEFI Specification v2.4A)
+ *
+ * Table 32. Functions that may be called after Machine Check, INIT and NMI
+ * 
+----------------------------+------------------------------------------+
+ * | Function                  | Called after Machine Check, INIT and NMI |
+ * 
+----------------------------+------------------------------------------+
+ * | GetTime()                 | Yes, even if previously busy.            |
+ * | GetVariable()             | Yes, even if previously busy             |
+ * | GetNextVariableName()     | Yes, even if previously busy             |
+ * | QueryVariableInfo()       | Yes, even if previously busy             |
+ * | SetVariable()             | Yes, even if previously busy             |
+ * | UpdateCapsule()           | Yes, even if previously busy             |
+ * | QueryCapsuleCapabilities()| Yes, even if previously busy             |
+ * | ResetSystem()             | Yes, even if previously busy             |
+ * 
+----------------------------+------------------------------------------+
+ *
+ * In order to prevent deadlocks under NMI, the wrappers for these functions
+ * may only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi().
+ * However, not all of the services listed are reachable through NMI code paths,
+ * so the the special handling as suggested by the UEFI spec is only implemented
+ * for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
+ * context through efi_pstore_write().

Are pstore calls into the EFI runtime reentrant?

Thanks,

	Ingo

Re: [GIT PULL] EFI changes for v3.18

[Peter Zijlstra]

On Mon, Sep 29, 2014 at 02:43:21PM +0200, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@console-pimps.org> wrote:
> 
> >  * Implement new EFI runtime lock which is required by the UEFI
> >    specification - Ard Biesheuvel
> 
> Firstly, under what circumstances can EFI call parallelism happen 
> currently? Most of the EFI code runs during early bootup, which 
> is serialized.
> 
> Secondly, this locking pattern looks pretty disgusting:
> 
> @@ -94,7 +187,17 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
>                                           unsigned long data_size,
>                                           void *data)
>  {
> -       return efi_call_virt(set_variable, name, vendor, attr, data_size, data);
> +       unsigned long flags;
> +       efi_status_t status;
> +       bool __in_nmi = efi_in_nmi();
> +
> +       if (!__in_nmi)
> +               spin_lock_irqsave(&efi_runtime_lock, flags);
> +       status = efi_call_virt(set_variable, name, vendor, attr, data_size,
> +                              data);
> +       if (!__in_nmi)
> +               spin_unlock_irqrestore(&efi_runtime_lock, flags);
> +       return status;
>  }
> 
> and that's repeated in virt_efi_query_variable_info() as well.
> 
> and that's the explanation given:
> 
> +/*
> + * Some runtime services calls can be reentrant under NMI, even if the table
> + * above says they are not. (source: UEFI Specification v2.4A)
> + *
> + * Table 32. Functions that may be called after Machine Check, INIT and NMI
> + * 
> +----------------------------+------------------------------------------+
> + * | Function                  | Called after Machine Check, INIT and NMI |
> + * 
> +----------------------------+------------------------------------------+
> + * | GetTime()                 | Yes, even if previously busy.            |
> + * | GetVariable()             | Yes, even if previously busy             |
> + * | GetNextVariableName()     | Yes, even if previously busy             |
> + * | QueryVariableInfo()       | Yes, even if previously busy             |
> + * | SetVariable()             | Yes, even if previously busy             |
> + * | UpdateCapsule()           | Yes, even if previously busy             |
> + * | QueryCapsuleCapabilities()| Yes, even if previously busy             |
> + * | ResetSystem()             | Yes, even if previously busy             |
> + * 
> +----------------------------+------------------------------------------+
> + *
> + * In order to prevent deadlocks under NMI, the wrappers for these functions
> + * may only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi().
> + * However, not all of the services listed are reachable through NMI code paths,
> + * so the the special handling as suggested by the UEFI spec is only implemented
> + * for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
> + * context through efi_pstore_write().

OMFG what a trainwreck... if they are reentrant like that, a lock isn't
going to help you in any way. The implementation of these calls must be
lockfree otherwise they cannot possibly be correct.

Conditional locking like above is just plain broken, disgusting doesn't
even begin to cover it. Full NAK on this. If this is required by the EFI
spec someone needs to pull their head from their arse and smell the real
world.

Re: [GIT PULL] EFI changes for v3.18

[Matt Fleming]

On Mon, 29 Sep, at 02:43:21PM, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@console-pimps.org> wrote:
> 
> >  * Implement new EFI runtime lock which is required by the UEFI
> >    specification - Ard Biesheuvel
> 
> Firstly, under what circumstances can EFI call parallelism happen 
> currently? Most of the EFI code runs during early bootup, which 
> is serialized.
 
Access to EFI variables needs to be serialized against each other, which
we currently do with the __efivars->lock spinlock and the accessor
functions in drivers/firmware/efi/vars.c. Additionally on arm64, access
to the EFI time functions needs to be serialized with access to the
variable functions (we don't use the time stuff on x86).

> Secondly, this locking pattern looks pretty disgusting:
> 
> @@ -94,7 +187,17 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
>                                           unsigned long data_size,
>                                           void *data)
>  {
> -       return efi_call_virt(set_variable, name, vendor, attr, data_size, data);
> +       unsigned long flags;
> +       efi_status_t status;
> +       bool __in_nmi = efi_in_nmi();
> +
> +       if (!__in_nmi)
> +               spin_lock_irqsave(&efi_runtime_lock, flags);
> +       status = efi_call_virt(set_variable, name, vendor, attr, data_size,
> +                              data);
> +       if (!__in_nmi)
> +               spin_unlock_irqrestore(&efi_runtime_lock, flags);
> +       return status;
>  }
> 
> and that's repeated in virt_efi_query_variable_info() as well.
> 
> and that's the explanation given:
> 
> +/*
> + * Some runtime services calls can be reentrant under NMI, even if the table
> + * above says they are not. (source: UEFI Specification v2.4A)
> + *
> + * Table 32. Functions that may be called after Machine Check, INIT and NMI
> + * 
> +----------------------------+------------------------------------------+
> + * | Function                  | Called after Machine Check, INIT and NMI |
> + * 
> +----------------------------+------------------------------------------+
> + * | GetTime()                 | Yes, even if previously busy.            |
> + * | GetVariable()             | Yes, even if previously busy             |
> + * | GetNextVariableName()     | Yes, even if previously busy             |
> + * | QueryVariableInfo()       | Yes, even if previously busy             |
> + * | SetVariable()             | Yes, even if previously busy             |
> + * | UpdateCapsule()           | Yes, even if previously busy             |
> + * | QueryCapsuleCapabilities()| Yes, even if previously busy             |
> + * | ResetSystem()             | Yes, even if previously busy             |
> + * 
> +----------------------------+------------------------------------------+
> + *
> + * In order to prevent deadlocks under NMI, the wrappers for these functions
> + * may only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi().
> + * However, not all of the services listed are reachable through NMI code paths,
> + * so the the special handling as suggested by the UEFI spec is only implemented
> + * for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
> + * context through efi_pstore_write().
> 
> Are pstore calls into the EFI runtime reentrant?

Yes, the pstore functions are reentrant in the case of being invoked
from the kmsg_dumper callback via pstore_dump(), which for the EFI
pstore backend means we call efi_pstore_write() -> efi_entry_set_safe().

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [GIT PULL] EFI changes for v3.18

[Matt Fleming]

On Mon, 29 Sep, at 04:05:16PM, Peter Zijlstra wrote:
> 
> OMFG what a trainwreck... if they are reentrant like that, a lock isn't
> going to help you in any way. The implementation of these calls must be
> lockfree otherwise they cannot possibly be correct.
 
The only way these services are going to be called is if we (the OS)
invoke them. These NMI shenanigans we're playing in the locking
functions are actually for our benefit, not the firmware's.

> Conditional locking like above is just plain broken, disgusting doesn't
> even begin to cover it. Full NAK on this. If this is required by the EFI
> spec someone needs to pull their head from their arse and smell the real
> world.

The conditional locking isn't intended to conform to the spec, it's
intended to write a pstore object to the EFI variable NVRAM with our
last dying breath, even if someone was in the middle of a SetVariable()
call. All the spec says is that, if we're in a non-recoverable state,
it's OK to do that. Whether that'll be implemented correctly across a
range of firmware implementations is another matter.

In fact, maybe we shouldn't support that lockless access at all. I've no
huge problem changing the code in efi_pstore_write() to bail if we can't
grab the lock, so that we can be serialized all of the time.

That would certainly make the runtime lock code much simpler.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [GIT PULL] EFI changes for v3.18

[Peter Zijlstra]

On Mon, Sep 29, 2014 at 04:00:09PM +0100, Matt Fleming wrote:
> On Mon, 29 Sep, at 04:05:16PM, Peter Zijlstra wrote:
> > 
> > OMFG what a trainwreck... if they are reentrant like that, a lock isn't
> > going to help you in any way. The implementation of these calls must be
> > lockfree otherwise they cannot possibly be correct.
>  
> The only way these services are going to be called is if we (the OS)
> invoke them. These NMI shenanigans we're playing in the locking
> functions are actually for our benefit, not the firmware's.
> 
> > Conditional locking like above is just plain broken, disgusting doesn't
> > even begin to cover it. Full NAK on this. If this is required by the EFI
> > spec someone needs to pull their head from their arse and smell the real
> > world.
> 
> The conditional locking isn't intended to conform to the spec, it's
> intended to write a pstore object to the EFI variable NVRAM with our
> last dying breath, even if someone was in the middle of a SetVariable()
> call. All the spec says is that, if we're in a non-recoverable state,
> it's OK to do that. Whether that'll be implemented correctly across a
> range of firmware implementations is another matter.
> 
> In fact, maybe we shouldn't support that lockless access at all. I've no
> huge problem changing the code in efi_pstore_write() to bail if we can't
> grab the lock, so that we can be serialized all of the time.
> 
> That would certainly make the runtime lock code much simpler.

Right, like we talked about on IRC, we need to either drop all from NMI
stuff or do the trylock-from-NMI thing you suggested and have a runtime
test to make sure that all actually works.

Because just not having any serialization is relying on the firmware to
not screw itself, which I think is unsound, esp. given that Windows is
unlikely to rely on this and we all know the quality of implementation,
esp. outside of what Windows does.

Re: [GIT PULL] EFI changes for v3.18

[Ard Biesheuvel]

On 29 September 2014 17:00, Matt Fleming <matt@console-pimps.org> wrote:
> On Mon, 29 Sep, at 04:05:16PM, Peter Zijlstra wrote:
>>
>> OMFG what a trainwreck... if they are reentrant like that, a lock isn't
>> going to help you in any way. The implementation of these calls must be
>> lockfree otherwise they cannot possibly be correct.
>
> The only way these services are going to be called is if we (the OS)
> invoke them. These NMI shenanigans we're playing in the locking
> functions are actually for our benefit, not the firmware's.
>

The thing to understand here is that these Runtime Services are never
supposed to be *reentrant*, even under NMI. The special case for NMI,
machine check etc allows the OS to issue a concurrent Runtime Services
call that /preempts/ the call that is already in progress. The spec
explicitly mentions that the preempted function call is entirely
expected to fail.

This is exactly why the spec is so specific on which calls can be
issued concurrently: it puts the burden of exclusive access on the OS,
and the implementation can abort/fail a call in progress if another
one comes in concurrently.

This does imply that even the NMI case should take the lock if it is
free, which makes my implementation incorrect.

Note that I am by no means suggesting that relying on any of this
having been implemented correctly in most firmwares is a good idea.

>> Conditional locking like above is just plain broken, disgusting doesn't
>> even begin to cover it. Full NAK on this. If this is required by the EFI
>> spec someone needs to pull their head from their arse and smell the real
>> world.
>
> The conditional locking isn't intended to conform to the spec, it's
> intended to write a pstore object to the EFI variable NVRAM with our
> last dying breath, even if someone was in the middle of a SetVariable()
> call. All the spec says is that, if we're in a non-recoverable state,
> it's OK to do that. Whether that'll be implemented correctly across a
> range of firmware implementations is another matter.
>
> In fact, maybe we shouldn't support that lockless access at all. I've no
> huge problem changing the code in efi_pstore_write() to bail if we can't
> grab the lock, so that we can be serialized all of the time.
>
> That would certainly make the runtime lock code much simpler.
>

I think there is value in the ability to issue those calls under
MCHECK to record fault metadata, and obviously, the authors of the
spec did as well. However, the question is indeed how robust existing
firmware implementations are under this kind of behavior, so I agree
it is better to make the exclusive access unconditional. Note that
these exception are specific to Intel architectures, for arm64 the
locking was unconditional already.

-- 
Ard.