[PATCH] efi/fdt: install new fdt config table

[PATCH] efi/fdt: install new fdt config table

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

If there is an fdt config table, replace it with the newly allocated one
before calling ExitBootServices().

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
The DtbLoader.efi[1] driver, which Leif created for EBBR boot on the
"windows" aarch64 laptops, tries to detect in an EBS hook, whether the
HLOS is using DT.  It does this by realizing that the kernel cmdline is
patched in to the fdt, and comparing the CRC.  If the CRC has changed,
that means the HLOS is using DT boot, so it removes the ACPI config
tables, so that the HLOS does not see two conflicting sets of config
tables.

However, I realized this mechanism does not work when directly loading
the linux kernel as an efi executable (ie. without an intermediary like
grub doing it's own DT modifications), because efi/libstub is modifying
a copy of the DT config table.

If we update the config table with the new DT, then it will realize
properly that the HLOS is using DT.

[1] https://git.linaro.org/people/leif.lindholm/edk2.git/log/?h=dtbloader

 .../firmware/efi/libstub/efi-stub-helper.c    | 32 +++++++++++++++++++
 drivers/firmware/efi/libstub/efistub.h        |  2 ++
 drivers/firmware/efi/libstub/fdt.c            |  2 ++
 3 files changed, 36 insertions(+)

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 35dbc2791c97..210070f3b231 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -953,3 +953,35 @@ void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid)
 	else
 		return get_efi_config_table32(sys_table, guid);
 }
+
+#define REPLACE_EFI_CONFIG_TABLE(bits)					\
+static void *replace_efi_config_table##bits(efi_system_table_t *_sys_table, \
+					efi_guid_t guid, void *table)	\
+{									\
+	efi_system_table_##bits##_t *sys_table;				\
+	efi_config_table_##bits##_t *tables;				\
+	int i;								\
+									\
+	sys_table = (typeof(sys_table))_sys_table;			\
+	tables = (typeof(tables))(unsigned long)sys_table->tables;	\
+									\
+	for (i = 0; i < sys_table->nr_tables; i++) {			\
+		if (efi_guidcmp(tables[i].guid, guid) != 0)		\
+			continue;					\
+									\
+		tables[i].table = (uintptr_t)table;			\
+		return;							\
+	}								\
+}
+REPLACE_EFI_CONFIG_TABLE(32)
+REPLACE_EFI_CONFIG_TABLE(64)
+
+/* replaces an existing config table: */
+void replace_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid,
+			  void *table)
+{
+	if (efi_is_64bit())
+		replace_efi_config_table64(sys_table, guid, table);
+	else
+		replace_efi_config_table32(sys_table, guid, table);
+}
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 7f1556fd867d..66f2927ce26f 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -66,6 +66,8 @@ efi_status_t check_platform_features(efi_system_table_t *sys_table_arg);
 efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg);
 
 void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid);
+void replace_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid,
+			  void *table);
 
 /* Helper macros for the usual case of using simple C variables: */
 #ifndef fdt_setprop_inplace_var
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 0bf0190917e0..15887ec2dc3b 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -313,6 +313,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	priv.runtime_entry_count	= &runtime_entry_count;
 	priv.new_fdt_addr		= (void *)*new_fdt_addr;
 
