Message-ID: <20191202122929.GC7359@bivouac.eciton.net>
Date:   Mon, 2 Dec 2019 12:29:29 +0000
From:   Leif Lindholm <leif.lindholm@...aro.org>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     Rob Clark <robdclark@...il.com>,
        Rob Clark <robdclark@...omium.org>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        Ingo Molnar <mingo@...nel.org>,
        Kairui Song <kasong@...hat.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Matthew Garrett <matthewgarrett@...gle.com>,
        "open list:EXTENSIBLE FIRMWARE INTERFACE (EFI)" 
        <linux-efi@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] efi/fdt: install new fdt config table

On Mon, Dec 02, 2019 at 10:35:05 +0100, Ard Biesheuvel wrote:
> On Sat, 30 Nov 2019 at 20:51, Rob Clark <robdclark@...il.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

Powered by blists - more mailing lists