[PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Javier Martinez Canillas]

Commit d9f283ae71af ("efi: Disable runtime services on RT") disabled EFI
runtime services by default when the CONFIG_PREEMPT_RT option is enabled.

The rationale for that commit is that some EFI calls could take too much
time, leading to large latencies which is an issue for Real-Time kernels.

But a side effect of that change was that now is not possible anymore to
enable the EFI runtime services by default when CONFIG_PREEMPT_RT is set,
without passing an efi=runtime command line parameter to the kernel.

Instead, let's add a new EFI_DISABLE_RUNTIME boolean Kconfig option, that
would be set to n by default but to y if CONFIG_PREEMPT_RT is enabled.

That way, the current behaviour is preserved but gives users a mechanism
to enable the EFI runtimes services in their kernels if that is required.
For example, if the firmware could guarantee bounded time for EFI calls.

Also, having a separate boolean config could allow users to disable the
EFI runtime services by default even when CONFIG_PREEMPT_RT is not set.

Reported-by: Alexander Larsson <alexl@redhat.com>
Fixes: d9f283ae71af ("efi: Disable runtime services on RT")
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- Improve commit description to make clear the motivation for the change
  (Sebastian Andrzej Siewior).

 drivers/firmware/efi/Kconfig | 15 +++++++++++++++
 drivers/firmware/efi/efi.c   |  2 +-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2c3dac5ecb36..243882f5e5f9 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -284,3 +284,18 @@ config EFI_CUSTOM_SSDT_OVERLAYS
 
 	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
 	  information.
