[PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[Tiezhu Yang]

There is no need to build and call check_bugs32() under CONFIG_64BIT,
just limit it under CONFIG_32BIT.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/include/asm/bugs.h | 4 +++-
 arch/mips/kernel/cpu-probe.c | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h
index d72dc6e..5f8d2bf 100644
--- a/arch/mips/include/asm/bugs.h
+++ b/arch/mips/include/asm/bugs.h
@@ -35,7 +35,9 @@ static inline void check_bugs(void)
 	unsigned int cpu = smp_processor_id();
 
 	cpu_data[cpu].udelay_val = loops_per_jiffy;
-	check_bugs32();
+
+	if (IS_ENABLED(CONFIG_32BIT))
+		check_bugs32();
 
 	if (IS_ENABLED(CONFIG_CPU_R4X00_BUGS64))
 		check_bugs64();
diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
index f21a230..85d7273 100644
--- a/arch/mips/kernel/cpu-probe.c
+++ b/arch/mips/kernel/cpu-probe.c
@@ -461,6 +461,7 @@ static inline void cpu_set_mt_per_tc_perf(struct cpuinfo_mips *c)
 		c->options |= MIPS_CPU_MT_PER_TC_PERF_COUNTERS;
 }
 
+#ifdef CONFIG_32BIT
 static inline void check_errata(void)
 {
 	struct cpuinfo_mips *c = &current_cpu_data;
@@ -484,6 +485,7 @@ void __init check_bugs32(void)
 {
 	check_errata();
 }
+#endif /* CONFIG_32BIT */
 
 /*
  * Probe whether cpu has config register by trying to play with
-- 
2.1.0

Re: [PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[YunQiang Su]

Tiezhu Yang <yangtiezhu@loongson.cn> 于2020年4月9日周四 上午11:10写道：
>
> There is no need to build and call check_bugs32() under CONFIG_64BIT,
> just limit it under CONFIG_32BIT.

Since 32bit is subset of 64bit, and due to the code, I think that the
initial purpose
of check_bugs32() is also willing to run even with CONFIG_64BIT.

For example, if we have a CPU which is 64bit, and work well on 64bit mode, while
has a bug only on 32bit mode, check_bugs32 should be used here.

Loongson's 3A 1000 is the example, I cannot support FP32 mode well.

>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  arch/mips/include/asm/bugs.h | 4 +++-
>  arch/mips/kernel/cpu-probe.c | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/include/asm/bugs.h b/arch/mips/include/asm/bugs.h
> index d72dc6e..5f8d2bf 100644
> --- a/arch/mips/include/asm/bugs.h
> +++ b/arch/mips/include/asm/bugs.h
> @@ -35,7 +35,9 @@ static inline void check_bugs(void)
>         unsigned int cpu = smp_processor_id();
>
>         cpu_data[cpu].udelay_val = loops_per_jiffy;
> -       check_bugs32();
> +
> +       if (IS_ENABLED(CONFIG_32BIT))
> +               check_bugs32();
>
>         if (IS_ENABLED(CONFIG_CPU_R4X00_BUGS64))
>                 check_bugs64();
> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
> index f21a230..85d7273 100644
> --- a/arch/mips/kernel/cpu-probe.c
> +++ b/arch/mips/kernel/cpu-probe.c
> @@ -461,6 +461,7 @@ static inline void cpu_set_mt_per_tc_perf(struct cpuinfo_mips *c)
>                 c->options |= MIPS_CPU_MT_PER_TC_PERF_COUNTERS;
>  }
>
> +#ifdef CONFIG_32BIT
>  static inline void check_errata(void)
>  {
>         struct cpuinfo_mips *c = &current_cpu_data;
> @@ -484,6 +485,7 @@ void __init check_bugs32(void)
>  {
>         check_errata();
>  }
> +#endif /* CONFIG_32BIT */
>
>  /*
>   * Probe whether cpu has config register by trying to play with
> --
> 2.1.0
>


-- 
YunQiang Su

Re: [PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[Jiaxun Yang]

On Thu, 9 Apr 2020 12:43:28 +0800
YunQiang Su <wzssyqa@gmail.com> wrote:

> Tiezhu Yang <yangtiezhu@loongson.cn> 于2020年4月9日周四
> 上午11:10写道：
> >
> > There is no need to build and call check_bugs32() under
> > CONFIG_64BIT, just limit it under CONFIG_32BIT.  
> 
> Since 32bit is subset of 64bit, and due to the code, I think that the
> initial purpose
> of check_bugs32() is also willing to run even with CONFIG_64BIT.
> 
> For example, if we have a CPU which is 64bit, and work well on 64bit
> mode, while has a bug only on 32bit mode, check_bugs32 should be used
> here.
> 
> Loongson's 3A 1000 is the example, I cannot support FP32 mode well.

In this case bugs32 only contains a workaround for MIPS34K, which is a
MIPS32 processor. It's safe to do so.

But my suggestion is if you're going to clean-up bugs and workarounds
you'd better establish a file for silicon bugs and provide Kconfig
options to enable & disable them. Manage bug dependencies by Kconfig
will be easier.

Thanks.

--
Jiaxun Yang

Re: [PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[Maciej W. Rozycki]

On Thu, 9 Apr 2020, Jiaxun Yang wrote:

> > > There is no need to build and call check_bugs32() under
> > > CONFIG_64BIT, just limit it under CONFIG_32BIT.  
> > 
> > Since 32bit is subset of 64bit, and due to the code, I think that the
> > initial purpose
> > of check_bugs32() is also willing to run even with CONFIG_64BIT.
> > 
> > For example, if we have a CPU which is 64bit, and work well on 64bit
> > mode, while has a bug only on 32bit mode, check_bugs32 should be used
> > here.
> > 
> > Loongson's 3A 1000 is the example, I cannot support FP32 mode well.
> 
> In this case bugs32 only contains a workaround for MIPS34K, which is a
> MIPS32 processor. It's safe to do so.

 This is because commit c65a5480ff29 ("[MIPS] Fix potential latency 
problem due to non-atomic cpu_wait.") moved the other generic workaround 
elsewhere.

 The intent has been since historical commit 450ad16ba0ab ("Get rid of 
arch/mips64/kernel.  9116 lines of code gone.") that `check_bugs32' is for 
generic errata affecting both 32-bit and 64-bit operation (e.g. 32-bit 
instructions, which naturally may occur in both cases) and `check_bugs64' 
is for errata affecting 64-bit operation only (e.g. 64-bit instructions).

 But currently it appears we have no generic errata handled, as surely a 
34K erratum cannot affect 64-bit operation.  So I think such a change 
makes sense in principle (if a generic erratum appears in the future we 
can add a third category, which includes workarounds that are always 
applied), but I think it has to be made in a cleaner way.

 Specifically `check_errata' has to be renamed to `check_errata32', some 
commentary added as to the intent, and last but not least a proper change 
description added that not only repeats what the change does (and what 
everyone sees regardless), but actually justifies why the change is made.  
Saying: "There is no need[...]" does not tell us *why* there is no need.

> But my suggestion is if you're going to clean-up bugs and workarounds
> you'd better establish a file for silicon bugs and provide Kconfig
> options to enable & disable them. Manage bug dependencies by Kconfig
> will be easier.

 Why is using Kconfig supposed to be better?  Several configurations 
support multiple processor types (e.g. swappable CPU daugthercards or FPGA 
soft-cores) and having to list CPU types across platforms as CPUs are 
added is going to be a maintenance nightmare.  Whereas having workarounds 
or panics associated with run-time determination of the actual CPU type 
guarantees they will trigger where necessary.  The use of `init' sections 
assures the reclaim of memory for use after bootstrap.

 OTOH I agree splitting off errata handling to a separate file may make 
sense for structural reasons; we have it already for `check_bugs64'.

  Maciej

Re: [PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[Jiaxun Yang]

于 2020年4月9日 GMT+08:00 下午11:07:46, "Maciej W. Rozycki" <macro@linux-mips.org> 写到:
>On Thu, 9 Apr 2020, Jiaxun Yang wrote:
>
>> > > There is no need to build and call check_bugs32() under
>> > > CONFIG_64BIT, just limit it under CONFIG_32BIT.  
>> > 
>> > Since 32bit is subset of 64bit, and due to the code, I think that
>the
>> > initial purpose
>> > of check_bugs32() is also willing to run even with CONFIG_64BIT.
>> > 
>> > For example, if we have a CPU which is 64bit, and work well on
>64bit
>> > mode, while has a bug only on 32bit mode, check_bugs32 should be
>used
>> > here.
>> > 
>> > Loongson's 3A 1000 is the example, I cannot support FP32 mode well.
>> 
>> In this case bugs32 only contains a workaround for MIPS34K, which is
>a
>> MIPS32 processor. It's safe to do so.
>
> This is because commit c65a5480ff29 ("[MIPS] Fix potential latency 
>problem due to non-atomic cpu_wait.") moved the other generic
>workaround 
>elsewhere.
>
> The intent has been since historical commit 450ad16ba0ab ("Get rid of 
>arch/mips64/kernel.  9116 lines of code gone.") that `check_bugs32' is
>for 
>generic errata affecting both 32-bit and 64-bit operation (e.g. 32-bit 
>instructions, which naturally may occur in both cases) and
>`check_bugs64' 
>is for errata affecting 64-bit operation only (e.g. 64-bit
>instructions).
>
>But currently it appears we have no generic errata handled, as surely a
>
>34K erratum cannot affect 64-bit operation.  So I think such a change 
>makes sense in principle (if a generic erratum appears in the future we
>
>can add a third category, which includes workarounds that are always 
>applied), but I think it has to be made in a cleaner way.
>
>Specifically `check_errata' has to be renamed to `check_errata32', some
>
>commentary added as to the intent, and last but not least a proper
>change 
>description added that not only repeats what the change does (and what 
>everyone sees regardless), but actually justifies why the change is
>made.  
>Saying: "There is no need[...]" does not tell us *why* there is no
>need.
>
>> But my suggestion is if you're going to clean-up bugs and workarounds
>> you'd better establish a file for silicon bugs and provide Kconfig
>> options to enable & disable them. Manage bug dependencies by Kconfig
>> will be easier.
>
> Why is using Kconfig supposed to be better?  Several configurations 
>support multiple processor types (e.g. swappable CPU daugthercards or
>FPGA 
>soft-cores) and having to list CPU types across platforms as CPUs are 
>added is going to be a maintenance nightmare.  Whereas having
>workarounds 
>or panics associated with run-time determination of the actual CPU type
>
>guarantees they will trigger where necessary.  The use of `init'
>sections 
>assures the reclaim of memory for use after bootstrap.

Actually I meant let bug checks depends on Kconfig's CPU selection.

It's guaranteed that you can only select one kind of CPU one time,
to prevent the overhead of checking bugs on irrelevant processors.

And we still have to check PRID/CPUTYPE during boot to enable
proper workarounds, because the Kconfig options are telling about the possibility,
which means a processor potentially has some kinds of bug.

In this case, M34K's errata should depends on or selected by 
CPU_MIPS32_R2 in Kconfig.

So there won't be any nightmare, but only reduced code :-)

Probably we can build-up a general framework for checks & workarounds,
and display affected bugs in /proc/cpuinfo?
That's my personal thought.

Thanks.

>
>OTOH I agree splitting off errata handling to a separate file may make 
>sense for structural reasons; we have it already for `check_bugs64'.
>
>  Maciej

-- 
Jiaxun Yang

Re: [PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[Maciej W. Rozycki]

On Thu, 9 Apr 2020, Jiaxun Yang wrote:

> > Why is using Kconfig supposed to be better?  Several configurations 
> >support multiple processor types (e.g. swappable CPU daugthercards or
> >FPGA 
> >soft-cores) and having to list CPU types across platforms as CPUs are 
> >added is going to be a maintenance nightmare.  Whereas having
> >workarounds 
> >or panics associated with run-time determination of the actual CPU type
> >
> >guarantees they will trigger where necessary.  The use of `init'
> >sections 
> >assures the reclaim of memory for use after bootstrap.
> 
> Actually I meant let bug checks depends on Kconfig's CPU selection.
> 
> It's guaranteed that you can only select one kind of CPU one time,
> to prevent the overhead of checking bugs on irrelevant processors.

 That makes no sense to me sorry.  When you select say a MIPS32r2 CPU for 
a Malta configuration, you can run it with a 4KE, 24K, 24KE, 34K, 74K, 
1004K, M14K, and probably a few other CPUs I have forgotten about.  Are 
you suggesting now that you want to require a separate kernel 
configuration for each of these CPUs?

> And we still have to check PRID/CPUTYPE during boot to enable
> proper workarounds, because the Kconfig options are telling about the possibility,
> which means a processor potentially has some kinds of bug.
> 
> In this case, M34K's errata should depends on or selected by 
> CPU_MIPS32_R2 in Kconfig.
> 
> So there won't be any nightmare, but only reduced code :-)

 You'll need to manually maintain CPU assignment to configurations, which 
is not needed now.

 Anyway, please show your patch to let us see any improvement brought by 
it and we can discuss it then.

  Maciej

Re: [PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[Jiaxun Yang]

于 2020年4月10日 GMT+08:00 上午12:15:27, "Maciej W. Rozycki" <macro@linux-mips.org> 写到:
>On Thu, 9 Apr 2020, Jiaxun Yang wrote:
>
>> > Why is using Kconfig supposed to be better?  Several configurations
>
>> >support multiple processor types (e.g. swappable CPU daugthercards
>or
>> >FPGA 
>> >soft-cores) and having to list CPU types across platforms as CPUs
>are 
>> >added is going to be a maintenance nightmare.  Whereas having
>> >workarounds 
>> >or panics associated with run-time determination of the actual CPU
>type
>> >
>> >guarantees they will trigger where necessary.  The use of `init'
>> >sections 
>> >assures the reclaim of memory for use after bootstrap.
>> 
>> Actually I meant let bug checks depends on Kconfig's CPU selection.
>> 
>> It's guaranteed that you can only select one kind of CPU one time,
>> to prevent the overhead of checking bugs on irrelevant processors.
>
>That makes no sense to me sorry.  When you select say a MIPS32r2 CPU
>for 
>a Malta configuration, you can run it with a 4KE, 24K, 24KE, 34K, 74K, 
>1004K, M14K, and probably a few other CPUs I have forgotten about.  Are
>
>you suggesting now that you want to require a separate kernel 
>configuration for each of these CPUs?

Nope. As the Kconfig is telling about the possibility,
the real CPUTYPE check is still done during boot.

Thanks.
-- 
Jiaxun Yang

Re: [PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[YunQiang Su]

Jiaxun Yang <jiaxun.yang@flygoat.com> 于2020年4月9日周四 下午11:47写道：
>
>
>
> 于 2020年4月9日 GMT+08:00 下午11:07:46, "Maciej W. Rozycki" <macro@linux-mips.org> 写到:
> >On Thu, 9 Apr 2020, Jiaxun Yang wrote:
> >
> >> > > There is no need to build and call check_bugs32() under
> >> > > CONFIG_64BIT, just limit it under CONFIG_32BIT.
> >> >
> >> > Since 32bit is subset of 64bit, and due to the code, I think that
> >the
> >> > initial purpose
> >> > of check_bugs32() is also willing to run even with CONFIG_64BIT.
> >> >
> >> > For example, if we have a CPU which is 64bit, and work well on
> >64bit
> >> > mode, while has a bug only on 32bit mode, check_bugs32 should be
> >used
> >> > here.
> >> >
> >> > Loongson's 3A 1000 is the example, I cannot support FP32 mode well.
> >>
> >> In this case bugs32 only contains a workaround for MIPS34K, which is
> >a
> >> MIPS32 processor. It's safe to do so.
> >
> > This is because commit c65a5480ff29 ("[MIPS] Fix potential latency
> >problem due to non-atomic cpu_wait.") moved the other generic
> >workaround
> >elsewhere.
> >
> > The intent has been since historical commit 450ad16ba0ab ("Get rid of
> >arch/mips64/kernel.  9116 lines of code gone.") that `check_bugs32' is
> >for
> >generic errata affecting both 32-bit and 64-bit operation (e.g. 32-bit
> >instructions, which naturally may occur in both cases) and
> >`check_bugs64'
> >is for errata affecting 64-bit operation only (e.g. 64-bit
> >instructions).
> >
> >But currently it appears we have no generic errata handled, as surely a
> >
> >34K erratum cannot affect 64-bit operation.  So I think such a change
> >makes sense in principle (if a generic erratum appears in the future we
> >
> >can add a third category, which includes workarounds that are always
> >applied), but I think it has to be made in a cleaner way.
> >
> >Specifically `check_errata' has to be renamed to `check_errata32', some
> >
> >commentary added as to the intent, and last but not least a proper
> >change
> >description added that not only repeats what the change does (and what
> >everyone sees regardless), but actually justifies why the change is
> >made.
> >Saying: "There is no need[...]" does not tell us *why* there is no
> >need.
> >
> >> But my suggestion is if you're going to clean-up bugs and workarounds
> >> you'd better establish a file for silicon bugs and provide Kconfig
> >> options to enable & disable them. Manage bug dependencies by Kconfig
> >> will be easier.
> >
> > Why is using Kconfig supposed to be better?  Several configurations
> >support multiple processor types (e.g. swappable CPU daugthercards or
> >FPGA
> >soft-cores) and having to list CPU types across platforms as CPUs are
> >added is going to be a maintenance nightmare.  Whereas having
> >workarounds
> >or panics associated with run-time determination of the actual CPU type
> >
> >guarantees they will trigger where necessary.  The use of `init'
> >sections
> >assures the reclaim of memory for use after bootstrap.
>
> Actually I meant let bug checks depends on Kconfig's CPU selection.
>

I don't think this is a good idea. Since the routine is executed single time,
so it is not performance critical.
And Kconfig for per-cpu is bad for the future if we want to try to archive
single kernel image.

> It's guaranteed that you can only select one kind of CPU one time,
> to prevent the overhead of checking bugs on irrelevant processors.
>
> And we still have to check PRID/CPUTYPE during boot to enable
> proper workarounds, because the Kconfig options are telling about the possibility,
> which means a processor potentially has some kinds of bug.
>
> In this case, M34K's errata should depends on or selected by
> CPU_MIPS32_R2 in Kconfig.
>
> So there won't be any nightmare, but only reduced code :-)
>
> Probably we can build-up a general framework for checks & workarounds,
> and display affected bugs in /proc/cpuinfo?
> That's my personal thought.
>
> Thanks.
>
> >
> >OTOH I agree splitting off errata handling to a separate file may make
> >sense for structural reasons; we have it already for `check_bugs64'.
> >
> >  Maciej
>
> --
> Jiaxun Yang



-- 
YunQiang Su

Re: [PATCH] MIPS: Limit check_bugs32() under CONFIG_32BIT

[Jiaxun Yang]

于 2020年4月11日 GMT+08:00 下午3:37:24, YunQiang Su <wzssyqa@gmail.com> 写到:
>Jiaxun Yang <jiaxun.yang@flygoat.com> 于2020年4月9日周四 下午11:47写道：
>>
>>
>>
>> 于 2020年4月9日 GMT+08:00 下午11:07:46, "Maciej W. Rozycki"
><macro@linux-mips.org> 写到:
>> >On Thu, 9 Apr 2020, Jiaxun Yang wrote:
>> >
>> >> > > There is no need to build and call check_bugs32() under
>> >> > > CONFIG_64BIT, just limit it under CONFIG_32BIT.
>> >> >
>> >> > Since 32bit is subset of 64bit, and due to the code, I think
>that
>> >the
>> >> > initial purpose
>> >> > of check_bugs32() is also willing to run even with CONFIG_64BIT.
>> >> >
>> >> > For example, if we have a CPU which is 64bit, and work well on
>> >64bit
>> >> > mode, while has a bug only on 32bit mode, check_bugs32 should be
>> >used
>> >> > here.
>> >> >
>> >> > Loongson's 3A 1000 is the example, I cannot support FP32 mode
>well.
>> >>
>> >> In this case bugs32 only contains a workaround for MIPS34K, which
>is
>> >a
>> >> MIPS32 processor. It's safe to do so.
>> >
>> > This is because commit c65a5480ff29 ("[MIPS] Fix potential latency
>> >problem due to non-atomic cpu_wait.") moved the other generic
>> >workaround
>> >elsewhere.
>> >
>> > The intent has been since historical commit 450ad16ba0ab ("Get rid
>of
>> >arch/mips64/kernel.  9116 lines of code gone.") that `check_bugs32'
>is
>> >for
>> >generic errata affecting both 32-bit and 64-bit operation (e.g.
>32-bit
>> >instructions, which naturally may occur in both cases) and
>> >`check_bugs64'
>> >is for errata affecting 64-bit operation only (e.g. 64-bit
>> >instructions).
>> >
>> >But currently it appears we have no generic errata handled, as
>surely a
>> >
>> >34K erratum cannot affect 64-bit operation.  So I think such a
>change
>> >makes sense in principle (if a generic erratum appears in the future
>we
>> >
>> >can add a third category, which includes workarounds that are always
>> >applied), but I think it has to be made in a cleaner way.
>> >
>> >Specifically `check_errata' has to be renamed to `check_errata32',
>some
>> >
>> >commentary added as to the intent, and last but not least a proper
>> >change
>> >description added that not only repeats what the change does (and
>what
>> >everyone sees regardless), but actually justifies why the change is
>> >made.
>> >Saying: "There is no need[...]" does not tell us *why* there is no
>> >need.
>> >
>> >> But my suggestion is if you're going to clean-up bugs and
>workarounds
>> >> you'd better establish a file for silicon bugs and provide Kconfig
>> >> options to enable & disable them. Manage bug dependencies by
>Kconfig
>> >> will be easier.
>> >
>> > Why is using Kconfig supposed to be better?  Several configurations
>> >support multiple processor types (e.g. swappable CPU daugthercards
>or
>> >FPGA
>> >soft-cores) and having to list CPU types across platforms as CPUs
>are
>> >added is going to be a maintenance nightmare.  Whereas having
>> >workarounds
>> >or panics associated with run-time determination of the actual CPU
>type
>> >
>> >guarantees they will trigger where necessary.  The use of `init'
>> >sections
>> >assures the reclaim of memory for use after bootstrap.
>>
>> Actually I meant let bug checks depends on Kconfig's CPU selection.
>>
>
>I don't think this is a good idea. Since the routine is executed single
>time,
>so it is not performance critical.
>And Kconfig for per-cpu is bad for the future if we want to try to
>archive
>single kernel image.

You can only select one kind of CPU one time,
according to our current design.
e.g. MIPS32R2 (Indicating MIPS32R2 IP Cores from MIPS company),
Ingenic Xburst, Loongson64, R4000.

We won't create a new entry for a kind of CPU unless
the difference is significant.
But when that happens, filtering out unnecessary code on certain
platforms can be beneficial.

Thanks

-- 
Jiaxun Yang