From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x232.google.com (mail-it0-x232.google.com [IPv6:2607:f8b0:4001:c0b::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 17B2C2050AB14 for ; Wed, 29 Mar 2017 12:10:23 -0700 (PDT) Received: by mail-it0-x232.google.com with SMTP id 190so62466596itm.0 for ; Wed, 29 Mar 2017 12:10:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=z/9hsmiN/XolRqvlR0sMngp0RZGlerCDdGl/IJt1G7g=; b=L3RVGpD6BXOHXsqcYoycrzGxreTCydRTcduYXcLhR8FZsXwdvGnMc87hF64hWT4vgz fvYKPOe2taJmt0cTYaFhqDuQApefC/BVxdzs2xIDFh8dwiDfdY2Rez0YBpc+cL2V5vYq lyh8wHWsSsbNa1TrthX5h6EcMmAEhSAxP6uAg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=z/9hsmiN/XolRqvlR0sMngp0RZGlerCDdGl/IJt1G7g=; b=EnJgX8MwFJsrW0ZLSRnbqHFghLTPKlUzpyq7zWDnkhcwjrDxWNfwFpK0t59QRST9cF i3BQ3eSBEt2qaTXJOZ/comZf5HRAJQBhdzslydIjGBpuXzkvLTbFrfd9TVbht8TiLNOi GmRjTXPP+18W3MwoQdTVxisVsgMQ/cMG4h6kAjVrawSAUIh/VwZZI3W/6xuFUBlvlao7 DLiwiaE6t6UuTQZLBLyYZQAGPIDPBWES6yVrDqbpUdRztUKW8SFctkU4G/3uSLVnDAQQ 6h4KmAD9Yjgr1K9lNxqoT8slqpmSosHqWyEblTUeYAGrYLD3dDppGRFY4VSpf42VGE23 8HJg== X-Gm-Message-State: AFeK/H1vz0a/3Lms8GKv11H8wrTV4xc3VUVmjPoGQSO2XSGxc40V0tIHxtweGQWbMacqVIdyu4MkyJ/ih7z/TSzA X-Received: by 10.36.23.74 with SMTP id 71mr2755586ith.37.1490814622325; Wed, 29 Mar 2017 12:10:22 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.10.27 with HTTP; Wed, 29 Mar 2017 12:10:21 -0700 (PDT) In-Reply-To: <18095962-76eb-7337-969d-4f6080dff4d7@redhat.com> References: <20170329175039.29635-1-ard.biesheuvel@linaro.org> <18095962-76eb-7337-969d-4f6080dff4d7@redhat.com> From: Ard Biesheuvel Date: Wed, 29 Mar 2017 20:10:21 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Marc Zyngier , Mark Rutland , Drew Jones , Jon Masters Subject: Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Mar 2017 19:10:23 -0000 Content-Type: text/plain; charset=UTF-8 On 29 March 2017 at 19:44, Laszlo Ersek 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 >> --- >> 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 >> #include >> #include >> +#include >> #include >> #include >> >> @@ -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 > > 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.