[PATCH 1/2] efi: arm64: abort boot on pending SError

[PATCH 1/2] efi: arm64: abort boot on pending SError

[Ard Biesheuvel]

It is the firmware's job to clear any pending SErrors before entering
the kernel. On UEFI, we can fail gracefully rather than panic during
early boot, so check for this condition in the stub.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/libstub/arm64-stub.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index eae693eb3e91..c7e7396de876 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -20,7 +20,14 @@ extern bool __nokaslr;
 
 efi_status_t check_platform_features(efi_system_table_t *sys_table_arg)
 {
-	u64 tg;
+	u64 tg, isr;
+
+	/* check for a pending SError */
+	asm ("mrs %0, isr_el1" : "=r"(isr));
+	if (isr & BIT(8)) {
+		pr_efi_err(sys_table_arg, "Pending SError detected -- aborting\n");
+		return EFI_LOAD_ERROR;
+	}
 
 	/* UEFI mandates support for 4 KB granularity, no need to check */
 	if (IS_ENABLED(CONFIG_ARM64_4K_PAGES))
-- 
2.7.4

[PATCH 2/2] arm64: document that pending SErrors are not allowed at kernel entry

[Ard Biesheuvel]

Our current strategy to deal with pending SErrors at boot is to panic.
So let's mention in our boot protocol documentation that no SErrors
should be pending when handing over to the kernel.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/arm64/booting.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
index 8d0df62c3fe0..75dcfead1a0c 100644
--- a/Documentation/arm64/booting.txt
+++ b/Documentation/arm64/booting.txt
@@ -154,8 +154,9 @@ Before jumping into the kernel, the following conditions must be met:
   x3 = 0 (reserved for future use)
 
 - CPU mode
-  All forms of interrupts must be masked in PSTATE.DAIF (Debug, SError,
-  IRQ and FIQ).
+  All forms of exceptions must be masked in PSTATE.DAIF (Debug, SError,
+  IRQ and FIQ), and any pending SError exceptions must be taken by the
+  boot loader or firmware before handing over to the kernel.
   The CPU must be in either EL2 (RECOMMENDED in order to have access to
   the virtualisation extensions) or non-secure EL1.
 
-- 
2.7.4

Re: [PATCH 2/2] arm64: document that pending SErrors are not allowed at kernel entry

[Mark Rutland]

On Fri, Jul 01, 2016 at 05:01:31PM +0200, Ard Biesheuvel wrote:
> Our current strategy to deal with pending SErrors at boot is to panic.
> So let's mention in our boot protocol documentation that no SErrors
> should be pending when handing over to the kernel.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/arm64/booting.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
> index 8d0df62c3fe0..75dcfead1a0c 100644
> --- a/Documentation/arm64/booting.txt
> +++ b/Documentation/arm64/booting.txt
> @@ -154,8 +154,9 @@ Before jumping into the kernel, the following conditions must be met:
>    x3 = 0 (reserved for future use)
>  
>  - CPU mode
> -  All forms of interrupts must be masked in PSTATE.DAIF (Debug, SError,
> -  IRQ and FIQ).
> +  All forms of exceptions must be masked in PSTATE.DAIF (Debug, SError,
> +  IRQ and FIQ), and any pending SError exceptions must be taken by the
> +  boot loader or firmware before handing over to the kernel.

That can be read to mean that a loader should silently swallow SError,
which isn't quite what we want (that no SError ever occurs). I don't
have a better wording to suggest, unfortunately. :/

As with patch 1, I'm not sure what this means for CPER and friends.

Thanks,
Mark.

>    The CPU must be in either EL2 (RECOMMENDED in order to have access to
>    the virtualisation extensions) or non-secure EL1.
>  
> -- 
> 2.7.4
> 

Re: [PATCH 2/2] arm64: document that pending SErrors are not allowed at kernel entry

[Ard Biesheuvel]

On 1 July 2016 at 17:25, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Fri, Jul 01, 2016 at 05:01:31PM +0200, Ard Biesheuvel wrote:
>> Our current strategy to deal with pending SErrors at boot is to panic.
>> So let's mention in our boot protocol documentation that no SErrors
>> should be pending when handing over to the kernel.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  Documentation/arm64/booting.txt | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
>> index 8d0df62c3fe0..75dcfead1a0c 100644
>> --- a/Documentation/arm64/booting.txt
>> +++ b/Documentation/arm64/booting.txt
>> @@ -154,8 +154,9 @@ Before jumping into the kernel, the following conditions must be met:
>>    x3 = 0 (reserved for future use)
>>
>>  - CPU mode
>> -  All forms of interrupts must be masked in PSTATE.DAIF (Debug, SError,
>> -  IRQ and FIQ).
>> +  All forms of exceptions must be masked in PSTATE.DAIF (Debug, SError,
>> +  IRQ and FIQ), and any pending SError exceptions must be taken by the
>> +  boot loader or firmware before handing over to the kernel.
>
> That can be read to mean that a loader should silently swallow SError,
> which isn't quite what we want (that no SError ever occurs). I don't
> have a better wording to suggest, unfortunately. :/
>

I don't think it is up to us to mandate how the firmware should be
dealing with early SErrors, as long as none are pending when we enter
the kernel.

Re: [PATCH 1/2] efi: arm64: abort boot on pending SError

[Mark Rutland]

On Fri, Jul 01, 2016 at 05:01:30PM +0200, Ard Biesheuvel wrote:
> It is the firmware's job to clear any pending SErrors before entering
> the kernel. On UEFI, we can fail gracefully rather than panic during
> early boot, so check for this condition in the stub.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

An SError could be triggered either asynchronously by FW, or as a result
of our actions at any point after this, e.g. due to the filesystem
accesses made to load an initrd.

So in practice, is checking here useful? Have we seen FW with masked but
pending SError at the point we enter the stub rather than that SError
being triggered later,?

I'm also not sure what this means for CPER, which may use SError to
signal to the OS. It's possible that the UEFI implementation polls
ISR_EL1 itself, and handles SError appropriately internally, or that the
OS can later deal with the SError based on CPER and friends.

> ---
>  drivers/firmware/efi/libstub/arm64-stub.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index eae693eb3e91..c7e7396de876 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -20,7 +20,14 @@ extern bool __nokaslr;
>  
>  efi_status_t check_platform_features(efi_system_table_t *sys_table_arg)
>  {
> -	u64 tg;
> +	u64 tg, isr;
> +
> +	/* check for a pending SError */
> +	asm ("mrs %0, isr_el1" : "=r"(isr));

I think you can use read_sysreg(isr_el1) here, given that the
read_sysreg helper is a macro function, and we already include sysreg.h
here.

> +	if (isr & BIT(8)) {

Perhaps add:

#define ISR_EL1_A_BIT	BIT(8)

to sysreg.h?

> +		pr_efi_err(sys_table_arg, "Pending SError detected -- aborting\n");
> +		return EFI_LOAD_ERROR;
> +	}

Otherwise, code-wise this looks fine, but as above I'm not sure if this
is the right thiing to do. :/

Thanks,
Mark.

>  
>  	/* UEFI mandates support for 4 KB granularity, no need to check */
>  	if (IS_ENABLED(CONFIG_ARM64_4K_PAGES))
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] efi: arm64: abort boot on pending SError

[Ard Biesheuvel]

On 1 July 2016 at 17:22, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jul 01, 2016 at 05:01:30PM +0200, Ard Biesheuvel wrote:
>> It is the firmware's job to clear any pending SErrors before entering
>> the kernel. On UEFI, we can fail gracefully rather than panic during
>> early boot, so check for this condition in the stub.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> An SError could be triggered either asynchronously by FW, or as a result
> of our actions at any point after this, e.g. due to the filesystem
> accesses made to load an initrd.
>
> So in practice, is checking here useful? Have we seen FW with masked but
> pending SError at the point we enter the stub rather than that SError
> being triggered later,?
>

Yes. EDK2 keeps SError masked throughout its execution by default, and
so any condition that triggered an SError up till this point is likely
to still be pending, and blow up the kernel as soon as it unmasks it.

> I'm also not sure what this means for CPER, which may use SError to
> signal to the OS. It's possible that the UEFI implementation polls
> ISR_EL1 itself, and handles SError appropriately internally, or that the
> OS can later deal with the SError based on CPER and friends.
>

Currently, the kernel panics on an SError, and so what the kernel
should do once we start dealing with them in a more sophisticated way
is hypothetical at the moment. Once that code arrives, it may revert
this change, but for now, being dropped back into the UEFI shell does
sound more appealing than panic early imo.

>> ---
>>  drivers/firmware/efi/libstub/arm64-stub.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
>> index eae693eb3e91..c7e7396de876 100644
>> --- a/drivers/firmware/efi/libstub/arm64-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
>> @@ -20,7 +20,14 @@ extern bool __nokaslr;
>>
>>  efi_status_t check_platform_features(efi_system_table_t *sys_table_arg)
>>  {
>> -     u64 tg;
>> +     u64 tg, isr;
>> +
>> +     /* check for a pending SError */
>> +     asm ("mrs %0, isr_el1" : "=r"(isr));
>
> I think you can use read_sysreg(isr_el1) here, given that the
> read_sysreg helper is a macro function, and we already include sysreg.h
> here.
>

OK

>> +     if (isr & BIT(8)) {
>
> Perhaps add:
>
> #define ISR_EL1_A_BIT   BIT(8)
>
> to sysreg.h?
>

OK

>> +             pr_efi_err(sys_table_arg, "Pending SError detected -- aborting\n");
>> +             return EFI_LOAD_ERROR;
>> +     }
>
> Otherwise, code-wise this looks fine, but as above I'm not sure if this
> is the right thiing to do. :/
>

Yes, let's clear that up first.

-- 
Ard,

Re: [PATCH 1/2] efi: arm64: abort boot on pending SError

[Mark Rutland]

On Fri, Jul 01, 2016 at 05:31:33PM +0200, Ard Biesheuvel wrote:
> On 1 July 2016 at 17:22, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Fri, Jul 01, 2016 at 05:01:30PM +0200, Ard Biesheuvel wrote:
> >> It is the firmware's job to clear any pending SErrors before entering
> >> the kernel. On UEFI, we can fail gracefully rather than panic during
> >> early boot, so check for this condition in the stub.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >
> > An SError could be triggered either asynchronously by FW, or as a result
> > of our actions at any point after this, e.g. due to the filesystem
> > accesses made to load an initrd.
> >
> > So in practice, is checking here useful? Have we seen FW with masked but
> > pending SError at the point we enter the stub rather than that SError
> > being triggered later,?
> 
> Yes. EDK2 keeps SError masked throughout its execution by default, and
> so any condition that triggered an SError up till this point is likely
> to still be pending, and blow up the kernel as soon as it unmasks it.

Ok.

> > I'm also not sure what this means for CPER, which may use SError to
> > signal to the OS. It's possible that the UEFI implementation polls
> > ISR_EL1 itself, and handles SError appropriately internally, or that the
> > OS can later deal with the SError based on CPER and friends.
> 
> Currently, the kernel panics on an SError, and so what the kernel
> should do once we start dealing with them in a more sophisticated way
> is hypothetical at the moment. Once that code arrives, it may revert
> this change, but for now, being dropped back into the UEFI shell does
> sound more appealing than panic early imo.

Logging something while the UART is available is certainly appealing.

As you say, we can change this later if/when we have more advanced
SError handling. So modulo my prior comments, I guess this is fine for
now.

Thanks,
Mark.

Re: [PATCH 1/2] efi: arm64: abort boot on pending SError

[Ard Biesheuvel]

On 1 July 2016 at 17:46, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jul 01, 2016 at 05:31:33PM +0200, Ard Biesheuvel wrote:
>> On 1 July 2016 at 17:22, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Fri, Jul 01, 2016 at 05:01:30PM +0200, Ard Biesheuvel wrote:
>> >> It is the firmware's job to clear any pending SErrors before entering
>> >> the kernel. On UEFI, we can fail gracefully rather than panic during
>> >> early boot, so check for this condition in the stub.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >
>> > An SError could be triggered either asynchronously by FW, or as a result
>> > of our actions at any point after this, e.g. due to the filesystem
>> > accesses made to load an initrd.
>> >
>> > So in practice, is checking here useful? Have we seen FW with masked but
>> > pending SError at the point we enter the stub rather than that SError
>> > being triggered later,?
>>
>> Yes. EDK2 keeps SError masked throughout its execution by default, and
>> so any condition that triggered an SError up till this point is likely
>> to still be pending, and blow up the kernel as soon as it unmasks it.
>
> Ok.
>
>> > I'm also not sure what this means for CPER, which may use SError to
>> > signal to the OS. It's possible that the UEFI implementation polls
>> > ISR_EL1 itself, and handles SError appropriately internally, or that the
>> > OS can later deal with the SError based on CPER and friends.
>>
>> Currently, the kernel panics on an SError, and so what the kernel
>> should do once we start dealing with them in a more sophisticated way
>> is hypothetical at the moment. Once that code arrives, it may revert
>> this change, but for now, being dropped back into the UEFI shell does
>> sound more appealing than panic early imo.
>
> Logging something while the UART is available is certainly appealing.
>

Not just the UART, the graphical console as well, if the system has one.

> As you say, we can change this later if/when we have more advanced
> SError handling. So modulo my prior comments, I guess this is fine for
> now.
>

OK, thanks.