Hello,
I was looking at the Local APIC code in coreboot and was wondering about `lapic_write_atomic` in src/include/cpu/x86/lapic.h. This uses an atomic XCHG to write to Local APIC registers. I would like to understand why this would be necessary, because none of the OSes I've seen or worked on do anything similar. ALso the Intel SDM discourages acceses that are not plain loads/stores.
In the coreboot code, this function seems to exist for a really long time. I've found this from 2004:
#ifdef CONFIG_X86_GOOD_APIC # define FORCE_READ_AROUND_WRITE 0 # define lapic_read_around(x) lapic_read(x) # define lapic_write_around(x,y) lapic_write((x),(y)) #else # define FORCE_READ_AROUND_WRITE 1 # define lapic_read_around(x) lapic_read(x) # define lapic_write_around(x,y) lapic_write_atomic((x),(y)) #endif
This seems to indicate that using atomic writes was a workaround of some kind. Does anyone know more?
Thanks! Julian
that was pre-git but is there any useful comment in git anyway? I only have the vaguest memory of why it went in.
On Mon, Oct 4, 2021 at 7:14 AM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
Hello,
I was looking at the Local APIC code in coreboot and was wondering about `lapic_write_atomic` in src/include/cpu/x86/lapic.h. This uses an atomic XCHG to write to Local APIC registers. I would like to understand why this would be necessary, because none of the OSes I've seen or worked on do anything similar. ALso the Intel SDM discourages acceses that are not plain loads/stores.
In the coreboot code, this function seems to exist for a really long time. I've found this from 2004:
#ifdef CONFIG_X86_GOOD_APIC # define FORCE_READ_AROUND_WRITE 0 # define lapic_read_around(x) lapic_read(x) # define lapic_write_around(x,y) lapic_write((x),(y)) #else # define FORCE_READ_AROUND_WRITE 1 # define lapic_read_around(x) lapic_read(x) # define lapic_write_around(x,y) lapic_write_atomic((x),(y)) #endif
This seems to indicate that using atomic writes was a workaround of some kind. Does anyone know more?
Thanks! Julian
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Mon, 2021-10-04 at 07:32 -0700, ron minnich wrote:
that was pre-git but is there any useful comment in git anyway? I only have the vaguest memory of why it went in.
It was introduced in c84c1906b7 and fcd5ace00b3 without explanation. I particularly don't understand the lapic_write_around and lapic_read_around functions that pop up. From my perspective, none of this is needed and you can just use the usual lapic_read and lapic_write functions.
Julian
On Mon, Oct 4, 2021 at 7:14 AM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
Hello,
I was looking at the Local APIC code in coreboot and was wondering about `lapic_write_atomic` in src/include/cpu/x86/lapic.h. This uses an atomic XCHG to write to Local APIC registers. I would like to understand why this would be necessary, because none of the OSes I've seen or worked on do anything similar. ALso the Intel SDM discourages acceses that are not plain loads/stores.
In the coreboot code, this function seems to exist for a really long time. I've found this from 2004:
#ifdef CONFIG_X86_GOOD_APIC # define FORCE_READ_AROUND_WRITE 0 # define lapic_read_around(x) lapic_read(x) # define lapic_write_around(x,y) lapic_write((x),(y)) #else # define FORCE_READ_AROUND_WRITE 1 # define lapic_read_around(x) lapic_read(x) # define lapic_write_around(x,y) lapic_write_atomic((x),(y)) #endif
This seems to indicate that using atomic writes was a workaround of some kind. Does anyone know more?
Thanks! Julian
The problem, at this point, is that a change this broad must also be tested across all platforms to make sure it's not breaking.
This looks like it was done for a hardware problem. We had a lot of x86 implementations in tree at that time, and they had lots of bugs.
On Mon, Oct 4, 2021 at 8:11 AM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
On Mon, 2021-10-04 at 07:32 -0700, ron minnich wrote:
that was pre-git but is there any useful comment in git anyway? I only have the vaguest memory of why it went in.
It was introduced in c84c1906b7 and fcd5ace00b3 without explanation. I particularly don't understand the lapic_write_around and lapic_read_around functions that pop up. From my perspective, none of this is needed and you can just use the usual lapic_read and lapic_write functions.
Julian
On Mon, Oct 4, 2021 at 7:14 AM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
Hello,
I was looking at the Local APIC code in coreboot and was wondering about `lapic_write_atomic` in src/include/cpu/x86/lapic.h. This uses an atomic XCHG to write to Local APIC registers. I would like to understand why this would be necessary, because none of the OSes I've seen or worked on do anything similar. ALso the Intel SDM discourages acceses that are not plain loads/stores.
In the coreboot code, this function seems to exist for a really long time. I've found this from 2004:
#ifdef CONFIG_X86_GOOD_APIC # define FORCE_READ_AROUND_WRITE 0 # define lapic_read_around(x) lapic_read(x) # define lapic_write_around(x,y) lapic_write((x),(y)) #else # define FORCE_READ_AROUND_WRITE 1 # define lapic_read_around(x) lapic_read(x) # define lapic_write_around(x,y) lapic_write_atomic((x),(y)) #endif
This seems to indicate that using atomic writes was a workaround of some kind. Does anyone know more?
Thanks! Julian
ron minnich wrote:
The problem, at this point, is that a change this broad must also be tested across all platforms to make sure it's not breaking.
While true it could be worthwhile to check how often CONFIG_X86_GOOD_APIC is unset...
This looks like it was done for a hardware problem. We had a lot of x86 implementations in tree at that time, and they had lots of bugs.
Maybe none of them are left.
//Peter
I would be happy if all those old buggy systems were gone, good idea Peter!
On Mon, Oct 4, 2021 at 9:39 AM Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
The problem, at this point, is that a change this broad must also be tested across all platforms to make sure it's not breaking.
While true it could be worthwhile to check how often CONFIG_X86_GOOD_APIC is unset...
This looks like it was done for a hardware problem. We had a lot of x86 implementations in tree at that time, and they had lots of bugs.
Maybe none of them are left.
//Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
The X86_GOOD_APIC was set in the past in a few configs. You can find them via:
git log -S GOOD_APIC --source --all
The define itself was finally removed in:
commit fc57d6c4c2848726be1361f6dee3c33e7551b857 Author: Patrick Rudolph siro@das-labor.org Date: Tue Nov 12 16:30:14 2019 +0100
cpu/x86/lapic: Support x86_64 and clean up code
From then on all APICs were "bad".
But it looks like the workaround was just carried forward with no discussion of whether it's still necessary or what it actually works around.
Julian
On Mon, 2021-10-04 at 09:41 -0700, ron minnich wrote:
I would be happy if all those old buggy systems were gone, good idea Peter!
On Mon, Oct 4, 2021 at 9:39 AM Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
The problem, at this point, is that a change this broad must also be tested across all platforms to make sure it's not breaking.
While true it could be worthwhile to check how often CONFIG_X86_GOOD_APIC is unset...
This looks like it was done for a hardware problem. We had a lot of x86 implementations in tree at that time, and they had lots of bugs.
Maybe none of them are left.
//Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
The X86_GOOD_APIC was set in the past in a few configs. You can find them via:
git log -S GOOD_APIC --source --all
The define itself was finally removed in:
commit fc57d6c4c2848726be1361f6dee3c33e7551b857 Author: Patrick Rudolph siro@das-labor.org Date: Tue Nov 12 16:30:14 2019 +0100
cpu/x86/lapic: Support x86_64 and clean up codeFrom then on all APICs were "bad".
But it looks like the workaround was just carried forward with no discussion of whether it's still necessary or what it actually works around.
Hi
Removal has been suggested with the X2APIC work:
https://review.coreboot.org/c/coreboot/+/55199
Regards, Kyösti
On Tue, 2021-10-05 at 09:29 +0300, Kyösti Mälkki wrote:
On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
But it looks like the workaround was just carried forward with no discussion of whether it's still necessary or what it actually works around.
Hi
Removal has been suggested with the X2APIC work:
I've been looking at 4.13 instead of master. My bad. In master, indeed most atomic accesses are gone and the ones writing to ICR are left. This mostly makes sense and is much clearer now. :)
Thanks, Julian
A quick google turned this up: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
On Tue, Oct 5, 2021 at 4:06 AM Julian Stecklina < julian.stecklina@cyberus-technology.de> wrote:
On Tue, 2021-10-05 at 09:29 +0300, Kyösti Mälkki wrote:
On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
But it looks like the workaround was just carried forward with no
discussion
of whether it's still necessary or what it actually works around.
Hi
Removal has been suggested with the X2APIC work:
I've been looking at 4.13 instead of master. My bad. In master, indeed most atomic accesses are gone and the ones writing to ICR are left. This mostly makes sense and is much clearer now. :)
Thanks, Julian
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
nice fine. Might be worth adding the text of this comment (modified as needed) to the CL so that in years to come people understand the reasons.
On Tue, Oct 5, 2021 at 12:51 PM Matt B matthewwbradley6@gmail.com wrote:
A quick google turned this up: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
On Tue, Oct 5, 2021 at 4:06 AM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
On Tue, 2021-10-05 at 09:29 +0300, Kyösti Mälkki wrote:
On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
But it looks like the workaround was just carried forward with no discussion of whether it's still necessary or what it actually works around.
Hi
Removal has been suggested with the X2APIC work:
I've been looking at 4.13 instead of master. My bad. In master, indeed most atomic accesses are gone and the ones writing to ICR are left. This mostly makes sense and is much clearer now. :)
Thanks, Julian
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
I should note I'm not 100% sure what they're doing there.
Are there any more of these buggy pentiums left in the coreboot tree? (If he chooses to update) I can imagine RMS getting real snippy if we break his thinkpad. :P
On Tue, Oct 5, 2021 at 3:53 PM ron minnich rminnich@gmail.com wrote:
nice fine. Might be worth adding the text of this comment (modified as needed) to the CL so that in years to come people understand the reasons.
On Tue, Oct 5, 2021 at 12:51 PM Matt B matthewwbradley6@gmail.com wrote:
A quick google turned this up:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
On Tue, Oct 5, 2021 at 4:06 AM Julian Stecklina <
julian.stecklina@cyberus-technology.de> wrote:
On Tue, 2021-10-05 at 09:29 +0300, Kyösti Mälkki wrote:
On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
But it looks like the workaround was just carried forward with no
discussion
of whether it's still necessary or what it actually works around.
Hi
Removal has been suggested with the X2APIC work:
I've been looking at 4.13 instead of master. My bad. In master, indeed
most
atomic accesses are gone and the ones writing to ICR are left. This
mostly makes
sense and is much clearer now. :)
Thanks, Julian
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
That's what versions are all about. It seems sensible to me to leave the old bad stuff behind; if people need it, it's all still there if they know the tag.
On Tue, Oct 5, 2021 at 1:02 PM Matt B matthewwbradley6@gmail.com wrote:
I should note I'm not 100% sure what they're doing there.
Are there any more of these buggy pentiums left in the coreboot tree? (If he chooses to update) I can imagine RMS getting real snippy if we break his thinkpad. :P
On Tue, Oct 5, 2021 at 3:53 PM ron minnich rminnich@gmail.com wrote:
nice fine. Might be worth adding the text of this comment (modified as needed) to the CL so that in years to come people understand the reasons.
On Tue, Oct 5, 2021 at 12:51 PM Matt B matthewwbradley6@gmail.com wrote:
A quick google turned this up: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
On Tue, Oct 5, 2021 at 4:06 AM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
On Tue, 2021-10-05 at 09:29 +0300, Kyösti Mälkki wrote:
On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
But it looks like the workaround was just carried forward with no discussion of whether it's still necessary or what it actually works around.
Hi
Removal has been suggested with the X2APIC work:
I've been looking at 4.13 instead of master. My bad. In master, indeed most atomic accesses are gone and the ones writing to ICR are left. This mostly makes sense and is much clearer now. :)
Thanks, Julian
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
My concern is more about surprise brokenness when trying to use the newest version, if any of those pentiums remain.
On Tue, Oct 5, 2021 at 4:06 PM ron minnich rminnich@gmail.com wrote:
That's what versions are all about. It seems sensible to me to leave the old bad stuff behind; if people need it, it's all still there if they know the tag.
On Tue, Oct 5, 2021 at 1:02 PM Matt B matthewwbradley6@gmail.com wrote:
I should note I'm not 100% sure what they're doing there.
Are there any more of these buggy pentiums left in the coreboot tree?
(If he chooses to update) I can imagine RMS getting real snippy if we break his thinkpad. :P
On Tue, Oct 5, 2021 at 3:53 PM ron minnich rminnich@gmail.com wrote:
nice fine. Might be worth adding the text of this comment (modified as needed) to the CL so that in years to come people understand the reasons.
On Tue, Oct 5, 2021 at 12:51 PM Matt B matthewwbradley6@gmail.com
wrote:
A quick google turned this up:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
On Tue, Oct 5, 2021 at 4:06 AM Julian Stecklina <
julian.stecklina@cyberus-technology.de> wrote:
On Tue, 2021-10-05 at 09:29 +0300, Kyösti Mälkki wrote:
On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina julian.stecklina@cyberus-technology.de wrote: > > But it looks like the workaround was just carried forward with
no discussion
> of > whether it's still necessary or what it actually works around. >
Hi
Removal has been suggested with the X2APIC work:
I've been looking at 4.13 instead of master. My bad. In master,
indeed most
atomic accesses are gone and the ones writing to ICR are left. This
mostly makes
sense and is much clearer now. :)
Thanks, Julian
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
I'm sorry. :(
On 6/10/21 7:47 am, Matt B wrote:
My concern is more about surprise brokenness when trying to use the newest version, if any of those pentiums remain.
On Tue, Oct 5, 2021 at 4:06 PM ron minnich <rminnich@gmail.com mailto:rminnich@gmail.com> wrote:
That's what versions are all about. It seems sensible to me to leave the old bad stuff behind; if people need it, it's all still there if they know the tag. On Tue, Oct 5, 2021 at 1:02 PM Matt B <matthewwbradley6@gmail.com <mailto:matthewwbradley6@gmail.com>> wrote: > > I should note I'm not 100% sure what they're doing there. > > Are there any more of these buggy pentiums left in the coreboot tree? (If he chooses to update) I can imagine RMS getting real snippy if we break his thinkpad. :P > > On Tue, Oct 5, 2021 at 3:53 PM ron minnich <rminnich@gmail.com <mailto:rminnich@gmail.com>> wrote: >> >> nice fine. Might be worth adding the text of this comment (modified as >> needed) to the CL so that in years to come people understand the >> reasons. >> >> On Tue, Oct 5, 2021 at 12:51 PM Matt B <matthewwbradley6@gmail.com <mailto:matthewwbradley6@gmail.com>> wrote: >> > >> > A quick google turned this up: >> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.0/arch/x86/kernel/cpu/intel.c#253 <https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.0/arch/x86/kernel/cpu/intel.c#253> >> > >> > On Tue, Oct 5, 2021 at 4:06 AM Julian Stecklina <julian.stecklina@cyberus-technology.de <mailto:julian.stecklina@cyberus-technology.de>> wrote: >> >> >> >> On Tue, 2021-10-05 at 09:29 +0300, Kyösti Mälkki wrote: >> >> > On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina >> >> > <julian.stecklina@cyberus-technology.de <mailto:julian.stecklina@cyberus-technology.de>> wrote: >> >> > > >> >> > > But it looks like the workaround was just carried forward with no discussion >> >> > > of >> >> > > whether it's still necessary or what it actually works around. >> >> > > >> >> > >> >> > Hi >> >> > >> >> > Removal has been suggested with the X2APIC work: >> >> > >> >> > https://review.coreboot.org/c/coreboot/+/55199 <https://review.coreboot.org/c/coreboot/+/55199> >> >> > >> >> >> >> I've been looking at 4.13 instead of master. My bad. In master, indeed most >> >> atomic accesses are gone and the ones writing to ICR are left. This mostly makes >> >> sense and is much clearer now. :) >> >> >> >> Thanks, >> >> Julian >> >> >> >> _______________________________________________ >> >> coreboot mailing list -- coreboot@coreboot.org <mailto:coreboot@coreboot.org> >> >> To unsubscribe send an email to coreboot-leave@coreboot.org <mailto:coreboot-leave@coreboot.org> >> > >> > _______________________________________________ >> > coreboot mailing list -- coreboot@coreboot.org <mailto:coreboot@coreboot.org> >> > To unsubscribe send an email to coreboot-leave@coreboot.org <mailto:coreboot-leave@coreboot.org> > > _______________________________________________ > coreboot mailing list -- coreboot@coreboot.org <mailto:coreboot@coreboot.org> > To unsubscribe send an email to coreboot-leave@coreboot.org <mailto:coreboot-leave@coreboot.org>
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
"Doctor, it hurts when I do this" "Then don't do that"
same applies to new software applied to antiques.
On Tue, Oct 5, 2021 at 1:48 PM Matt B matthewwbradley6@gmail.com wrote:
My concern is more about surprise brokenness when trying to use the newest version, if any of those pentiums remain.
On Tue, Oct 5, 2021 at 4:06 PM ron minnich rminnich@gmail.com wrote:
That's what versions are all about. It seems sensible to me to leave the old bad stuff behind; if people need it, it's all still there if they know the tag.
On Tue, Oct 5, 2021 at 1:02 PM Matt B matthewwbradley6@gmail.com wrote:
I should note I'm not 100% sure what they're doing there.
Are there any more of these buggy pentiums left in the coreboot tree? (If he chooses to update) I can imagine RMS getting real snippy if we break his thinkpad. :P
On Tue, Oct 5, 2021 at 3:53 PM ron minnich rminnich@gmail.com wrote:
nice fine. Might be worth adding the text of this comment (modified as needed) to the CL so that in years to come people understand the reasons.
On Tue, Oct 5, 2021 at 12:51 PM Matt B matthewwbradley6@gmail.com wrote:
A quick google turned this up: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3...
On Tue, Oct 5, 2021 at 4:06 AM Julian Stecklina julian.stecklina@cyberus-technology.de wrote:
On Tue, 2021-10-05 at 09:29 +0300, Kyösti Mälkki wrote: > On Mon, Oct 4, 2021 at 8:12 PM Julian Stecklina > julian.stecklina@cyberus-technology.de wrote: > > > > But it looks like the workaround was just carried forward with no discussion > > of > > whether it's still necessary or what it actually works around. > > > > Hi > > Removal has been suggested with the X2APIC work: > > https://review.coreboot.org/c/coreboot/+/55199 >
I've been looking at 4.13 instead of master. My bad. In master, indeed most atomic accesses are gone and the ones writing to ICR are left. This mostly makes sense and is much clearer now. :)
Thanks, Julian
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
ron minnich wrote:
same applies to new software applied to antiques.
While you are correct for some software and some antiques I find this premise completely unacceptable. This attitude may be convenient for developers but it only further normalizes planned obsolecense. Not OK!
Software can make it a high priority to be compatible. Windows is a great example of that, and I'm sure that backwards compatibility (has) contributes significantly to its success.
Hardware is no different and can of course also make it a priority to be backwards compatible. If we consider the x86 instruction set in isolation then that's another great example.
I don't see this problem as lack of compatibility but more as lack of transparency, openness and/or collaboration - those are the ingredients for a general hardware initialization software without all the ridiculous fights that coreboot must endure to this day.
//Peter
The specific case is code that is technical debt for *two* steppings (B and C2) of *one* instance of a CPU (P54C). There's no need to keep that around, especially since, as pointed out in the CL, it's causing trouble.
I +2'ed the CL based on this discussion.
On Wed, Oct 6, 2021 at 5:27 AM Peter Stuge peter@stuge.se wrote:
ron minnich wrote:
same applies to new software applied to antiques.
While you are correct for some software and some antiques I find this premise completely unacceptable. This attitude may be convenient for developers but it only further normalizes planned obsolecense. Not OK!
Software can make it a high priority to be compatible. Windows is a great example of that, and I'm sure that backwards compatibility (has) contributes significantly to its success.
Hardware is no different and can of course also make it a priority to be backwards compatible. If we consider the x86 instruction set in isolation then that's another great example.
I don't see this problem as lack of compatibility but more as lack of transparency, openness and/or collaboration - those are the ingredients for a general hardware initialization software without all the ridiculous fights that coreboot must endure to this day.
//Peter _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
-
Julian Stecklina -
Keith Emery -
Kyösti Mälkki -
Matt B -
Peter Stuge -
ron minnich