+
+config EFI_DISABLE_RUNTIME
+	bool "Disable EFI runtime services support by default"
+	default y if PREEMPT_RT
+	help
+	  Allow to disable the EFI runtime services support by default. This can
+	  already be achieved by using the efi=noruntime option, but it could be
+	  useful to have this default without any kernel command line parameter.
+
+	  The EFI runtime services are disabled by default when PREEMPT_RT is
+	  enabled, because measurements have shown that some EFI functions calls
+	  might take too much time to complete, causing large latencies which is
+	  an issue for Real-Time kernels.
+
+	  This default can be overridden by using the efi=runtime option.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 5502e176d51b..ff57db8f8d05 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -66,7 +66,7 @@ struct mm_struct efi_mm = {
 
 struct workqueue_struct *efi_rts_wq;
 
-static bool disable_runtime = IS_ENABLED(CONFIG_PREEMPT_RT);
+static bool disable_runtime = IS_ENABLED(CONFIG_EFI_DISABLE_RUNTIME);
 static int __init setup_noefi(char *arg)
 {
 	disable_runtime = true;
-- 
2.35.1

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Ard Biesheuvel]

On Thu, 31 Mar 2022 at 17:17, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Commit d9f283ae71af ("efi: Disable runtime services on RT") disabled EFI
> runtime services by default when the CONFIG_PREEMPT_RT option is enabled.
>
> The rationale for that commit is that some EFI calls could take too much
> time, leading to large latencies which is an issue for Real-Time kernels.
>
> But a side effect of that change was that now is not possible anymore to
> enable the EFI runtime services by default when CONFIG_PREEMPT_RT is set,
> without passing an efi=runtime command line parameter to the kernel.
>
> Instead, let's add a new EFI_DISABLE_RUNTIME boolean Kconfig option, that
> would be set to n by default but to y if CONFIG_PREEMPT_RT is enabled.
>
> That way, the current behaviour is preserved but gives users a mechanism
> to enable the EFI runtimes services in their kernels if that is required.
> For example, if the firmware could guarantee bounded time for EFI calls.
>
> Also, having a separate boolean config could allow users to disable the
> EFI runtime services by default even when CONFIG_PREEMPT_RT is not set.
>
> Reported-by: Alexander Larsson <alexl@redhat.com>
> Fixes: d9f283ae71af ("efi: Disable runtime services on RT")
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> Changes in v2:
> - Improve commit description to make clear the motivation for the change
>   (Sebastian Andrzej Siewior).
>

This looks ok to me. I'll queue this up once the merge window closes.

>  drivers/firmware/efi/Kconfig | 15 +++++++++++++++
>  drivers/firmware/efi/efi.c   |  2 +-
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 2c3dac5ecb36..243882f5e5f9 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -284,3 +284,18 @@ config EFI_CUSTOM_SSDT_OVERLAYS
>
>           See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
>           information.
> +
> +config EFI_DISABLE_RUNTIME
> +       bool "Disable EFI runtime services support by default"
> +       default y if PREEMPT_RT
> +       help
> +         Allow to disable the EFI runtime services support by default. This can
> +         already be achieved by using the efi=noruntime option, but it could be
> +         useful to have this default without any kernel command line parameter.
> +
> +         The EFI runtime services are disabled by default when PREEMPT_RT is
> +         enabled, because measurements have shown that some EFI functions calls
> +         might take too much time to complete, causing large latencies which is
> +         an issue for Real-Time kernels.
> +
> +         This default can be overridden by using the efi=runtime option.
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 5502e176d51b..ff57db8f8d05 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -66,7 +66,7 @@ struct mm_struct efi_mm = {
>
>  struct workqueue_struct *efi_rts_wq;
>
> -static bool disable_runtime = IS_ENABLED(CONFIG_PREEMPT_RT);
> +static bool disable_runtime = IS_ENABLED(CONFIG_EFI_DISABLE_RUNTIME);
>  static int __init setup_noefi(char *arg)
>  {
>         disable_runtime = true;
> --
> 2.35.1
>

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Ahmed S. Darwish]

Hi Ard, Javier,

Am Do, Mar 31, 2022, schrieb Ard Biesheuvel:
> On Thu, 31 Mar 2022 at 17:17, Javier Martinez Canillas
> <javierm@redhat.com> wrote:
> >
> > Commit d9f283ae71af ("efi: Disable runtime services on RT") disabled EFI
> > runtime services by default when the CONFIG_PREEMPT_RT option is enabled.
> >
> > The rationale for that commit is that some EFI calls could take too much
> > time, leading to large latencies which is an issue for Real-Time kernels.
> >
> > But a side effect of that change was that now is not possible anymore to
> > enable the EFI runtime services by default when CONFIG_PREEMPT_RT is set,
> > without passing an efi=runtime command line parameter to the kernel.
> >
> > Instead, let's add a new EFI_DISABLE_RUNTIME boolean Kconfig option, that
> > would be set to n by default but to y if CONFIG_PREEMPT_RT is enabled.
> >
> > That way, the current behaviour is preserved but gives users a mechanism
> > to enable the EFI runtimes services in their kernels if that is required.
> > For example, if the firmware could guarantee bounded time for EFI calls.
> >
> > Also, having a separate boolean config could allow users to disable the
> > EFI runtime services by default even when CONFIG_PREEMPT_RT is not set.
> >
> > Reported-by: Alexander Larsson <alexl@redhat.com>
> > Fixes: d9f283ae71af ("efi: Disable runtime services on RT")
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> >
> > Changes in v2:
> > - Improve commit description to make clear the motivation for the change
> >   (Sebastian Andrzej Siewior).
> >
>
> This looks ok to me. I'll queue this up once the merge window closes.
>

In case of (CONFIG_PREEMPT_RT=y && CONFIG_EFI_DISABLE_RUNTIME=n),
shouldn't we add a small message in the kernel log warning that EFI
runtime services are enabled for the RT kernel?

In almost all HW, except custom ones with "verified" firmware, such a
warning would be useful... This is especially true since in the embedded
domain, manually-configured RT kernels are almost always the norm.

Thanks,

--
Ahmed S. Darwish
Linutronix GmbH | Bahnhofstrasse 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 31; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Informations on data privacy
can be found here): https://linutronix.de/kontakt/Datenschutz.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen |
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700
806 | Geschäftsführer (Managing Directors): Heinz Egger, Thomas Gleixner

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Javier Martinez Canillas]

Hello Ahmed,

