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

[Laszlo Ersek <lersek@redhat.com>]

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?

Thanks
Laszlo