* [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override @ 2017-03-29 17:50 Ard Biesheuvel 2017-03-29 18:44 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2017-03-29 17:50 UTC (permalink / raw) To: edk2-devel, lersek; +Cc: marc.zyngier, mark.rutland, Ard Biesheuvel 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 -- 2.9.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-29 17:50 [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override Ard Biesheuvel @ 2017-03-29 18:44 ` Laszlo Ersek 2017-03-29 19:10 ` Ard Biesheuvel 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2017-03-29 18:44 UTC (permalink / raw) To: Ard Biesheuvel Cc: edk2-devel, marc.zyngier, mark.rutland, Drew Jones, Jon Masters 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. 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. * 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. 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. (We'll revert the patch in RH downstream.) Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-29 18:44 ` Laszlo Ersek @ 2017-03-29 19:10 ` Ard Biesheuvel 2017-03-29 19:35 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2017-03-29 19:10 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Marc Zyngier, Mark Rutland, Drew Jones, Jon Masters 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-29 19:10 ` Ard Biesheuvel @ 2017-03-29 19:35 ` Laszlo Ersek 2017-03-30 8:40 ` Ard Biesheuvel 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2017-03-29 19:35 UTC (permalink / raw) To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, Marc Zyngier, Mark Rutland, Drew Jones, Jon Masters 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-29 19:35 ` Laszlo Ersek @ 2017-03-30 8:40 ` Ard Biesheuvel 2017-03-30 16:16 ` Laszlo Ersek [not found] ` <e3ab9b91-8e0f-52ab-bb3a-53bd0cacf17c@arm.com> 0 siblings, 2 replies; 12+ messages in thread From: Ard Biesheuvel @ 2017-03-30 8:40 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Marc Zyngier, Mark Rutland, Drew Jones, Jon Masters On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/29/17 21:10, Ard Biesheuvel wrote: [...] >> >> 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> > Thanks Laszlo. I am glad we have a solution we can both live with. I will wait for Marc to confirm that this works as expected for him. Thanks, Ard. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-30 8:40 ` Ard Biesheuvel @ 2017-03-30 16:16 ` Laszlo Ersek 2017-03-31 10:48 ` Ard Biesheuvel [not found] ` <e3ab9b91-8e0f-52ab-bb3a-53bd0cacf17c@arm.com> 1 sibling, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2017-03-30 16:16 UTC (permalink / raw) To: Ard Biesheuvel Cc: edk2-devel@lists.01.org, Marc Zyngier, Mark Rutland, Drew Jones, Jon Masters On 03/30/17 10:40, Ard Biesheuvel wrote: > On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote: >> On 03/29/17 21:10, Ard Biesheuvel wrote: > [...] >>> >>> 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> >> > > Thanks Laszlo. I am glad we have a solution we can both live with. > > I will wait for Marc to confirm that this works as expected for him. Good idea. In order to save some adrenaline down the line for both of us, I have some suggestions: - Please try clarify with the reporter of the regression what he or she prefers as a solution, before giving me a heart attack :) Regarding the "NACK" in all caps -- I wasn't yelling, that's just a way of formatting we use downstream (we mostly use ACK and NACK), which regrettably leaked into my upstream correspondence. Sorry about the confusion. (NB, I'm not apologizing for nacking v1 per se. There's a world of difference between exposing the exlusivity with some additional switch and getting cold feet on the exclusivity en bloc. In my opinion.) - Please always include an incremental v2, v3, ... notes / info section in the patch (or blurb, if there is one), so I can more easily find out about the inter-version changes near the end of a 14 hour work day. (When I finally went to bed my uptime was past 18 hours.) In this instance, there was no v2 info section, and I thought you only addressed the superficial technical suggestions that I made under v1. *Importantly*, this is not to say that I did not do a shit job at reviewing v2. I absolutely did. Lack of a v2 info section in the patch / blurb is no excuse for missing the -- happy! -- elefant in the room. It's quite embarrassing; I'm sorry about that. I'll strive to do the formal v1<->v2 diffing in the future unconditionally. Thanks, Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-30 16:16 ` Laszlo Ersek @ 2017-03-31 10:48 ` Ard Biesheuvel 0 siblings, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2017-03-31 10:48 UTC (permalink / raw) To: Laszlo Ersek Cc: edk2-devel@lists.01.org, Marc Zyngier, Mark Rutland, Drew Jones, Jon Masters On 30 March 2017 at 17:16, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/30/17 10:40, Ard Biesheuvel wrote: >> On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 03/29/17 21:10, Ard Biesheuvel wrote: >> [...] >>>> >>>> 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> >>> >> >> Thanks Laszlo. I am glad we have a solution we can both live with. >> >> I will wait for Marc to confirm that this works as expected for him. > > Good idea. > > In order to save some adrenaline down the line for both of us, I have > some suggestions: > > - Please try clarify with the reporter of the regression what he or she > prefers as a solution, before giving me a heart attack :) > > Regarding the "NACK" in all caps -- I wasn't yelling, that's just a way > of formatting we use downstream (we mostly use ACK and NACK), which > regrettably leaked into my upstream correspondence. Sorry about the > confusion. > > (NB, I'm not apologizing for nacking v1 per se. There's a world of > difference between exposing the exlusivity with some additional switch > and getting cold feet on the exclusivity en bloc. In my opinion.) > > - Please always include an incremental v2, v3, ... notes / info section > in the patch (or blurb, if there is one), so I can more easily find out > about the inter-version changes near the end of a 14 hour work day. > (When I finally went to bed my uptime was past 18 hours.) > > In this instance, there was no v2 info section, and I thought you only > addressed the superficial technical suggestions that I made under v1. > > *Importantly*, this is not to say that I did not do a shit job at > reviewing v2. I absolutely did. Lack of a v2 info section in the patch / > blurb is no excuse for missing the -- happy! -- elefant in the room. > It's quite embarrassing; I'm sorry about that. I'll strive to do the > formal v1<->v2 diffing in the future unconditionally. > Yeah good point. Keystrokes are cheap ... ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <e3ab9b91-8e0f-52ab-bb3a-53bd0cacf17c@arm.com>]
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override [not found] ` <e3ab9b91-8e0f-52ab-bb3a-53bd0cacf17c@arm.com> @ 2017-03-31 9:59 ` Laszlo Ersek 2017-03-31 10:10 ` Laszlo Ersek 2017-03-31 10:16 ` Ard Biesheuvel 2017-03-31 10:52 ` Ard Biesheuvel 1 sibling, 2 replies; 12+ messages in thread From: Laszlo Ersek @ 2017-03-31 9:59 UTC (permalink / raw) To: Marc Zyngier, Ard Biesheuvel Cc: edk2-devel@lists.01.org, Mark Rutland, Drew Jones, Jon Masters On 03/31/17 11:20, Marc Zyngier wrote: > On 30/03/17 09:40, Ard Biesheuvel wrote: >> On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 03/29/17 21:10, Ard Biesheuvel wrote: >> [...] >>>> >>>> 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> >>> >> >> Thanks Laszlo. I am glad we have a solution we can both live with. >> >> I will wait for Marc to confirm that this works as expected for him. > > [ This email won't make it on the edk2 list because it filters out > non-subscribers. Boo! ] I fully agree: boo. I've raised this several times in the past, because it is rude to people with occasional interest, and diverges sharply from the custom on most other open source development lists. But, I had no success in changing the policy. I really don't understand what other subscribers are worried about. Spam is generally not a problem. > This patch does a very good job at restoring a functionality I'd have > otherwise lost. So for the record: > > Tested-by: Marc Zyngier <marc.zyngier@arm.com> > Acked-by: Marc Zyngier <marc.zyngier@arm.com> > > My only comment is that it that the enabling method is slightly cryptic, > and suffers from a lack of documentation. For example, it doesn't seem > to appear in Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/FV/Guid.xref, which > is where I'd have expected to find it. That happens because BaseTools approaches the Guid.xref file generation from the direction of modules -- it goes over all modules and checks what plain GUIDs, protocol GUIDs, and PPI GUIDs it uses. However, in this case, gArmVirtVariableGuid is never directly used in PlatformHasAcpiDtDxe -- the driver never directly reads the UEFI variable in question, so it doesn't need the GUID for naming the variable's namespace. Instead, the connection to UEFI variables is made in the "ArmVirtQemu.dsc" platform description file; that's where the GUID is also named. [PcdsDynamicHii] gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS I guess we could file a BaseTools enhancement request to print out the variable namespace GUIDs used with dynamic HII PCDs. Ard, what do you think? Thanks Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-31 9:59 ` Laszlo Ersek @ 2017-03-31 10:10 ` Laszlo Ersek 2017-03-31 10:16 ` Ard Biesheuvel 1 sibling, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2017-03-31 10:10 UTC (permalink / raw) To: Marc Zyngier, Ard Biesheuvel Cc: edk2-devel@lists.01.org, Mark Rutland, Drew Jones, Jon Masters On 03/31/17 11:59, Laszlo Ersek wrote: > On 03/31/17 11:20, Marc Zyngier wrote: >> On 30/03/17 09:40, Ard Biesheuvel wrote: >>> On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote: >>>> On 03/29/17 21:10, Ard Biesheuvel wrote: >>> [...] >>>>> >>>>> 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> >>>> >>> >>> Thanks Laszlo. I am glad we have a solution we can both live with. >>> >>> I will wait for Marc to confirm that this works as expected for him. >> >> [ This email won't make it on the edk2 list because it filters out >> non-subscribers. Boo! ] > > I fully agree: boo. I've raised this several times in the past, because > it is rude to people with occasional interest, and diverges sharply from > the custom on most other open source development lists. But, I had no > success in changing the policy. I really don't understand what other > subscribers are worried about. Spam is generally not a problem. I've just filed <https://bugzilla.tianocore.org/show_bug.cgi?id=451> about this. Thanks Laszlo > >> This patch does a very good job at restoring a functionality I'd have >> otherwise lost. So for the record: >> >> Tested-by: Marc Zyngier <marc.zyngier@arm.com> >> Acked-by: Marc Zyngier <marc.zyngier@arm.com> >> >> My only comment is that it that the enabling method is slightly cryptic, >> and suffers from a lack of documentation. For example, it doesn't seem >> to appear in Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/FV/Guid.xref, which >> is where I'd have expected to find it. > > That happens because BaseTools approaches the Guid.xref file generation > from the direction of modules -- it goes over all modules and checks > what plain GUIDs, protocol GUIDs, and PPI GUIDs it uses. > > However, in this case, gArmVirtVariableGuid is never directly used in > PlatformHasAcpiDtDxe -- the driver never directly reads the UEFI > variable in question, so it doesn't need the GUID for naming the > variable's namespace. Instead, the connection to UEFI variables is made > in the "ArmVirtQemu.dsc" platform description file; that's where the > GUID is also named. > > [PcdsDynamicHii] > gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS > > I guess we could file a BaseTools enhancement request to print out the > variable namespace GUIDs used with dynamic HII PCDs. Ard, what do you think? > > Thanks > Laszlo > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-31 9:59 ` Laszlo Ersek 2017-03-31 10:10 ` Laszlo Ersek @ 2017-03-31 10:16 ` Ard Biesheuvel 2017-03-31 10:46 ` Laszlo Ersek 1 sibling, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2017-03-31 10:16 UTC (permalink / raw) To: Laszlo Ersek Cc: Marc Zyngier, edk2-devel@lists.01.org, Mark Rutland, Drew Jones, Jon Masters On 31 March 2017 at 10:59, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/31/17 11:20, Marc Zyngier wrote: >> On 30/03/17 09:40, Ard Biesheuvel wrote: >>> On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote: >>>> On 03/29/17 21:10, Ard Biesheuvel wrote: >>> [...] >>>>> >>>>> 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> >>>> >>> >>> Thanks Laszlo. I am glad we have a solution we can both live with. >>> >>> I will wait for Marc to confirm that this works as expected for him. >> >> [ This email won't make it on the edk2 list because it filters out >> non-subscribers. Boo! ] > > I fully agree: boo. I've raised this several times in the past, because > it is rude to people with occasional interest, and diverges sharply from > the custom on most other open source development lists. But, I had no > success in changing the policy. I really don't understand what other > subscribers are worried about. Spam is generally not a problem. > >> This patch does a very good job at restoring a functionality I'd have >> otherwise lost. So for the record: >> >> Tested-by: Marc Zyngier <marc.zyngier@arm.com> >> Acked-by: Marc Zyngier <marc.zyngier@arm.com> >> >> My only comment is that it that the enabling method is slightly cryptic, >> and suffers from a lack of documentation. For example, it doesn't seem >> to appear in Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/FV/Guid.xref, which >> is where I'd have expected to find it. > > That happens because BaseTools approaches the Guid.xref file generation > from the direction of modules -- it goes over all modules and checks > what plain GUIDs, protocol GUIDs, and PPI GUIDs it uses. > > However, in this case, gArmVirtVariableGuid is never directly used in > PlatformHasAcpiDtDxe -- the driver never directly reads the UEFI > variable in question, so it doesn't need the GUID for naming the > variable's namespace. Instead, the connection to UEFI variables is made > in the "ArmVirtQemu.dsc" platform description file; that's where the > GUID is also named. > > [PcdsDynamicHii] > gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS > > I guess we could file a BaseTools enhancement request to print out the > variable namespace GUIDs used with dynamic HII PCDs. Ard, what do you think? > I guess this is an omission, but I have never used Guid.xref in my life so I have no strong feelings as to how this should be fixed. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override 2017-03-31 10:16 ` Ard Biesheuvel @ 2017-03-31 10:46 ` Laszlo Ersek 0 siblings, 0 replies; 12+ messages in thread From: Laszlo Ersek @ 2017-03-31 10:46 UTC (permalink / raw) To: Ard Biesheuvel Cc: Marc Zyngier, edk2-devel@lists.01.org, Mark Rutland, Drew Jones, Jon Masters On 03/31/17 12:16, Ard Biesheuvel wrote: > On 31 March 2017 at 10:59, Laszlo Ersek <lersek@redhat.com> wrote: >> On 03/31/17 11:20, Marc Zyngier wrote: >>> On 30/03/17 09:40, Ard Biesheuvel wrote: >>>> On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> On 03/29/17 21:10, Ard Biesheuvel wrote: >>>> [...] >>>>>> >>>>>> 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> >>>>> >>>> >>>> Thanks Laszlo. I am glad we have a solution we can both live with. >>>> >>>> I will wait for Marc to confirm that this works as expected for him. >>> >>> [ This email won't make it on the edk2 list because it filters out >>> non-subscribers. Boo! ] >> >> I fully agree: boo. I've raised this several times in the past, because >> it is rude to people with occasional interest, and diverges sharply from >> the custom on most other open source development lists. But, I had no >> success in changing the policy. I really don't understand what other >> subscribers are worried about. Spam is generally not a problem. >> >>> This patch does a very good job at restoring a functionality I'd have >>> otherwise lost. So for the record: >>> >>> Tested-by: Marc Zyngier <marc.zyngier@arm.com> >>> Acked-by: Marc Zyngier <marc.zyngier@arm.com> >>> >>> My only comment is that it that the enabling method is slightly cryptic, >>> and suffers from a lack of documentation. For example, it doesn't seem >>> to appear in Build/ArmVirtQemu-AARCH64/RELEASE_GCC5/FV/Guid.xref, which >>> is where I'd have expected to find it. >> >> That happens because BaseTools approaches the Guid.xref file generation >> from the direction of modules -- it goes over all modules and checks >> what plain GUIDs, protocol GUIDs, and PPI GUIDs it uses. >> >> However, in this case, gArmVirtVariableGuid is never directly used in >> PlatformHasAcpiDtDxe -- the driver never directly reads the UEFI >> variable in question, so it doesn't need the GUID for naming the >> variable's namespace. Instead, the connection to UEFI variables is made >> in the "ArmVirtQemu.dsc" platform description file; that's where the >> GUID is also named. >> >> [PcdsDynamicHii] >> gArmVirtTokenSpaceGuid.PcdForceNoAcpi|L"ForceNoAcpi"|gArmVirtVariableGuid|0x0|FALSE|NV,BS >> >> I guess we could file a BaseTools enhancement request to print out the >> variable namespace GUIDs used with dynamic HII PCDs. Ard, what do you think? >> > > I guess this is an omission, but I have never used Guid.xref in my > life so I have no strong feelings as to how this should be fixed. > I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=452>; we'll see how far it goes. Thanks Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override [not found] ` <e3ab9b91-8e0f-52ab-bb3a-53bd0cacf17c@arm.com> 2017-03-31 9:59 ` Laszlo Ersek @ 2017-03-31 10:52 ` Ard Biesheuvel 1 sibling, 0 replies; 12+ messages in thread From: Ard Biesheuvel @ 2017-03-31 10:52 UTC (permalink / raw) To: Marc Zyngier Cc: Laszlo Ersek, edk2-devel@lists.01.org, Mark Rutland, Drew Jones, Jon Masters On 31 March 2017 at 10:20, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 30/03/17 09:40, Ard Biesheuvel wrote: >> On 29 March 2017 at 20:35, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 03/29/17 21:10, Ard Biesheuvel wrote: >> [...] >>>> >>>> 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> >>> >> >> Thanks Laszlo. I am glad we have a solution we can both live with. >> >> I will wait for Marc to confirm that this works as expected for him. > > [ This email won't make it on the edk2 list because it filters out > non-subscribers. Boo! ] > > This patch does a very good job at restoring a functionality I'd have > otherwise lost. So for the record: > > Tested-by: Marc Zyngier <marc.zyngier@arm.com> > Acked-by: Marc Zyngier <marc.zyngier@arm.com> > Thanks all. Pushed as 7e5f1b673870, with the commit log expanded to explain how to remove the variable again (as requested by Laszlo) ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-03-31 10:52 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-29 17:50 [PATCH v2] ArmVirtPkg/PlatformHasAcpiDtDxe: allow guest level ACPI disable override Ard Biesheuvel
2017-03-29 18:44 ` Laszlo Ersek
2017-03-29 19:10 ` Ard Biesheuvel
2017-03-29 19:35 ` Laszlo Ersek
2017-03-30 8:40 ` Ard Biesheuvel
2017-03-30 16:16 ` Laszlo Ersek
2017-03-31 10:48 ` Ard Biesheuvel
[not found] ` <e3ab9b91-8e0f-52ab-bb3a-53bd0cacf17c@arm.com>
2017-03-31 9:59 ` Laszlo Ersek
2017-03-31 10:10 ` Laszlo Ersek
2017-03-31 10:16 ` Ard Biesheuvel
2017-03-31 10:46 ` Laszlo Ersek
2017-03-31 10:52 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox