[PATCH] MIPS - fix cycle counter timing calculations

[PATCH] MIPS - fix cycle counter timing calculations

[Simon Burge]

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

[Philippe Mathieu-Daudé]

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

[Philippe Mathieu-Daudé]

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

[Simon Burge]

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] MIPS - fix cycle counter timing calculations

[Simon Burge]

Hi Phil,

=?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= wrote:

> Oops, missing your Signed-off-by tag, see:
> https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-includ
e-a-signed-off-by-line
>
> Do you mind re-sending with your S-o-b? Meanwhile, patch dropped.

Hopefully I've configured "git format-patch" and "git send-email"
correctly and sent a better patch to the mailing list.  I'll make
sure to include the maintainers in future patches.

Cheers,
Simon.

Re: [PATCH] MIPS - fix cycle counter timing calculations

[Daniel P. Berrangé]

On Tue, Dec 14, 2021 at 12:54:05AM +1100, Simon Burge wrote:
> Hi Phil,
> 
> =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= wrote:
> 
> > Oops, missing your Signed-off-by tag, see:
> > https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-includ
> e-a-signed-off-by-line
> >
> > Do you mind re-sending with your S-o-b? Meanwhile, patch dropped.
> 
> Hopefully I've configured "git format-patch" and "git send-email"
> correctly and sent a better patch to the mailing list.  I'll make
> sure to include the maintainers in future patches.

Your v2 looks ok to me.

FWIW, if you'll be sending more patches in future, it is worth giving
'git-publish' a try. It is a higher level tool around send-email and
format-patch, that makes it much harder to make mistakes. It pretty
much just 'does the right thing' without you needing to worry, including
CC'ing the people listed in MAINTAINERS for the patch diff you have.

Regards,
Daniel

[1] https://github.com/stefanha/git-publish
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] MIPS - fix cycle counter timing calculations

[Philippe Mathieu-Daudé]

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.

Re: [PATCH v2] MIPS - fix cycle counter timing calculations

[Simon Burge]

=?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= wrote:

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

Ah, my bad.  I misread the part about using in-reply-to in the patch
submission page.

> Patch queued to mips-next, thanks.

Thanks!

Cheers,
Simon.