* [PATCH] [RFC] apic: fix timer base macro definitions
@ 2022-02-02 14:02 Daniel Vacek
2022-04-06 11:47 ` Thomas Gleixner
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vacek @ 2022-02-02 14:02 UTC (permalink / raw)
To: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen
Cc: x86, linux-kernel
I was wondering if the aliasing of APIC_TIMER_BASE_TMBASE and
APIC_LVT_TIMER_TSCDEADLINE was intentional or we need to << 19?
Also it seems the GET_APIC_TIMER_BASE, APIC_TIMER_BASE_CLKIN and
APIC_TIMER_BASE_TMBASE are not even being used. Perhaps, can we
just remove them?
Signed-off-by: Daniel Vacek <neelx@redhat.com>
---
arch/x86/include/asm/apicdef.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index 5716f22f81ac..00b4ca49f3ea 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -95,9 +95,9 @@
#define APIC_LVTTHMR 0x330
#define APIC_LVTPC 0x340
#define APIC_LVT0 0x350
-#define APIC_LVT_TIMER_BASE_MASK (0x3 << 18)
-#define GET_APIC_TIMER_BASE(x) (((x) >> 18) & 0x3)
-#define SET_APIC_TIMER_BASE(x) (((x) << 18))
+#define APIC_LVT_TIMER_BASE_MASK (0x3 << 19)
+#define GET_APIC_TIMER_BASE(x) (((x) >> 19) & 0x3)
+#define SET_APIC_TIMER_BASE(x) (((x) << 19))
#define APIC_TIMER_BASE_CLKIN 0x0
#define APIC_TIMER_BASE_TMBASE 0x1
#define APIC_TIMER_BASE_DIV 0x2
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] [RFC] apic: fix timer base macro definitions
2022-02-02 14:02 [PATCH] [RFC] apic: fix timer base macro definitions Daniel Vacek
@ 2022-04-06 11:47 ` Thomas Gleixner
2022-04-06 14:54 ` Daniel Vacek
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2022-04-06 11:47 UTC (permalink / raw)
To: Daniel Vacek, H. Peter Anvin, Ingo Molnar, Borislav Petkov, Dave Hansen
Cc: x86, linux-kernel
Daniel,
On Wed, Feb 02 2022 at 15:02, Daniel Vacek wrote:
> I was wondering if the aliasing of APIC_TIMER_BASE_TMBASE and
> APIC_LVT_TIMER_TSCDEADLINE was intentional or we need to << 19?
That's intentional. This is only used for the !lapic_is_integrated()
case, which is the ancient i82489DX.
Something like the below should make this more clear.
Thanks,
tglx
---
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -95,12 +95,6 @@
#define APIC_LVTTHMR 0x330
#define APIC_LVTPC 0x340
#define APIC_LVT0 0x350
-#define APIC_LVT_TIMER_BASE_MASK (0x3 << 18)
-#define GET_APIC_TIMER_BASE(x) (((x) >> 18) & 0x3)
-#define SET_APIC_TIMER_BASE(x) (((x) << 18))
-#define APIC_TIMER_BASE_CLKIN 0x0
-#define APIC_TIMER_BASE_TMBASE 0x1
-#define APIC_TIMER_BASE_DIV 0x2
#define APIC_LVT_TIMER_ONESHOT (0 << 17)
#define APIC_LVT_TIMER_PERIODIC (1 << 17)
#define APIC_LVT_TIMER_TSCDEADLINE (2 << 17)
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -320,6 +320,9 @@ int lapic_get_maxlvt(void)
#define APIC_DIVISOR 16
#define TSC_DIVISOR 8
+/* i82489DX specific */
+#define I82489DX_BASE_DIVIDER (((0x2) << 18))
+
/*
* This function sets up the local APIC timer, with a timeout of
* 'clocks' APIC bus clock. During calibration we actually call
@@ -340,8 +343,14 @@ static void __setup_APIC_LVTT(unsigned i
else if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
lvtt_value |= APIC_LVT_TIMER_TSCDEADLINE;
+ /*
+ * The i82489DX APIC uses bit 18 and 19 for the base divider. This
+ * overlaps with bit 18 on integrated APICs, but is not documented
+ * in the SDM. No problem though. i82489DX equipped systems do not
+ * have TSC deadline timer.
+ */
if (!lapic_is_integrated())
- lvtt_value |= SET_APIC_TIMER_BASE(APIC_TIMER_BASE_DIV);
+ lvtt_value |= I82489DX_BASE_DIVIDER;
if (!irqen)
lvtt_value |= APIC_LVT_MASKED;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] [RFC] apic: fix timer base macro definitions
2022-04-06 11:47 ` Thomas Gleixner
@ 2022-04-06 14:54 ` Daniel Vacek
2022-04-12 20:34 ` [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0 Thomas Gleixner
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vacek @ 2022-04-06 14:54 UTC (permalink / raw)
To: Thomas Gleixner
Cc: H. Peter Anvin, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
open list
On Wed, Apr 6, 2022 at 1:56 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Daniel,
>
> On Wed, Feb 02 2022 at 15:02, Daniel Vacek wrote:
> > I was wondering if the aliasing of APIC_TIMER_BASE_TMBASE and
> > APIC_LVT_TIMER_TSCDEADLINE was intentional or we need to << 19?
>
> That's intentional. This is only used for the !lapic_is_integrated()
> case, which is the ancient i82489DX.
>
> Something like the below should make this more clear.
Nah. Makes sense. Thanks for clearing that up. Looks good to me now.
--nX
> Thanks,
>
> tglx
> ---
> --- a/arch/x86/include/asm/apicdef.h
> +++ b/arch/x86/include/asm/apicdef.h
> @@ -95,12 +95,6 @@
> #define APIC_LVTTHMR 0x330
> #define APIC_LVTPC 0x340
> #define APIC_LVT0 0x350
> -#define APIC_LVT_TIMER_BASE_MASK (0x3 << 18)
> -#define GET_APIC_TIMER_BASE(x) (((x) >> 18) & 0x3)
> -#define SET_APIC_TIMER_BASE(x) (((x) << 18))
> -#define APIC_TIMER_BASE_CLKIN 0x0
> -#define APIC_TIMER_BASE_TMBASE 0x1
> -#define APIC_TIMER_BASE_DIV 0x2
> #define APIC_LVT_TIMER_ONESHOT (0 << 17)
> #define APIC_LVT_TIMER_PERIODIC (1 << 17)
> #define APIC_LVT_TIMER_TSCDEADLINE (2 << 17)
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -320,6 +320,9 @@ int lapic_get_maxlvt(void)
> #define APIC_DIVISOR 16
> #define TSC_DIVISOR 8
>
> +/* i82489DX specific */
> +#define I82489DX_BASE_DIVIDER (((0x2) << 18))
> +
> /*
> * This function sets up the local APIC timer, with a timeout of
> * 'clocks' APIC bus clock. During calibration we actually call
> @@ -340,8 +343,14 @@ static void __setup_APIC_LVTT(unsigned i
> else if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
> lvtt_value |= APIC_LVT_TIMER_TSCDEADLINE;
>
> + /*
> + * The i82489DX APIC uses bit 18 and 19 for the base divider. This
> + * overlaps with bit 18 on integrated APICs, but is not documented
> + * in the SDM. No problem though. i82489DX equipped systems do not
> + * have TSC deadline timer.
> + */
> if (!lapic_is_integrated())
> - lvtt_value |= SET_APIC_TIMER_BASE(APIC_TIMER_BASE_DIV);
> + lvtt_value |= I82489DX_BASE_DIVIDER;
>
> if (!irqen)
> lvtt_value |= APIC_LVT_MASKED;
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0
2022-04-06 14:54 ` Daniel Vacek
@ 2022-04-12 20:34 ` Thomas Gleixner
2022-04-12 22:17 ` Maciej W. Rozycki
2022-04-13 16:44 ` [tip: x86/apic] " tip-bot2 for Thomas Gleixner
0 siblings, 2 replies; 8+ messages in thread
From: Thomas Gleixner @ 2022-04-12 20:34 UTC (permalink / raw)
To: Daniel Vacek
Cc: H. Peter Anvin, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
open list
Daniel stumbled over the undocumented bit overlap of the i82498DX external
APIC and the TSC deadline timer configuration bit in modern APICs.
Remove the i82489DX macro maze, use a i82489DX specific define in the apic
code and document the overlap in a comment.
Reported-by: Daniel Vacek <neelx@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
arch/x86/include/asm/apicdef.h | 6 ------
arch/x86/kernel/apic/apic.c | 11 ++++++++++-
2 files changed, 10 insertions(+), 7 deletions(-)
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -95,12 +95,6 @@
#define APIC_LVTTHMR 0x330
#define APIC_LVTPC 0x340
#define APIC_LVT0 0x350
-#define APIC_LVT_TIMER_BASE_MASK (0x3 << 18)
-#define GET_APIC_TIMER_BASE(x) (((x) >> 18) & 0x3)
-#define SET_APIC_TIMER_BASE(x) (((x) << 18))
-#define APIC_TIMER_BASE_CLKIN 0x0
-#define APIC_TIMER_BASE_TMBASE 0x1
-#define APIC_TIMER_BASE_DIV 0x2
#define APIC_LVT_TIMER_ONESHOT (0 << 17)
#define APIC_LVT_TIMER_PERIODIC (1 << 17)
#define APIC_LVT_TIMER_TSCDEADLINE (2 << 17)
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -320,6 +320,9 @@ int lapic_get_maxlvt(void)
#define APIC_DIVISOR 16
#define TSC_DIVISOR 8
+/* i82489DX specific */
+#define I82489DX_BASE_DIVIDER (((0x2) << 18))
+
/*
* This function sets up the local APIC timer, with a timeout of
* 'clocks' APIC bus clock. During calibration we actually call
@@ -340,8 +343,14 @@ static void __setup_APIC_LVTT(unsigned i
else if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
lvtt_value |= APIC_LVT_TIMER_TSCDEADLINE;
+ /*
+ * The i82489DX APIC uses bit 18 and 19 for the base divider. This
+ * overlaps with bit 18 on integrated APICs, but is not documented
+ * in the SDM. No problem though. i82489DX equipped systems do not
+ * have TSC deadline timer.
+ */
if (!lapic_is_integrated())
- lvtt_value |= SET_APIC_TIMER_BASE(APIC_TIMER_BASE_DIV);
+ lvtt_value |= I82489DX_BASE_DIVIDER;
if (!irqen)
lvtt_value |= APIC_LVT_MASKED;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0
2022-04-12 20:34 ` [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0 Thomas Gleixner
@ 2022-04-12 22:17 ` Maciej W. Rozycki
2022-04-13 13:24 ` Thomas Gleixner
2022-04-13 16:44 ` [tip: x86/apic] " tip-bot2 for Thomas Gleixner
1 sibling, 1 reply; 8+ messages in thread
From: Maciej W. Rozycki @ 2022-04-12 22:17 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Daniel Vacek, H. Peter Anvin, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, open list
On Tue, 12 Apr 2022, Thomas Gleixner wrote:
> Daniel stumbled over the undocumented bit overlap of the i82498DX external
> APIC and the TSC deadline timer configuration bit in modern APICs.
For the record, it's documented in the i82498DX datasheet[1] and user
manual[2]:
'Bits [19:18] Timer Base: This field selects the time base input to be
used by the timer.
00: (Base 0): Uses "CLKIN" as input.
01: (Base 1): Uses "TMBASE".
10: (Base 2): Uses the output of the divider (Base 2).'
(the wording is virtually the same in both sources). Base 2 setting is
compatible with later APIC implementations.
Since you're removing the macros and the documents referred aren't easily
available it may be worth to mention the settings somewhere, such as the
comment you're adding.
Intel indeed did not document the two-bit field in any later literature,
and the i82498DX part cannot be used with any other APIC device due to a
protocol (and also wiring) difference in the inter-APIC communication bus.
There's also bit 2 of the Divide Configuration Register. That bit is
hardwired to 0 in later APIC versions, however in the i82498DX it selects
the time base input to be used by the divider, 0 for CLK (CLKIN) or 1 for
TMBASE. Conversely bit 3 is hardwired to 0 in the i82498DX.
References:
[1] "82489DX Advanced Programmable Interrupt Controller", Intel
Corporation, Order Number: 290446-002, October 1993, Section 6.12
"Timer Registers", p.27
[2] M. Jayakumar, "AP-388 82489DX User's Manual", Multiprocessor
Technology Group, Intel Corporation, Order Number: 292116-002,
November 1995, Section "Register Programming Details", p.22
FWIW,
Maciej
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0
2022-04-12 22:17 ` Maciej W. Rozycki
@ 2022-04-13 13:24 ` Thomas Gleixner
2022-04-13 15:55 ` Maciej W. Rozycki
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2022-04-13 13:24 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Daniel Vacek, H. Peter Anvin, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, open list
Maciej,
On Tue, Apr 12 2022 at 23:17, Maciej W. Rozycki wrote:
> On Tue, 12 Apr 2022, Thomas Gleixner wrote:
>
>> Daniel stumbled over the undocumented bit overlap of the i82498DX external
>> APIC and the TSC deadline timer configuration bit in modern APICs.
>
> For the record, it's documented in the i82498DX datasheet[1] and user
> manual[2]:
>
> 'Bits [19:18] Timer Base: This field selects the time base input to be
> used by the timer.
That's true, but how many people aside of you and me still have access
to the i82498DX related documentation? The interwebs has no trace of
them.
With the above I explicitely meant the undocumented bit overlap both in
the current SDMs and the kernel source.
Thanks,
tglx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0
2022-04-13 13:24 ` Thomas Gleixner
@ 2022-04-13 15:55 ` Maciej W. Rozycki
0 siblings, 0 replies; 8+ messages in thread
From: Maciej W. Rozycki @ 2022-04-13 15:55 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Daniel Vacek, H. Peter Anvin, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, open list
Thomas,
> > For the record, it's documented in the i82498DX datasheet[1] and user
> > manual[2]:
> >
> > 'Bits [19:18] Timer Base: This field selects the time base input to be
> > used by the timer.
>
> That's true, but how many people aside of you and me still have access
> to the i82498DX related documentation? The interwebs has no trace of
> them.
Hmm, actually now that you mention it I recall that archive.org does have
scanned copies of the 1995 Intel486 Microprocessors and Pentium Processors
databooks, and they include the i82489DX datasheet and manual respectively
starting from pages 4-220 (857) and 2-579 (600). See:
<https://archive.org/details/bitsavers_intel80486croprocessorsandRelatedProductsJan95_58561506>
<https://archive.org/details/bitsavers_intelpentirocessorsandRelatedComponents_64170750>
I have uploaded clean copies of the discrete documents to my site now too,
made via a PostScript printer driver with the "print" function of software
included with Intel Data on Demand CDs to handle the proprietary document
format used there. Sadly these are bitmaps rather than searchable PDFs,
but they might be easier to refer to. No better format has been tracked
down. See:
<ftp://ftp.linux-mips.org/pub/linux/mips/people/macro/APIC/>
Because why not?
> With the above I explicitely meant the undocumented bit overlap both in
> the current SDMs and the kernel source.
Fair enough. The lore has probably been already forgotten within Intel.
Maciej
^ permalink raw reply [flat|nested] 8+ messages in thread
* [tip: x86/apic] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0
2022-04-12 20:34 ` [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0 Thomas Gleixner
2022-04-12 22:17 ` Maciej W. Rozycki
@ 2022-04-13 16:44 ` tip-bot2 for Thomas Gleixner
1 sibling, 0 replies; 8+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2022-04-13 16:44 UTC (permalink / raw)
To: linux-tip-commits
Cc: Daniel Vacek, Thomas Gleixner, Maciej W. Rozycki, x86, linux-kernel
The following commit has been merged into the x86/apic branch of tip:
Commit-ID: daf3af4705ba8f49d33ea9b7bafdc9fd9efd49e0
Gitweb: https://git.kernel.org/tip/daf3af4705ba8f49d33ea9b7bafdc9fd9efd49e0
Author: Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Tue, 12 Apr 2022 22:34:21 +02:00
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Wed, 13 Apr 2022 18:39:48 +02:00
x86/apic: Clarify i82489DX bit overlap in APIC_LVT0
Daniel stumbled over the bit overlap of the i82498DX external APIC and the
TSC deadline timer configuration bit in modern APICs, which is neither
documented in the code nor in the current SDM. Maciej provided links to
the original i82489DX/486 documentation. See Link.
Remove the i82489DX macro maze, use a i82489DX specific define in the apic
code and document the overlap in a comment.
Reported-by: Daniel Vacek <neelx@redhat.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Maciej W. Rozycki <macro@orcam.me.uk>
Link: https://lore.kernel.org/r/87ee22f3ci.ffs@tglx
---
arch/x86/include/asm/apicdef.h | 6 ------
arch/x86/kernel/apic/apic.c | 11 ++++++++++-
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index 5716f22..92035eb 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -95,12 +95,6 @@
#define APIC_LVTTHMR 0x330
#define APIC_LVTPC 0x340
#define APIC_LVT0 0x350
-#define APIC_LVT_TIMER_BASE_MASK (0x3 << 18)
-#define GET_APIC_TIMER_BASE(x) (((x) >> 18) & 0x3)
-#define SET_APIC_TIMER_BASE(x) (((x) << 18))
-#define APIC_TIMER_BASE_CLKIN 0x0
-#define APIC_TIMER_BASE_TMBASE 0x1
-#define APIC_TIMER_BASE_DIV 0x2
#define APIC_LVT_TIMER_ONESHOT (0 << 17)
#define APIC_LVT_TIMER_PERIODIC (1 << 17)
#define APIC_LVT_TIMER_TSCDEADLINE (2 << 17)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b70344b..13819bf 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -320,6 +320,9 @@ int lapic_get_maxlvt(void)
#define APIC_DIVISOR 16
#define TSC_DIVISOR 8
+/* i82489DX specific */
+#define I82489DX_BASE_DIVIDER (((0x2) << 18))
+
/*
* This function sets up the local APIC timer, with a timeout of
* 'clocks' APIC bus clock. During calibration we actually call
@@ -340,8 +343,14 @@ static void __setup_APIC_LVTT(unsigned int clocks, int oneshot, int irqen)
else if (boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER))
lvtt_value |= APIC_LVT_TIMER_TSCDEADLINE;
+ /*
+ * The i82489DX APIC uses bit 18 and 19 for the base divider. This
+ * overlaps with bit 18 on integrated APICs, but is not documented
+ * in the SDM. No problem though. i82489DX equipped systems do not
+ * have TSC deadline timer.
+ */
if (!lapic_is_integrated())
- lvtt_value |= SET_APIC_TIMER_BASE(APIC_TIMER_BASE_DIV);
+ lvtt_value |= I82489DX_BASE_DIVIDER;
if (!irqen)
lvtt_value |= APIC_LVT_MASKED;
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-13 16:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-02 14:02 [PATCH] [RFC] apic: fix timer base macro definitions Daniel Vacek
2022-04-06 11:47 ` Thomas Gleixner
2022-04-06 14:54 ` Daniel Vacek
2022-04-12 20:34 ` [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0 Thomas Gleixner
2022-04-12 22:17 ` Maciej W. Rozycki
2022-04-13 13:24 ` Thomas Gleixner
2022-04-13 15:55 ` Maciej W. Rozycki
2022-04-13 16:44 ` [tip: x86/apic] " tip-bot2 for Thomas Gleixner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.