All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hw/intc/sifive_clint: Fix expiration time logic
       [not found] <aa466b05545e360b9a96249a659c5d1e@m101.nthu.edu.tw>
@ 2021-08-28 15:46 ` s101062801
  2021-08-29  2:27   ` [PATCH v2] " s101062801
  0 siblings, 1 reply; 4+ messages in thread
From: s101062801 @ 2021-08-28 15:46 UTC (permalink / raw)
  To: Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv, qemu-devel

After timecmp is modified, the value is converted into nanosecond,
and pass to timer_mod.  However, timer_mod perceives the value as
a signed integer.  An example that goes wrong is as follows:

OpenSBI v0.9 initializes the cold boot hart's timecmp to
0xffffffff_ffffffff.  timer_mod then interprets the product of the
following calculation as a negative value.  As a result, the clint
timer is pulled out of timerlist because it looks like it has
expired, which cause the MIP.MTIP is set before any real timer
interrupt.

Signed-off-by: Quey-Liang Kao <s101062801@m101.nthu.edu.tw>
---
  hw/intc/sifive_clint.c | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
index 0f41e5ea1c..78f01d17d3 100644
--- a/hw/intc/sifive_clint.c
+++ b/hw/intc/sifive_clint.c
@@ -44,6 +44,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, 
uint64_t value,
  {
      uint64_t next;
      uint64_t diff;
+    uint64_t now;

      uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);

@@ -59,9 +60,11 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, 
uint64_t value,
      riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
      diff = cpu->env.timecmp - rtc_r;
      /* back to ns (note args switched in muldiv64) */
-    next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-        muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
-    timer_mod(cpu->env.timer, next);
+    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    next = next + muldiv64(diff, NANOSECONDS_PER_SECOND, 
timebase_freq);
+    timer_mod(cpu->env.timer, (next <= now) ?
+                              (int64_t)((1ULL << 63) - 1) :
+                              next);
  }

  /*
--
2.32.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH v2] hw/intc/sifive_clint: Fix expiration time logic
  2021-08-28 15:46 ` [PATCH] hw/intc/sifive_clint: Fix expiration time logic s101062801
@ 2021-08-29  2:27   ` s101062801
  2021-08-30  6:25       ` Alistair Francis
  0 siblings, 1 reply; 4+ messages in thread
From: s101062801 @ 2021-08-29  2:27 UTC (permalink / raw)
  To: Alistair Francis, Bin Meng, Palmer Dabbelt, qemu-riscv, qemu-devel

After timecmp is modified, the value is converted into nanosecond,
and pass to timer_mod.  However, timer_mod perceives the value as
a signed integer.  An example that goes wrong is as follows:

OpenSBI v0.9 initializes the cold boot hart's timecmp to
0xffffffff_ffffffff.  timer_mod then interprets the product of the
following calculation as a negative value.  As a result, the clint
timer is pulled out of timerlist because it looks like it has
expired, which cause the MIP.MTIP is set before any real timer
interrupt.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/493
Signed-off-by: Quey-Liang Kao <s101062801@m101.nthu.edu.tw>
---
v2:
  - Fix the calculation for next.
  - Link to issue 493.
  - I saw David's after I made this patch.  Yet I want to correct the 
error
    in v1.
---
  hw/intc/sifive_clint.c | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
index 0f41e5ea1c..78f01d17d3 100644
--- a/hw/intc/sifive_clint.c
+++ b/hw/intc/sifive_clint.c
@@ -44,6 +44,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, 
uint64_t value,
  {
      uint64_t next;
      uint64_t diff;
+    uint64_t now;

      uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);

@@ -59,9 +60,11 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, 
uint64_t value,
      riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
      diff = cpu->env.timecmp - rtc_r;
      /* back to ns (note args switched in muldiv64) */
