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

[Rob Clark]

On Mon, Dec 2, 2019 at 4:29 AM Leif Lindholm <leif.lindholm@xxxxxxxxxx> 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@xxxxxxxxx> 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