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

[Laszlo Ersek <lersek@redhat.com>]

On 03/29/17 19:30, Ard Biesheuvel wrote:
> On 29 March 2017 at 18:23, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/29/17 19:07, Ard Biesheuvel wrote:
>>> On 29 March 2017 at 18:01, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> On 29/03/17 17:40, Laszlo Ersek wrote:
> [...]
>>>>> 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.
>>>>
>>>> My personal choice would be *not* to expose both tables at the same
>>>> time, but instead to be able to shift the preference from one side or
>>>> the other, similarly to what a menu on a bare metal system would do.
>>>>
>>>
>>> So to clarify, you want something sticky in the firmware settings
>>> rather than having to use the -no-acpi command line argument to QEMU?
>>
>> If you *really* want to control it within the guest, on the guest
>> firmware level, while keeping the exclusive nature, then there are
>> better options for that.
>>
>> Namely, a variation of your HII-exposed driver added in:
>>
>> https://github.com/tianocore/edk2/commit/779cc439e881
>>
>> In this case, PlatformHasAcpiDtDxe should be replaced with a simple HII
>> driver that exposes the same exclusive toggle as an HII checkbox. The
>> HII checkbox would control the installation of EdkiiPlatformHasAcpiGuid
>> vs EdkiiPlatformHasDeviceTreeGuid.
>>
>> Note that I disagree with *combining* a HII toggle with the current
>> fw_cfg-based knob (that is, the -no-acpi switch). That means there are
>> multiple masters for the same decision, which invariably leads to confusion.
>>
>> Therefore any ArmVirtQemu platform build should either offer the fw_cfg
>> swtich (== -no-acpi QEMU command line option), *or* the HII toggle.
>> Never both.
>>
> 
> I don't see why we couldn't have two ways to disable ACPI.

I argue against it because it will confuse users and will lead to bug
reports we'll have to field. I'm sure you would not like two separate
config files, with two differently called options in them, for the same
daemon process. You would flip one setting, and wonder why the daemon
doesn't care.

> 
>> In fact I've fully expected Ard to post such an HII driver down the
>> road, for the (upcoming) "showcase QEMU" virtualization platform (and
>> QEMU  machine type). The target environment of that platform wouldn't be
>> the data center (where you really want to control most things from the
>> host side) -- the goal would be to model physical hardware very closely,
>> even as far as human interaction goes.
>>
>> So, let us figure out what we ultimately want (well, I know what I want,
>> what do you guys want)?
>>
>> - For the current (data center virtualization oriented case), the
>> DT<->ACPI exclusion is controlled by QEMU's -no-acpi flag.
>> - For the "showcase QEMU" case, another HII driver would be necessary.
>> DT and ACPI would remain exclusive, but they would be controlled
>> (visually, interactively) from the guest firmware.
>> - Both of these control methods should never be enabled in the same
>> firmware build, at the same time. If Ard writes the HII driver, we can
>> introduce a build time flag that switches between the two control
>> methods. In no case would it be possible for DT and ACPI to appear together.
>>
> 
> Again, the aim is to recover some of the lost functionality for users
> with special requirements. If the alternative implementation will not
> be made available as widely, and needs to be built from source, it is
> not really an improvement over the current situation.
> 

I understand your point.

Laszlo