The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
wrong. Since INTEL_P4_PRESCOTT is numerically greater than
INTEL_P4_WILLAMETTE, the logic always results in false and never sets
X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.
The error was introduced while replacing the x86_model check with a VFM
one. The original check was as follows:
        if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
                (c->x86 == 0x6 && c->x86_model >= 0x0e))
                set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);

Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
Cedarmill (model 6) which is the last model released in Family 15.

Fixes: fadb6f569b10 ("x86/cpu/intel: Limit the non-architectural constant_tsc model checks")

Cc: <stable@vger.kernel.org> # v6.15

Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>

---

Changes since v2:
- Improve commit message

Changes since v1:
- Fix incorrect logic

 arch/x86/kernel/cpu/intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 076eaa41b8c8..6f5bd5dbc249 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -262,7 +262,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 	if (c->x86_power & (1 << 8)) {
 		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
 		set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
-	} else if ((c->x86_vfm >= INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_WILLAMETTE) ||
+	} else if ((c->x86_vfm >=  INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_CEDARMILL) ||
 		   (c->x86_vfm >= INTEL_CORE_YONAH  && c->x86_vfm <= INTEL_IVYBRIDGE)) {
 		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
 	}
-- 
2.50.1

On 7/29/25 21:26, Suchit Karunakaran wrote:
> The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
> wrong. Since INTEL_P4_PRESCOTT is numerically greater than
> INTEL_P4_WILLAMETTE, the logic always results in false and never sets
> X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.
> The error was introduced while replacing the x86_model check with a VFM
> one. The original check was as follows:
>         if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
>                 (c->x86 == 0x6 && c->x86_model >= 0x0e))
>                 set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> 
> Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
> Cedarmill (model 6) which is the last model released in Family 15.

Could we have a slightly different changelog, please? The fact that the
logic results in the bit never getting set for P4's is IMNHO immaterial.
This looks like a plain and simple typo, not a logical error on the
patch author's part.

How about this as a changelog?

--