+	replace_efi_config_table(sys_table, DEVICE_TREE_GUID, priv.new_fdt_addr);
+
 	status = efi_exit_boot_services(sys_table, handle, &map, &priv, exit_boot_func);
 
 	if (status == EFI_SUCCESS) {
-- 
2.23.0

Re: [PATCH] efi/fdt: install new fdt config table

[Ard Biesheuvel]

On Sat, 30 Nov 2019 at 20:51, Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> If there is an fdt config table, replace it with the newly allocated one
> before calling ExitBootServices().
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> The DtbLoader.efi[1] driver, which Leif created for EBBR boot on the
> "windows" aarch64 laptops, tries to detect in an EBS hook, whether the
> HLOS is using DT.  It does this by realizing that the kernel cmdline is
> patched in to the fdt, and comparing the CRC.  If the CRC has changed,
> that means the HLOS is using DT boot, so it removes the ACPI config
> tables, so that the HLOS does not see two conflicting sets of config
> tables.
>
> However, I realized this mechanism does not work when directly loading
> the linux kernel as an efi executable (ie. without an intermediary like
> grub doing it's own DT modifications), because efi/libstub is modifying
> a copy of the DT config table.
>
> If we update the config table with the new DT, then it will realize
> properly that the HLOS is using DT.
>

Hey Rob,

I understand what you are trying to do here, but this is not the right solution.

I have rejected patches in the past where config tables are used to
communicate information back to the firmware, as creating reverse
dependencies like this is a recipe for disaster.

IIUC, the problem is that the DtbLoader attempts to be smart about
whether to install the DT config table, and to only do so if the OS is
going to boot in DT mode. This is based on the principle that we
should not expose both ACPI and DT tables, and make it the OS's
problem to reason about which one is the preferred description.

I agree with that approach in general, but in this particular case, I
don't think it makes sense. Windows only cares about ACPI, and Linux
only cares about DT unless you instruct it specifically to prioritize
ACPI over DT. So the problem that this solves does not exist in
practice, and we're much better off just exposing the DT alongside the
ACPI tables, and let the OS use whichever one it wants.

Also, the stub always reallocates the FDT, and so the CRC check is
only detecting whether the DT is being touched by GRUB or not.

So removing the ACPI tables like this is not something I think we
should be doing at all. As a compromise, you might add 'acpi=off' to
/chosen/cmdline so that GRUBless boot into Linux does not
inadvertently ends up booting in ACPI mode.

Thanks,
Ard.




> [1] https://git.linaro.org/people/leif.lindholm/edk2.git/log/?h=dtbloader
>
>  .../firmware/efi/libstub/efi-stub-helper.c    | 32 +++++++++++++++++++
>  drivers/firmware/efi/libstub/efistub.h        |  2 ++
>  drivers/firmware/efi/libstub/fdt.c            |  2 ++
>  3 files changed, 36 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 35dbc2791c97..210070f3b231 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -953,3 +953,35 @@ void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid)
>         else
>                 return get_efi_config_table32(sys_table, guid);
>  }
> +
> +#define REPLACE_EFI_CONFIG_TABLE(bits)                                 \
> +static void *replace_efi_config_table##bits(efi_system_table_t *_sys_table, \
> +                                       efi_guid_t guid, void *table)   \
> +{                                                                      \
> +       efi_system_table_##bits##_t *sys_table;                         \
> +       efi_config_table_##bits##_t *tables;                            \
> +       int i;                                                          \
> +                                                                       \
> +       sys_table = (typeof(sys_table))_sys_table;                      \
> +       tables = (typeof(tables))(unsigned long)sys_table->tables;      \
> +                                                                       \
> +       for (i = 0; i < sys_table->nr_tables; i++) {                    \
> +               if (efi_guidcmp(tables[i].guid, guid) != 0)             \
> +                       continue;                                       \
> +                                                                       \
> +               tables[i].table = (uintptr_t)table;                     \
> +               return;                                                 \
> +       }                                                               \
> +}
> +REPLACE_EFI_CONFIG_TABLE(32)
> +REPLACE_EFI_CONFIG_TABLE(64)
> +
> +/* replaces an existing config table: */
> +void replace_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid,
> +                         void *table)
> +{
> +       if (efi_is_64bit())
> +               replace_efi_config_table64(sys_table, guid, table);
> +       else
> +               replace_efi_config_table32(sys_table, guid, table);
> +}
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index 7f1556fd867d..66f2927ce26f 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -66,6 +66,8 @@ efi_status_t check_platform_features(efi_system_table_t *sys_table_arg);
>  efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg);
>
>  void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid);
> +void replace_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid,
> +                         void *table);
>
>  /* Helper macros for the usual case of using simple C variables: */
>  #ifndef fdt_setprop_inplace_var
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 0bf0190917e0..15887ec2dc3b 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -313,6 +313,8 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>         priv.runtime_entry_count        = &runtime_entry_count;
>         priv.new_fdt_addr               = (void *)*new_fdt_addr;
>
> +       replace_efi_config_table(sys_table, DEVICE_TREE_GUID, priv.new_fdt_addr);
> +
>         status = efi_exit_boot_services(sys_table, handle, &map, &priv, exit_boot_func);
>
>         if (status == EFI_SUCCESS) {
> --
> 2.23.0
>

Re: [PATCH] efi/fdt: install new fdt config table

[Leif Lindholm]

On Mon, Dec 02, 2019 at 10:35:05 +0100, Ard Biesheuvel wrote:
> On Sat, 30 Nov 2019 at 20:51, Rob Clark <robdclark@gmail.com> wrote:
>
> I understand what you are trying to do here, but this is not the right solution.
> 
> I have rejected patches in the past where config tables are used to
> communicate information back to the firmware, as creating reverse
> dependencies like this is a recipe for disaster.

This isn't *technically* communicating information back to the
firmware, since the agent that ends up being invoked is a driver that
has been explicitly installed in order to permit running Linux without
hacking about with GRUB. (But it's certainly communicating it back to
the firmware context.)

> IIUC, the problem is that the DtbLoader attempts to be smart about
> whether to install the DT config table, and to only do so if the OS is
> going to boot in DT mode. This is based on the principle that we
> should not expose both ACPI and DT tables, and make it the OS's
> problem to reason about which one is the preferred description.
> 
> I agree with that approach in general, but in this particular case, I
> don't think it makes sense. Windows only cares about ACPI, and Linux
> only cares about DT unless you instruct it specifically to prioritize
> ACPI over DT.

I guess it's worth pointing out here that this approach (DtbLoader)
predates the finding out that these devices use PEP instead of ACPI
for power management (which I guess makes it ACI) - so ACPI can never
be usefully supported.

> So the problem that this solves does not exist in
> practice, and we're much better off just exposing the DT alongside the
> ACPI tables, and let the OS use whichever one it wants.

Wo-hoo...

> Also, the stub always reallocates the FDT, and so the CRC check is
> only detecting whether the DT is being touched by GRUB or not.

So does GRUB.

DtbLoader looks up the DT through the system table again as part of
the ExitBootservices hook. An address change *or* a CRC change
triggers the ACPI deletion.

This was the problem Rob was trying to address - ensuring the hook
gets invoked even where the stub was the one that updated the DT.

But given the situation we're in, I don't really disagree with you
anyway.

Let's just be clear that this isn't a free-for-all - this is because
the abstraction of power management on this family of machines is
broken by design.

> So removing the ACPI tables like this is not something I think we
> should be doing at all. As a compromise, you might add 'acpi=off' to
> /chosen/cmdline so that GRUBless boot into Linux does not
> inadvertently ends up booting in ACPI mode.

If so, some form of (out-of-tree) sanity check would be needed on
distros carrying out-of-tree patches that disable DT boot.

It *is* possible to boot these machines using only ACPI. It's just not
a very great user experience with all cores running at minimum
frequency.

/
    Leif

Re: [PATCH] efi/fdt: install new fdt config table

[Rob Clark]

On Mon, Dec 2, 2019 at 4:29 AM Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Mon, Dec 02, 2019 at 10:35:05 +0100, Ard Biesheuvel wrote:
> > On Sat, 30 Nov 2019 at 20:51, Rob Clark <robdclark@gmail.com> wrote:
> >
> > I understand what you are trying to do here, but this is not the right solution.
> >
> > I have rejected patches in the past where config tables are used to
> > communicate information back to the firmware, as creating reverse
> > dependencies like this is a recipe for disaster.
>
> This isn't *technically* communicating information back to the
> firmware, since the agent that ends up being invoked is a driver that
> has been explicitly installed in order to permit running Linux without
> hacking about with GRUB. (But it's certainly communicating it back to
> the firmware context.)
>
> > IIUC, the problem is that the DtbLoader attempts to be smart about
> > whether to install the DT config table, and to only do so if the OS is
> > going to boot in DT mode. This is based on the principle that we
> > should not expose both ACPI and DT tables, and make it the OS's
> > problem to reason about which one is the preferred description.
> >
> > I agree with that approach in general, but in this particular case, I
> > don't think it makes sense. Windows only cares about ACPI, and Linux
> > only cares about DT unless you instruct it specifically to prioritize
> > ACPI over DT.

well, I don't mind too much if the patch is rejected, it doesn't
really cause a problem (at least upstream) to have both ACPI and DT
config tables.  But I figured I'd at least point out why DtbLoader
wasn't removing the ACPI tables (when grub is not involved) in the
form of a patch..

>
> I guess it's worth pointing out here that this approach (DtbLoader)
> predates the finding out that these devices use PEP instead of ACPI
> for power management (which I guess makes it ACI) - so ACPI can never
> be usefully supported.

I don't think this is necessarily true.  With ACPI boot you can still
get far enough to run an installer and get to the point where you can
install OS updates.  The experience isn't great without PEP.. but more
or less the same is true on x86 laptops, where you need to get to the
point of installing updated kernel and/or mesa to have accelerated
graphics and other nice things.  The "PEP" driver is just another
thing that needs to get installed via HLOS updates.

That said, coming up with some sort of PEP mechanism for linux is
beyond what I have weekend hacking bandwidth for, so I'll leave that
for someone else.  Right now I'd be happy to get the various patchsets
we need to have things working landed upstream, and have some sort of
sane/standardized way to manage DT boot.  (Either DtbLoader or
something in grub picking the correct dtb file based on what we can
pull out of SMBIOS tables seems the sanest thing to me.)

Eventually if we have enough different aarch64 "ACI" laptops and users
out there, I think the DT approach will become too painful and we'll
have to tackle PEP for linux.. at least thats a problem I hope we have
eventually.

BR,
-R

>
> > So the problem that this solves does not exist in
> > practice, and we're much better off just exposing the DT alongside the
> > ACPI tables, and let the OS use whichever one it wants.
>
> Wo-hoo...
>
> > Also, the stub always reallocates the FDT, and so the CRC check is
> > only detecting whether the DT is being touched by GRUB or not.
>
> So does GRUB.
>
> DtbLoader looks up the DT through the system table again as part of
> the ExitBootservices hook. An address change *or* a CRC change
> triggers the ACPI deletion.
>
> This was the problem Rob was trying to address - ensuring the hook
> gets invoked even where the stub was the one that updated the DT.
>
> But given the situation we're in, I don't really disagree with you
> anyway.
>
> Let's just be clear that this isn't a free-for-all - this is because
> the abstraction of power management on this family of machines is
> broken by design.
>
> > So removing the ACPI tables like this is not something I think we
> > should be doing at all. As a compromise, you might add 'acpi=off' to
> > /chosen/cmdline so that GRUBless boot into Linux does not
> > inadvertently ends up booting in ACPI mode.
>
> If so, some form of (out-of-tree) sanity check would be needed on
> distros carrying out-of-tree patches that disable DT boot.
>
> It *is* possible to boot these machines using only ACPI. It's just not
> a very great user experience with all cores running at minimum
> frequency.
>
> /
>     Leif