Re: [PATCH 2/2] efi: Introduce efi_get_supported_rt_services() to get the runtime services mask

From: Ard Biesheuvel <ardb@kernel.org>
To: Jia He <justin.he@arm.com>
Cc: Huacai Chen <chenhuacai@kernel.org>,
	linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
	Alexandru Elisei <alexandru.elisei@arm.com>
Subject: Re: [PATCH 2/2] efi: Introduce efi_get_supported_rt_services() to get the runtime services mask
Date: Tue, 31 Jan 2023 08:06:38 +0100	[thread overview]
Message-ID: <CAMj1kXE5Ogcxi=Mk7rC8YDo7E825Lk6sxKeTwBmGM8TkrSPs2g@mail.gmail.com> (raw)
In-Reply-To: <20230131040355.3116-3-justin.he@arm.com>

On Tue, 31 Jan 2023 at 05:04, Jia He <justin.he@arm.com> wrote:
>
> Previously, efi.runtime_supported_mask will always be the initial value
> EFI_RT_SUPPORTED_ALL and can't be retrieved in efi_config_parse_tables()
> if rt_prop is EFI_INVALID_TABLE_ADDR. Thus this can cause the wrong
> return value of efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)
> on the Ampere Emag server.
>
> Besides, abstract the runtime services retrieving into a new exported
> helper efi_get_supported_rt_services() to set or clear the runtime service
> bit of efi.flags.
>
> Signed-off-by: Jia He <justin.he@arm.com>
> ---
>  drivers/firmware/efi/arm-runtime.c |  5 ++++-
>  drivers/firmware/efi/efi.c         | 28 +++++++++++++++++++---------
>  include/linux/efi.h                |  1 +
>  3 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> index 83f5bb57fa4c..ce93133f9abc 100644
> --- a/drivers/firmware/efi/arm-runtime.c
> +++ b/drivers/firmware/efi/arm-runtime.c
> @@ -146,7 +146,10 @@ static int __init arm_enable_runtime_services(void)
>
>         /* Set up runtime services function pointers */
>         efi_native_runtime_setup();
> -       set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +       if (efi_get_supported_rt_services())
> +               set_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +       else
> +               clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
>

This is not right. There are now other users of the EFI runtime
service infrastructure (ACPI PRM), so even if all EFI runtime services
are marked as unsupported, we should still set the
EFI_RUNTIME_SERVICES bit if the EFI runtime memory map is provided.

>         return 0;
>  }
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index a2b0cbc8741c..96475cb8088e 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -595,6 +595,24 @@ static __init int match_config_table(const efi_guid_t *guid,
>         return 0;
>  }
>
> +unsigned int __init efi_get_supported_rt_services(void)
> +{
> +       unsigned int runtime_supported_mask = EFI_RT_SUPPORTED_ALL;
> +
> +       if (rt_prop != EFI_INVALID_TABLE_ADDR) {
> +               efi_rt_properties_table_t *tbl;
> +
> +               tbl = early_memremap(rt_prop, sizeof(*tbl));
> +               if (tbl) {
> +                       runtime_supported_mask &= tbl->runtime_services_supported;
> +                       early_memunmap(tbl, sizeof(*tbl));
> +               }
> +       } else
> +               runtime_supported_mask = 0;
> +
> +       return runtime_supported_mask;
> +}
> +
>  int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>                                    int count,
>                                    const efi_config_table_type_t *arch_tables)
> @@ -695,15 +713,7 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
>                 }
>         }
>
> -       if (rt_prop != EFI_INVALID_TABLE_ADDR) {
> -               efi_rt_properties_table_t *tbl;
> -
> -               tbl = early_memremap(rt_prop, sizeof(*tbl));
> -               if (tbl) {
> -                       efi.runtime_supported_mask &= tbl->runtime_services_supported;
> -                       early_memunmap(tbl, sizeof(*tbl));
> -               }
> -       }
> +       efi.runtime_supported_mask &= efi_get_supported_rt_services();
>
>         if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) &&
>             initrd != EFI_INVALID_TABLE_ADDR && phys_initrd_size == 0) {
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 4b27519143f5..fcaf0d7fc07e 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -717,6 +717,7 @@ extern void __init efi_esrt_init(void);
>  #else
>  static inline void efi_esrt_init(void) { }
>  #endif
> +extern unsigned int efi_get_supported_rt_services(void);
>  extern int efi_config_parse_tables(const efi_config_table_t *config_tables,
>                                    int count,
>                                    const efi_config_table_type_t *arch_tables);
> --
> 2.25.1
>