[PATCH] x86/of: Restore possibility to use both ACPI and FDT from bootloader

[PATCH] x86/of: Restore possibility to use both ACPI and FDT from bootloader

[Dmytro Maluka]

There are cases when the bootloader provides information to the kernel
in both ACPI and DTB, not interchangeably. One such use case is virtual
machines in Android. When running on x86, the Android Virtualization
Framework (AVF) boots VMs with ACPI like it is usually done on x86 (i.e.
the virtual LAPIC, IOAPIC, HPET, PCI MMCONFIG etc are described in ACPI)
but also passes various AVF-specific boot parameters in DTB. This allows
reusing the same implementations of various AVF components on both
arm64 and x86.

Commit 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware")
removed the possibility to do that, since among other things
it introduced forcing emptying the bootloader-provided DTB if ACPI is
enabled (probably assuming that if ACPI is available, a DTB can only be
useful for applying overlays to it afterwards, for testing purposes).

So restore this possibility. At the same time, since the aforementioned
recently introduced restriction is actually useful for preventing
conflicts between ACPI and DT for LAPIC/IOAPIC/HPET setup, don't remove
this restriction completely but relax it: unflatten the bootloader
supplied DTB but don't try to use it for SMP setup (i.e. don't override
the .parse_smp_cfg callback) if ACPI is enabled. Precisely, right now
this prevents at least:

- incorrectly calling register_lapic_address(APIC_DEFAULT_PHYS_BASE)
  after the LAPIC was already successfully enumerated via ACPI, causing
  noisy kernel warnings and probably potential real issues as well

- failed IOAPIC setup in the case when IOAPIC is enumerated via mptable
  instead of ACPI (e.g. with acpi=noirq), due to
  mpparse_parse_smp_config() overridden by x86_dtb_parse_smp_config()

Fixes: 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware")
Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>
---
 arch/x86/kernel/devicetree.c |  3 ++-
 drivers/of/fdt.c             | 10 +---------
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 59d23cdf4ed0..dd8748c45529 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -2,6 +2,7 @@
 /*
  * Architecture specific OF callbacks.
  */
+#include <linux/acpi.h>
 #include <linux/export.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
@@ -313,6 +314,6 @@ void __init x86_flattree_get_config(void)
 	if (initial_dtb)
 		early_memunmap(dt, map_len);
 #endif
-	if (of_have_populated_dt())
+	if (acpi_disabled && of_have_populated_dt())
 		x86_init.mpparse.parse_smp_cfg = x86_dtb_parse_smp_config;
 }
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 0121100372b4..3b29a5c50e2e 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -8,7 +8,6 @@
 
 #define pr_fmt(fmt)	"OF: fdt: " fmt
 
-#include <linux/acpi.h>
 #include <linux/crash_dump.h>
 #include <linux/crc32.h>
 #include <linux/kernel.h>
@@ -1215,14 +1214,7 @@ void __init unflatten_device_tree(void)
 	/* Save the statically-placed regions in the reserved_mem array */
 	fdt_scan_reserved_mem_reg_nodes();
 
-	/* Don't use the bootloader provided DTB if ACPI is enabled */
-	if (!acpi_disabled)
-		fdt = NULL;
-
-	/*
-	 * Populate an empty root node when ACPI is enabled or bootloader
-	 * doesn't provide one.
-	 */
+	/* Populate an empty root node when bootloader doesn't provide one */
 	if (!fdt) {
 		fdt = (void *) __dtb_empty_root_begin;
 		/* fdt_totalsize() will be used for copy size */
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH] x86/of: Restore possibility to use both ACPI and FDT from bootloader

[Rob Herring]

