Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220202140244.1681140-1-neelx@redhat.com> <874k361liu.ffs@tglx>
In-Reply-To: <874k361liu.ffs@tglx>
From:   Daniel Vacek <neelx@redhat.com>
Date:   Wed, 6 Apr 2022 16:54:39 +0200
Message-ID: <CACjP9X_A7aLmvypyOz1UrXM571gx_X5q7=w-1j+G+MSbCteiEw@mail.gmail.com>
Subject: Re: [PATCH] [RFC] apic: fix timer base macro definitions
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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