Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33652
Change subject: arch/x86/acpi: Add os.asl ......................................................................
arch/x86/acpi: Add os.asl
As time goes by, newer OS come into the market, requiring modification on ACPI code to identify the new OS. With this new file, a single file will need maintenance in the future, and it will fix the code for every board including this file.
It assumes OSVR is already declared elsewhere within the ACPI code.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I87200cafdafcf078db16c95463a86c3e9105cea6 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- A src/arch/x86/acpi/os.asl 1 file changed, 135 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/33652/1
diff --git a/src/arch/x86/acpi/os.asl b/src/arch/x86/acpi/os.asl new file mode 100644 index 0000000..17fe29e --- /dev/null +++ b/src/arch/x86/acpi/os.asl @@ -0,0 +1,135 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Silverback, Ltd + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +/* Assumes OSVR is declared elsewhere */ +Method(OSFL, 0, NotSerialized) +{ + If(LNotEqual(OSVR, Ones)) + { + Return(OSVR) + } + Store(0x03, OSVR) + If(CondRefOf(_OSI, Local0)) + { + If(_OSI("Windows 2001")) + { + Store(0x04, OSVR) + } + If(_OSI("Windows 2001.1")) + { + Store(0x05, OSVR) + } + If(_OSI("FreeBSD")) + { + Store(0x06, OSVR) + } + If(_OSI("HP-UX")) + { + Store(0x07, OSVR) + } + If(_OSI("OpenVMS")) + { + Store(0x08, OSVR) + } + If(_OSI("Windows 2001 SP1")) + { + Store(0x09, OSVR) + } + If(_OSI("Windows 2001 SP2")) + { + Store(0x0A, OSVR) + } + If(_OSI("Windows 2001 SP3")) + { + Store(0x0B, OSVR) + } + If(_OSI("Windows 2006")) + { + Store(0x0C, OSVR) + } + If(_OSI("Windows 2006 SP1")) + { + Store(0x0D, OSVR) + } + If(_OSI("Windows 2009")) + { + Store(0x0E, OSVR) + } + If(_OSI("Windows 2012")) + { + Store(0x0F, OSVR) + } + If(_OSI("Windows 2013")) + { + Store(0x10, OSVR) + } + } + Else + { + Store (_OS, local0) + If(SCPM(local0, "Microsoft Windows NT")) + { + Store(Zero, OSVR) + } + If(SCPM(local0, "Microsoft Windows")) + { + Store(One, OSVR) + } + If(SCPM(local0, "Microsoft WindowsME: Millennium Edition")) + { + Store(0x02, OSVR) + } + If(SCPM(local0, "Linux")) + { + Store(0x03, OSVR) + } + If(SCPM(local0, "FreeBSD")) + { + Store(0x06, OSVR) + } + If(SCPM(local0, "HP-UX")) + { + Store(0x07, OSVR) + } + If(SCPM(local0, "OpenVMS")) + { + Store(0x08, OSVR) + } + } + Return(OSVR) +} + +/* String compare */ +Method(SCPM, 2, Serialized) +{ + If(LLess(SizeOf(Arg0), SizeOf(Arg1))) + { + Return(Zero) + } + Add(SizeOf(Arg0), One, Local0) + Name(BUF0, Buffer(Local0){}) + Name(BUF1, Buffer(Local0){}) + Store(Arg0, BUF0) + Store(Arg1, BUF1) + While(Local0) + { + Decrement(Local0) + If(LNotEqual(DerefOf(Index(BUF0, Local0)), DerefOf(Index(BUF1, Local0)))) + { + Return(Zero) + } + } + Return(One) +}
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1: Code-Review-1
(4 comments)
I don't like this. Is there a better approach?
NB this -1 is not blocking the change from being merged.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl File src/arch/x86/acpi/os.asl:
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@84 PS1, Line 84: Zero Why not 0x00 instead?
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@90 PS1, Line 90: If(SCPM(local0, "Microsoft WindowsME: Millennium Edition")) Do we need to care?
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@94 PS1, Line 94: If(SCPM(local0, "Linux")) I would not recommend that, see: https://www.kernel.org/doc/Documentation/acpi/osi.txt
ACPI: [Firmware Bug]: BIOS _OSI(Linux) query ignored
Linux considers _OSI queries for Linux to be a firmware bug, and since this does the same query, I would call it a bug as well.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@115 PS1, Line 115: SCPM Is this method specific to this file, or could it be reused?
Also, unless there's a good reason to use 'SCPM', I would say 'SCMP' reflects "String CoMPare" better.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1:
(4 comments)
Patch Set 1: Code-Review-1
(4 comments)
I don't like this. Is there a better approach?
NB this -1 is not blocking the change from being merged.
This was originally part of stoneyridge ACPI code, Paul Manzel asked to have it available for everyone.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl File src/arch/x86/acpi/os.asl:
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@84 PS1, Line 84: Zero
Why not 0x00 instead?
Just because I kind of copied the code from another BIOS (UEFI) using RWeverithing, with some modifications so it won't be called an actual copy. Yes, I could probably use 0x00 and 0x01 (bellow).
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@90 PS1, Line 90: If(SCPM(local0, "Microsoft WindowsME: Millennium Edition"))
Do we need to care?
Probably not. Same for Windows NT. I just am not sure if anyone still uses either OS. Even WINXP is no longer supported, though it was still used recently (not sure if still used) on the embedded world.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@94 PS1, Line 94: If(SCPM(local0, "Linux"))
I would not recommend that, see: https://www.kernel.org/doc/Documentation/acpi/osi.txt […]
The work around is in line 24, which verifies if _OSI is implemented. Therefor, _OSI will only be used if implemented.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@115 PS1, Line 115: SCPM
Is this method specific to this file, or could it be reused? […]
My bad, agree. Will change.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1:
(2 comments)
Patch Set 1:
(4 comments)
Patch Set 1: Code-Review-1
(4 comments)
I don't like this. Is there a better approach?
NB this -1 is not blocking the change from being merged.
This was originally part of stoneyridge ACPI code, Paul Manzel asked to have it available for everyone.
Ack
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl File src/arch/x86/acpi/os.asl:
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@90 PS1, Line 90: If(SCPM(local0, "Microsoft WindowsME: Millennium Edition"))
Probably not. Same for Windows NT. I just am not sure if anyone still uses either OS. […]
I believe the "Windows NT" ACPI name is still in use. I'd be more worried about HP-UX and OpenVMS.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@94 PS1, Line 94: If(SCPM(local0, "Linux"))
The work around is in line 24, which verifies if _OSI is implemented. […]
What I mean is that I would leave out all queries for "Linux". Linux documentation suggests only using _OSI to query for specific capabilities, and that would be device-specific.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl File src/arch/x86/acpi/os.asl:
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@90 PS1, Line 90: If(SCPM(local0, "Microsoft WindowsME: Millennium Edition"))
I believe the "Windows NT" ACPI name is still in use. I'd be more worried about HP-UX and OpenVMS.
Ok, easy to remove, as coreboot will probably never be used with these OS.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@94 PS1, Line 94: If(SCPM(local0, "Linux"))
What I mean is that I would leave out all queries for "Linux". […]
So how do I identify Linux? From the documentation you provided, Linux answers ""Microsoft Windows NT" to _OS, and answers true to almost all _OSI"something". I understand that the true use should be what interface you support, which is specially important between ACPI 1.0 and 2.0, though WIN XP only partially supported ACPI 2.0. I once had a problem because I needed to support several windows version, including XP... and WIN XP interpreted one particular command (don't remember exactly which) the exact opposite way (ACPI 1.0) of all other windows versions after WIN XP.
Really the only reason for OSFL is to identify what is supported, though as document says, "tradition" has it identifying OS instead of ACPI capabilities, and the engineer had to know exactly what was supported for each OS version.
So "tradition" would make this code work with all Windows versions (Microsoft made sure of it once this tradition started.), but within Linux you would think you are running whatever OS you first asked (thus making sense to ask _OSI(Linux) as the very first query as only Linux would answer true to it... but you'd still not know what version of Linux and thus what is supported.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl File src/arch/x86/acpi/os.asl:
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@77 PS1, Line 77: } missing _OSI Strings for
(OSI String Target OS) Windows 2013 Windows 8.1 Windows 2015 Windows 10 Windows 2016 Windows 10, version 1607 Windows 2017 Windows 10, version 1703 Windows 2017.2 Windows 10, version 1709 Windows 2018 Windows 10, version 1803 Windows 2018.2 Windows 10, version 1809
( https://docs.microsoft.com/en-us/windows-hardware/drivers/acpi/winacpi-osi )
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@84 PS1, Line 84: Zero
Just because I kind of copied the code from another BIOS (UEFI) using RWeverithing, with some modifi […]
if I'm not wrong, "Zero" is also ok
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl File src/arch/x86/acpi/os.asl:
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@84 PS1, Line 84: Zero
if I'm not wrong, "Zero" is also ok
Yes, Zero and One are the same as 0x00 and 0x01, but the latter are more consistent with the rest of the values. I seem to have some sort of OCD with consistency.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@94 PS1, Line 94: If(SCPM(local0, "Linux"))
So how do I identify Linux? From the documentation you provided, Linux answers ""Microsoft Windows N […]
Right. AFAIUI, the problem with _OSI(Linux) is that linux does not maintain an interface, so probing just for "linux" can match anything, even custom kernels. So, as to how to identify linux, I would say the answer is to not do so. If you still need to treat Linux differently, there's probably a bug in Linux which should be patched.
Oh, even ACPICA has a section about that: Section 8.1.7.2 of https://acpica.org/sites/acpica/files/acpica-reference_18.pdf
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1:
Isn't OS specific ACPI code discouraged? I mean I only know this from historians, but didn't we have this in the beginning of ACPI and it turned out very bad?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1:
Patch Set 1:
Isn't OS specific ACPI code discouraged? I mean I only know this from historians, but didn't we have this in the beginning of ACPI and it turned out very bad?
Yes, hence my negative review. My understanding of the matter is that testing for OS in ACPI is done to implement OS-specific workarounds.
Windows' implementation of the ACPI spec is rather... "weird". I don't know much about it, but I think Windows has a stable interface within versions, so that's why people writing ACPI code check for Windows versions. That does not necessarily mean these people are right, but who am I to judge? (I don't know about Windows internals)
OTOH, Linux does not have a stable interface. Let's suppose there's a check for Linux in some ACPI code to disable a specific PNP device that makes Linux crash. Let's also suppose Linux 3.4 fixes the issue. Checking for Linux matches *any* Linux, and a 2.6.11.1 kernel can behave very differently compared to a 5.2-rc5 kernel. Any newer kernel with the fix would still have that device disabled. And even if the check in ACPI was modified to check the Linux version, there are many distributions which use kernels with backported patches, so it would still fail on those.
And the above example assumes the firmware vendor actually tests stuff and issues updates, which unfortunately is not common. Linux documentation suggests using a _OSI("Linux-OEM-my_interface_name") query, to which the kernel will answer YES to when it has a fix for that bug. That also means the firmware does not need to be changed.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl File src/arch/x86/acpi/os.asl:
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@77 PS1, Line 77: }
missing _OSI Strings for […]
I'm starting to get with Angel Pons and I might either change this to be used only with platforms that will use some version of windows (not sure if anyone uses coreboot with windows) or completely abandon it. If I go the first option, I'll take your advice into consideration. Anyway, I have to change 33620 to NOT USE this patch before I do anything else with this patch.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@84 PS1, Line 84: Zero
Yes, Zero and One are the same as 0x00 and 0x01, but the latter are more consistent with the rest of […]
If I decide to keep this code I'll implement the change for consistency.
https://review.coreboot.org/#/c/33652/1/src/arch/x86/acpi/os.asl@94 PS1, Line 94: If(SCPM(local0, "Linux"))
Right. […]
Ugh... almost going for abandoning this patch.
Richard Spiegel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Abandoned
Only useful if supporting Windows. I doubt an open source project would use $$$ Microsoft.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33652 )
Change subject: arch/x86/acpi: Add os.asl ......................................................................
Patch Set 1:
Only useful if supporting Windows. I doubt an open source project would use $$$ Microsoft.
Many do, but I assume mostly only support for a single version is necessary. Linux has its bugs too, so one cannot say that this is only about Windows. But generally, I prefer to add work- arounds for specific OS's only case-by-case when one really has the means to test it. And then the workaround should be commen- ted as what it is. That's much better than a choice of per OS paths by default where it's not recognizable which path has to stay as is (workaround) and which can be adapted/unified (not saying that this change would have led to the latter, it's just what happened in some firmware in the past).
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)
-
Richard Spiegel (Code Review)