On Mon, Dec 23, 2024 at 06:18:03PM +0000, Dmytro Maluka wrote:
> There are cases when the bootloader provides information to the kernel
> in both ACPI and DTB, not interchangeably. One such use case is virtual
> machines in Android. When running on x86, the Android Virtualization
> Framework (AVF) boots VMs with ACPI like it is usually done on x86 (i.e.
> the virtual LAPIC, IOAPIC, HPET, PCI MMCONFIG etc are described in ACPI)
> but also passes various AVF-specific boot parameters in DTB. This allows
> reusing the same implementations of various AVF components on both
> arm64 and x86.

Anyone booting Arm ACPI based systems with AVF?

Where's this AVF binding documented? 

> Commit 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware")
> removed the possibility to do that, since among other things
> it introduced forcing emptying the bootloader-provided DTB if ACPI is
> enabled (probably assuming that if ACPI is available, a DTB can only be
> useful for applying overlays to it afterwards, for testing purposes).
> 
> So restore this possibility. At the same time, since the aforementioned
> recently introduced restriction is actually useful for preventing
> conflicts between ACPI and DT for LAPIC/IOAPIC/HPET setup, don't remove
> this restriction completely but relax it: unflatten the bootloader
> supplied DTB but don't try to use it for SMP setup (i.e. don't override
> the .parse_smp_cfg callback) if ACPI is enabled. Precisely, right now
> this prevents at least:
> 
> - incorrectly calling register_lapic_address(APIC_DEFAULT_PHYS_BASE)
>   after the LAPIC was already successfully enumerated via ACPI, causing
>   noisy kernel warnings and probably potential real issues as well
> 
> - failed IOAPIC setup in the case when IOAPIC is enumerated via mptable
>   instead of ACPI (e.g. with acpi=noirq), due to
>   mpparse_parse_smp_config() overridden by x86_dtb_parse_smp_config()

It would be better if we explicitly opt'ed into "things we want to get 
from DT" rather than allowing anything except what we check for. There's 
a strong desire at least for arm64 to prevent systems from using both 
at the same time. There are growing usecases for doing just that, but I 
think we need to have some control or restrictions in place to define 
what we support in the kernel.

> Fixes: 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware")
> Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>
> ---
>  arch/x86/kernel/devicetree.c |  3 ++-
>  drivers/of/fdt.c             | 10 +---------
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index 59d23cdf4ed0..dd8748c45529 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -2,6 +2,7 @@
>  /*
>   * Architecture specific OF callbacks.
>   */
> +#include <linux/acpi.h>
>  #include <linux/export.h>
>  #include <linux/io.h>
>  #include <linux/interrupt.h>
> @@ -313,6 +314,6 @@ void __init x86_flattree_get_config(void)
>  	if (initial_dtb)
>  		early_memunmap(dt, map_len);
>  #endif
> -	if (of_have_populated_dt())
> +	if (acpi_disabled && of_have_populated_dt())
>  		x86_init.mpparse.parse_smp_cfg = x86_dtb_parse_smp_config;

I would make this a separate patch. Then it can be backported to kernel 
versions without 7b937cc243e5. And then Thomas can take it and I can 
take the DT part.

>  }

Re: [PATCH] x86/of: Restore possibility to use both ACPI and FDT from bootloader

[Dmytro Maluka]

On Thu, Jan 02, 2025 at 12:08:07PM -0600, Rob Herring wrote:
> On Mon, Dec 23, 2024 at 06:18:03PM +0000, Dmytro Maluka wrote:
> > There are cases when the bootloader provides information to the kernel
> > in both ACPI and DTB, not interchangeably. One such use case is virtual
> > machines in Android. When running on x86, the Android Virtualization
> > Framework (AVF) boots VMs with ACPI like it is usually done on x86 (i.e.
> > the virtual LAPIC, IOAPIC, HPET, PCI MMCONFIG etc are described in ACPI)
> > but also passes various AVF-specific boot parameters in DTB. This allows
> > reusing the same implementations of various AVF components on both
> > arm64 and x86.
> 
> Anyone booting Arm ACPI based systems with AVF?

