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

[Laszlo Ersek <lersek@redhat.com>]

On 03/31/17 12:16, Ard Biesheuvel wrote:
> 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.
> 

I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=452>; we'll see
how far it goes.

Thanks
Laszlo