From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id ED0E92050AB09 for ; Wed, 29 Mar 2017 12:35:48 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5F59661B91; Wed, 29 Mar 2017 19:35:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5F59661B91 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 5F59661B91 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-134.phx2.redhat.com [10.3.116.134]) by smtp.corp.redhat.com (Postfix) with ESMTP id AD24D17AEB; Wed, 29 Mar 2017 19:35:46 +0000 (UTC) To: Ard Biesheuvel References: <20170329175039.29635-1-ard.biesheuvel@linaro.org> <18095962-76eb-7337-969d-4f6080dff4d7@redhat.com> Cc: "edk2-devel@lists.01.org" , Marc Zyngier , Mark Rutland , Drew Jones , Jon Masters From: Laszlo Ersek Message-ID: <41a87740-7634-bfde-d2fb-3767a5c33140@redhat.com> Date: Wed, 29 Mar 2017 21:35:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 29 Mar 2017 19:35:48 +0000 (UTC) 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:35:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/29/17 21:10, Ard Biesheuvel wrote: > 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. 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 Laszlo