[PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Sebastian Andrzej Siewior]

Based on measurements the EFI functions get_variable /
get_next_variable take up to 2us. The functions get_time, set_time take
around 10ms. Those 10ms are too much. Even one ms would be too much.
The funny part is that EFI is invoked with enabled interrupts.
According to my tracing I see the interrupt almost 10ms later which
indicates that EFI is disabling interrupts while doing its thing.

This was observed on "EFI v2.60 by SoftIron Overdrive 1000". I don't say
that every EFI implementation does this but given that it has to access a
slow bus like I2C/SPI it is expected.

Disable EFI's RTC driver on RT.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/rtc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index a2ba5db36145..248d72711650 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1087,7 +1087,7 @@ config RTC_DRV_DA9063
 
 config RTC_DRV_EFI
 	tristate "EFI RTC"
-	depends on EFI && !X86
+	depends on EFI && !X86 && !PREEMPT_RT_BASE
 	help
 	  If you say yes here you will get support for the EFI
 	  Real Time Clock.
-- 
2.18.0

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Ard Biesheuvel]

On 26 July 2018 at 11:04, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> Based on measurements the EFI functions get_variable /
> get_next_variable take up to 2us. The functions get_time, set_time take
> around 10ms. Those 10ms are too much. Even one ms would be too much.

You cannot extrapolate from these numbers. If the lack of worst case
guarantees on an EFI system is a problem for -rt, the only meaningful
course of action is to disable EFI runtime services entirely.

I think SetVariable() may be even worse than GetTime(), given that NOR
flash updates may involve erasing blocks.

> The funny part is that EFI is invoked with enabled interrupts.
> According to my tracing I see the interrupt almost 10ms later which
> indicates that EFI is disabling interrupts while doing its thing.
>

Yes, and this is also highly implementation specific. Basing this kind
of policy on a single implementation is not very wise imo.

> This was observed on "EFI v2.60 by SoftIron Overdrive 1000". I don't say
> that every EFI implementation does this but given that it has to access a
> slow bus like I2C/SPI it is expected.
>
> Disable EFI's RTC driver on RT.
>

Other calls to GetTime() would still be permitted in this case, so
this seems like a partial solution (although no other calls seem to
exist atm)

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/rtc/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index a2ba5db36145..248d72711650 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1087,7 +1087,7 @@ config RTC_DRV_DA9063
>
>  config RTC_DRV_EFI
>         tristate "EFI RTC"
> -       depends on EFI && !X86
> +       depends on EFI && !X86 && !PREEMPT_RT_BASE
>         help
>           If you say yes here you will get support for the EFI
>           Real Time Clock.
> --
> 2.18.0
>

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Sebastian Andrzej Siewior]

On 2018-07-26 11:15:52 [+0200], Ard Biesheuvel wrote:
> On 26 July 2018 at 11:04, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> > Based on measurements the EFI functions get_variable /
> > get_next_variable take up to 2us. The functions get_time, set_time take
> > around 10ms. Those 10ms are too much. Even one ms would be too much.
> 
> You cannot extrapolate from these numbers. If the lack of worst case
> guarantees on an EFI system is a problem for -rt, the only meaningful
> course of action is to disable EFI runtime services entirely.

Oh boy. I disable EFI entirely because then the bootloader won't boot
due to missing efi-stub. But I could disable the runtime-wrappers if
that is what you meant. Patch below.

> I think SetVariable() may be even worse than GetTime(), given that NOR
> flash updates may involve erasing blocks.

Here we go. "Read" can be triggered from userland via sysfs / efivars.
"Write" is (currently) limited to pstore?

> > The funny part is that EFI is invoked with enabled interrupts.
> > According to my tracing I see the interrupt almost 10ms later which
> > indicates that EFI is disabling interrupts while doing its thing.
> 
> Yes, and this is also highly implementation specific. Basing this kind
> of policy on a single implementation is not very wise imo.

even if interrupts were not disabled then there would be no context
switch on exit from interrupt path due to the preempt-disable part. So
no improvement.