Pentium 4's which are INTEL_P4_PRESCOTT (mode 0x03) and later have a
constant TSC. This was correctly captured until fadb6f569b10
("x86/cpu/intel: Limit the non-architectural constant_tsc model
checks"). In that commit, the model was transposed from 0x03 to
INTEL_P4_WILLAMETTE, which is just plain wrong. That was presumably a
simple typo, probably just copying and pasting the wrong P4 model.

Fix the constant TSC logic to cover all later P4 models. End at
INTEL_P4_CEDARMILL which is the last P4 model.

On 7/31/2025 8:57 AM, Dave Hansen wrote:

> Could we have a slightly different changelog, please? The fact that the
> logic results in the bit never getting set for P4's is IMNHO immaterial.
> This looks like a plain and simple typo, not a logical error on the
> patch author's part.
> 

I can confirm it was a typing error that led to this bogus logic.

> How about this as a changelog?
> 

The changelog is fine, except the error happened while choosing the
upper bound (INTEL_P4_WILLAMETTE instead of INTEL_P4_CEDARMILL) and
*not* the lower bound (INTEL_P4_PRESCOTT) as suggested below.

> --
> 
> Pentium 4's which are INTEL_P4_PRESCOTT (mode 0x03) and later have a
> constant TSC. This was correctly captured until fadb6f569b10
> ("x86/cpu/intel: Limit the non-architectural constant_tsc model
> checks"). In that commit, the model was transposed from 0x03 to
> INTEL_P4_WILLAMETTE, which is just plain wrong. That was presumably a
> simple typo, probably just copying and pasting the wrong P4 model.
> 
> Fix the constant TSC logic to cover all later P4 models. End at
> INTEL_P4_CEDARMILL which is the last P4 model.

Tweaked this slightly for accuracy:

Pentium 4's which are INTEL_P4_PRESCOTT (model 0x03) and later have
a constant TSC. This was correctly captured until commit fadb6f569b10
("x86/cpu/intel: Limit the non-architectural constant_tsc model checks").

In that commit, an error was introduced while selecting the last P4
model (0x06) as the upper bound. Model 0x06 was transposed to
INTEL_P4_WILLAMETTE, which is just plain wrong. That was presumably a
simple typo, probably just copying and pasting the wrong P4 model.

Fix the constant TSC logic to cover all later P4 models. End at
INTEL_P4_CEDARMILL which accurately corresponds to the last P4 model.

On Thu, 31 Jul 2025 at 21:29, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 7/29/25 21:26, Suchit Karunakaran wrote:
> > The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
> > wrong. Since INTEL_P4_PRESCOTT is numerically greater than
> > INTEL_P4_WILLAMETTE, the logic always results in false and never sets
> > X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.
> > The error was introduced while replacing the x86_model check with a VFM
> > one. The original check was as follows:
> >         if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> >                 (c->x86 == 0x6 && c->x86_model >= 0x0e))
> >                 set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> >
> > Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
> > Cedarmill (model 6) which is the last model released in Family 15.
>
> Could we have a slightly different changelog, please? The fact that the
> logic results in the bit never getting set for P4's is IMNHO immaterial.
> This looks like a plain and simple typo, not a logical error on the
> patch author's part.
>
> How about this as a changelog?
>
> --
>
> Pentium 4's which are INTEL_P4_PRESCOTT (mode 0x03) and later have a
> constant TSC. This was correctly captured until fadb6f569b10
> ("x86/cpu/intel: Limit the non-architectural constant_tsc model
> checks"). In that commit, the model was transposed from 0x03 to
> INTEL_P4_WILLAMETTE, which is just plain wrong. That was presumably a
> simple typo, probably just copying and pasting the wrong P4 model.
>
> Fix the constant TSC logic to cover all later P4 models. End at
> INTEL_P4_CEDARMILL which is the last P4 model.

Yeah, I agree it's more of a typo than a logical error.

Hi Suchit,

The patch looks good to me except for a few nits below.

On 7/29/2025 9:26 PM, Suchit Karunakaran wrote:
> The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
> wrong. Since INTEL_P4_PRESCOTT is numerically greater than
> INTEL_P4_WILLAMETTE, the logic always results in false and never sets
> X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.

A blank line here would be useful to separate the two paragraphs.

> The error was introduced while replacing the x86_model check with a VFM
> one. The original check was as follows:

Maybe to make it more precise and avoid confusion.

The original check before the erroneous code was as follows:

>         if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
>                 (c->x86 == 0x6 && c->x86_model >= 0x0e))
>                 set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> 
> Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
> Cedarmill (model 6) which is the last model released in Family 15.
> 
> Fixes: fadb6f569b10 ("x86/cpu/intel: Limit the non-architectural constant_tsc model checks")
> 
> Cc: <stable@vger.kernel.org> # v6.15
> 
> Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>
> 

Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
(without the blank lines as Greg mentioned)

> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 076eaa41b8c8..6f5bd5dbc249 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -262,7 +262,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>  	if (c->x86_power & (1 << 8)) {
>  		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>  		set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> -	} else if ((c->x86_vfm >= INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_WILLAMETTE) ||
> +	} else if ((c->x86_vfm >=  INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_CEDARMILL) ||

The alignment here seems to have changed because the extra space after
INTEL_P4_PRESCOTT was moved before it. The current code has alignment
matched with the below line. It isn't a big deal, but keeps the code
easier to read.


>  		   (c->x86_vfm >= INTEL_CORE_YONAH  && c->x86_vfm <= INTEL_IVYBRIDGE)) {
>  		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>  	}

On Wed, 30 Jul 2025 at 20:12, Sohil Mehta <sohil.mehta@intel.com> wrote:
>
> Hi Suchit,
>
> The patch looks good to me except for a few nits below.
>
> On 7/29/2025 9:26 PM, Suchit Karunakaran wrote:
> > The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
> > wrong. Since INTEL_P4_PRESCOTT is numerically greater than
> > INTEL_P4_WILLAMETTE, the logic always results in false and never sets
> > X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.
>
> A blank line here would be useful to separate the two paragraphs.
>
> > The error was introduced while replacing the x86_model check with a VFM
> > one. The original check was as follows:
>
> Maybe to make it more precise and avoid confusion.
>
> The original check before the erroneous code was as follows:
>
> >         if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> >                 (c->x86 == 0x6 && c->x86_model >= 0x0e))
> >                 set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> >
> > Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
> > Cedarmill (model 6) which is the last model released in Family 15.
> >
> > Fixes: fadb6f569b10 ("x86/cpu/intel: Limit the non-architectural constant_tsc model checks")
> >
> > Cc: <stable@vger.kernel.org> # v6.15
> >
> > Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>
> >
>
> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
> (without the blank lines as Greg mentioned)
>
> >
> > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > index 076eaa41b8c8..6f5bd5dbc249 100644
> > --- a/arch/x86/kernel/cpu/intel.c
> > +++ b/arch/x86/kernel/cpu/intel.c
> > @@ -262,7 +262,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> >       if (c->x86_power & (1 << 8)) {
> >               set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> >               set_cpu_cap(c, X86_FEATURE_NONSTOP_TSC);
> > -     } else if ((c->x86_vfm >= INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_WILLAMETTE) ||
> > +     } else if ((c->x86_vfm >=  INTEL_P4_PRESCOTT && c->x86_vfm <= INTEL_P4_CEDARMILL) ||
>
> The alignment here seems to have changed because the extra space after
> INTEL_P4_PRESCOTT was moved before it. The current code has alignment
> matched with the below line. It isn't a big deal, but keeps the code
> easier to read.
>
>
> >                  (c->x86_vfm >= INTEL_CORE_YONAH  && c->x86_vfm <= INTEL_IVYBRIDGE)) {
> >               set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> >       }
>
>

