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