[PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[Mark Rutland]

Currently each architecture must implement two macros, efi_call_virt and
__efi_call_virt, which only differ by the presence or absence of a
return type. Otherwise, the logic surrounding the call is identical.

As each architecture must define the entire body of each, we can't place
any generic manipulation (e.g. irq flag validation) in the middle.

This patch adds template implementations of these macros. With these,
arch code can implement three template macros, avoiding reptition for
the void/non-void return cases:

* arch_efi_call_virt_setup

  Sets up the environment for the call (e.g. switching page tables,
  allowing kernel-mode use of floating point, if required).

* arch_efi_call_virt

  Performs the call. The last expression in the macro must be the call
  itself, allowing the logic to be shared by the void and non-void
  cases.

* arch_efi_call_virt_teardown

  Restores the usual kernel environment once the call has returned.

While the savings from repition are minimal, we additionally gain the
ability to add common code around the call with the call enviroment set
up. This can be used to detect common firmware issues (e.g. bad irq mask
management).

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/efi/runtime-wrappers.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index de69530..1b9fa54 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -20,6 +20,27 @@
 #include <linux/spinlock.h>
 #include <asm/efi.h>
 
+
+#ifndef efi_call_virt
+#define efi_call_virt(f, args...)					\
+({									\
+	efi_status_t __s;						\
+	arch_efi_call_virt_setup();					\
+	__s = arch_efi_call_virt(f, args);				\
+	arch_efi_call_virt_teardown();					\
+	__s;								\
+})
+#endif
+
+#ifndef __efi_call_virt
+#define __efi_call_virt(f, args...)					\
+({									\
+	arch_efi_call_virt_setup();					\
+	arch_efi_call_virt(f, args);					\
+	arch_efi_call_virt_teardown();					\
+})
+#endif
+
 /*
  * According to section 7.1 of the UEFI spec, Runtime Services are not fully
  * reentrant, and there are particular combinations of calls that need to be
-- 
1.9.1

Re: [PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[Leif Lindholm]

On Thu, Apr 21, 2016 at 12:35:25PM +0100, Mark Rutland wrote:
> Currently each architecture must implement two macros, efi_call_virt and
> __efi_call_virt, which only differ by the presence or absence of a
> return type. Otherwise, the logic surrounding the call is identical.
> 
> As each architecture must define the entire body of each, we can't place
> any generic manipulation (e.g. irq flag validation) in the middle.
> 
> This patch adds template implementations of these macros. With these,
> arch code can implement three template macros, avoiding reptition for
> the void/non-void return cases:
> 
> * arch_efi_call_virt_setup
> 
>   Sets up the environment for the call (e.g. switching page tables,
>   allowing kernel-mode use of floating point, if required).
> 
> * arch_efi_call_virt
> 
>   Performs the call. The last expression in the macro must be the call
>   itself, allowing the logic to be shared by the void and non-void
>   cases.
> 
> * arch_efi_call_virt_teardown
> 
>   Restores the usual kernel environment once the call has returned.
> 
> While the savings from repition are minimal, we additionally gain the
> ability to add common code around the call with the call enviroment set
> up. This can be used to detect common firmware issues (e.g. bad irq mask
> management).
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: linux-efi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index de69530..1b9fa54 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -20,6 +20,27 @@
>  #include <linux/spinlock.h>
>  #include <asm/efi.h>
>  
> +
> +#ifndef efi_call_virt

So ... not a strong complaint, but I would prefer if these weren't
ifdefd. I presume this is because ia64?
Could that be given a dummy pass-through version instead? If not,
could a comment be added that this is to retain compatibility with
ia64 (so that if that architecture was to mysteriously disappear from
the tree, someone might remember to deconditionalise it)?

> +#define efi_call_virt(f, args...)					\
> +({									\
> +	efi_status_t __s;						\
> +	arch_efi_call_virt_setup();					\
> +	__s = arch_efi_call_virt(f, args);				\
> +	arch_efi_call_virt_teardown();					\
> +	__s;								\
> +})
> +#endif
> +
> +#ifndef __efi_call_virt
> +#define __efi_call_virt(f, args...)					\
> +({									\
> +	arch_efi_call_virt_setup();					\
> +	arch_efi_call_virt(f, args);					\
> +	arch_efi_call_virt_teardown();					\
> +})
> +#endif
> +
>  /*
>   * According to section 7.1 of the UEFI spec, Runtime Services are not fully
>   * reentrant, and there are particular combinations of calls that need to be
> -- 
> 1.9.1
> 

Re: [PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[Mark Rutland]

On Thu, Apr 21, 2016 at 12:42:56PM +0100, Leif Lindholm wrote:
> On Thu, Apr 21, 2016 at 12:35:25PM +0100, Mark Rutland wrote:
> > Currently each architecture must implement two macros, efi_call_virt and
> > __efi_call_virt, which only differ by the presence or absence of a
> > return type. Otherwise, the logic surrounding the call is identical.
> > 
> > As each architecture must define the entire body of each, we can't place
> > any generic manipulation (e.g. irq flag validation) in the middle.
> > 
> > This patch adds template implementations of these macros. With these,
> > arch code can implement three template macros, avoiding reptition for
> > the void/non-void return cases:
> > 
> > * arch_efi_call_virt_setup
> > 
> >   Sets up the environment for the call (e.g. switching page tables,
> >   allowing kernel-mode use of floating point, if required).
> > 
> > * arch_efi_call_virt
> > 
> >   Performs the call. The last expression in the macro must be the call
> >   itself, allowing the logic to be shared by the void and non-void
> >   cases.
> > 
> > * arch_efi_call_virt_teardown
> > 
> >   Restores the usual kernel environment once the call has returned.
> > 
> > While the savings from repition are minimal, we additionally gain the
> > ability to add common code around the call with the call enviroment set
> > up. This can be used to detect common firmware issues (e.g. bad irq mask
> > management).
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Matt Fleming <matt@codeblueprint.co.uk>
> > Cc: linux-efi@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/firmware/efi/runtime-wrappers.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> > index de69530..1b9fa54 100644
> > --- a/drivers/firmware/efi/runtime-wrappers.c
> > +++ b/drivers/firmware/efi/runtime-wrappers.c
> > @@ -20,6 +20,27 @@
> >  #include <linux/spinlock.h>
> >  #include <asm/efi.h>
> >  
> > +
> > +#ifndef efi_call_virt
> 
> So ... not a strong complaint, but I would prefer if these weren't
> ifdefd. I presume this is because ia64?

Yup, and to allow the gradual migration of arm/arm64/x86 without a new
CONFIG_WANT_GENERIC_EFI_CALL_VIRT or something to that effect.

> Could that be given a dummy pass-through version instead?

I'm not exactly sure what you mean by that. Could you elaborate?

> If not, could a comment be added that this is to retain compatibility
> with ia64 (so that if that architecture was to mysteriously disappear
> from the tree, someone might remember to deconditionalise it)?

Sure. I can also add a note to the commit message regarding the
temporary need while arm/arm64/x86 are moved over.

Thanks,
Mark.

> > +#define efi_call_virt(f, args...)					\
> > +({									\
> > +	efi_status_t __s;						\
> > +	arch_efi_call_virt_setup();					\
> > +	__s = arch_efi_call_virt(f, args);				\
> > +	arch_efi_call_virt_teardown();					\
> > +	__s;								\
> > +})
> > +#endif
> > +
> > +#ifndef __efi_call_virt
> > +#define __efi_call_virt(f, args...)					\
> > +({									\
> > +	arch_efi_call_virt_setup();					\
> > +	arch_efi_call_virt(f, args);					\
> > +	arch_efi_call_virt_teardown();					\
> > +})
> > +#endif
> > +
> >  /*
> >   * According to section 7.1 of the UEFI spec, Runtime Services are not fully
> >   * reentrant, and there are particular combinations of calls that need to be
> > -- 
> > 1.9.1
> > 
> 

Re: [PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[Mark Rutland]

On Thu, Apr 21, 2016 at 01:55:07PM +0100, Mark Rutland wrote:
> On Thu, Apr 21, 2016 at 12:42:56PM +0100, Leif Lindholm wrote:
> > On Thu, Apr 21, 2016 at 12:35:25PM +0100, Mark Rutland wrote:
> > > Currently each architecture must implement two macros, efi_call_virt and
> > > __efi_call_virt, which only differ by the presence or absence of a
> > > return type. Otherwise, the logic surrounding the call is identical.
> > > 
> > > As each architecture must define the entire body of each, we can't place
> > > any generic manipulation (e.g. irq flag validation) in the middle.
> > > 
> > > This patch adds template implementations of these macros. With these,
> > > arch code can implement three template macros, avoiding reptition for
> > > the void/non-void return cases:
> > > 
> > > * arch_efi_call_virt_setup
> > > 
> > >   Sets up the environment for the call (e.g. switching page tables,
> > >   allowing kernel-mode use of floating point, if required).
> > > 
> > > * arch_efi_call_virt
> > > 
> > >   Performs the call. The last expression in the macro must be the call
> > >   itself, allowing the logic to be shared by the void and non-void
> > >   cases.
> > > 
> > > * arch_efi_call_virt_teardown
> > > 
> > >   Restores the usual kernel environment once the call has returned.
> > > 
> > > While the savings from repition are minimal, we additionally gain the
> > > ability to add common code around the call with the call enviroment set
> > > up. This can be used to detect common firmware issues (e.g. bad irq mask
> > > management).
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Matt Fleming <matt@codeblueprint.co.uk>
> > > Cc: linux-efi@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >  drivers/firmware/efi/runtime-wrappers.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> > > index de69530..1b9fa54 100644
> > > --- a/drivers/firmware/efi/runtime-wrappers.c
> > > +++ b/drivers/firmware/efi/runtime-wrappers.c
> > > @@ -20,6 +20,27 @@
> > >  #include <linux/spinlock.h>
> > >  #include <asm/efi.h>
> > >  
> > > +
> > > +#ifndef efi_call_virt
> > 
> > So ... not a strong complaint, but I would prefer if these weren't
> > ifdefd. I presume this is because ia64?
> 
> Yup, and to allow the gradual migration of arm/arm64/x86 without a new
> CONFIG_WANT_GENERIC_EFI_CALL_VIRT or something to that effect.

Actually, ia64 never uses this code, so I can add a final patch to
remove the ifdefs after the existing arch code is moved over.

I'll do that...

Mark.

[PATCHv2 2/5] arm64/efi: move to generic {__,}efi_call_virt

[Mark Rutland]

Now there's a common template for {__,}efi_call_virt, remove the
duplicate logic from the arm64 efi code.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Will Deacon <will.deacon@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm64/include/asm/efi.h | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a69..f4f71224 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -14,28 +14,21 @@ extern void efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 
-#define efi_call_virt(f, ...)						\
+#define arch_efi_call_virt_setup()					\
 ({									\
-	efi_##f##_t *__f;						\
-	efi_status_t __s;						\
-									\
 	kernel_neon_begin();						\
 	efi_virtmap_load();						\
-	__f = efi.systab->runtime->f;					\
-	__s = __f(__VA_ARGS__);						\
-	efi_virtmap_unload();						\
-	kernel_neon_end();						\
-	__s;								\
 })
 
-#define __efi_call_virt(f, ...)						\
+#define arch_efi_call_virt(f, args...)					\
 ({									\
 	efi_##f##_t *__f;						\
-									\
-	kernel_neon_begin();						\
-	efi_virtmap_load();						\
 	__f = efi.systab->runtime->f;					\
-	__f(__VA_ARGS__);						\
+	__f(args);							\
+})
+
+#define arch_efi_call_virt_teardown()					\
+({									\
 	efi_virtmap_unload();						\
 	kernel_neon_end();						\
 })
-- 
1.9.1

Re: [PATCHv2 2/5] arm64/efi: move to generic {__,}efi_call_virt

[Will Deacon]

On Thu, Apr 21, 2016 at 12:35:26PM +0100, Mark Rutland wrote:
> Now there's a common template for {__,}efi_call_virt, remove the
> duplicate logic from the arm64 efi code.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-efi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  arch/arm64/include/asm/efi.h | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e88a69..f4f71224 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -14,28 +14,21 @@ extern void efi_init(void);
>  
>  int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
>  
> -#define efi_call_virt(f, ...)						\
> +#define arch_efi_call_virt_setup()					\
>  ({									\
> -	efi_##f##_t *__f;						\
> -	efi_status_t __s;						\
> -									\
>  	kernel_neon_begin();						\
>  	efi_virtmap_load();						\
> -	__f = efi.systab->runtime->f;					\
> -	__s = __f(__VA_ARGS__);						\
> -	efi_virtmap_unload();						\
> -	kernel_neon_end();						\
> -	__s;								\
>  })
>  
> -#define __efi_call_virt(f, ...)						\
> +#define arch_efi_call_virt(f, args...)					\
>  ({									\
>  	efi_##f##_t *__f;						\
> -									\
> -	kernel_neon_begin();						\
> -	efi_virtmap_load();						\
>  	__f = efi.systab->runtime->f;					\
> -	__f(__VA_ARGS__);						\
> +	__f(args);							\

Any reason to change this to a named argument? This patch is hard enough
to review as it is, given the way the diff has been generated!

Either way:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

Re: [PATCHv2 2/5] arm64/efi: move to generic {__,}efi_call_virt

[Mark Rutland]

On Thu, Apr 21, 2016 at 05:48:40PM +0100, Will Deacon wrote:
> On Thu, Apr 21, 2016 at 12:35:26PM +0100, Mark Rutland wrote:
> > Now there's a common template for {__,}efi_call_virt, remove the
> > duplicate logic from the arm64 efi code.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Matt Fleming <matt@codeblueprint.co.uk>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-efi@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  arch/arm64/include/asm/efi.h | 21 +++++++--------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> > index 8e88a69..f4f71224 100644
> > --- a/arch/arm64/include/asm/efi.h
> > +++ b/arch/arm64/include/asm/efi.h
> > @@ -14,28 +14,21 @@ extern void efi_init(void);
> >  
> >  int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
> >  
> > -#define efi_call_virt(f, ...)						\
> > +#define arch_efi_call_virt_setup()					\
> >  ({									\
> > -	efi_##f##_t *__f;						\
> > -	efi_status_t __s;						\
> > -									\
> >  	kernel_neon_begin();						\
> >  	efi_virtmap_load();						\
> > -	__f = efi.systab->runtime->f;					\
> > -	__s = __f(__VA_ARGS__);						\
> > -	efi_virtmap_unload();						\
> > -	kernel_neon_end();						\
> > -	__s;								\
> >  })
> >  
> > -#define __efi_call_virt(f, ...)						\
> > +#define arch_efi_call_virt(f, args...)					\
> >  ({									\
> >  	efi_##f##_t *__f;						\
> > -									\
> > -	kernel_neon_begin();						\
> > -	efi_virtmap_load();						\
> >  	__f = efi.systab->runtime->f;					\
> > -	__f(__VA_ARGS__);						\
> > +	__f(args);							\
> 
> Any reason to change this to a named argument? This patch is hard enough
> to review as it is, given the way the diff has been generated!

That was to enforce consistency across arm/arm64/x86 for this.

It seemed nicer to make them all use a named argument than it did to
make them all use __VA_ARGS__, either of which was nice than each of
them doing something slightly different.

Sorry for the pain that evidently caused!

> Either way:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>

Cheers!

Mark.

[PATCHv2 3/5] arm/efi: move to generic {__,}efi_call_virt

[Mark Rutland]

Now there's a common template for {__,}efi_call_virt, remove the
duplicate logic from the arm efi code.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/include/asm/efi.h | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72..e1461fc 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -23,26 +23,14 @@ void efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 
-#define efi_call_virt(f, ...)						\
-({									\
-	efi_##f##_t *__f;						\
-	efi_status_t __s;						\
-									\
-	efi_virtmap_load();						\
-	__f = efi.systab->runtime->f;					\
-	__s = __f(__VA_ARGS__);						\
-	efi_virtmap_unload();						\
-	__s;								\
-})
+#define arch_efi_call_virt_setup()	efi_virtmap_load()
+#define arch_efi_call_virt_teardown()	efi_virtmap_unload()
 
-#define __efi_call_virt(f, ...)						\
+#define arch_efi_call_virt(f, args...)					\
 ({									\
 	efi_##f##_t *__f;						\
-									\
-	efi_virtmap_load();						\
 	__f = efi.systab->runtime->f;					\
-	__f(__VA_ARGS__);						\
-	efi_virtmap_unload();						\
+	__f(args);							\
 })
 
 static inline void efi_set_pgd(struct mm_struct *mm)
-- 
1.9.1

[PATCHv2 4/5] x86/efi: move to generic {__,}efi_call_virt

[Mark Rutland]

Now there's a common template for {__,}efi_call_virt, remove the
duplicate logic from the x86 efi code.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/include/asm/efi.h | 41 ++++++++++++-----------------------------
 1 file changed, 12 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 53748c4..cf79c8f 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -33,28 +33,16 @@
 
 extern unsigned long asmlinkage efi_call_phys(void *, ...);
 
+#define arch_efi_call_virt_setup()	kernel_fpu_begin()
+#define arch_efi_call_virt_teardown()	kernel_fpu_end()
+
 /*
  * Wrap all the virtual calls in a way that forces the parameters on the stack.
  */
-
-/* Use this macro if your virtual returns a non-void value */
-#define efi_call_virt(f, args...) \
-({									\
-	efi_status_t __s;						\
-	kernel_fpu_begin();						\
-	__s = ((efi_##f##_t __attribute__((regparm(0)))*)		\
-		efi.systab->runtime->f)(args);				\
-	kernel_fpu_end();						\
-	__s;								\
-})
-
-/* Use this macro if your virtual call does not return any value */
-#define __efi_call_virt(f, args...) \
+#define arch_efi_call_virt(f, args...)					\
 ({									\
-	kernel_fpu_begin();						\
 	((efi_##f##_t __attribute__((regparm(0)))*)			\
 		efi.systab->runtime->f)(args);				\
-	kernel_fpu_end();						\
 })
 
 #define efi_ioremap(addr, size, type, attr)	ioremap_cache(addr, size)
@@ -78,10 +66,8 @@ struct efi_scratch {
 	u64	phys_stack;
 } __packed;
 
-#define efi_call_virt(f, ...)						\
+#define arch_efi_call_virt_setup()					\
 ({									\
-	efi_status_t __s;						\
-									\
 	efi_sync_low_kernel_mappings();					\
 	preempt_disable();						\
 	__kernel_fpu_begin();						\
@@ -91,9 +77,13 @@ struct efi_scratch {
 		write_cr3((unsigned long)efi_scratch.efi_pgt);		\
 		__flush_tlb_all();					\
 	}								\
-									\
-	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
-									\
+})
+
+#define arch_efi_call_virt(f, args...)					\
+	efi_call((void *)efi.systab->runtime->f, args);			\
+
+#define arch_efi_call_virt_teardown()					\
+({									\
 	if (efi_scratch.use_pgd) {					\
 		write_cr3(efi_scratch.prev_cr3);			\
 		__flush_tlb_all();					\
@@ -101,15 +91,8 @@ struct efi_scratch {
 									\
 	__kernel_fpu_end();						\
 	preempt_enable();						\
-	__s;								\
 })
 
-/*
- * All X86_64 virt calls return non-void values. Thus, use non-void call for
- * virt calls that would be void on X86_32.
- */
-#define __efi_call_virt(f, args...) efi_call_virt(f, args)
-
 extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
 					u32 type, u64 attribute);
 
-- 
1.9.1

[PATCHv2 5/5] efi/runtime-wrappers: detect FW irq flag corruption

[Mark Rutland]

The UEFI spec allows runtime services to be called with interrupts
masked or unmasked, and if a runtime service function needs to mask
interrupts, it must restore the mask to its original state before
returning (i.e. from the PoV of the OS, this does not change across a
call). Firmware should never unmask exceptions, as these may then be
taken by the OS unexpectedly.

Unfortunately, some firmware has been seen to unmask IRQs (and
potentially other maskable exceptions) across runtime services calls,
leaving irq flags corrupted after returning from a runtime services
function call. This may be detected by the IRQ tracing code, but often
goes unnoticed, leaving a potentially disastrous bug hidden.

This patch detects when the irq flags are corrupted by an EFI runtime
services call, logging the call and specific corruption to the console.
While restoring the expected value of the flags is insufficient to avoid
problems, we do so to avoid redundant warnings from elsewhere (e.g. IRQ
tracing).

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 1b9fa54..aeb65f2 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -16,8 +16,10 @@
 
 #include <linux/bug.h>
 #include <linux/efi.h>
+#include <linux/irqflags.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
+#include <linux/stringify.h>
 #include <asm/efi.h>
 
 
@@ -25,8 +27,11 @@
 #define efi_call_virt(f, args...)					\
 ({									\
 	efi_status_t __s;						\
+	unsigned long flags;						\
 	arch_efi_call_virt_setup();					\
+	local_save_flags(flags);					\
 	__s = arch_efi_call_virt(f, args);				\
+	efi_call_virt_check_flags(flags, __stringify(f));		\
 	arch_efi_call_virt_teardown();					\
 	__s;								\
 })
@@ -35,12 +40,29 @@
 #ifndef __efi_call_virt
 #define __efi_call_virt(f, args...)					\
 ({									\
+	unsigned long flags;						\
 	arch_efi_call_virt_setup();					\
+	local_irq_save(flags);						\
 	arch_efi_call_virt(f, args);					\
+	efi_call_virt_check_flags(flags, __stringify(f));		\
 	arch_efi_call_virt_teardown();					\
 })
 #endif
 
+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
+{
+	unsigned long cur_flags;
+
+	local_save_flags(cur_flags);
+	if (!WARN_ON_ONCE(cur_flags != flags))
+		return;
+
+	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);
+	pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n",
+			   flags, cur_flags, call);
+	local_irq_restore(flags);
+}
+
 /*
  * According to section 7.1 of the UEFI spec, Runtime Services are not fully
  * reentrant, and there are particular combinations of calls that need to be
-- 
1.9.1

Re: [PATCHv2 5/5] efi/runtime-wrappers: detect FW irq flag corruption

[Ard Biesheuvel]

On 21 April 2016 at 13:35, Mark Rutland <mark.rutland@arm.com> wrote:
> The UEFI spec allows runtime services to be called with interrupts
> masked or unmasked, and if a runtime service function needs to mask
> interrupts, it must restore the mask to its original state before
> returning (i.e. from the PoV of the OS, this does not change across a
> call). Firmware should never unmask exceptions, as these may then be
> taken by the OS unexpectedly.
>
> Unfortunately, some firmware has been seen to unmask IRQs (and
> potentially other maskable exceptions) across runtime services calls,
> leaving irq flags corrupted after returning from a runtime services
> function call. This may be detected by the IRQ tracing code, but often
> goes unnoticed, leaving a potentially disastrous bug hidden.
>
> This patch detects when the irq flags are corrupted by an EFI runtime
> services call, logging the call and specific corruption to the console.
> While restoring the expected value of the flags is insufficient to avoid
> problems, we do so to avoid redundant warnings from elsewhere (e.g. IRQ
> tracing).
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: linux-efi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index 1b9fa54..aeb65f2 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -16,8 +16,10 @@
>
>  #include <linux/bug.h>
>  #include <linux/efi.h>
> +#include <linux/irqflags.h>
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
> +#include <linux/stringify.h>
>  #include <asm/efi.h>
>
>
> @@ -25,8 +27,11 @@
>  #define efi_call_virt(f, args...)                                      \
>  ({                                                                     \
>         efi_status_t __s;                                               \
> +       unsigned long flags;                                            \
>         arch_efi_call_virt_setup();                                     \
> +       local_save_flags(flags);                                        \
>         __s = arch_efi_call_virt(f, args);                              \
> +       efi_call_virt_check_flags(flags, __stringify(f));               \
>         arch_efi_call_virt_teardown();                                  \
>         __s;                                                            \
>  })
> @@ -35,12 +40,29 @@
>  #ifndef __efi_call_virt
>  #define __efi_call_virt(f, args...)                                    \
>  ({                                                                     \
> +       unsigned long flags;                                            \
>         arch_efi_call_virt_setup();                                     \
> +       local_irq_save(flags);                                          \

We shouldn't disable interrupts here. I assume this is a typo, and you
intended to use local_save_flags() as above?

>         arch_efi_call_virt(f, args);                                    \
> +       efi_call_virt_check_flags(flags, __stringify(f));               \
>         arch_efi_call_virt_teardown();                                  \
>  })
>  #endif
>
> +static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> +{
> +       unsigned long cur_flags;
> +
> +       local_save_flags(cur_flags);
> +       if (!WARN_ON_ONCE(cur_flags != flags))
> +               return;
> +
> +       add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);
> +       pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n",
> +                          flags, cur_flags, call);
> +       local_irq_restore(flags);
> +}
> +
>  /*
>   * According to section 7.1 of the UEFI spec, Runtime Services are not fully
>   * reentrant, and there are particular combinations of calls that need to be

Other than that, this series looks fine to me.

With the above fixed:

For the series (except the x86 patch)

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Re: [PATCHv2 5/5] efi/runtime-wrappers: detect FW irq flag corruption

[Mark Rutland]

On Thu, Apr 21, 2016 at 07:05:32PM +0200, Ard Biesheuvel wrote:
> On 21 April 2016 at 13:35, Mark Rutland <mark.rutland@arm.com> wrote:
> > @@ -25,8 +27,11 @@
> >  #define efi_call_virt(f, args...)                                      \
> >  ({                                                                     \
> >         efi_status_t __s;                                               \
> > +       unsigned long flags;                                            \
> >         arch_efi_call_virt_setup();                                     \
> > +       local_save_flags(flags);                                        \
> >         __s = arch_efi_call_virt(f, args);                              \
> > +       efi_call_virt_check_flags(flags, __stringify(f));               \
> >         arch_efi_call_virt_teardown();                                  \
> >         __s;                                                            \
> >  })
> > @@ -35,12 +40,29 @@
> >  #ifndef __efi_call_virt
> >  #define __efi_call_virt(f, args...)                                    \
> >  ({                                                                     \
> > +       unsigned long flags;                                            \
> >         arch_efi_call_virt_setup();                                     \
> > +       local_irq_save(flags);                                          \
> 
> We shouldn't disable interrupts here. I assume this is a typo, and you
> intended to use local_save_flags() as above?

Oh, yes. That's an impressive mistake on my behalf; thanks for spotting
that!

I've been seeing issues with GetVariable and GetNextVariable, which
happen to only exercise the correct macro above.

> Other than that, this series looks fine to me.
> 
> With the above fixed:
> 
> For the series (except the x86 patch)
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Cheers!

Mark.