No, on Arm side AVF is DT only.

> Where's this AVF binding documented? 

The strictly AVF-specific properties are described in [1]. When it comes
to Linux guests, actually AFAIK currently all those properties are
consumed by the guest userspace, not the guest kernel, so the role of
the kernel is in fact just to pass them over to the userspace via sysfs.

Besides that, one important DT binding used by AVF's Linux guest
kernels, not userspace, is google,open-dice documented in
Documentation/devicetree/bindings/reserved-memory/google,open-dice.yaml.
[2] describes how it is used by the protected VM firmware (pvmfw) to
securely boot protected VMs.

Cc'ing some AVF folks to keep me honest.

(AVF used to support protected VMs on arm64 only, but now we are trying
to make them work on x86. So, taking google,open-dice as an example, we
could add an ACPI binding to the open-dice driver, however bloating
pvmfw with AML support is a no go, so we want to keep passing it via DT
on x86 as well.)

> > Commit 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware")
> > removed the possibility to do that, since among other things
> > it introduced forcing emptying the bootloader-provided DTB if ACPI is
> > enabled (probably assuming that if ACPI is available, a DTB can only be
> > useful for applying overlays to it afterwards, for testing purposes).
> > 
> > So restore this possibility. At the same time, since the aforementioned
> > recently introduced restriction is actually useful for preventing
> > conflicts between ACPI and DT for LAPIC/IOAPIC/HPET setup, don't remove
> > this restriction completely but relax it: unflatten the bootloader
> > supplied DTB but don't try to use it for SMP setup (i.e. don't override
> > the .parse_smp_cfg callback) if ACPI is enabled. Precisely, right now
> > this prevents at least:
> > 
> > - incorrectly calling register_lapic_address(APIC_DEFAULT_PHYS_BASE)
> >   after the LAPIC was already successfully enumerated via ACPI, causing
> >   noisy kernel warnings and probably potential real issues as well
> > 
> > - failed IOAPIC setup in the case when IOAPIC is enumerated via mptable
> >   instead of ACPI (e.g. with acpi=noirq), due to
> >   mpparse_parse_smp_config() overridden by x86_dtb_parse_smp_config()
> 
> It would be better if we explicitly opt'ed into "things we want to get 
> from DT" rather than allowing anything except what we check for. There's 
> a strong desire at least for arm64 to prevent systems from using both 
> at the same time. There are growing usecases for doing just that, but I 
> think we need to have some control or restrictions in place to define 
> what we support in the kernel.

When it comes to arm64, AFAICS it already enforces this mutual
exclusion (in setup_arch()):

	if (acpi_disabled)
		unflatten_device_tree();

For Android use cases it is fine (Android doesn't use ACPI on Arm,
not for VMs anyway), so this patch doesn't change it.

> > Fixes: 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware")
> > Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>
> > ---
> >  arch/x86/kernel/devicetree.c |  3 ++-
> >  drivers/of/fdt.c             | 10 +---------
> >  2 files changed, 3 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> > index 59d23cdf4ed0..dd8748c45529 100644
> > --- a/arch/x86/kernel/devicetree.c
> > +++ b/arch/x86/kernel/devicetree.c
> > @@ -2,6 +2,7 @@
> >  /*
> >   * Architecture specific OF callbacks.
> >   */
> > +#include <linux/acpi.h>
> >  #include <linux/export.h>
> >  #include <linux/io.h>
> >  #include <linux/interrupt.h>
> > @@ -313,6 +314,6 @@ void __init x86_flattree_get_config(void)
> >  	if (initial_dtb)
> >  		early_memunmap(dt, map_len);
> >  #endif
> > -	if (of_have_populated_dt())
> > +	if (acpi_disabled && of_have_populated_dt())
> >  		x86_init.mpparse.parse_smp_cfg = x86_dtb_parse_smp_config;
> 
> I would make this a separate patch. Then it can be backported to kernel 
> versions without 7b937cc243e5. And then Thomas can take it and I can 
> take the DT part.

