| Message ID | 20170329151940.23397-1-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: > 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 > OK, fair enough. How do you propose to handle this regression then? _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 18:02, Ard Biesheuvel wrote: > On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: >> 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 >> > > OK, fair enough. How do you propose to handle this regression then? > I don't. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 29 March 2017 at 17:03, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/29/17 18:02, Ard Biesheuvel wrote: >> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: >>> 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 >>> >> >> OK, fair enough. How do you propose to handle this regression then? >> > > I don't. I think I am entitled to a more constructive attitude from you. I don't care about politics, but I do care about not breaking people's workflows. So if you insist on getting on your high horse and throw capitalized NACKs at me, you could at least *pretend* to care about other users than *your* downstream, and come up with some alternative. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 29 March 2017 at 17:09, Jon Masters <jcm@redhat.com> wrote: > Thanks Laszlo. A quick note from me that regardless of this discussion I will be pushing to ensure the version Red Hat ships makes ACPI the default with it being extremely painful to use DT. It is time the ecosystem got with the program we all agreed upon and there will be no more excuses. > This has *nothing* to do with the ecosystem. This has to do with existing users of ArmVirtQemu (admittedly, a small fraction) that will be forced to compile their UEFI images from source because we made a backwards incompatible change. I am 100% on board with making ACPI and DT mutually exclusive. But I don't believe for a second that 'everybody just exposes ACPI and DT at the same time' if this gets merged. That happens because people are clueless, not because they are deliberately spending time and effort on producing two hardware descriptions. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 18:07, Ard Biesheuvel wrote: > On 29 March 2017 at 17:03, Laszlo Ersek <lersek@redhat.com> wrote: >> On 03/29/17 18:02, Ard Biesheuvel wrote: >>> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: >>>> 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 >>>> >>> >>> OK, fair enough. How do you propose to handle this regression then? >>> >> >> I don't. > > I think I am entitled to a more constructive attitude from you. I > don't care about politics, but I do care about not breaking people's > workflows. So if you insist on getting on your high horse and throw > capitalized NACKs at me, you could at least *pretend* to care about > other users than *your* downstream, and come up with some alternative. Ard, I'm not on my high horse. I thought we were in agreement about this. It was obvious (to me at least) that this policy would be considered a regression by those who wanted both DT and ACPI in the guest. From my side, breaking that was all intentional. I'm not deliberately screwing with users (just because they have "niche" needs), and normally I would fully agree with you. The problem is that by providing an automated, upstream (centralized) opt-out from the mutual exclusion, the ecosystem will be allowed to suffer from the same nonchalance towards ACPI that has been the case until now. It's clear that your well-meaning change will be immediately exploited as the new-old default. Do you plan to add the same loophole to the HII-enabled driver that you recently committed as well? Also, please don't accuse me of caring only about RH's downstream. The following blog post wasn't authored by a Red Hat employee: http://www.secretlab.ca/archives/151 The following document is also not Red Hat exclusive: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/index.html What you are arguing for is, indirectly, to relicense platform vendors to continue ignoring ACPI, while (falsely) labeling their systems SBBR conformant. And then any OS vendor that actually cares about SBBR conformance (hint: it's not just Red Hat) sees brutal boot splats. That is not your intent (obviously), but that is the (seen, experienced) effect of providing both DT and ACPI. My Nacked-by is not for the specific technical solution that you proposed. The technical solution is fine. The goal is wrong. Which makes any technical solution (original or alternative) wrong. If you want, you can go ahead and commit this patch, adding my Nacked-by from above. I won't get into a fight with you over this (or anything else), but I want my conviction -- namely, that this is entirely wrong -- clearly marked. On the technical side: - I think a dynamic boolean PCD would be superior, if that is possible with HII (= directly nvvar-backed) PCDs -- absence of the variable should map to FALSE. I'm unsure though if that were as easy to set from the UEFI shell as a UINT8. So stick with the current data type if you deem that superior (maybe comment on it in the commit message). - please include <Library/PcdLib.h> in the C source, to reflect the [LibraryClasses] update in the INF. Peace Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 18:17, Ard Biesheuvel wrote: > On 29 March 2017 at 17:09, Jon Masters <jcm@redhat.com> wrote: >> Thanks Laszlo. A quick note from me that regardless of this discussion I will be pushing to ensure the version Red Hat ships makes ACPI the default with it being extremely painful to use DT. It is time the ecosystem got with the program we all agreed upon and there will be no more excuses. >> > > This has *nothing* to do with the ecosystem. This has to do with > existing users of ArmVirtQemu (admittedly, a small fraction) that will > be forced to compile their UEFI images from source because we made a > backwards incompatible change. > > I am 100% on board with making ACPI and DT mutually exclusive. But I > don't believe for a second that 'everybody just exposes ACPI and DT at > the same time' if this gets merged. That's where we disagree, 100%. > That happens because people are > clueless, not because they are deliberately spending time and effort > on producing two hardware descriptions. If this were true, then the kernel's preference would have been changed to ACPI aeons ago (assuming both DT and ACPI were present). ACPI is superior to DT (cue again Grant Likely's blog post), yet kernel people resist it. That's not cluelessness. If the kernel's DT camp has any influence on platform vendors (and that's rather more a "given that" than an "if"), when they find out about this loophole, I expect them to actively recommend it as a way to perpetuate the status quo. IMO, with this patch you are eviscerating the work we've been doing in the last few weeks. Well, politics is nasty. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 18:16, Marc Zyngier wrote: > On 29/03/17 17:03, Laszlo Ersek wrote: >> On 03/29/17 18:02, Ard Biesheuvel wrote: >>> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: >>>> 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 >>>> >>> >>> OK, fair enough. How do you propose to handle this regression then? >>> >> >> I don't. > > Well, we then have an issue here. As a user of QEMU+UEFI, I expect my > existing VM images to continue to work (mostly because I'm trying to > track regression in KVM itself). And some are pretty old (circa 4.2). I > upgrade my "firmware" (which is QEMU+UEFI), and my VM doesn't boot > anymore, because someone decided that ACPI was better for my soul. I understand. In my opinion, this can be considered an advanced enough use case that justifies rebuilding the firmware on your side. The same way I rebuild both QEMU and (occasionally) the kernel when I try to track down something related to, or exposed by, the guest firmware. ( Case in point: https://www.mail-archive.com/qemu-devel@nongnu.org/msg440255.html To me it seems like a recent vhost regression in the host kernel, and I'd already be bisecting the kernel if I had hardware in my home where the issue reproduces. But, I digress; a team mate is doing that now. ) > I'm sorry, but I don't think that's the right thing to do. If RH decides > to mandate ACPI VMs, that's RH's call. You can eviscerate DT support > from your build of QEMU and UEFI, and I'm fine with it. Imposing this on > all users with existing setups is just wrong. What is wrong is the *practical* effect of the freedom that ACPI+DT together provide. It undermines SBBR conformance in the wild. If people *can* not care, they *will* not care. And SBBR is not just an RH preference. (Sorry about the over-use of bold -- I'm not excited, just trying to get the emphases over the wire.) > I appreciate you may not care, but I had to say it. I appreciate that you said it! Thanks, Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 29 March 2017 at 18:01, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 29/03/17 17:40, Laszlo Ersek wrote: >> On 03/29/17 18:07, Ard Biesheuvel wrote: >>> On 29 March 2017 at 17:03, Laszlo Ersek <lersek@redhat.com> wrote: >>>> On 03/29/17 18:02, Ard Biesheuvel wrote: >>>>> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: >>>>>> 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(-) >>>>>> > > [snip the policy argumentation, I only care about the technical aspects] > >> On the technical side: >> >> - I think a dynamic boolean PCD would be superior, if that is possible >> with HII (= directly nvvar-backed) PCDs -- absence of the variable >> should map to FALSE. I'm unsure though if that were as easy to set from >> the UEFI shell as a UINT8. So stick with the current data type if you >> deem that superior (maybe comment on it in the commit message). >> >> - please include <Library/PcdLib.h> in the C source, to reflect the >> [LibraryClasses] update in the INF. > > My personal choice would be *not* to expose both tables at the same > time, but instead to be able to shift the preference from one side or > the other, similarly to what a menu on a bare metal system would do. > So to clarify, you want something sticky in the firmware settings rather than having to use the -no-acpi command line argument to QEMU? > Lets call the variable PreferDT (rather than ForceDT), with the > following (exhaustive) behaviour : > > - PreferDT==0 and ACPI+DT present -> ACPI > - PreferDT==0 and ACPI present -> ACPI > - PreferDT==0 and DT present -> DT > - PreferDT==1 and ACPI+DT present -> DT > - PreferDT==1 and ACPI present -> ACPI > - PreferDT==1 and DT present -> DT > DT is always available, so this condenses to > - PreferDT==0 and ACPI+DT present -> ACPI > - PreferDT==1 and ACPI+DT present -> DT unless -no-acpi is set, which gives us > - PreferDT==0 and DT present -> DT > - PreferDT==1 and DT present -> DT > It allows people with existing setups to still have something that works > with minimal effort (still need to set this variable though). > For symmetry, it would make sense to call it ForceNoAcpi then, after the command line param. > Could people agree on something like this? > Works for me. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 19:01, Marc Zyngier wrote: > On 29/03/17 17:40, Laszlo Ersek wrote: >> On 03/29/17 18:07, Ard Biesheuvel wrote: >>> On 29 March 2017 at 17:03, Laszlo Ersek <lersek@redhat.com> wrote: >>>> On 03/29/17 18:02, Ard Biesheuvel wrote: >>>>> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: >>>>>> 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(-) >>>>>> > > [snip the policy argumentation, I only care about the technical aspects] > >> On the technical side: >> >> - I think a dynamic boolean PCD would be superior, if that is possible >> with HII (= directly nvvar-backed) PCDs -- absence of the variable >> should map to FALSE. I'm unsure though if that were as easy to set from >> the UEFI shell as a UINT8. So stick with the current data type if you >> deem that superior (maybe comment on it in the commit message). >> >> - please include <Library/PcdLib.h> in the C source, to reflect the >> [LibraryClasses] update in the INF. > > My personal choice would be *not* to expose both tables at the same > time, but instead to be able to shift the preference from one side or > the other, similarly to what a menu on a bare metal system would do. Umm... Are we in violent agreement? This works already. If you invoke QEMU with the normal command like, like you've always done, you get ACPI only (right now). If you pass the "-no-acpi" switch in addition to your normal command line, you get DT only. This is documented in detail on the following commit: https://github.com/tianocore/edk2/commit/110316a995ed If you pass -no-acpi on your QEMU command line, you can perform the whole guest kernel bisection using purely DT, without ever having to re-launch QEMU. > > Lets call the variable PreferDT (rather than ForceDT), with the > following (exhaustive) behaviour : > > - PreferDT==0 and ACPI+DT present -> ACPI > - PreferDT==0 and ACPI present -> ACPI > - PreferDT==0 and DT present -> DT > - PreferDT==1 and ACPI+DT present -> DT > - PreferDT==1 and ACPI present -> ACPI > - PreferDT==1 and DT present -> DT > > It allows people with existing setups to still have something that works > with minimal effort (still need to set this variable though). > > Could people agree on something like this? It's overly complex. With QEMU (and the current edk2 master state) you already have a single (host-side) master knob for controlling the ACPI vs. DT exposure. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 29 March 2017 at 17:40, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/29/17 18:07, Ard Biesheuvel wrote: >> On 29 March 2017 at 17:03, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 03/29/17 18:02, Ard Biesheuvel wrote: >>>> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: [..] >>>>> NACK >>>>> >>>> >>>> OK, fair enough. How do you propose to handle this regression then? >>>> >>> >>> I don't. >> >> I think I am entitled to a more constructive attitude from you. I >> don't care about politics, but I do care about not breaking people's >> workflows. So if you insist on getting on your high horse and throw >> capitalized NACKs at me, you could at least *pretend* to care about >> other users than *your* downstream, and come up with some alternative. > > Ard, I'm not on my high horse. I thought we were in agreement about > this. It was obvious (to me at least) that this policy would be > considered a regression by those who wanted both DT and ACPI in the > guest. From my side, breaking that was all intentional. > Well, speaking for myself, I did not consider the need of some to have both descriptions available. I take those needs very seriously. > I'm not deliberately screwing with users (just because they have "niche" > needs), and normally I would fully agree with you. The problem is that > by providing an automated, upstream (centralized) opt-out from the > mutual exclusion, the ecosystem will be allowed to suffer from the same > nonchalance towards ACPI that has been the case until now. It's clear > that your well-meaning change will be immediately exploited as the > new-old default. > > Do you plan to add the same loophole to the HII-enabled driver that you > recently committed as well? > No. That is mostly intended for new users (such as the Marvell platform), though, which is why I only proposed to add it to TC2 (which is 32-bit so it does not use ACPI to begin with) and FVP (which serves as a reference implementation for us.) Whether Juno can be ported as well remains to be seen, but I would prefer to make ACPI and DT mutually exclusive on that platform as well. > > Also, please don't accuse me of caring only about RH's downstream. The > following blog post wasn't authored by a Red Hat employee: > > http://www.secretlab.ca/archives/151 > > The following document is also not Red Hat exclusive: > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/index.html > > What you are arguing for is, indirectly, to relicense platform vendors > to continue ignoring ACPI, while (falsely) labeling their systems SBBR > conformant. And then any OS vendor that actually cares about SBBR > conformance (hint: it's not just Red Hat) sees brutal boot splats. That > is not your intent (obviously), but that is the (seen, experienced) > effect of providing both DT and ACPI. > Call me naive, but I really don't see how this change is going to bring about all of that. I understand that RedHat intends to start complaining noisily if ACPI and DT are both exposed, and that having this wrong behavior in the bundled VM firmware is undesirable, but that does not mean the world is going to end for everybody if there is a manual override. > My Nacked-by is not for the specific technical solution that you > proposed. The technical solution is fine. The goal is wrong. Which makes > any technical solution (original or alternative) wrong. > > If you want, you can go ahead and commit this patch, adding my Nacked-by > from above. I won't get into a fight with you over this (or anything > else), but I want my conviction -- namely, that this is entirely wrong > -- clearly marked. > I'd rather have a solution that we can agree on. > On the technical side: > > - I think a dynamic boolean PCD would be superior, if that is possible > with HII (= directly nvvar-backed) PCDs -- absence of the variable > should map to FALSE. I'm unsure though if that were as easy to set from > the UEFI shell as a UINT8. So stick with the current data type if you > deem that superior (maybe comment on it in the commit message). > > - please include <Library/PcdLib.h> in the C source, to reflect the > [LibraryClasses] update in the INF. > Much appreciated. I will keep this in mind. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 19:07, Ard Biesheuvel wrote: > On 29 March 2017 at 18:01, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 29/03/17 17:40, Laszlo Ersek wrote: >>> On 03/29/17 18:07, Ard Biesheuvel wrote: >>>> On 29 March 2017 at 17:03, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> On 03/29/17 18:02, Ard Biesheuvel wrote: >>>>>> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: >>>>>>> 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(-) >>>>>>> >> >> [snip the policy argumentation, I only care about the technical aspects] >> >>> On the technical side: >>> >>> - I think a dynamic boolean PCD would be superior, if that is possible >>> with HII (= directly nvvar-backed) PCDs -- absence of the variable >>> should map to FALSE. I'm unsure though if that were as easy to set from >>> the UEFI shell as a UINT8. So stick with the current data type if you >>> deem that superior (maybe comment on it in the commit message). >>> >>> - please include <Library/PcdLib.h> in the C source, to reflect the >>> [LibraryClasses] update in the INF. >> >> My personal choice would be *not* to expose both tables at the same >> time, but instead to be able to shift the preference from one side or >> the other, similarly to what a menu on a bare metal system would do. >> > > So to clarify, you want something sticky in the firmware settings > rather than having to use the -no-acpi command line argument to QEMU? If you *really* want to control it within the guest, on the guest firmware level, while keeping the exclusive nature, then there are better options for that. Namely, a variation of your HII-exposed driver added in: https://github.com/tianocore/edk2/commit/779cc439e881 In this case, PlatformHasAcpiDtDxe should be replaced with a simple HII driver that exposes the same exclusive toggle as an HII checkbox. The HII checkbox would control the installation of EdkiiPlatformHasAcpiGuid vs EdkiiPlatformHasDeviceTreeGuid. Note that I disagree with *combining* a HII toggle with the current fw_cfg-based knob (that is, the -no-acpi switch). That means there are multiple masters for the same decision, which invariably leads to confusion. Therefore any ArmVirtQemu platform build should either offer the fw_cfg swtich (== -no-acpi QEMU command line option), *or* the HII toggle. Never both. In fact I've fully expected Ard to post such an HII driver down the road, for the (upcoming) "showcase QEMU" virtualization platform (and QEMU machine type). The target environment of that platform wouldn't be the data center (where you really want to control most things from the host side) -- the goal would be to model physical hardware very closely, even as far as human interaction goes. So, let us figure out what we ultimately want (well, I know what I want, what do you guys want)? - For the current (data center virtualization oriented case), the DT<->ACPI exclusion is controlled by QEMU's -no-acpi flag. - For the "showcase QEMU" case, another HII driver would be necessary. DT and ACPI would remain exclusive, but they would be controlled (visually, interactively) from the guest firmware. - Both of these control methods should never be enabled in the same firmware build, at the same time. If Ard writes the HII driver, we can introduce a build time flag that switches between the two control methods. In no case would it be possible for DT and ACPI to appear together. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 29 March 2017 at 18:23, Laszlo Ersek <lersek@redhat.com> wrote: > On 03/29/17 19:07, Ard Biesheuvel wrote: >> On 29 March 2017 at 18:01, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> On 29/03/17 17:40, Laszlo Ersek wrote: [...] >>>> On the technical side: >>>> >>>> - I think a dynamic boolean PCD would be superior, if that is possible >>>> with HII (= directly nvvar-backed) PCDs -- absence of the variable >>>> should map to FALSE. I'm unsure though if that were as easy to set from >>>> the UEFI shell as a UINT8. So stick with the current data type if you >>>> deem that superior (maybe comment on it in the commit message). >>>> >>>> - please include <Library/PcdLib.h> in the C source, to reflect the >>>> [LibraryClasses] update in the INF. >>> >>> My personal choice would be *not* to expose both tables at the same >>> time, but instead to be able to shift the preference from one side or >>> the other, similarly to what a menu on a bare metal system would do. >>> >> >> So to clarify, you want something sticky in the firmware settings >> rather than having to use the -no-acpi command line argument to QEMU? > > If you *really* want to control it within the guest, on the guest > firmware level, while keeping the exclusive nature, then there are > better options for that. > > Namely, a variation of your HII-exposed driver added in: > > https://github.com/tianocore/edk2/commit/779cc439e881 > > In this case, PlatformHasAcpiDtDxe should be replaced with a simple HII > driver that exposes the same exclusive toggle as an HII checkbox. The > HII checkbox would control the installation of EdkiiPlatformHasAcpiGuid > vs EdkiiPlatformHasDeviceTreeGuid. > > Note that I disagree with *combining* a HII toggle with the current > fw_cfg-based knob (that is, the -no-acpi switch). That means there are > multiple masters for the same decision, which invariably leads to confusion. > > Therefore any ArmVirtQemu platform build should either offer the fw_cfg > swtich (== -no-acpi QEMU command line option), *or* the HII toggle. > Never both. > I don't see why we couldn't have two ways to disable ACPI. > In fact I've fully expected Ard to post such an HII driver down the > road, for the (upcoming) "showcase QEMU" virtualization platform (and > QEMU machine type). The target environment of that platform wouldn't be > the data center (where you really want to control most things from the > host side) -- the goal would be to model physical hardware very closely, > even as far as human interaction goes. > > So, let us figure out what we ultimately want (well, I know what I want, > what do you guys want)? > > - For the current (data center virtualization oriented case), the > DT<->ACPI exclusion is controlled by QEMU's -no-acpi flag. > - For the "showcase QEMU" case, another HII driver would be necessary. > DT and ACPI would remain exclusive, but they would be controlled > (visually, interactively) from the guest firmware. > - Both of these control methods should never be enabled in the same > firmware build, at the same time. If Ard writes the HII driver, we can > introduce a build time flag that switches between the two control > methods. In no case would it be possible for DT and ACPI to appear together. > Again, the aim is to recover some of the lost functionality for users with special requirements. If the alternative implementation will not be made available as widely, and needs to be built from source, it is not really an improvement over the current situation. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Hi, On Wed, Mar 29, 2017 at 06:55:26PM +0200, Laszlo Ersek wrote: > On 03/29/17 18:17, Ard Biesheuvel wrote: > > On 29 March 2017 at 17:09, Jon Masters <jcm@redhat.com> wrote: > >> Thanks Laszlo. A quick note from me that regardless of this > >> discussion I will be pushing to ensure the version Red Hat ships > >> makes ACPI the default with it being extremely painful to use DT. > >> It is time the ecosystem got with the program we all agreed upon > >> and there will be no more excuses. > > > > This has *nothing* to do with the ecosystem. This has to do with > > existing users of ArmVirtQemu (admittedly, a small fraction) that will > > be forced to compile their UEFI images from source because we made a > > backwards incompatible change. > > > > I am 100% on board with making ACPI and DT mutually exclusive. But I > > don't believe for a second that 'everybody just exposes ACPI and DT at > > the same time' if this gets merged. > > That's where we disagree, 100%. > > > That happens because people are clueless, not because they are > > deliberately spending time and effort on producing two hardware > > descriptions. > > If this were true, then the kernel's preference would have been changed > to ACPI aeons ago (assuming both DT and ACPI were present). As has been covered elsewhere, unilaterally changing the default breaks existing systems, regardless of what vendors do from now on. > ACPI is superior to DT (cue again Grant Likely's blog post), yet > kernel people resist it. In several respects, sure, though let's not be under the illusion that it is not free of novel technical problems. The big deal with ACPI is that there are other major OS vendors who will support ACPI, but not DT. To avoid a fragmented market, commodity servers must use ACPI. > That's not cluelessness. If the kernel's DT camp has any > influence on platform vendors (and that's rather more a "given that" > than an "if"), when they find out about this loophole, I expect them to > actively recommend it as a way to perpetuate the status quo. Who is this "DT camp"? As a DT binding maintainer, I would be livid were people recommending using this in generic OS images. Thanks, Mark. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 19:30, Ard Biesheuvel wrote: > On 29 March 2017 at 18:23, Laszlo Ersek <lersek@redhat.com> wrote: >> On 03/29/17 19:07, Ard Biesheuvel wrote: >>> On 29 March 2017 at 18:01, Marc Zyngier <marc.zyngier@arm.com> wrote: >>>> On 29/03/17 17:40, Laszlo Ersek wrote: > [...] >>>>> On the technical side: >>>>> >>>>> - I think a dynamic boolean PCD would be superior, if that is possible >>>>> with HII (= directly nvvar-backed) PCDs -- absence of the variable >>>>> should map to FALSE. I'm unsure though if that were as easy to set from >>>>> the UEFI shell as a UINT8. So stick with the current data type if you >>>>> deem that superior (maybe comment on it in the commit message). >>>>> >>>>> - please include <Library/PcdLib.h> in the C source, to reflect the >>>>> [LibraryClasses] update in the INF. >>>> >>>> My personal choice would be *not* to expose both tables at the same >>>> time, but instead to be able to shift the preference from one side or >>>> the other, similarly to what a menu on a bare metal system would do. >>>> >>> >>> So to clarify, you want something sticky in the firmware settings >>> rather than having to use the -no-acpi command line argument to QEMU? >> >> If you *really* want to control it within the guest, on the guest >> firmware level, while keeping the exclusive nature, then there are >> better options for that. >> >> Namely, a variation of your HII-exposed driver added in: >> >> https://github.com/tianocore/edk2/commit/779cc439e881 >> >> In this case, PlatformHasAcpiDtDxe should be replaced with a simple HII >> driver that exposes the same exclusive toggle as an HII checkbox. The >> HII checkbox would control the installation of EdkiiPlatformHasAcpiGuid >> vs EdkiiPlatformHasDeviceTreeGuid. >> >> Note that I disagree with *combining* a HII toggle with the current >> fw_cfg-based knob (that is, the -no-acpi switch). That means there are >> multiple masters for the same decision, which invariably leads to confusion. >> >> Therefore any ArmVirtQemu platform build should either offer the fw_cfg >> swtich (== -no-acpi QEMU command line option), *or* the HII toggle. >> Never both. >> > > I don't see why we couldn't have two ways to disable ACPI. I argue against it because it will confuse users and will lead to bug reports we'll have to field. I'm sure you would not like two separate config files, with two differently called options in them, for the same daemon process. You would flip one setting, and wonder why the daemon doesn't care. > >> In fact I've fully expected Ard to post such an HII driver down the >> road, for the (upcoming) "showcase QEMU" virtualization platform (and >> QEMU machine type). The target environment of that platform wouldn't be >> the data center (where you really want to control most things from the >> host side) -- the goal would be to model physical hardware very closely, >> even as far as human interaction goes. >> >> So, let us figure out what we ultimately want (well, I know what I want, >> what do you guys want)? >> >> - For the current (data center virtualization oriented case), the >> DT<->ACPI exclusion is controlled by QEMU's -no-acpi flag. >> - For the "showcase QEMU" case, another HII driver would be necessary. >> DT and ACPI would remain exclusive, but they would be controlled >> (visually, interactively) from the guest firmware. >> - Both of these control methods should never be enabled in the same >> firmware build, at the same time. If Ard writes the HII driver, we can >> introduce a build time flag that switches between the two control >> methods. In no case would it be possible for DT and ACPI to appear together. >> > > Again, the aim is to recover some of the lost functionality for users > with special requirements. If the alternative implementation will not > be made available as widely, and needs to be built from source, it is > not really an improvement over the current situation. > I understand your point. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 19:44, Mark Rutland wrote: > Hi, > > On Wed, Mar 29, 2017 at 06:55:26PM +0200, Laszlo Ersek wrote: >> On 03/29/17 18:17, Ard Biesheuvel wrote: >>> On 29 March 2017 at 17:09, Jon Masters <jcm@redhat.com> wrote: >>>> Thanks Laszlo. A quick note from me that regardless of this >>>> discussion I will be pushing to ensure the version Red Hat ships >>>> makes ACPI the default with it being extremely painful to use DT. >>>> It is time the ecosystem got with the program we all agreed upon >>>> and there will be no more excuses. >>> >>> This has *nothing* to do with the ecosystem. This has to do with >>> existing users of ArmVirtQemu (admittedly, a small fraction) that will >>> be forced to compile their UEFI images from source because we made a >>> backwards incompatible change. >>> >>> I am 100% on board with making ACPI and DT mutually exclusive. But I >>> don't believe for a second that 'everybody just exposes ACPI and DT at >>> the same time' if this gets merged. >> >> That's where we disagree, 100%. >> >>> That happens because people are clueless, not because they are >>> deliberately spending time and effort on producing two hardware >>> descriptions. >> >> If this were true, then the kernel's preference would have been changed >> to ACPI aeons ago (assuming both DT and ACPI were present). > > As has been covered elsewhere, unilaterally changing the default breaks > existing systems, regardless of what vendors do from now on. > >> ACPI is superior to DT (cue again Grant Likely's blog post), yet >> kernel people resist it. > > In several respects, sure, though let's not be under the illusion that > it is not free of novel technical problems. > > The big deal with ACPI is that there are other major OS vendors who will > support ACPI, but not DT. To avoid a fragmented market, commodity > servers must use ACPI. > >> That's not cluelessness. If the kernel's DT camp has any >> influence on platform vendors (and that's rather more a "given that" >> than an "if"), when they find out about this loophole, I expect them to >> actively recommend it as a way to perpetuate the status quo. > > Who is this "DT camp"? I assume the people who have thus far prevented the preference switch-over from DT to ACPI, provided a system exposed both. I realize (as you say above) that this might render some systems temporarily unbootable. Yes. But without that pain, no platform vendor will *ever* be incentivized to get their ACPI tables in order. So the idea here is to force that pain from the firmware side. On one hand, it is a regression, yes. On the other hand, if it's not broken (intentionally), why would anyone ever fix it? (This argument is based on the fact that the shippers of broken ACPI tables actually label their machines SBBR-conformant. Nominally, they *want* to support ACPI.) > As a DT binding maintainer, I would be livid were people recommending > using this in generic OS images. The kernel still prefers DT over ACPI if both are present; that's just the same mindset. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 03/29/17 19:30, Marc Zyngier wrote: > On 29/03/17 18:15, Laszlo Ersek wrote: >> On 03/29/17 19:01, Marc Zyngier wrote: >>> On 29/03/17 17:40, Laszlo Ersek wrote: >>>> On 03/29/17 18:07, Ard Biesheuvel wrote: >>>>> On 29 March 2017 at 17:03, Laszlo Ersek <lersek@redhat.com> wrote: >>>>>> On 03/29/17 18:02, Ard Biesheuvel wrote: >>>>>>> On 29 March 2017 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: >>>>>>>> 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(-) >>>>>>>> >>> >>> [snip the policy argumentation, I only care about the technical aspects] >>> >>>> On the technical side: >>>> >>>> - I think a dynamic boolean PCD would be superior, if that is possible >>>> with HII (= directly nvvar-backed) PCDs -- absence of the variable >>>> should map to FALSE. I'm unsure though if that were as easy to set from >>>> the UEFI shell as a UINT8. So stick with the current data type if you >>>> deem that superior (maybe comment on it in the commit message). >>>> >>>> - please include <Library/PcdLib.h> in the C source, to reflect the >>>> [LibraryClasses] update in the INF. >>> >>> My personal choice would be *not* to expose both tables at the same >>> time, but instead to be able to shift the preference from one side or >>> the other, similarly to what a menu on a bare metal system would do. >> >> Umm... Are we in violent agreement? This works already. >> >> If you invoke QEMU with the normal command like, like you've always >> done, you get ACPI only (right now). >> >> If you pass the "-no-acpi" switch in addition to your normal command >> line, you get DT only. This is documented in detail on the following commit: >> >> https://github.com/tianocore/edk2/commit/110316a995ed >> >> If you pass -no-acpi on your QEMU command line, you can perform the >> whole guest kernel bisection using purely DT, without ever having to >> re-launch QEMU. >> >>> >>> Lets call the variable PreferDT (rather than ForceDT), with the >>> following (exhaustive) behaviour : >>> >>> - PreferDT==0 and ACPI+DT present -> ACPI >>> - PreferDT==0 and ACPI present -> ACPI >>> - PreferDT==0 and DT present -> DT >>> - PreferDT==1 and ACPI+DT present -> DT >>> - PreferDT==1 and ACPI present -> ACPI >>> - PreferDT==1 and DT present -> DT >>> >>> It allows people with existing setups to still have something that works >>> with minimal effort (still need to set this variable though). >>> >>> Could people agree on something like this? >> >> It's overly complex. With QEMU (and the current edk2 master state) you >> already have a single (host-side) master knob for controlling the ACPI >> vs. DT exposure. > > You're missing the essential bit. I don't want a knob in QEMU. Having to > mess with QEMU means that I can't have a uniform way of running a VM. I > want one way of booting a VM, and let the VM pick the description it has > been configured for. > > On bare metal, I can switch UEFI from a menu entry to pick ACPI or DT. > And that's good. I want the same freedom in a VM, at the same level. > There is no technical reason to have a different behaviour. Correct, you can have the exact same menu entry (which is what I've been calling the HII checkbox) in virtual firmware, assuming someone writes the platform DXE driver that implements this kind of policy. As I wrote, I expected Ard to submit (at some undeterminate time in the future) such a driver, which would replace the current policy drivers in: - the Xen build of ArmVirtQemu, which has no access to fw_cfg, and - the upcoming "showcase QEMU" platform, which would entirely ignore fw_cfg for this purpose. Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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
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(-) -- 2.9.3 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel