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

[Laszlo Ersek <lersek@redhat.com>]

On 03/29/17 17:19, 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 rely on both
> descriptions being available, even if the use cases in question are
> uncommon.
> 
> So allow a secret handshake with the UEFI Shell, to set a variable that
> will result in both descriptions being exposed on the next boot, if
> executing in the default 'ACPI-only' mode.
> 
>   setvar -nv -bs -guid 50bea1e5-a2c5-46e9-9b3a-59596516b00a ForceDt =01
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtPkg.dec                                | 8 ++++++++
>  ArmVirtPkg/ArmVirtQemu.dsc                               | 3 +++
>  ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.c   | 7 ++++++-
>  ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 5 +++++
>  4 files changed, 22 insertions(+), 1 deletion(-)

Nacked-by: Laszlo Ersek <lersek@redhat.com>

This will cause everyone *at all* to set the secret handshake, and we
will be back to square one, where everyone just exposes ACPI and DT at
the same time, and delegate the decision to the guest kernel.

And then vendors will continue to ignore ACPI testing, and they will
continue shipping crap in their ACPI tables.

We might as well rip out the recent patches that implement the mechanism
and the policy for the mutual exclusion.

As Leif proved so eloquently (in the pub) in Budapest during Connect, no
OS needs both descriptions at the same time. Virt users can make up
their minds about what to expose. We (RH virt) had been worriedly
planning to make the same proposal to Leif, you, et al, and then we were
happy to see the violent agreement that ensued.

Sorry for getting political, but the kernel's unwavering preference for
DT over ACPI is political, and the recent edk2 patches only exist to
rectify that, from the firmware side. Users don't lose DT. What they do
lose is the (harmful) freedom of not choosing one over the other. That
freedom has had a terrible effect on the quality of ACPI tables shipped
with *allegedly* SBBR-compliant hardware.

Feel free to diverge from this in downstream distributions, but this
loophole has no place in upstream edk2.

NACK

Thanks
Laszlo

> 
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index efe83a383d55..ae5473a3f3f3 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,9 @@ [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 the DT to be exposed to the OS, even in the presence of
> +  # ACPI tables.
> +  gArmVirtTokenSpaceGuid.PcdAcpiDtForceDt|0x0|UINT8|0x00000003
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 4075b92aa2cb..bbb517e40242 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -210,6 +210,9 @@ [PcdsDynamicDefault.common]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosDocRev|0x0
>    gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
>  
> +[PcdsDynamicHii]
> +  gArmVirtTokenSpaceGuid.PcdAcpiDtForceDt|L"ForceDt"|gArmVirtVariableGuid|0x0|0|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..4c53825d54ad 100644
> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.c
> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.c
> @@ -58,7 +58,12 @@ PlatformHasAcpiDt (
>        goto Failed;
>      }
>  
> -    return Status;
> +    if (!PcdGet8 (PcdAcpiDtForceDt)) {
> +      return EFI_SUCCESS;
> +    }
> +    DEBUG ((DEBUG_WARN,
> +      "%a: ForceDt is set: installing both ACPI and DT tables\n",
> +      __FUNCTION__));
>    }
>  
>    //
> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf
> index 4466bead57c2..5bc9ea43c05b 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.PcdAcpiDtForceDt
> +
>  [Depex]
>    TRUE
>