On 03/29/17 21:10, Ard Biesheuvel wrote:
> On 29 March 2017 at 19:44, Laszlo Ersek <ler...@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.biesheu...@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 <ler...@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 <ler...@redhat.com>
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