On Thu, Mar 31, 2022 at 9:36 PM Ahmed S. Darwish
<a.darwish@linutronix.de> wrote:
>
> Hi Ard, Javier,
>
> Am Do, Mar 31, 2022, schrieb Ard Biesheuvel:
> > On Thu, 31 Mar 2022 at 17:17, Javier Martinez Canillas
> > <javierm@redhat.com> wrote:
>

[snip]

> In case of (CONFIG_PREEMPT_RT=y && CONFIG_EFI_DISABLE_RUNTIME=n),
> shouldn't we add a small message in the kernel log warning that EFI
> runtime services are enabled for the RT kernel?
>
> In almost all HW, except custom ones with "verified" firmware, such a
> warning would be useful... This is especially true since in the embedded

I considered that as well but was not sure about what that message should be.

Since it will be printed even on systems whose EFI firmwares do not
have such long call times as the ones described in the commit that
disabled the runtime services for RT.

And in that case the warning may be misleading and make users believe
that a problem exists, which might not be accurate.

> domain, manually-configured RT kernels are almost always the norm.
>

Agreed. That is why the default for CONFIG_EFI_DISABLE_RUNTIME=y, if
CONFIG_PREEMPT_RT=y. So users will need to explicitly disable the
option if they want the EFI runtime services to be enabled with
CONFIG_PREEMPT_RT.

Best regards,
Javier

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Sebastian Andrzej Siewior]

On 2022-04-01 00:19:57 [+0200], Javier Martinez Canillas wrote:
> > In case of (CONFIG_PREEMPT_RT=y && CONFIG_EFI_DISABLE_RUNTIME=n),
> > shouldn't we add a small message in the kernel log warning that EFI
> > runtime services are enabled for the RT kernel?
> >
> > In almost all HW, except custom ones with "verified" firmware, such a
> > warning would be useful... This is especially true since in the embedded
> 
> I considered that as well but was not sure about what that message should be.

This makes sense and we had this in the past but dropped it for some
reason.

> Since it will be printed even on systems whose EFI firmwares do not
> have such long call times as the ones described in the commit that
> disabled the runtime services for RT.
> 
> And in that case the warning may be misleading and make users believe
> that a problem exists, which might not be accurate.

Does this matter? The efi-rtc driver is known to cause latencies but it
does not happen if the driver is not used. The same is probably true for
efi-vars: It won't cause high latencies on _read_ but then a certain
number of bit flips during read _may_ lead to write+erase which will
cause higher latencies.
Having a warning at boot (similar to trace_printk's warning) with the
options listed that are known to case high latencies might be a help.
There are some options that nobody will argue about like LOCKDEP. Then
there are other like WATCHDOG or this one, where a debate might start ;)

> Best regards,
> Javier

Sebastian

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Javier Martinez Canillas]

Hello Sebastian,

On 4/1/22 09:42, Sebastian Andrzej Siewior wrote:
> On 2022-04-01 00:19:57 [+0200], Javier Martinez Canillas wrote:
>>> In case of (CONFIG_PREEMPT_RT=y && CONFIG_EFI_DISABLE_RUNTIME=n),
>>> shouldn't we add a small message in the kernel log warning that EFI
>>> runtime services are enabled for the RT kernel?
>>>
>>> In almost all HW, except custom ones with "verified" firmware, such a
>>> warning would be useful... This is especially true since in the embedded
>>
>> I considered that as well but was not sure about what that message should be.
> 
> This makes sense and we had this in the past but dropped it for some
> reason.
> 

Ok, something like the following maybe? If you agree, I'll squash in v3:

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index ff57db8f8d05..08d329a5179b 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -362,6 +362,8 @@ static int __init efisubsys_init(void)
 
        if (!efi_enabled(EFI_RUNTIME_SERVICES))
                efi.runtime_supported_mask = 0;
+       else if (IS_ENABLED(CONFIG_PREEMPT_RT))
+               pr_warn("EFI runtime services can lead to high latencies on Real-Time kernels\n");
 
        if (!efi_enabled(EFI_BOOT))
                return 0;

