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 <[email protected]>
---
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
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;
On Wed, Apr 6, 2022 at 1:56 PM Thomas Gleixner <[email protected]> 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;
>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
---
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;
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
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 <[email protected]>
AuthorDate: Tue, 12 Apr 2022 22:34:21 +02:00
Committer: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Maciej W. Rozycki <[email protected]>
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;
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
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