diff mbox series

MIPS - fix cycle counter timing calculations

Message ID 20211116072606.BE9C8A1856@thoreau.thistledown.com.au (mailing list archive)
State New, archived
Headers show
Series MIPS - fix cycle counter timing calculations | expand

Commit Message

Simon Burge Nov. 16, 2021, 7:26 a.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Dec. 13, 2021, 10:10 a.m. UTC | #1
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.
Philippe Mathieu-Daudé Dec. 13, 2021, 10:23 a.m. UTC | #2
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.
Daniel P. Berrangé Dec. 13, 2021, 2:36 p.m. UTC | #3
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
diff mbox series

Patch

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