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

[Laszlo Ersek <lersek@redhat.com>]

On 03/29/17 21:10, Ard Biesheuvel wrote:
> On 29 March 2017 at 19:44, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 03/29/17 19:50, Ard Biesheuvel wrote:
>>> In general, we should not present two separate (and inevitably different)
>>> hardware descriptions to the OS, in the form of ACPI tables and a device
>>> tree blob. For this reason, we recently added the logic to ArmVirtQemu to
>>> only expose the ACPI 2.0 entry point if no DT binary is being passed, and
>>> vice versa.
>>>
>>> However, this is arguably a regression for those who relied on DT
>>> descriptions being available, even if the former behavior can be
>>> restored by passing the -no-acpi switch to QEMU.
>>>
>>> So allow a secret handshake with the UEFI Shell, to set a variable that
>>> will result in ACPI to be disabled on subsequent boots even if -no-acpi
>>> was not passed on the QEMU command line.
>>>
>>>   setvar -nv -bs -guid 50bea1e5-a2c5-46e9-9b3a-59596516b00a ForceNoAcpi =01
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  ArmVirtPkg/ArmVirtPkg.dec                                | 9 +++++++++
>>>  ArmVirtPkg/ArmVirtQemu.dsc                               | 3 +++
>>>  ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.c   | 2 ++
>>>  ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 5 +++++
>>>  4 files changed, 19 insertions(+)
>>>
>>> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
>>> index efe83a383d55..a8603e1b80e5 100644
>>> --- a/ArmVirtPkg/ArmVirtPkg.dec
>>> +++ b/ArmVirtPkg/ArmVirtPkg.dec
>>> @@ -34,6 +34,8 @@ [Guids.common]
>>>    gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>>>    gEarlyPL011BaseAddressGuid       = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
>>>
>>> +  gArmVirtVariableGuid   = { 0x50bea1e5, 0xa2c5, 0x46e9, { 0x9b, 0x3a, 0x59, 0x59, 0x65, 0x16, 0xb0, 0x0a } }
>>> +
>>>  [Protocols]
>>>    gFdtClientProtocolGuid = { 0xE11FACA0, 0x4710, 0x4C8E, { 0xA7, 0xA2, 0x01, 0xBA, 0xA2, 0x59, 0x1B, 0x4C } }
>>>
>>> @@ -58,3 +60,10 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>>>    # EFI_VT_100_GUID.
>>>    #
>>>    gArmVirtTokenSpaceGuid.PcdTerminalTypeGuidBuffer|{0x65, 0x60, 0xA6, 0xDF, 0x19, 0xB4, 0xD3, 0x11, 0x9A, 0x2D, 0x00, 0x90, 0x27, 0x3F, 0xC1, 0x4D}|VOID*|0x00000007
>>> +
>>> +[PcdsDynamic]
>>> +  #
>>> +  # Whether to force disable ACPI, regardless of the fw_cfg settings
>>> +  # exposed by QEMU
>>> +  #
>>> +  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|0x0|BOOLEAN|0x00000003
>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
>>> index 4075b92aa2cb..76a7908105ab 100644
>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc
>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
>>> @@ -210,6 +210,9 @@ [PcdsDynamicDefault.common]
>>>    gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
>>>
>>> +[PcdsDynamicHii]
>>> +  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS
>>> +
>>>  ################################################################################
>>>  #
>>>  # Components Section - list of all EDK II Modules needed by this Platform
>>> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.c b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.c
>>> index 8932dacabec5..da3cee645cfb 100644
>>> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.c
>>> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.c
>>> @@ -17,6 +17,7 @@
>>>  #include <Guid/PlatformHasDeviceTree.h>
>>>  #include <Library/BaseLib.h>
>>>  #include <Library/DebugLib.h>
>>> +#include <Library/PcdLib.h>
>>>  #include <Library/QemuFwCfgLib.h>
>>>  #include <Library/UefiBootServicesTableLib.h>
>>>
>>> @@ -37,6 +38,7 @@ PlatformHasAcpiDt (
>>>    // errors here.
>>>    //
>>>    if (MAX_UINTN == MAX_UINT64 &&
>>> +      !PcdGetBool (PcdForceNoAcpi) &&
>>>        !EFI_ERROR (
>>>           QemuFwCfgFindFile (
>>>             "etc/table-loader",
>>> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>>> index 4466bead57c2..08025f0c3722 100644
>>> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>>> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
>>> @@ -25,6 +25,7 @@ [Sources]
>>>    PlatformHasAcpiDtDxe.c
>>>
>>>  [Packages]
>>> +  ArmVirtPkg/ArmVirtPkg.dec
>>>    EmbeddedPkg/EmbeddedPkg.dec
>>>    MdePkg/MdePkg.dec
>>>    OvmfPkg/OvmfPkg.dec
>>> @@ -32,6 +33,7 @@ [Packages]
>>>  [LibraryClasses]
>>>    BaseLib
>>>    DebugLib
>>> +  PcdLib
>>>    QemuFwCfgLib
>>>    UefiBootServicesTableLib
>>>    UefiDriverEntryPoint
>>> @@ -40,5 +42,8 @@ [Guids]
>>>    gEdkiiPlatformHasAcpiGuid       ## SOMETIMES_PRODUCES ## PROTOCOL
>>>    gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL
>>>
>>> +[Pcd]
>>> +  gArmVirtTokenSpaceGuid.PcdForceNoAcpi
>>> +
>>>  [Depex]
>>>    TRUE
>>>
>>
>> Technically the patch is sound. I continue to disagree with its goal though.
>>
>> Technically, the patch could be improved (towards its wrong goal) by
>> exposing the boolean knob with an HII checkbox, called "disable ACPI
>> regardless of what the QEMU command line says". That would mirror Marc's
>> comments from earlier.
>>
>> For now, I actually agree with you that we shouldn't expose the knob
>> through HII however. Your reason for not doing HII is to mitigate what
>> you perceive as a regression as quickly as possible.
> 
> Not quite. I just think there is no reason to advertise the existence
> of this facility.
> 
>> My reason is that I
>> want to keep this loophole out of public view as much as possible, and
>> the UEFI shell is arguably harder to approach than an HII form.
>>
> 
> Indeed.
> 
>> * Please extend the commit message with the UEFI shell command that
>> closes the loophole again.
>>
>> * Also, please get Marc and Mark to ACK this patch, using their @arm.com
>> email addresses. (I wish I could get Leif to ACK the patch as well, but
>> he's on vacation.)
>>
>> * Finally, please add:
>>
>> Abstained-by: Laszlo Ersek <lersek@redhat.com>
>>
>> before pushing the patch.
>>
> 
> Thanks, I guess.
> 
>> The commit log has to show that ARM people were okay with this, and that
>> my own self was opposed. I generally abhor regressions, but in this case
>> I feel the risk for the ecosystem is too large, so abstaining (in a
>> documented way) is the best I can do for you now. I'll re-state for one
>> last time that IMO this patch will contribute to the fragmentation that
>> we see in the hardware description space.
>>
> 
> 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>

Laszlo