Sure, I can do that.

> 
> >  }

[1] https://android.googlesource.com/platform/packages/modules/Virtualization/+/refs/heads/main/docs/device_trees.md#dt-for-guests
[2] https://android.googlesource.com/platform/packages/modules/Virtualization/+/refs/heads/main/guest/pvmfw/#handover-abi

Re: [PATCH] x86/of: Restore possibility to use both ACPI and FDT from bootloader

[Rob Herring]

On Thu, Jan 2, 2025 at 4:18 PM Dmytro Maluka <dmaluka@chromium.org> wrote:
>
> On Thu, Jan 02, 2025 at 12:08:07PM -0600, Rob Herring wrote:
> > On Mon, Dec 23, 2024 at 06:18:03PM +0000, Dmytro Maluka wrote:
> > > There are cases when the bootloader provides information to the kernel
> > > in both ACPI and DTB, not interchangeably. One such use case is virtual
> > > machines in Android. When running on x86, the Android Virtualization
> > > Framework (AVF) boots VMs with ACPI like it is usually done on x86 (i.e.
> > > the virtual LAPIC, IOAPIC, HPET, PCI MMCONFIG etc are described in ACPI)
> > > but also passes various AVF-specific boot parameters in DTB. This allows
> > > reusing the same implementations of various AVF components on both
> > > arm64 and x86.
> >
> > Anyone booting Arm ACPI based systems with AVF?
>
> No, on Arm side AVF is DT only.
>
> > Where's this AVF binding documented?
>
> The strictly AVF-specific properties are described in [1]. When it comes
> to Linux guests, actually AFAIK currently all those properties are
> consumed by the guest userspace, not the guest kernel, so the role of
> the kernel is in fact just to pass them over to the userspace via sysfs.
>
> Besides that, one important DT binding used by AVF's Linux guest
> kernels, not userspace, is google,open-dice documented in
> Documentation/devicetree/bindings/reserved-memory/google,open-dice.yaml.
> [2] describes how it is used by the protected VM firmware (pvmfw) to
> securely boot protected VMs.
>
> Cc'ing some AVF folks to keep me honest.
>
> (AVF used to support protected VMs on arm64 only, but now we are trying
> to make them work on x86. So, taking google,open-dice as an example, we
> could add an ACPI binding to the open-dice driver, however bloating
> pvmfw with AML support is a no go, so we want to keep passing it via DT
> on x86 as well.)
>
> > > Commit 7b937cc243e5 ("of: Create of_root if no dtb provided by firmware")
> > > removed the possibility to do that, since among other things
> > > it introduced forcing emptying the bootloader-provided DTB if ACPI is
> > > enabled (probably assuming that if ACPI is available, a DTB can only be
> > > useful for applying overlays to it afterwards, for testing purposes).
> > >
> > > So restore this possibility. At the same time, since the aforementioned
> > > recently introduced restriction is actually useful for preventing
> > > conflicts between ACPI and DT for LAPIC/IOAPIC/HPET setup, don't remove
> > > this restriction completely but relax it: unflatten the bootloader
> > > supplied DTB but don't try to use it for SMP setup (i.e. don't override
> > > the .parse_smp_cfg callback) if ACPI is enabled. Precisely, right now
> > > this prevents at least:
> > >
> > > - incorrectly calling register_lapic_address(APIC_DEFAULT_PHYS_BASE)
> > >   after the LAPIC was already successfully enumerated via ACPI, causing
> > >   noisy kernel warnings and probably potential real issues as well
> > >
> > > - failed IOAPIC setup in the case when IOAPIC is enumerated via mptable
> > >   instead of ACPI (e.g. with acpi=noirq), due to
> > >   mpparse_parse_smp_config() overridden by x86_dtb_parse_smp_config()
> >
> > It would be better if we explicitly opt'ed into "things we want to get
> > from DT" rather than allowing anything except what we check for. There's
> > a strong desire at least for arm64 to prevent systems from using both
> > at the same time. There are growing usecases for doing just that, but I
> > think we need to have some control or restrictions in place to define
> > what we support in the kernel.
>
> When it comes to arm64, AFAICS it already enforces this mutual
> exclusion (in setup_arch()):
>
>         if (acpi_disabled)
>                 unflatten_device_tree();