> > This was observed on "EFI v2.60 by SoftIron Overdrive 1000". I don't say
> > that every EFI implementation does this but given that it has to access a
> > slow bus like I2C/SPI it is expected.
> >
> > Disable EFI's RTC driver on RT.
> >
> 
> Other calls to GetTime() would still be permitted in this case, so
> this seems like a partial solution (although no other calls seem to
> exist atm)

I *assumed* pstore would use the SetVariable during warning/bug. The
pstore thingy might be useful. Not sure.

------ >8

Subject: [PATCH RT] arm64: Disable EFI runtime services on RT

Based on meassurements the EFI functions get_variable /
get_next_variable take up to 2us which looks okay.
The functions get_time, set_time take around 10ms. Those 10ms are too
much. Even one ms would be too much.
Ard mentioned that SetVariable might even trigger larger latencies if
the firware will erase flash blocks on NOR.

The time-functions are used by efi-rtc and can be triggered during
runtimed (either via explicit read/write or ntp sync).

The variable write could be used by pstore.
These functions can be disabled without much of a loss. The poweroff /
reboot hooks will be provided by PSCI.

Disable EFI's runtime wrappers.

This was observed on "EFI v2.60 by SoftIron Overdrive 1000".

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm64/Kconfig                 | 2 +-
 drivers/firmware/efi/arm-runtime.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 111db4e44fcf..68adcb0f4de6 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1250,7 +1250,7 @@ config EFI
 	select LIBFDT
 	select UCS2_STRING
 	select EFI_PARAMS_FROM_FDT
-	select EFI_RUNTIME_WRAPPERS
+	select EFI_RUNTIME_WRAPPERS if !PREEMPT_RT_BASE
 	select EFI_STUB
 	select EFI_ARMSTUB
 	default y
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 5889cbea60b8..3e96db10c034 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -140,8 +140,10 @@ static int __init arm_enable_runtime_services(void)
 	}
 
 	/* Set up runtime services function pointers */
+#ifdef CONFIG_EFI_RUNTIME_WRAPPERS
 	efi_native_runtime_setup();
 	set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+#endif
 
 	return 0;
 }
-- 
2.18.0

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Ard Biesheuvel]

> On 26 Jul 2018, at 14:15, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
>> On 2018-07-26 11:15:52 [+0200], Ard Biesheuvel wrote:
>> On 26 July 2018 at 11:04, Sebastian Andrzej Siewior
>> <bigeasy@linutronix.de> wrote:
>>> Based on measurements the EFI functions get_variable /
>>> get_next_variable take up to 2us. The functions get_time, set_time take
>>> around 10ms. Those 10ms are too much. Even one ms would be too much.
>> 
>> You cannot extrapolate from these numbers. If the lack of worst case
>> guarantees on an EFI system is a problem for -rt, the only meaningful
>> course of action is to disable EFI runtime services entirely.
> 
> Oh boy. I disable EFI entirely because then the bootloader won't boot
> due to missing efi-stub. But I could disable the runtime-wrappers if
> that is what you meant. Patch below.
> 

What i mean is whatever you end up with if you pass efi=noruntime to the kernel.

But as i mentioned before, you may also lose the ability to reboot/shut down your system this way. Can you please double check?

>> I think SetVariable() may be even worse than GetTime(), given that NOR
>> flash updates may involve erasing blocks.
> 
> Here we go. "Read" can be triggered from userland via sysfs / efivars.
> "Write" is (currently) limited to pstore?
> 

No you can also write to variables via efivarfs and sysfs.

