[PATCH] MIPS - fix cycle counter timing calculations

Simon Burge posted 1 patch 4 years, 2 months ago
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211116072606.BE9C8A1856@thoreau.thistledown.com.au
Maintainers: "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Aurelien Jarno <aurelien@aurel32.net>
There is a newer version of this series
target/mips/cpu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] MIPS - fix cycle counter timing calculations
Posted by Simon Burge 4 years, 2 months ago
The cp0_count_ns value is calculated from the CP0_COUNT_RATE_DEFAULT
constant in target/mips/cpu.c.  The cycle counter resolution is defined
per-CPU in target/mips/cpu-defs.c.inc; use this value for calculating
cp0_count_ns.  Fixings timing problems on guest OSs for the 20Kc CPU
which has a CCRes of 1.
---
 target/mips/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 4aae23934b..0766e25693 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -440,8 +440,9 @@ static void mips_cp0_period_set(MIPSCPU *cpu)
 {
     CPUMIPSState *env = &cpu->env;
 
+    /* env->CCRes isn't initialised this early, use env->cpu_model->CCRes. */
     env->cp0_count_ns = clock_ticks_to_ns(MIPS_CPU(cpu)->clock,
-                                          cpu->cp0_count_rate);
+                                          env->cpu_model->CCRes);
     assert(env->cp0_count_ns);
 }
 

Re: [PATCH] MIPS - fix cycle counter timing calculations
Posted by Philippe Mathieu-Daudé 4 years, 1 month ago
On 11/16/21 08:26, Simon Burge wrote:
> The cp0_count_ns value is calculated from the CP0_COUNT_RATE_DEFAULT
> constant in target/mips/cpu.c.  The cycle counter resolution is defined
> per-CPU in target/mips/cpu-defs.c.inc; use this value for calculating
> cp0_count_ns.  Fixings timing problems on guest OSs for the 20Kc CPU
> which has a CCRes of 1.
> ---
>  target/mips/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
Good catch. Too bad you didn't Cc'ed the maintainers, this patch
would have been in the 6.2 release.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Queued to mips-next.

Re: [PATCH] MIPS - fix cycle counter timing calculations
Posted by Philippe Mathieu-Daudé 4 years, 1 month ago
On 12/13/21 11:10, Philippe Mathieu-Daudé wrote:
> On 11/16/21 08:26, Simon Burge wrote:
>> The cp0_count_ns value is calculated from the CP0_COUNT_RATE_DEFAULT
>> constant in target/mips/cpu.c.  The cycle counter resolution is defined
>> per-CPU in target/mips/cpu-defs.c.inc; use this value for calculating
>> cp0_count_ns.  Fixings timing problems on guest OSs for the 20Kc CPU
>> which has a CCRes of 1.
>> ---
>>  target/mips/cpu.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
> Good catch. Too bad you didn't Cc'ed the maintainers, this patch
> would have been in the 6.2 release.
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Queued to mips-next.

Oops, missing your Signed-off-by tag, see:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line

Do you mind re-sending with your S-o-b? Meanwhile, patch dropped.

Thanks,

Phil.

[PATCH v2] MIPS - fix cycle counter timing calculations
Posted by Simon Burge 4 years, 1 month ago
The cp0_count_ns value is calculated from the CP0_COUNT_RATE_DEFAULT
constant in target/mips/cpu.c.  The cycle counter resolution is defined
per-CPU in target/mips/cpu-defs.c.inc; use this value for calculating
cp0_count_ns.  Fixings timing problems on guest OSs for the 20Kc CPU
which has a CCRes of 1.

Signed-off-by: Simon Burge <simonb@NetBSD.org>
---
 target/mips/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 4aae23934b..0766e25693 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -440,8 +440,9 @@ static void mips_cp0_period_set(MIPSCPU *cpu)
 {
     CPUMIPSState *env = &cpu->env;
 
+    /* env->CCRes isn't initialised this early, use env->cpu_model->CCRes. */
     env->cp0_count_ns = clock_ticks_to_ns(MIPS_CPU(cpu)->clock,
-                                          cpu->cp0_count_rate);
+                                          env->cpu_model->CCRes);
     assert(env->cp0_count_ns);
 }
 
-- 
2.33.0


Re: [PATCH v2] MIPS - fix cycle counter timing calculations
Posted by Philippe Mathieu-Daudé 4 years, 1 month ago
On 12/13/21 14:51, Simon Burge wrote:
> The cp0_count_ns value is calculated from the CP0_COUNT_RATE_DEFAULT
> constant in target/mips/cpu.c.  The cycle counter resolution is defined
> per-CPU in target/mips/cpu-defs.c.inc; use this value for calculating
> cp0_count_ns.  Fixings timing problems on guest OSs for the 20Kc CPU
> which has a CCRes of 1.
> 
> Signed-off-by: Simon Burge <simonb@NetBSD.org>
> ---
>  target/mips/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 4aae23934b..0766e25693 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -440,8 +440,9 @@ static void mips_cp0_period_set(MIPSCPU *cpu)
>  {
>      CPUMIPSState *env = &cpu->env;
>  
> +    /* env->CCRes isn't initialised this early, use env->cpu_model->CCRes. */
>      env->cp0_count_ns = clock_ticks_to_ns(MIPS_CPU(cpu)->clock,
> -                                          cpu->cp0_count_rate);
> +                                          env->cpu_model->CCRes);
>      assert(env->cp0_count_ns);
>  }
>  

Minor comment, it is better to post patch iterations as new thread,
and not as reply to older patch, because in thread view your new
patch might ended hidden / lost.

Patch queued to mips-next, thanks.