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@xxxxxxxxxxxx>
> > ---
> > 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