That's right. The patch to remove the if here was rejected, but I
applied the rest of the series. The acpi_disabled check you are
removing from unflatten_device_tree() was there to handle the arm64
desire to not use the bootloader provided DT in case it contained
parallel h/w descriptions. So with removing it, we'll have to come up
with another way if we enable arm64. But I'm leaving fighting over
supporting the usecases on Arm to someone who needs it.

Note that there's also an interaction with kexec that has to be
considered since kexec needs the bootloader provided DT. Probably not
an issue in your use on x86 though.

Rob

Re: [PATCH] x86/of: Restore possibility to use both ACPI and FDT from bootloader

[Christoph Hellwig]

On Mon, Dec 23, 2024 at 06:18:03PM +0000, Dmytro Maluka wrote:
> There are cases when the bootloader provides information to the kernel
> in both ACPI and DTB, not interchangeably. One such use case is virtual
> machines in Android. When running on x86, the Android Virtualization
> Framework (AVF) boots VMs with ACPI like it is usually done on x86 (i.e.
> the virtual LAPIC, IOAPIC, HPET, PCI MMCONFIG etc are described in ACPI)
> but also passes various AVF-specific boot parameters in DTB. This allows
> reusing the same implementations of various AVF components on both
> arm64 and x86.

What are these "AVF-specific boot parameters"?

Re: [PATCH] x86/of: Restore possibility to use both ACPI and FDT from bootloader

[Dmytro Maluka]

On Mon, Jan 06, 2025 at 03:16:15AM -0800, Christoph Hellwig wrote:
> On Mon, Dec 23, 2024 at 06:18:03PM +0000, Dmytro Maluka wrote:
> > There are cases when the bootloader provides information to the kernel
> > in both ACPI and DTB, not interchangeably. One such use case is virtual
> > machines in Android. When running on x86, the Android Virtualization
> > Framework (AVF) boots VMs with ACPI like it is usually done on x86 (i.e.
> > the virtual LAPIC, IOAPIC, HPET, PCI MMCONFIG etc are described in ACPI)
> > but also passes various AVF-specific boot parameters in DTB. This allows
> > reusing the same implementations of various AVF components on both
> > arm64 and x86.
> 
> What are these "AVF-specific boot parameters"?

Let me quote my reply to Rob:

The strictly AVF-specific properties are described in [1]. When it comes
to Linux guests, actually AFAIK currently all those properties are
consumed by the guest userspace, not the guest kernel, so the role of
the kernel is in fact just to pass them over to the userspace via sysfs.

Besides that, one important DT binding used by AVF's Linux guest
kernels, not userspace, is google,open-dice documented in
Documentation/devicetree/bindings/reserved-memory/google,open-dice.yaml.
[2] describes how it is used by the protected VM firmware (pvmfw) to
securely boot protected VMs.

Cc'ing some AVF folks to keep me honest.

(AVF used to support protected VMs on arm64 only, but now we are trying
to make them work on x86. So, taking google,open-dice as an example, we
could add an ACPI binding to the open-dice driver, however bloating
pvmfw with AML support is a no go, so we want to keep passing it via DT
on x86 as well.)

[1] https://android.googlesource.com/platform/packages/modules/Virtualization/+/refs/heads/main/docs/device_trees.md#dt-for-guests
[2] https://android.googlesource.com/platform/packages/modules/Virtualization/+/refs/heads/main/guest/pvmfw/#handover-abi