* [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.