-    next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-        muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
-    timer_mod(cpu->env.timer, next);
+    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    next = now + muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
+    timer_mod(cpu->env.timer, (next <= now) ?
+                              (int64_t)((1ULL << 63) - 1) :
+                              next);
  }

  /*
--
2.32.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] hw/intc/sifive_clint: Fix expiration time logic
  2021-08-29  2:27   ` [PATCH v2] " s101062801
@ 2021-08-30  6:25       ` Alistair Francis
  0 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2021-08-30  6:25 UTC (permalink / raw)
  To: s101062801
  Cc: Palmer Dabbelt, Bin Meng, Alistair Francis, open list:RISC-V,
	qemu-devel@nongnu.org Developers

On Sun, Aug 29, 2021 at 12:27 PM s101062801 <s101062801@m101.nthu.edu.tw> wrote:
>
> After timecmp is modified, the value is converted into nanosecond,
> and pass to timer_mod.  However, timer_mod perceives the value as
> a signed integer.  An example that goes wrong is as follows:
>
> OpenSBI v0.9 initializes the cold boot hart's timecmp to
> 0xffffffff_ffffffff.  timer_mod then interprets the product of the
> following calculation as a negative value.  As a result, the clint
> timer is pulled out of timerlist because it looks like it has
> expired, which cause the MIP.MTIP is set before any real timer
> interrupt.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/493
> Signed-off-by: Quey-Liang Kao <s101062801@m101.nthu.edu.tw>
> ---
> v2:
>   - Fix the calculation for next.
>   - Link to issue 493.
>   - I saw David's after I made this patch.  Yet I want to correct the
> error
>     in v1.

Hello,

Thanks for the patch!

As David's was sent first I am going to apply that instead of this
one. Sorry about that.

Please feel free to send more QEMU patches in the future or to review
other people's patches.

Hopefully we will see you again :)

Alistair

> ---
>   hw/intc/sifive_clint.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
> index 0f41e5ea1c..78f01d17d3 100644
> --- a/hw/intc/sifive_clint.c
> +++ b/hw/intc/sifive_clint.c
> @@ -44,6 +44,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> uint64_t value,
>   {
>       uint64_t next;
>       uint64_t diff;
> +    uint64_t now;
>
>       uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);
>
> @@ -59,9 +60,11 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> uint64_t value,
>       riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
>       diff = cpu->env.timecmp - rtc_r;
>       /* back to ns (note args switched in muldiv64) */
> -    next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -        muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
> -    timer_mod(cpu->env.timer, next);
> +    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    next = now + muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
> +    timer_mod(cpu->env.timer, (next <= now) ?
> +                              (int64_t)((1ULL << 63) - 1) :
> +                              next);
>   }
>
>   /*
> --
> 2.32.0
>


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] hw/intc/sifive_clint: Fix expiration time logic
@ 2021-08-30  6:25       ` Alistair Francis
  0 siblings, 0 replies; 4+ messages in thread
From: Alistair Francis @ 2021-08-30  6:25 UTC (permalink / raw)
  To: s101062801
  Cc: Alistair Francis, Bin Meng, Palmer Dabbelt, open list:RISC-V,
	qemu-devel@nongnu.org Developers

On Sun, Aug 29, 2021 at 12:27 PM s101062801 <s101062801@m101.nthu.edu.tw> wrote:
>
> After timecmp is modified, the value is converted into nanosecond,
> and pass to timer_mod.  However, timer_mod perceives the value as
> a signed integer.  An example that goes wrong is as follows:
>
> OpenSBI v0.9 initializes the cold boot hart's timecmp to
> 0xffffffff_ffffffff.  timer_mod then interprets the product of the
> following calculation as a negative value.  As a result, the clint
> timer is pulled out of timerlist because it looks like it has
> expired, which cause the MIP.MTIP is set before any real timer
> interrupt.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/493
> Signed-off-by: Quey-Liang Kao <s101062801@m101.nthu.edu.tw>
> ---
> v2:
>   - Fix the calculation for next.
>   - Link to issue 493.
>   - I saw David's after I made this patch.  Yet I want to correct the
> error
>     in v1.

Hello,

Thanks for the patch!

As David's was sent first I am going to apply that instead of this
one. Sorry about that.

Please feel free to send more QEMU patches in the future or to review
other people's patches.

Hopefully we will see you again :)

Alistair

> ---
>   hw/intc/sifive_clint.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
> index 0f41e5ea1c..78f01d17d3 100644
> --- a/hw/intc/sifive_clint.c
> +++ b/hw/intc/sifive_clint.c
> @@ -44,6 +44,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> uint64_t value,
>   {
>       uint64_t next;
>       uint64_t diff;
> +    uint64_t now;
>
>       uint64_t rtc_r = cpu_riscv_read_rtc(timebase_freq);
>
> @@ -59,9 +60,11 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> uint64_t value,
>       riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
>       diff = cpu->env.timecmp - rtc_r;
>       /* back to ns (note args switched in muldiv64) */
> -    next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -        muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
> -    timer_mod(cpu->env.timer, next);
> +    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    next = now + muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
> +    timer_mod(cpu->env.timer, (next <= now) ?
> +                              (int64_t)((1ULL << 63) - 1) :
> +                              next);
>   }
>
>   /*
> --
> 2.32.0
>


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-30  7:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <aa466b05545e360b9a96249a659c5d1e@m101.nthu.edu.tw>
2021-08-28 15:46 ` [PATCH] hw/intc/sifive_clint: Fix expiration time logic s101062801
2021-08-29  2:27   ` [PATCH v2] " s101062801
2021-08-30  6:25     ` Alistair Francis
2021-08-30  6:25       ` Alistair Francis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.