Hi Sohil. Thanks for reviewing it. Should I send a new version of the
patch fixing the minor issues with the reviewed by tag?

On 7/30/2025 7:58 AM, Suchit Karunakaran wrote:

> Hi Sohil. Thanks for reviewing it. Should I send a new version of the
> patch fixing the minor issues with the reviewed by tag?

Yes please, I think one more revision would be helpful and reduce manual
steps for the maintainers. (Likely after the merge window has closed).

Also, please try to trim your responses to only include the essential
context:
https://subspace.kernel.org/etiquette.html#trim-your-quotes-when-replying.

On Wed, 30 Jul 2025 at 21:03, Sohil Mehta <sohil.mehta@intel.com> wrote:
>
> On 7/30/2025 7:58 AM, Suchit Karunakaran wrote:
>
> > Hi Sohil. Thanks for reviewing it. Should I send a new version of the
> > patch fixing the minor issues with the reviewed by tag?
>
> Yes please, I think one more revision would be helpful and reduce manual
> steps for the maintainers. (Likely after the merge window has closed).
>
> Also, please try to trim your responses to only include the essential
> context:
> https://subspace.kernel.org/etiquette.html#trim-your-quotes-when-replying.
>
>

Alright. I will send v4 after the merge window closes. Thanks!

On 7/30/2025 9:27 AM, Suchit Karunakaran wrote:
> 
> Alright. I will send v4 after the merge window closes. Thanks!

Sending the patch sooner is fine, if you want. I meant it is likely to
be picked up after the merge window has closed.

On Thu, 31 Jul 2025 at 00:59, Sohil Mehta <sohil.mehta@intel.com> wrote:
>
> On 7/30/2025 9:27 AM, Suchit Karunakaran wrote:
> >
> > Alright. I will send v4 after the merge window closes. Thanks!
>
> Sending the patch sooner is fine, if you want. I meant it is likely to
> be picked up after the merge window has closed.

Yup got it.

On Wed, Jul 30, 2025 at 09:56:17AM +0530, Suchit Karunakaran wrote:
> The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
> wrong. Since INTEL_P4_PRESCOTT is numerically greater than
> INTEL_P4_WILLAMETTE, the logic always results in false and never sets
> X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.
> The error was introduced while replacing the x86_model check with a VFM
> one. The original check was as follows:
>         if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
>                 (c->x86 == 0x6 && c->x86_model >= 0x0e))
>                 set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);

What do you mean by "original check"?  Before the change that caused
this, or what it should be?

> Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
> Cedarmill (model 6) which is the last model released in Family 15.
> 
> Fixes: fadb6f569b10 ("x86/cpu/intel: Limit the non-architectural constant_tsc model checks")
> 
> Cc: <stable@vger.kernel.org> # v6.15
> 
> Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>

Nit, no blank lines beween all of those last lines.  Hint, look at all
of the patches on the mailing lists AND in the tree already, you have
hundreds of thousands of examples here of how to format things :)

thanks,

greg k-h

On Wed, 30 Jul 2025 at 10:22, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jul 30, 2025 at 09:56:17AM +0530, Suchit Karunakaran wrote:
> > The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
> > wrong. Since INTEL_P4_PRESCOTT is numerically greater than
> > INTEL_P4_WILLAMETTE, the logic always results in false and never sets
> > X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.
> > The error was introduced while replacing the x86_model check with a VFM
> > one. The original check was as follows:
> >         if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> >                 (c->x86 == 0x6 && c->x86_model >= 0x0e))
> >                 set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>
> What do you mean by "original check"?  Before the change that caused
> this, or what it should be?
>