>>> The funny part is that EFI is invoked with enabled interrupts.
>>> According to my tracing I see the interrupt almost 10ms later which
>>> indicates that EFI is disabling interrupts while doing its thing.
>> 
>> Yes, and this is also highly implementation specific. Basing this kind
>> of policy on a single implementation is not very wise imo.
> 
> even if interrupts were not disabled then there would be no context
> switch on exit from interrupt path due to the preempt-disable part. So
> no improvement.
> 
>>> This was observed on "EFI v2.60 by SoftIron Overdrive 1000". I don't say
>>> that every EFI implementation does this but given that it has to access a
>>> slow bus like I2C/SPI it is expected.
>>> 
>>> Disable EFI's RTC driver on RT.
>>> 
>> 
>> Other calls to GetTime() would still be permitted in this case, so
>> this seems like a partial solution (although no other calls seem to
>> exist atm)
> 
> I *assumed* pstore would use the SetVariable during warning/bug. The
> pstore thingy might be useful. Not sure.
> 
> ------ >8
> 
> Subject: [PATCH RT] arm64: Disable EFI runtime services on RT
> 
> Based on meassurements the EFI functions get_variable /
> get_next_variable take up to 2us which looks okay.
> The functions get_time, set_time take around 10ms. Those 10ms are too
> much. Even one ms would be too much.
> Ard mentioned that SetVariable might even trigger larger latencies if
> the firware will erase flash blocks on NOR.
> 
> The time-functions are used by efi-rtc and can be triggered during
> runtimed (either via explicit read/write or ntp sync).
> 
> The variable write could be used by pstore.
> These functions can be disabled without much of a loss. The poweroff /
> reboot hooks will be provided by PSCI.
> 
> Disable EFI's runtime wrappers.
> 
> This was observed on "EFI v2.60 by SoftIron Overdrive 1000".
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> arch/arm64/Kconfig                 | 2 +-
> drivers/firmware/efi/arm-runtime.c | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 111db4e44fcf..68adcb0f4de6 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1250,7 +1250,7 @@ config EFI
>    select LIBFDT
>    select UCS2_STRING
>    select EFI_PARAMS_FROM_FDT
> -    select EFI_RUNTIME_WRAPPERS
> +    select EFI_RUNTIME_WRAPPERS if !PREEMPT_RT_BASE
>    select EFI_STUB
>    select EFI_ARMSTUB
>    default y
> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> index 5889cbea60b8..3e96db10c034 100644
> --- a/drivers/firmware/efi/arm-runtime.c
> +++ b/drivers/firmware/efi/arm-runtime.c
> @@ -140,8 +140,10 @@ static int __init arm_enable_runtime_services(void)
>    }
> 
>    /* Set up runtime services function pointers */
> +#ifdef CONFIG_EFI_RUNTIME_WRAPPERS
>    efi_native_runtime_setup();
>    set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +#endif
> 
>    return 0;
> }
> -- 
> 2.18.0
> 

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Sebastian Andrzej Siewior]

On 2018-07-26 14:26:25 [+0200], Ard Biesheuvel wrote:
> What i mean is whatever you end up with if you pass efi=noruntime to the kernel.
ah. So I wouldn't have to patch this, just document it. That might work.

> But as i mentioned before, you may also lose the ability to reboot/shut down your system this way. Can you please double check?

I checked before posting the patch. psci is used instead (on softiron
here, have nothing else). psci_sys_poweroff() works.

Sebastian

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Ard Biesheuvel]

> On 26 Jul 2018, at 14:46, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
>> On 2018-07-26 14:26:25 [+0200], Ard Biesheuvel wrote:
>> What i mean is whatever you end up with if you pass efi=noruntime to the kernel.
> ah. So I wouldn't have to patch this, just document it. That might work.
> 

We could also make it the default on -rt, but not disable it entirely, so that efi=runtime can be used to re-enable it.


>> But as i mentioned before, you may also lose the ability to reboot/shut down your system this way. Can you please double check?
> 
> I checked before posting the patch. psci is used instead (on softiron
> here, have nothing else). psci_sys_poweroff() works.
> 
> Sebastian

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Sebastian Andrzej Siewior]

On 2018-07-26 14:52:21 [+0200], Ard Biesheuvel wrote:
> We could also make it the default on -rt, but not disable it entirely, so that efi=runtime can be used to re-enable it.

Oh. I like that. We have something similar for RCU. So I would need
that:

------- >8

Subject: [PATCH] efi: Allow efi=runtime