>> Since it will be printed even on systems whose EFI firmwares do not
>> have such long call times as the ones described in the commit that
>> disabled the runtime services for RT.
>>
>> And in that case the warning may be misleading and make users believe
>> that a problem exists, which might not be accurate.
> 
> Does this matter? The efi-rtc driver is known to cause latencies but it
> does not happen if the driver is not used. The same is probably true for
> efi-vars: It won't cause high latencies on _read_ but then a certain
> number of bit flips during read _may_ lead to write+erase which will
> cause higher latencies.
> Having a warning at boot (similar to trace_printk's warning) with the
> options listed that are known to case high latencies might be a help.
> There are some options that nobody will argue about like LOCKDEP. Then
> there are other like WATCHDOG or this one, where a debate might start ;)
>

Yes, you are correct.
 
>> Best regards,
>> Javier
> 
> Sebastian
> 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Ard Biesheuvel]

On Fri, 1 Apr 2022 at 10:33, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello Sebastian,
>
> On 4/1/22 09:42, Sebastian Andrzej Siewior wrote:
> > On 2022-04-01 00:19:57 [+0200], Javier Martinez Canillas wrote:
> >>> In case of (CONFIG_PREEMPT_RT=y && CONFIG_EFI_DISABLE_RUNTIME=n),
> >>> shouldn't we add a small message in the kernel log warning that EFI
> >>> runtime services are enabled for the RT kernel?
> >>>
> >>> In almost all HW, except custom ones with "verified" firmware, such a
> >>> warning would be useful... This is especially true since in the embedded
> >>
> >> I considered that as well but was not sure about what that message should be.
> >
> > This makes sense and we had this in the past but dropped it for some
> > reason.
> >
>
> Ok, something like the following maybe? If you agree, I'll squash in v3:
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index ff57db8f8d05..08d329a5179b 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -362,6 +362,8 @@ static int __init efisubsys_init(void)
>
>         if (!efi_enabled(EFI_RUNTIME_SERVICES))
>                 efi.runtime_supported_mask = 0;
> +       else if (IS_ENABLED(CONFIG_PREEMPT_RT))
> +               pr_warn("EFI runtime services can lead to high latencies on Real-Time kernels\n");
>
>         if (!efi_enabled(EFI_BOOT))
>                 return 0;
>


I don't think we need another warning.

The kernel log already tells you whether or not EFI runtime services
are enabled.

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Javier Martinez Canillas]

Hello Ard,

On 4/1/22 10:34, Ard Biesheuvel wrote:
> On Fri, 1 Apr 2022 at 10:33, Javier Martinez Canillas
> <javierm@redhat.com> wrote:

[snip]

>>
>>         if (!efi_enabled(EFI_RUNTIME_SERVICES))
>>                 efi.runtime_supported_mask = 0;
>> +       else if (IS_ENABLED(CONFIG_PREEMPT_RT))
>> +               pr_warn("EFI runtime services can lead to high latencies on Real-Time kernels\n");
>>
>>         if (!efi_enabled(EFI_BOOT))
>>                 return 0;
>>
> 
> 
> I don't think we need another warning.
> 
> The kernel log already tells you whether or not EFI runtime services
> are enabled.
> 

I'm fine with this too. As mentioned to Ahmed I was torn on this, since I
understand the ask for the warning but also don't want to pollute the log.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Sebastian Andrzej Siewior]

On 2022-04-01 10:34:03 [+0200], Ard Biesheuvel wrote:
> I don't think we need another warning.

exactly.

> The kernel log already tells you whether or not EFI runtime services
> are enabled.

Correct. We either warn about all problematic options or none but this
is independent of this patch.

Sebastian

Re: [PATCH v2] efi: Allow to enable EFI runtime services by default on RT

[Ard Biesheuvel]

On Fri, 1 Apr 2022 at 11:05, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2022-04-01 10:34:03 [+0200], Ard Biesheuvel wrote:
> > I don't think we need another warning.
>
> exactly.
>
> > The kernel log already tells you whether or not EFI runtime services
> > are enabled.
>
> Correct. We either warn about all problematic options or none but this
> is independent of this patch.
>

Queued up now in efi/next. Thanks all.