ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC interrupt is possible.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com --- M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 2 files changed, 29 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/43323/1
diff --git a/payloads/libpayload/arch/x86/apic.c b/payloads/libpayload/arch/x86/apic.c index a484163..ba7ab99 100644 --- a/payloads/libpayload/arch/x86/apic.c +++ b/payloads/libpayload/arch/x86/apic.c @@ -71,7 +71,7 @@ static int _apic_initialized; // TODO: Build a lookup table to avoid calculating it. static uint32_t ticks_per_ms; -static volatile uint8_t timer_waiting; +volatile uint8_t timer_waiting;
enum APIC_CAPABILITY { DISABLED = 0, @@ -104,7 +104,7 @@ return id; }
-void apic_delay(unsigned int usec) +void apic_start_delay(unsigned int usec) { die_if(!ticks_per_ms, "apic_init_timer was not run."); die_if(timer_waiting, "timer already started."); @@ -124,7 +124,12 @@ timer_waiting = 1;
apic_write32(APIC_TIMER_INIT_COUNT, ticks); + enable_interrupts(); +}
+ +void apic_wait_delay(void) +{ /* Loop in case another interrupt has fired and resumed execution. */ do { asm volatile( @@ -140,9 +145,16 @@ enable_interrupts(); }
+void apic_delay(unsigned int usec) +{ + apic_start_delay(usec); + apic_wait_delay(); +} + static void timer_interrupt_handler(u8 vector) { timer_waiting = 0; + apic_eoi(APIC_TIMER_VECTOR); }
static void suprious_interrupt_handler(u8 vector) {} @@ -204,6 +216,7 @@ uint8_t max = apic_max_lvt_entries(); for (int i = 0; i <= max; ++i) { uint32_t offset = APIC_LVT_TIMER + APIC_LVT_SIZE * i; + apic_eoi(i); apic_write32(offset, APIC_MASKED_BIT); } } @@ -248,6 +261,16 @@ apic_write32(APIC_LVT_TIMER, APIC_TIMER_VECTOR); }
+static void apic_sw_disable(void) +{ + uint32_t reg = apic_read32(APIC_SPURIOUS); + + reg &= ~APIC_SW_ENABLED_BIT; + printf("%s: writing %#x to %#x\n", __func__, reg, APIC_SPURIOUS); + + apic_write32(APIC_SPURIOUS, reg); +} + static void apic_sw_enable(void) { uint32_t reg = apic_read32(APIC_SPURIOUS); @@ -280,6 +303,7 @@ die_if(!(apic_capabilities() & XACPI), "APIC is not supported");
apic_bar_reg = _rdmsr(APIC_BASE_MSR); + printf("apic_bar_reg is 0x%llx\n", apic_bar_reg);
die_if(!(apic_bar_reg & XAPIC_ENABLED_BIT), "APIC is not enabled"); die_if(apic_bar_reg & X2APIC_ENABLED_BIT, @@ -287,6 +311,7 @@
apic_bar = (uint32_t)(apic_bar_reg & APIC_BASE_MASK);
+ apic_sw_disable(); apic_reset_all_lvts(); apic_set_task_priority(0); apic_setup_spurious(); diff --git a/payloads/libpayload/include/x86/arch/apic.h b/payloads/libpayload/include/x86/arch/apic.h index 3c9877c..eedf1b4 100644 --- a/payloads/libpayload/include/x86/arch/apic.h +++ b/payloads/libpayload/include/x86/arch/apic.h @@ -40,5 +40,7 @@ void apic_eoi(uint8_t vector);
void apic_delay(unsigned int usec); +void apic_start_delay(unsigned int usec); +void apic_wait_delay(void);
#endif /* __ARCH_X86_INCLUDES_ARCH_APIC_H__ */
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... PS1, Line 74: volatile uint8_t timer_waiting; Should this remain static?
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... PS1, Line 127: enable_interrupts(); Should there be a blocking version of this function so we can leave interrupts disabled until apic_wait_delay() is done?
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... PS1, Line 74: volatile uint8_t timer_waiting;
Should this remain static?
Done
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... PS1, Line 127: enable_interrupts();
Should there be a blocking version of this function so we can leave interrupts disabled until apic_w […]
Not totally sure what you mean. This is being called in a single threaded environment, so if you block here, apic_wait_delay can't be run. If you want blocking behavior you can use apic_delay at line 148?
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth, Paul Menzel, Stefan Reinauer, Christian Walter,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43323
to look at the new patch set (#2).
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC interrupt is possible.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com --- M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 2 files changed, 28 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/43323/2
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... PS1, Line 127: enable_interrupts();
Not totally sure what you mean. […]
You're right, I was a little paranoid that this would change behavior of apic_delay(), but it should be fine.
Another thing though - Do you need to change the do-loop in apic_wait_delay() to a while-loop and check timer_waiting beforehand? I'm just thinking that if you you enable_interrupts() here then we might end up servicing the interrupt too soon, reintroducing the race condition that is described above on lines 120-121.
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth, David Hendricks, Paul Menzel, Stefan Reinauer, Christian Walter,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43323
to look at the new patch set (#3).
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC interrupt is possible.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com --- M 3rdparty/amd_blobs M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 3 files changed, 31 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/43323/3
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth, David Hendricks, Paul Menzel, Stefan Reinauer, Christian Walter,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43323
to look at the new patch set (#4).
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC timer interrupt is possible. Previous to this, because there was no EOI, the first timeer interrupt the CPU took was also the last it would take -- apic_delay would only work one time.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com --- M 3rdparty/amd_blobs M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 3 files changed, 31 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/43323/4
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/4/3rdparty/amd_blobs File 3rdparty/amd_blobs:
https://review.coreboot.org/c/coreboot/+/43323/4/3rdparty/amd_blobs@1 PS4, Line 1: Subproject commit 17d028869899e1ea0f6f385501088f7623121fcb Do we need this change?
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth, David Hendricks, Paul Menzel, Stefan Reinauer, Christian Walter,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43323
to look at the new patch set (#6).
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC timer interrupt is possible. Previous to this, because there was no EOI, the first timeer interrupt the CPU took was also the last it would take -- apic_delay would only work one time.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com --- M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 2 files changed, 30 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/43323/6
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/4/3rdparty/amd_blobs File 3rdparty/amd_blobs:
https://review.coreboot.org/c/coreboot/+/43323/4/3rdparty/amd_blobs@1 PS4, Line 1: Subproject commit 17d028869899e1ea0f6f385501088f7623121fcb
Do we need this change?
no, I took that out on the latest. Thanks.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
minor nit, otherwise looks good to me
https://review.coreboot.org/c/coreboot/+/43323/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43323/6//COMMIT_MSG@13 PS6, Line 13: timeer timer
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/6/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/6/payloads/libpayload/arch/x8... PS6, Line 121: * starting the timer and the hlt instruction. */ While we're at it, it would be good to remove or modify this comment. Heck, do we still even need disable_interrupts() and enable_interrupts() in this function anymore?
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43323/6//COMMIT_MSG@13 PS6, Line 13: timeer
timer
:-)
fixed.
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth, David Hendricks, Paul Menzel, Stefan Reinauer, Christian Walter,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43323
to look at the new patch set (#7).
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC timer interrupt is possible. Previous to this, because there was no EOI, the first timer interrupt the CPU took was also the last it would take -- apic_delay would only work one time.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com --- M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 2 files changed, 30 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/43323/7
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 7: Code-Review-1
(1 comment)
sorry, one more thing that I missed earlier...
https://review.coreboot.org/c/coreboot/+/43323/7/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/7/payloads/libpayload/arch/x8... PS7, Line 133: /* Loop in case another interrupt has fired and resumed execution. */ I think we also need to disable_interrupts() before entering this loop to avoid the race condition described below.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/7/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/7/payloads/libpayload/arch/x8... PS7, Line 133: /* Loop in case another interrupt has fired and resumed execution. */
I think we also need to disable_interrupts() before entering this loop to avoid the race condition d […]
Done
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth, David Hendricks, Paul Menzel, Stefan Reinauer, Christian Walter,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43323
to look at the new patch set (#8).
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC timer interrupt is possible. Previous to this, because there was no EOI, the first timer interrupt the CPU took was also the last it would take -- apic_delay would only work one time.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com --- M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 2 files changed, 33 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/43323/8
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 8: Code-Review+1
(2 comments)
Looks good to me. I guess apic_start_delay() no longer needs to enable/disable interrupts, should we clean those up or is there a reason to keep them?
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... PS8, Line 122: disable_interrupts(); Is there any reason to keep this?
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... PS8, Line 127: enable_interrupts(); and this?
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... PS8, Line 122: disable_interrupts();
Is there any reason to keep this?
I think it should be there. while timer_waiting is a volatile, wrapping changes to it in disable/enable just feels a bit better to me.
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... PS8, Line 127: enable_interrupts();
and this?
yes, because in my mind lines 123-126 represent a critical section that ought to be protected with a cli/sti.
It's a bit of probably unnecessary drama, but I don't see that it does any harm.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 8: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... PS8, Line 122: disable_interrupts();
I think it should be there. […]
Ack
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... PS8, Line 127: enable_interrupts();
yes, because in my mind lines 123-126 represent a critical section that ought to be protected with a […]
Good point.
As an aside, perhaps when there are more use cases we'll want to return or block if timer_waiting is already set.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 8: Code-Review+2
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43323/6//COMMIT_MSG@13 PS6, Line 13: timeer
:-) […]
Done
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/4/3rdparty/amd_blobs File 3rdparty/amd_blobs:
https://review.coreboot.org/c/coreboot/+/43323/4/3rdparty/amd_blobs@1 PS4, Line 1: Subproject commit 17d028869899e1ea0f6f385501088f7623121fcb
no, I took that out on the latest. Thanks.
Done
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43323/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43323/6//COMMIT_MSG@13 PS6, Line 13: timeer
Done
how many times to I have to tell this interface I'm done?
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... PS8, Line 122: disable_interrupts();
Ack
done done done done done
https://review.coreboot.org/c/coreboot/+/43323/8/payloads/libpayload/arch/x8... PS8, Line 127: enable_interrupts();
Good point. […]
done done done done dammit
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
Patch Set 9:
(2 comments)
Worst. UI. Ever.
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/1/payloads/libpayload/arch/x8... PS1, Line 127: enable_interrupts();
You're right, I was a little paranoid that this would change behavior of apic_delay(), but it should […]
Done
https://review.coreboot.org/c/coreboot/+/43323/6/payloads/libpayload/arch/x8... File payloads/libpayload/arch/x86/apic.c:
https://review.coreboot.org/c/coreboot/+/43323/6/payloads/libpayload/arch/x8... PS6, Line 121: * starting the timer and the hlt instruction. */
While we're at it, it would be good to remove or modify this comment. […]
Done
Hello Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth, David Hendricks, Paul Menzel, Stefan Reinauer, Christian Walter,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43323
to look at the new patch set (#10).
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC timer interrupt is possible. Previous to this, because there was no EOI, the first timer interrupt the CPU took was also the last it would take -- apic_delay would only work one time.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com --- M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 2 files changed, 33 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/43323/10
ron minnich has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43323 )
Change subject: libpayload: allow nonblocking delay and more than one delay ......................................................................
libpayload: allow nonblocking delay and more than one delay
Extend the local APIC timer delay so that it can be started, and waited for, independently.
Add an EOI so that more than one APIC timer interrupt is possible. Previous to this, because there was no EOI, the first timer interrupt the CPU took was also the last it would take -- apic_delay would only work one time.
Change-Id: Ib11aeee5b7da81287166ac68fc327e7ae62d1b84 Signed-off-by: Ronald G Minnich rminnich@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43323 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: David Hendricks david.hendricks@gmail.com Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M payloads/libpayload/arch/x86/apic.c M payloads/libpayload/include/x86/arch/apic.h 2 files changed, 33 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified David Hendricks: Looks good to me, approved Philipp Deppenwiese: Looks good to me, approved
diff --git a/payloads/libpayload/arch/x86/apic.c b/payloads/libpayload/arch/x86/apic.c index a484163..6201161 100644 --- a/payloads/libpayload/arch/x86/apic.c +++ b/payloads/libpayload/arch/x86/apic.c @@ -104,7 +104,7 @@ return id; }
-void apic_delay(unsigned int usec) +void apic_start_delay(unsigned int usec) { die_if(!ticks_per_ms, "apic_init_timer was not run."); die_if(timer_waiting, "timer already started."); @@ -124,9 +124,17 @@ timer_waiting = 1;
apic_write32(APIC_TIMER_INIT_COUNT, ticks); + enable_interrupts(); +}
+ +void apic_wait_delay(void) +{ /* Loop in case another interrupt has fired and resumed execution. */ - do { + disable_interrupts(); + /* Note: when we test timer_waiting, interrupts are disabled by the line + * above and the cli below. */ + while (timer_waiting) { asm volatile( "sti\n\t" "hlt\n\t" @@ -134,15 +142,22 @@ * between checking timer_waiting and executing the hlt * instruction again. */ "cli\n\t"); - } while (timer_waiting); + }
/* Leave hardware interrupts enabled. */ enable_interrupts(); }
+void apic_delay(unsigned int usec) +{ + apic_start_delay(usec); + apic_wait_delay(); +} + static void timer_interrupt_handler(u8 vector) { timer_waiting = 0; + apic_eoi(APIC_TIMER_VECTOR); }
static void suprious_interrupt_handler(u8 vector) {} @@ -204,6 +219,7 @@ uint8_t max = apic_max_lvt_entries(); for (int i = 0; i <= max; ++i) { uint32_t offset = APIC_LVT_TIMER + APIC_LVT_SIZE * i; + apic_eoi(i); apic_write32(offset, APIC_MASKED_BIT); } } @@ -248,6 +264,16 @@ apic_write32(APIC_LVT_TIMER, APIC_TIMER_VECTOR); }
+static void apic_sw_disable(void) +{ + uint32_t reg = apic_read32(APIC_SPURIOUS); + + reg &= ~APIC_SW_ENABLED_BIT; + printf("%s: writing %#x to %#x\n", __func__, reg, APIC_SPURIOUS); + + apic_write32(APIC_SPURIOUS, reg); +} + static void apic_sw_enable(void) { uint32_t reg = apic_read32(APIC_SPURIOUS); @@ -280,6 +306,7 @@ die_if(!(apic_capabilities() & XACPI), "APIC is not supported");
apic_bar_reg = _rdmsr(APIC_BASE_MSR); + printf("apic_bar_reg is 0x%llx\n", apic_bar_reg);
die_if(!(apic_bar_reg & XAPIC_ENABLED_BIT), "APIC is not enabled"); die_if(apic_bar_reg & X2APIC_ENABLED_BIT, @@ -287,6 +314,7 @@
apic_bar = (uint32_t)(apic_bar_reg & APIC_BASE_MASK);
+ apic_sw_disable(); apic_reset_all_lvts(); apic_set_task_priority(0); apic_setup_spurious(); diff --git a/payloads/libpayload/include/x86/arch/apic.h b/payloads/libpayload/include/x86/arch/apic.h index 3c9877c..eedf1b4 100644 --- a/payloads/libpayload/include/x86/arch/apic.h +++ b/payloads/libpayload/include/x86/arch/apic.h @@ -40,5 +40,7 @@ void apic_eoi(uint8_t vector);
void apic_delay(unsigned int usec); +void apic_start_delay(unsigned int usec); +void apic_wait_delay(void);
#endif /* __ARCH_X86_INCLUDES_ARCH_APIC_H__ */
-
David Hendricks (Code Review) -
Philipp Deppenwiese (Code Review)
-
ron minnich (Code Review)