[PATCH 1/2] hw/intc: Fix upper/lower mtime write calculation

[PATCH 1/2] hw/intc: Fix upper/lower mtime write calculation

[Jason Chien]

When writing the upper mtime, we should keep the original lower mtime
whose value is given by cpu_riscv_read_rtc() instead of
cpu_riscv_read_rtc_raw(). The same logic applies to writes to lower mtime.

Signed-off-by: Jason Chien <jason.chien@sifive.com>
---
 hw/intc/riscv_aclint.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
index b466a6abaf..bf77e29a70 100644
--- a/hw/intc/riscv_aclint.c
+++ b/hw/intc/riscv_aclint.c
@@ -208,11 +208,12 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
         return;
     } else if (addr == mtimer->time_base || addr == mtimer->time_base + 4) {
         uint64_t rtc_r = cpu_riscv_read_rtc_raw(mtimer->timebase_freq);
+        uint64_t rtc = cpu_riscv_read_rtc(mtimer);
 
         if (addr == mtimer->time_base) {
             if (size == 4) {
                 /* time_lo for RV32/RV64 */
-                mtimer->time_delta = ((rtc_r & ~0xFFFFFFFFULL) | value) - rtc_r;
+                mtimer->time_delta = ((rtc & ~0xFFFFFFFFULL) | value) - rtc_r;
             } else {
                 /* time for RV64 */
                 mtimer->time_delta = value - rtc_r;
@@ -220,7 +221,7 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
         } else {
             if (size == 4) {
                 /* time_hi for RV32/RV64 */
-                mtimer->time_delta = (value << 32 | (rtc_r & 0xFFFFFFFF)) - rtc_r;
+                mtimer->time_delta = (value << 32 | (rtc & 0xFFFFFFFF)) - rtc_r;
             } else {
                 qemu_log_mask(LOG_GUEST_ERROR,
                               "aclint-mtimer: invalid time_hi write: %08x",
-- 
2.17.1

[PATCH 2/2] hw/intc: Make rtc variable names consistent

[Jason Chien]

The variables whose values are given by cpu_riscv_read_rtc() should be named
"rtc". The variables whose value are given by cpu_riscv_read_rtc_raw()
should be named "rtc_r".

Signed-off-by: Jason Chien <jason.chien@sifive.com>
---
 hw/intc/riscv_aclint.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
index bf77e29a70..25cf7a5d9d 100644
--- a/hw/intc/riscv_aclint.c
+++ b/hw/intc/riscv_aclint.c
@@ -64,13 +64,13 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
     uint64_t next;
     uint64_t diff;
 
-    uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
+    uint64_t rtc = cpu_riscv_read_rtc(mtimer);
 
     /* Compute the relative hartid w.r.t the socket */
     hartid = hartid - mtimer->hartid_base;
 
     mtimer->timecmp[hartid] = value;
-    if (mtimer->timecmp[hartid] <= rtc_r) {
+    if (mtimer->timecmp[hartid] <= rtc) {
         /*
          * If we're setting an MTIMECMP value in the "past",
          * immediately raise the timer interrupt
@@ -81,7 +81,7 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
 
     /* otherwise, set up the future timer interrupt */
     qemu_irq_lower(mtimer->timer_irqs[hartid]);
-    diff = mtimer->timecmp[hartid] - rtc_r;
+    diff = mtimer->timecmp[hartid] - rtc;
     /* back to ns (note args switched in muldiv64) */
     uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
 
-- 
2.17.1

Re: [PATCH 2/2] hw/intc: Make rtc variable names consistent

[Jason Chien]

[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]

Hi,
The patch seems to be ignored. I am not sure who to ping. Could someone
please review this patch? Thank you!
patch link:
https://lore.kernel.org/qemu-devel/20230728082502.26439-2-jason.chien@sifive.com/

Jason

On Fri, Jul 28, 2023 at 4:25 PM Jason Chien <jason.chien@sifive.com> wrote:

> The variables whose values are given by cpu_riscv_read_rtc() should be
> named
> "rtc". The variables whose value are given by cpu_riscv_read_rtc_raw()
> should be named "rtc_r".
>
> Signed-off-by: Jason Chien <jason.chien@sifive.com>
> ---
>  hw/intc/riscv_aclint.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index bf77e29a70..25cf7a5d9d 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -64,13 +64,13 @@ static void
> riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>      uint64_t next;
>      uint64_t diff;
>
> -    uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
> +    uint64_t rtc = cpu_riscv_read_rtc(mtimer);
>
>      /* Compute the relative hartid w.r.t the socket */
>      hartid = hartid - mtimer->hartid_base;
>
>      mtimer->timecmp[hartid] = value;
> -    if (mtimer->timecmp[hartid] <= rtc_r) {
> +    if (mtimer->timecmp[hartid] <= rtc) {
>          /*
>           * If we're setting an MTIMECMP value in the "past",
>           * immediately raise the timer interrupt
> @@ -81,7 +81,7 @@ static void
> riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>
>      /* otherwise, set up the future timer interrupt */
>      qemu_irq_lower(mtimer->timer_irqs[hartid]);
> -    diff = mtimer->timecmp[hartid] - rtc_r;
> +    diff = mtimer->timecmp[hartid] - rtc;
>      /* back to ns (note args switched in muldiv64) */
>      uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND,
> timebase_freq);
>
> --
> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 2621 bytes --]

Re: [PATCH 1/2] hw/intc: Fix upper/lower mtime write calculation

[Jason Chien]

[-- Attachment #1: Type: text/plain, Size: 2133 bytes --]

Hi,
The patch seems to be ignored. I am not sure who to ping. Could someone
please review this patch? Thank you!
patch link:
https://lore.kernel.org/qemu-devel/20230728082502.26439-1-jason.chien@sifive.com/

On Fri, Jul 28, 2023 at 4:25 PM Jason Chien <jason.chien@sifive.com> wrote:

> When writing the upper mtime, we should keep the original lower mtime
> whose value is given by cpu_riscv_read_rtc() instead of
> cpu_riscv_read_rtc_raw(). The same logic applies to writes to lower mtime.
>
> Signed-off-by: Jason Chien <jason.chien@sifive.com>
> ---
>  hw/intc/riscv_aclint.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index b466a6abaf..bf77e29a70 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -208,11 +208,12 @@ static void riscv_aclint_mtimer_write(void *opaque,
> hwaddr addr,
>          return;
>      } else if (addr == mtimer->time_base || addr == mtimer->time_base +
> 4) {
>          uint64_t rtc_r = cpu_riscv_read_rtc_raw(mtimer->timebase_freq);
> +        uint64_t rtc = cpu_riscv_read_rtc(mtimer);
>
>          if (addr == mtimer->time_base) {
>              if (size == 4) {
>                  /* time_lo for RV32/RV64 */
> -                mtimer->time_delta = ((rtc_r & ~0xFFFFFFFFULL) | value) -
> rtc_r;
> +                mtimer->time_delta = ((rtc & ~0xFFFFFFFFULL) | value) -
> rtc_r;
>              } else {
>                  /* time for RV64 */
>                  mtimer->time_delta = value - rtc_r;
> @@ -220,7 +221,7 @@ static void riscv_aclint_mtimer_write(void *opaque,
> hwaddr addr,
>          } else {
>              if (size == 4) {
>                  /* time_hi for RV32/RV64 */
> -                mtimer->time_delta = (value << 32 | (rtc_r & 0xFFFFFFFF))
> - rtc_r;
> +                mtimer->time_delta = (value << 32 | (rtc & 0xFFFFFFFF)) -
> rtc_r;
>              } else {
>                  qemu_log_mask(LOG_GUEST_ERROR,
>                                "aclint-mtimer: invalid time_hi write:
> %08x",
> --
> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 2883 bytes --]

Re: [PATCH 1/2] hw/intc: Fix upper/lower mtime write calculation

[Alistair Francis]

On Fri, Jul 28, 2023 at 5:13 AM Jason Chien <jason.chien@sifive.com> wrote:
>
> When writing the upper mtime, we should keep the original lower mtime
> whose value is given by cpu_riscv_read_rtc() instead of
> cpu_riscv_read_rtc_raw(). The same logic applies to writes to lower mtime.
>
> Signed-off-by: Jason Chien <jason.chien@sifive.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/intc/riscv_aclint.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index b466a6abaf..bf77e29a70 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -208,11 +208,12 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
>          return;
>      } else if (addr == mtimer->time_base || addr == mtimer->time_base + 4) {
>          uint64_t rtc_r = cpu_riscv_read_rtc_raw(mtimer->timebase_freq);
> +        uint64_t rtc = cpu_riscv_read_rtc(mtimer);
>
>          if (addr == mtimer->time_base) {
>              if (size == 4) {
>                  /* time_lo for RV32/RV64 */
> -                mtimer->time_delta = ((rtc_r & ~0xFFFFFFFFULL) | value) - rtc_r;
> +                mtimer->time_delta = ((rtc & ~0xFFFFFFFFULL) | value) - rtc_r;
>              } else {
>                  /* time for RV64 */
>                  mtimer->time_delta = value - rtc_r;
> @@ -220,7 +221,7 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
>          } else {
>              if (size == 4) {
>                  /* time_hi for RV32/RV64 */
> -                mtimer->time_delta = (value << 32 | (rtc_r & 0xFFFFFFFF)) - rtc_r;
> +                mtimer->time_delta = (value << 32 | (rtc & 0xFFFFFFFF)) - rtc_r;
>              } else {
>                  qemu_log_mask(LOG_GUEST_ERROR,
>                                "aclint-mtimer: invalid time_hi write: %08x",
> --
> 2.17.1
>
>

Re: [PATCH 2/2] hw/intc: Make rtc variable names consistent

[Alistair Francis]

On Fri, Jul 28, 2023 at 4:57 AM Jason Chien <jason.chien@sifive.com> wrote:
>
> The variables whose values are given by cpu_riscv_read_rtc() should be named
> "rtc". The variables whose value are given by cpu_riscv_read_rtc_raw()
> should be named "rtc_r".
>
> Signed-off-by: Jason Chien <jason.chien@sifive.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/intc/riscv_aclint.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index bf77e29a70..25cf7a5d9d 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -64,13 +64,13 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>      uint64_t next;
>      uint64_t diff;
>
> -    uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
> +    uint64_t rtc = cpu_riscv_read_rtc(mtimer);
>
>      /* Compute the relative hartid w.r.t the socket */
>      hartid = hartid - mtimer->hartid_base;
>
>      mtimer->timecmp[hartid] = value;
> -    if (mtimer->timecmp[hartid] <= rtc_r) {
> +    if (mtimer->timecmp[hartid] <= rtc) {
>          /*
>           * If we're setting an MTIMECMP value in the "past",
>           * immediately raise the timer interrupt
> @@ -81,7 +81,7 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>
>      /* otherwise, set up the future timer interrupt */
>      qemu_irq_lower(mtimer->timer_irqs[hartid]);
> -    diff = mtimer->timecmp[hartid] - rtc_r;
> +    diff = mtimer->timecmp[hartid] - rtc;
>      /* back to ns (note args switched in muldiv64) */
>      uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
>
> --
> 2.17.1
>
>

Re: [PATCH 2/2] hw/intc: Make rtc variable names consistent

[Jason Chien]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

Ping.

On Fri, Aug 11, 2023 at 2:25 AM Alistair Francis <alistair23@gmail.com>
wrote:

> On Fri, Jul 28, 2023 at 4:57 AM Jason Chien <jason.chien@sifive.com>
> wrote:
> >
> > The variables whose values are given by cpu_riscv_read_rtc() should be
> named
> > "rtc". The variables whose value are given by cpu_riscv_read_rtc_raw()
> > should be named "rtc_r".
> >
> > Signed-off-by: Jason Chien <jason.chien@sifive.com>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
>
> > ---
> >  hw/intc/riscv_aclint.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> > index bf77e29a70..25cf7a5d9d 100644
> > --- a/hw/intc/riscv_aclint.c
> > +++ b/hw/intc/riscv_aclint.c
> > @@ -64,13 +64,13 @@ static void
> riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> >      uint64_t next;
> >      uint64_t diff;
> >
> > -    uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
> > +    uint64_t rtc = cpu_riscv_read_rtc(mtimer);
> >
> >      /* Compute the relative hartid w.r.t the socket */
> >      hartid = hartid - mtimer->hartid_base;
> >
> >      mtimer->timecmp[hartid] = value;
> > -    if (mtimer->timecmp[hartid] <= rtc_r) {
> > +    if (mtimer->timecmp[hartid] <= rtc) {
> >          /*
> >           * If we're setting an MTIMECMP value in the "past",
> >           * immediately raise the timer interrupt
> > @@ -81,7 +81,7 @@ static void
> riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> >
> >      /* otherwise, set up the future timer interrupt */
> >      qemu_irq_lower(mtimer->timer_irqs[hartid]);
> > -    diff = mtimer->timecmp[hartid] - rtc_r;
> > +    diff = mtimer->timecmp[hartid] - rtc;
> >      /* back to ns (note args switched in muldiv64) */
> >      uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND,
> timebase_freq);
> >
> > --
> > 2.17.1
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 2782 bytes --]

Re: [PATCH 2/2] hw/intc: Make rtc variable names consistent

[Alistair Francis]

On Mon, Aug 21, 2023 at 12:15 PM Jason Chien <jason.chien@sifive.com> wrote:
>
> Ping.

This has been applied to the RISC-V tree. It will go in after the QEMU
release freeze is over (probably a week or two).

Alistair

>
> On Fri, Aug 11, 2023 at 2:25 AM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Fri, Jul 28, 2023 at 4:57 AM Jason Chien <jason.chien@sifive.com> wrote:
>> >
>> > The variables whose values are given by cpu_riscv_read_rtc() should be named
>> > "rtc". The variables whose value are given by cpu_riscv_read_rtc_raw()
>> > should be named "rtc_r".
>> >
>> > Signed-off-by: Jason Chien <jason.chien@sifive.com>
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>
>> Alistair
>>
>> > ---
>> >  hw/intc/riscv_aclint.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
>> > index bf77e29a70..25cf7a5d9d 100644
>> > --- a/hw/intc/riscv_aclint.c
>> > +++ b/hw/intc/riscv_aclint.c
>> > @@ -64,13 +64,13 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>> >      uint64_t next;
>> >      uint64_t diff;
>> >
>> > -    uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
>> > +    uint64_t rtc = cpu_riscv_read_rtc(mtimer);
>> >
>> >      /* Compute the relative hartid w.r.t the socket */
>> >      hartid = hartid - mtimer->hartid_base;
>> >
>> >      mtimer->timecmp[hartid] = value;
>> > -    if (mtimer->timecmp[hartid] <= rtc_r) {
>> > +    if (mtimer->timecmp[hartid] <= rtc) {
>> >          /*
>> >           * If we're setting an MTIMECMP value in the "past",
>> >           * immediately raise the timer interrupt
>> > @@ -81,7 +81,7 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>> >
>> >      /* otherwise, set up the future timer interrupt */
>> >      qemu_irq_lower(mtimer->timer_irqs[hartid]);
>> > -    diff = mtimer->timecmp[hartid] - rtc_r;
>> > +    diff = mtimer->timecmp[hartid] - rtc;
>> >      /* back to ns (note args switched in muldiv64) */
>> >      uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
>> >
>> > --
>> > 2.17.1
>> >
>> >

Re: [PATCH 2/2] hw/intc: Make rtc variable names consistent

[Jason Chien]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

Thanks for the update.

On Tue, Aug 22, 2023 at 1:53 AM Alistair Francis <alistair23@gmail.com>
wrote:

> On Mon, Aug 21, 2023 at 12:15 PM Jason Chien <jason.chien@sifive.com>
> wrote:
> >
> > Ping.
>
> This has been applied to the RISC-V tree. It will go in after the QEMU
> release freeze is over (probably a week or two).
>
> Alistair
>
> >
> > On Fri, Aug 11, 2023 at 2:25 AM Alistair Francis <alistair23@gmail.com>
> wrote:
> >>
> >> On Fri, Jul 28, 2023 at 4:57 AM Jason Chien <jason.chien@sifive.com>
> wrote:
> >> >
> >> > The variables whose values are given by cpu_riscv_read_rtc() should
> be named
> >> > "rtc". The variables whose value are given by cpu_riscv_read_rtc_raw()
> >> > should be named "rtc_r".
> >> >
> >> > Signed-off-by: Jason Chien <jason.chien@sifive.com>
> >>
> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >>
> >> Alistair
> >>
> >> > ---
> >> >  hw/intc/riscv_aclint.c | 6 +++---
> >> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> >> > index bf77e29a70..25cf7a5d9d 100644
> >> > --- a/hw/intc/riscv_aclint.c
> >> > +++ b/hw/intc/riscv_aclint.c
> >> > @@ -64,13 +64,13 @@ static void
> riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> >> >      uint64_t next;
> >> >      uint64_t diff;
> >> >
> >> > -    uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
> >> > +    uint64_t rtc = cpu_riscv_read_rtc(mtimer);
> >> >
> >> >      /* Compute the relative hartid w.r.t the socket */
> >> >      hartid = hartid - mtimer->hartid_base;
> >> >
> >> >      mtimer->timecmp[hartid] = value;
> >> > -    if (mtimer->timecmp[hartid] <= rtc_r) {
> >> > +    if (mtimer->timecmp[hartid] <= rtc) {
> >> >          /*
> >> >           * If we're setting an MTIMECMP value in the "past",
> >> >           * immediately raise the timer interrupt
> >> > @@ -81,7 +81,7 @@ static void
> riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> >> >
> >> >      /* otherwise, set up the future timer interrupt */
> >> >      qemu_irq_lower(mtimer->timer_irqs[hartid]);
> >> > -    diff = mtimer->timecmp[hartid] - rtc_r;
> >> > +    diff = mtimer->timecmp[hartid] - rtc;
> >> >      /* back to ns (note args switched in muldiv64) */
> >> >      uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND,
> timebase_freq);
> >> >
> >> > --
> >> > 2.17.1
> >> >
> >> >
>

[-- Attachment #2: Type: text/html, Size: 3760 bytes --]