Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override

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

On 31 March 2017 at 10:59, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/31/17 11:20, Marc Zyngier wrote:
>> On 30/03/17 09:40, Ard Biesheuvel wrote:
>>> On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 03/29/17 21:10, Ard Biesheuvel wrote:
>>> [...]
>>>>>
>>>>> How on earth is having two ways to disable ACPI rather than one going
>>>>> to cause fragmentation? Unlike v1, this patch does not allow you to
>>>>> expose both DT and ACPI tables at the same time.
>>>>
>>>> Oopsie daisy. You actually updated the commit message too. (I have now
>>>> formally diffed v1 vs. v2, including commit msg -- I generally do that
>>>> when reviewing incremental versions of patches, but it has been a very
>>>> long day, and I failed to get my mind off the track set up by v1). I got
>>>> really no good explanation for missing the fundamental logic change
>>>> between v1 and v2. As you say, version 2 preserves the mutual exclusion
>>>> between DT and ACPI that I'm so annoyingly obsessed about. Thank you for
>>>> the update.
>>>>
>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>
>>> Thanks Laszlo. I am glad we have a solution we can both live with.
>>>
>>> I will wait for Marc to confirm that this works as expected for him.
>>
>> [ This email won't make it on the edk2 list because it filters out
>> non-subscribers. Boo! ]
>
> I fully agree: boo. I've raised this several times in the past, because
> it is rude to people with occasional interest, and diverges sharply from
> the custom on most other open source development lists. But, I had no
> success in changing the policy. I really don't understand what other
> subscribers are worried about. Spam is generally not a problem.
>
>> This patch does a very good job at restoring a functionality I'd have
>> otherwise lost. So for the record:
>>
>> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>> My only comment is that it that the enabling method is slightly cryptic,
>> and suffers from a lack of documentation. For example, it doesn't seem
>> to appear in Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/FV/Guid.xref, which
>> is where I'd have expected to find it.
>
> That happens because BaseTools approaches the Guid.xref file generation
> from the direction of modules -- it goes over all modules and checks
> what plain GUIDs, protocol GUIDs, and PPI GUIDs it uses.
>
> However, in this case, gArmVirtVariableGuid is never directly used in
> PlatformHasAcpiDtDxe -- the driver never directly reads the UEFI
> variable in question, so it doesn't need the GUID for naming the
> variable's namespace. Instead, the connection to UEFI variables is made
> in the "ArmVirtQemu.dsc" platform description file; that's where the
> GUID is also named.
>
> [PcdsDynamicHii]
> gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS
>
> I guess we could file a BaseTools enhancement request to print out the
> variable namespace GUIDs used with dynamic HII PCDs. Ard, what do you think?
>

I guess this is an omission, but I have never used Guid.xref in my
life so I have no strong feelings as to how this should be fixed.