[PATCH] [RFC] apic: fix timer base macro definitions

[PATCH] [RFC] apic: fix timer base macro definitions

[Daniel Vacek]

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

Re: [PATCH] [RFC] apic: fix timer base macro definitions

[Thomas Gleixner]

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;

Re: [PATCH] [RFC] apic: fix timer base macro definitions

[Daniel Vacek]

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;
>

[PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0

[Thomas Gleixner]

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;

Re: [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0

[Maciej W. Rozycki]

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

Re: [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0

[Thomas Gleixner]

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

Re: [PATCH] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0

[Maciej W. Rozycki]

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

[tip: x86/apic] x86/apic: Clarify i82489DX bit overlap in APIC_LVT0

[tip-bot2 for Thomas Gleixner]

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;