Re: [PATCH] ArmVirtPkg/PlatformHasAcpiDtDxe: allow manual override for DT installation

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 29 March 2017 at 17:40, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/29/17 18:07, Ard Biesheuvel wrote:
>> On 29 March 2017 at 17:03, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 03/29/17 18:02, Ard Biesheuvel wrote:
>>>> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote:
[..]
>>>>> NACK
>>>>>
>>>>
>>>> OK, fair enough. How do you propose to handle this regression then?
>>>>
>>>
>>> I don't.
>>
>> I think I am entitled to a more constructive attitude from you. I
>> don't care about politics, but I do care about not breaking people's
>> workflows. So if you insist on getting on your high horse and throw
>> capitalized NACKs at me, you could at least *pretend* to care about
>> other users than *your* downstream, and come up with some alternative.
>
> Ard, I'm not on my high horse. I thought we were in agreement about
> this. It was obvious (to me at least) that this policy would be
> considered a regression by those who wanted both DT and ACPI in the
> guest. From my side, breaking that was all intentional.
>

Well, speaking for myself, I did not consider the need of some to have
both descriptions available. I take those needs very seriously.

> I'm not deliberately screwing with users (just because they have "niche"
> needs), and normally I would fully agree with you. The problem is that
> by providing an automated, upstream (centralized) opt-out from the
> mutual exclusion, the ecosystem will be allowed to suffer from the same
> nonchalance towards ACPI that has been the case until now. It's clear
> that your well-meaning change will be immediately exploited as the
> new-old default.
>
> Do you plan to add the same loophole to the HII-enabled driver that you
> recently committed as well?
>

No. That is mostly intended for new users (such as the Marvell
platform), though, which is why I only proposed to add it to TC2
(which is 32-bit so it does not use ACPI to begin with) and FVP (which
serves as a reference implementation for us.) Whether Juno can be
ported as well remains to be seen, but I would prefer to make ACPI and
DT mutually exclusive on that platform as well.

>
> Also, please don't accuse me of caring only about RH's downstream. The
> following blog post wasn't authored by a Red Hat employee:
>
> http://www.secretlab.ca/archives/151
>
> The following document is also not Red Hat exclusive:
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/index.html
>
> What you are arguing for is, indirectly, to relicense platform vendors
> to continue ignoring ACPI, while (falsely) labeling their systems SBBR
> conformant. And then any OS vendor that actually cares about SBBR
> conformance (hint: it's not just Red Hat) sees brutal boot splats. That
> is not your intent (obviously), but that is the (seen, experienced)
> effect of providing both DT and ACPI.
>

Call me naive, but I really don't see how this change is going to
bring about all of that.

I understand that RedHat intends to start complaining noisily if ACPI
and DT are both exposed, and that having this wrong behavior in the
bundled VM firmware is undesirable, but that does not mean the world
is going to end for everybody if there is a manual override.

> My Nacked-by is not for the specific technical solution that you
> proposed. The technical solution is fine. The goal is wrong. Which makes
> any technical solution (original or alternative) wrong.
>
> If you want, you can go ahead and commit this patch, adding my Nacked-by
> from above. I won't get into a fight with you over this (or anything
> else), but I want my conviction -- namely, that this is entirely wrong
> -- clearly marked.
>

I'd rather have a solution that we can agree on.

> On the technical side:
>
> - I think a dynamic boolean PCD would be superior, if that is possible
> with HII (= directly nvvar-backed) PCDs -- absence of the variable
> should map to FALSE. I'm unsure though if that were as easy to set from
> the UEFI shell as a UINT8. So stick with the current data type if you
> deem that superior (maybe comment on it in the commit message).
>
> - please include <Library/PcdLib.h> in the C source, to reflect the
> [LibraryClasses] update in the INF.
>

Much appreciated. I will keep this in mind.