In case the option "efi=noruntime" is default at built-time, the user
could overwrite its sate by `efi=runtime' and allow it again.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/firmware/efi/efi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 62c6e4b6ce3e..d6176ce50b45 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -110,6 +110,9 @@ static int __init parse_efi_cmdline(char *str)
 	if (parse_option_str(str, "noruntime"))
 		disable_runtime = true;
 
+	if (parse_option_str(str, "runtime"))
+		disable_runtime = false;
+
 	return 0;
 }
 early_param("efi", parse_efi_cmdline);
-- 
2.18.0

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Sebastian Andrzej Siewior]

On 2018-07-26 15:13:23 [+0200], To Ard Biesheuvel wrote:
> On 2018-07-26 14:52:21 [+0200], Ard Biesheuvel wrote:
> > We could also make it the default on -rt, but not disable it entirely, so that efi=runtime can be used to re-enable it.
> 
> Oh. I like that. We have something similar for RCU. So I would need
> that:
and then I could make it default off:

----- >8

Subject: [PATCH] efi: Disable runtime services on RT

Based on meassurements the EFI functions get_variable /
get_next_variable take up to 2us which looks okay.
The functions get_time, set_time take around 10ms. Those 10ms are too
much. Even one ms would be too much.
Ard mentioned that SetVariable might even trigger larger latencies if
the firware will erase flash blocks on NOR.

The time-functions are used by efi-rtc and can be triggered during
runtimed (either via explicit read/write or ntp sync).

The variable write could be used by pstore.
These functions can be disabled without much of a loss. The poweroff /
reboot hooks may be provided by PSCI.

Disable EFI's runtime wrappers.

This was observed on "EFI v2.60 by SoftIron Overdrive 1000".

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 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 232f4915223b..62c6e4b6ce3e 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -84,7 +84,7 @@ struct mm_struct efi_mm = {
 	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
 };
 
-static bool disable_runtime;
+static bool disable_runtime = IS_ENABLED(CONFIG_PREEMPT_RT_BASE);
 static int __init setup_noefi(char *arg)
 {
 	disable_runtime = true;
-- 
2.18.0

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Ard Biesheuvel]

On 26 July 2018 at 15:14, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2018-07-26 15:13:23 [+0200], To Ard Biesheuvel wrote:
>> On 2018-07-26 14:52:21 [+0200], Ard Biesheuvel wrote:
>> > We could also make it the default on -rt, but not disable it entirely, so that efi=runtime can be used to re-enable it.
>>
>> Oh. I like that. We have something similar for RCU. So I would need
>> that:
> and then I could make it default off:
>
> ----- >8
>
> Subject: [PATCH] efi: Disable runtime services on RT
>
> Based on meassurements the EFI functions get_variable /
> get_next_variable take up to 2us which looks okay.
> The functions get_time, set_time take around 10ms. Those 10ms are too
> much. Even one ms would be too much.
> Ard mentioned that SetVariable might even trigger larger latencies if
> the firware will erase flash blocks on NOR.
>
> The time-functions are used by efi-rtc and can be triggered during
> runtimed (either via explicit read/write or ntp sync).
>
> The variable write could be used by pstore.
> These functions can be disabled without much of a loss. The poweroff /
> reboot hooks may be provided by PSCI.
>
> Disable EFI's runtime wrappers.
>
> This was observed on "EFI v2.60 by SoftIron Overdrive 1000".
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  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 232f4915223b..62c6e4b6ce3e 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -84,7 +84,7 @@ struct mm_struct efi_mm = {
>         .mmlist                 = LIST_HEAD_INIT(efi_mm.mmlist),
>  };
>
> -static bool disable_runtime;
> +static bool disable_runtime = IS_ENABLED(CONFIG_PREEMPT_RT_BASE);
>  static int __init setup_noefi(char *arg)
>  {
>         disable_runtime = true;
> --
> 2.18.0
>

Yes, that's what I was thinking. This way, you can still reboot into
the same kernel occasionally with EFI runtime services enabled to,
e.g., use efibootmgr.

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

for both patches if you queue them in the -rt tree. If you want them
in mainline, please resend them as proper patches once
CONFIG_PREEMPT_RT_BASE has been declared in mainline as well.

Re: [PATCH RT] rtc: Disable RTC_DRV_EFI on RT

[Sebastian Andrzej Siewior]

On 2018-07-26 18:01:47 [+0200], Ard Biesheuvel wrote:
> Yes, that's what I was thinking. This way, you can still reboot into
> the same kernel occasionally with EFI runtime services enabled to,
> e.g., use efibootmgr.
> 
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> for both patches if you queue them in the -rt tree. If you want them
> in mainline, please resend them as proper patches once
> CONFIG_PREEMPT_RT_BASE has been declared in mainline as well.
Okay, thank you.

Sebastian