Original check in this context refers to the change before the erroneous code.

> > Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
> > Cedarmill (model 6) which is the last model released in Family 15.
> >
> > Fixes: fadb6f569b10 ("x86/cpu/intel: Limit the non-architectural constant_tsc model checks")
> >
> > Cc: <stable@vger.kernel.org> # v6.15
> >
> > Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>
>
> Nit, no blank lines beween all of those last lines.  Hint, look at all
> of the patches on the mailing lists AND in the tree already, you have
> hundreds of thousands of examples here of how to format things :)
>

Sorry about it. Should I send a new version of the patch removing the
blank lines?

On Wed, Jul 30, 2025 at 11:05:31AM +0530, Suchit Karunakaran wrote:
> On Wed, 30 Jul 2025 at 10:22, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Jul 30, 2025 at 09:56:17AM +0530, Suchit Karunakaran wrote:
> > > The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
> > > wrong. Since INTEL_P4_PRESCOTT is numerically greater than
> > > INTEL_P4_WILLAMETTE, the logic always results in false and never sets
> > > X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.
> > > The error was introduced while replacing the x86_model check with a VFM
> > > one. The original check was as follows:
> > >         if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> > >                 (c->x86 == 0x6 && c->x86_model >= 0x0e))
> > >                 set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> >
> > What do you mean by "original check"?  Before the change that caused
> > this, or what it should be?
> >
> 
> Original check in this context refers to the change before the erroneous code.
> 
> > > Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
> > > Cedarmill (model 6) which is the last model released in Family 15.
> > >
> > > Fixes: fadb6f569b10 ("x86/cpu/intel: Limit the non-architectural constant_tsc model checks")
> > >
> > > Cc: <stable@vger.kernel.org> # v6.15
> > >
> > > Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>
> >
> > Nit, no blank lines beween all of those last lines.  Hint, look at all
> > of the patches on the mailing lists AND in the tree already, you have
> > hundreds of thousands of examples here of how to format things :)
> >
> 
> Sorry about it. Should I send a new version of the patch removing the
> blank lines?

That's up to the maintainer(s) of this subsystem, if they want to
manually edit the change or not.  As it's the middle of the merge
window, no one will probably be doing anything for another 2 weeks on it
anyway, so just relax and see what happens :)

thanks,

greg k-h

On Wed, 30 Jul 2025 at 11:25, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jul 30, 2025 at 11:05:31AM +0530, Suchit Karunakaran wrote:
> > On Wed, 30 Jul 2025 at 10:22, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jul 30, 2025 at 09:56:17AM +0530, Suchit Karunakaran wrote:
> > > > The logic to synthesize constant_tsc for Pentium 4s (Family 15) is
> > > > wrong. Since INTEL_P4_PRESCOTT is numerically greater than
> > > > INTEL_P4_WILLAMETTE, the logic always results in false and never sets
> > > > X86_FEATURE_CONSTANT_TSC for any Pentium 4 model.
> > > > The error was introduced while replacing the x86_model check with a VFM
> > > > one. The original check was as follows:
> > > >         if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> > > >                 (c->x86 == 0x6 && c->x86_model >= 0x0e))
> > > >                 set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> > >
> > > What do you mean by "original check"?  Before the change that caused
> > > this, or what it should be?
> > >
> >
> > Original check in this context refers to the change before the erroneous code.
> >
> > > > Fix the logic to cover all Pentium 4 models from Prescott (model 3) to
> > > > Cedarmill (model 6) which is the last model released in Family 15.
> > > >
> > > > Fixes: fadb6f569b10 ("x86/cpu/intel: Limit the non-architectural constant_tsc model checks")
> > > >
> > > > Cc: <stable@vger.kernel.org> # v6.15
> > > >
> > > > Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>
> > >
> > > Nit, no blank lines beween all of those last lines.  Hint, look at all
> > > of the patches on the mailing lists AND in the tree already, you have
> > > hundreds of thousands of examples here of how to format things :)
> > >
> >
> > Sorry about it. Should I send a new version of the patch removing the
> > blank lines?
>
> That's up to the maintainer(s) of this subsystem, if they want to
> manually edit the change or not.  As it's the middle of the merge
> window, no one will probably be doing anything for another 2 weeks on it
> anyway, so just relax and see what happens :)
>
> thanks,
>
> greg k-h

Hi Greg,
Thank you for the clarification. I apologize for the mistakes and
appreciate your patience in reviewing.

Thanks,
Suchit