[PATCH 0/5] Nested virtualization fixes for QEMU

[PATCH 0/5] Nested virtualization fixes for QEMU

[Anup Patel]

This series mainly includes fixes discovered while developing nested
virtualization running on QEMU.

These patches can also be found in the riscv_nested_fixes_v1 branch at:
https://github.com/avpatel/qemu.git

Anup Patel (5):
  target/riscv: Typo fix in sstc() predicate
  target/riscv: Update VS timer whenever htimedelta changes
  target/riscv: Don't clear mask in riscv_cpu_update_mip() for VSTIP
  target/riscv: No need to re-start QEMU timer when timecmp ==
    UINT64_MAX
  target/riscv: Ensure opcode is saved for all relevant instructions

 target/riscv/cpu_helper.c                   |  2 --
 target/riscv/csr.c                          | 18 +++++++++++++++++-
 target/riscv/insn_trans/trans_rva.c.inc     | 10 +++++++---
 target/riscv/insn_trans/trans_rvd.c.inc     |  2 ++
 target/riscv/insn_trans/trans_rvf.c.inc     |  2 ++
 target/riscv/insn_trans/trans_rvh.c.inc     |  3 +++
 target/riscv/insn_trans/trans_rvi.c.inc     |  2 ++
 target/riscv/insn_trans/trans_rvzfh.c.inc   |  2 ++
 target/riscv/insn_trans/trans_svinval.c.inc |  3 +++
 target/riscv/time_helper.c                  | 20 ++++++++++++++++----
 10 files changed, 54 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH 1/5] target/riscv: Typo fix in sstc() predicate

[Anup Patel]

We should use "&&" instead of "&" when checking hcounteren.TM and
henvcfg.STCE bits.

Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp suppor")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 target/riscv/csr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 5c9a7ee287..716f9d960e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -838,7 +838,7 @@ static RISCVException sstc(CPURISCVState *env, int csrno)
     }
 
     if (riscv_cpu_virt_enabled(env)) {
-        if (!(get_field(env->hcounteren, COUNTEREN_TM) &
+        if (!(get_field(env->hcounteren, COUNTEREN_TM) &&
               get_field(env->henvcfg, HENVCFG_STCE))) {
             return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
         }
-- 
2.34.1

Re: [PATCH 1/5] target/riscv: Typo fix in sstc() predicate

[Alistair Francis]

On Fri, Oct 28, 2022 at 2:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> We should use "&&" instead of "&" when checking hcounteren.TM and
> henvcfg.STCE bits.
>
> Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp suppor")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/csr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 5c9a7ee287..716f9d960e 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -838,7 +838,7 @@ static RISCVException sstc(CPURISCVState *env, int csrno)
>      }
>
>      if (riscv_cpu_virt_enabled(env)) {
> -        if (!(get_field(env->hcounteren, COUNTEREN_TM) &
> +        if (!(get_field(env->hcounteren, COUNTEREN_TM) &&
>                get_field(env->henvcfg, HENVCFG_STCE))) {
>              return RISCV_EXCP_VIRT_INSTRUCTION_FAULT;
>          }
> --
> 2.34.1
>
>

[PATCH 2/5] target/riscv: Update VS timer whenever htimedelta changes

[Anup Patel]

The htimedelta[h] CSR has impact on the VS timer comparison so we
should call riscv_timer_write_timecmp() whenever htimedelta changes.

Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp suppor")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 target/riscv/csr.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 716f9d960e..4b1a608260 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2722,6 +2722,8 @@ static RISCVException read_htimedelta(CPURISCVState *env, int csrno,
 static RISCVException write_htimedelta(CPURISCVState *env, int csrno,
                                        target_ulong val)
 {
+    RISCVCPU *cpu = env_archcpu(env);
+
     if (!env->rdtime_fn) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
@@ -2731,6 +2733,12 @@ static RISCVException write_htimedelta(CPURISCVState *env, int csrno,
     } else {
         env->htimedelta = val;
     }
+
+    if (cpu->cfg.ext_sstc && env->rdtime_fn) {
+        riscv_timer_write_timecmp(cpu, env->vstimer, env->vstimecmp,
+                                  env->htimedelta, MIP_VSTIP);
+    }
+
     return RISCV_EXCP_NONE;
 }
 
@@ -2748,11 +2756,19 @@ static RISCVException read_htimedeltah(CPURISCVState *env, int csrno,
 static RISCVException write_htimedeltah(CPURISCVState *env, int csrno,
                                         target_ulong val)
 {
+    RISCVCPU *cpu = env_archcpu(env);
+
     if (!env->rdtime_fn) {
         return RISCV_EXCP_ILLEGAL_INST;
     }
 
     env->htimedelta = deposit64(env->htimedelta, 32, 32, (uint64_t)val);
+
+    if (cpu->cfg.ext_sstc && env->rdtime_fn) {
+        riscv_timer_write_timecmp(cpu, env->vstimer, env->vstimecmp,
+                                  env->htimedelta, MIP_VSTIP);
+    }
+
     return RISCV_EXCP_NONE;
 }
 
-- 
2.34.1

Re: [PATCH 2/5] target/riscv: Update VS timer whenever htimedelta changes

[Alistair Francis]

On Fri, Oct 28, 2022 at 2:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The htimedelta[h] CSR has impact on the VS timer comparison so we
> should call riscv_timer_write_timecmp() whenever htimedelta changes.
>
> Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp suppor")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/csr.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 716f9d960e..4b1a608260 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -2722,6 +2722,8 @@ static RISCVException read_htimedelta(CPURISCVState *env, int csrno,
>  static RISCVException write_htimedelta(CPURISCVState *env, int csrno,
>                                         target_ulong val)
>  {
> +    RISCVCPU *cpu = env_archcpu(env);
> +
>      if (!env->rdtime_fn) {
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
> @@ -2731,6 +2733,12 @@ static RISCVException write_htimedelta(CPURISCVState *env, int csrno,
>      } else {
>          env->htimedelta = val;
>      }
> +
> +    if (cpu->cfg.ext_sstc && env->rdtime_fn) {
> +        riscv_timer_write_timecmp(cpu, env->vstimer, env->vstimecmp,
> +                                  env->htimedelta, MIP_VSTIP);
> +    }
> +
>      return RISCV_EXCP_NONE;
>  }
>
> @@ -2748,11 +2756,19 @@ static RISCVException read_htimedeltah(CPURISCVState *env, int csrno,
>  static RISCVException write_htimedeltah(CPURISCVState *env, int csrno,
>                                          target_ulong val)
>  {
> +    RISCVCPU *cpu = env_archcpu(env);
> +
>      if (!env->rdtime_fn) {
>          return RISCV_EXCP_ILLEGAL_INST;
>      }
>
>      env->htimedelta = deposit64(env->htimedelta, 32, 32, (uint64_t)val);
> +
> +    if (cpu->cfg.ext_sstc && env->rdtime_fn) {
> +        riscv_timer_write_timecmp(cpu, env->vstimer, env->vstimecmp,
> +                                  env->htimedelta, MIP_VSTIP);
> +    }
> +
>      return RISCV_EXCP_NONE;
>  }
>
> --
> 2.34.1
>
>

[PATCH 3/5] target/riscv: Don't clear mask in riscv_cpu_update_mip() for VSTIP

[Anup Patel]

Instead of clearing mask in riscv_cpu_update_mip() for VSTIP, we
should call riscv_cpu_update_mip() with mask == 0 from timer_helper.c
for VSTIP.

Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp suppor")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 target/riscv/cpu_helper.c  |  2 --
 target/riscv/time_helper.c | 12 ++++++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 5d66246c2c..a403825e49 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -617,8 +617,6 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, uint64_t mask, uint64_t value)
         vsgein = (env->hgeip & (1ULL << gein)) ? MIP_VSEIP : 0;
     }
 
-    /* No need to update mip for VSTIP */
-    mask = ((mask == MIP_VSTIP) && env->vstime_irq) ? 0 : mask;
     vstip = env->vstime_irq ? MIP_VSTIP : 0;
 
     if (!qemu_mutex_iothread_locked()) {
diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
index 8cce667dfd..4fb2a471a9 100644
--- a/target/riscv/time_helper.c
+++ b/target/riscv/time_helper.c
@@ -27,7 +27,7 @@ static void riscv_vstimer_cb(void *opaque)
     RISCVCPU *cpu = opaque;
     CPURISCVState *env = &cpu->env;
     env->vstime_irq = 1;
-    riscv_cpu_update_mip(cpu, MIP_VSTIP, BOOL_TO_MASK(1));
+    riscv_cpu_update_mip(cpu, 0, BOOL_TO_MASK(1));
 }
 
 static void riscv_stimer_cb(void *opaque)
@@ -57,16 +57,20 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer,
          */
         if (timer_irq == MIP_VSTIP) {
             env->vstime_irq = 1;
+            riscv_cpu_update_mip(cpu, 0, BOOL_TO_MASK(1));
+        } else {
+            riscv_cpu_update_mip(cpu, MIP_STIP, BOOL_TO_MASK(1));
         }
-        riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(1));
         return;
     }
 
+    /* Clear the [VS|S]TIP bit in mip */
     if (timer_irq == MIP_VSTIP) {
         env->vstime_irq = 0;
+        riscv_cpu_update_mip(cpu, 0, BOOL_TO_MASK(0));
+    } else {
+        riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
     }
-    /* Clear the [V]STIP bit in mip */
-    riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
 
     /* otherwise, set up the future timer interrupt */
     diff = timecmp - rtc_r;
-- 
2.34.1

Re: [PATCH 3/5] target/riscv: Don't clear mask in riscv_cpu_update_mip() for VSTIP

[Alistair Francis]

On Fri, Oct 28, 2022 at 2:52 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> Instead of clearing mask in riscv_cpu_update_mip() for VSTIP, we
> should call riscv_cpu_update_mip() with mask == 0 from timer_helper.c
> for VSTIP.
>
> Fixes: 3ec0fe18a31f ("target/riscv: Add vstimecmp suppor")
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>

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

Alistair

> ---
>  target/riscv/cpu_helper.c  |  2 --
>  target/riscv/time_helper.c | 12 ++++++++----
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 5d66246c2c..a403825e49 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -617,8 +617,6 @@ uint64_t riscv_cpu_update_mip(RISCVCPU *cpu, uint64_t mask, uint64_t value)
>          vsgein = (env->hgeip & (1ULL << gein)) ? MIP_VSEIP : 0;
>      }
>
> -    /* No need to update mip for VSTIP */
> -    mask = ((mask == MIP_VSTIP) && env->vstime_irq) ? 0 : mask;
>      vstip = env->vstime_irq ? MIP_VSTIP : 0;
>
>      if (!qemu_mutex_iothread_locked()) {
> diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> index 8cce667dfd..4fb2a471a9 100644
> --- a/target/riscv/time_helper.c
> +++ b/target/riscv/time_helper.c
> @@ -27,7 +27,7 @@ static void riscv_vstimer_cb(void *opaque)
>      RISCVCPU *cpu = opaque;
>      CPURISCVState *env = &cpu->env;
>      env->vstime_irq = 1;
> -    riscv_cpu_update_mip(cpu, MIP_VSTIP, BOOL_TO_MASK(1));
> +    riscv_cpu_update_mip(cpu, 0, BOOL_TO_MASK(1));
>  }
>
>  static void riscv_stimer_cb(void *opaque)
> @@ -57,16 +57,20 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer,
>           */
>          if (timer_irq == MIP_VSTIP) {
>              env->vstime_irq = 1;
> +            riscv_cpu_update_mip(cpu, 0, BOOL_TO_MASK(1));
> +        } else {
> +            riscv_cpu_update_mip(cpu, MIP_STIP, BOOL_TO_MASK(1));
>          }
> -        riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(1));
>          return;
>      }
>
> +    /* Clear the [VS|S]TIP bit in mip */
>      if (timer_irq == MIP_VSTIP) {
>          env->vstime_irq = 0;
> +        riscv_cpu_update_mip(cpu, 0, BOOL_TO_MASK(0));
> +    } else {
> +        riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
>      }
> -    /* Clear the [V]STIP bit in mip */
> -    riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
>
>      /* otherwise, set up the future timer interrupt */
>      diff = timecmp - rtc_r;
> --
> 2.34.1
>
>

[PATCH 4/5] target/riscv: No need to re-start QEMU timer when timecmp == UINT64_MAX

[Anup Patel]

The time CSR will wrap-around immediately after reaching UINT64_MAX
so we don't need to re-start QEMU timer when timecmp == UINT64_MAX
in riscv_timer_write_timecmp().

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 target/riscv/time_helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
index 4fb2a471a9..1ee9f94813 100644
--- a/target/riscv/time_helper.c
+++ b/target/riscv/time_helper.c
@@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer,
         riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
     }
 
+    /*
+     * Don't re-start the QEMU timer when timecmp == UINT64_MAX because
+     * time CSR will wrap-around immediately after reaching UINT64_MAX.
+     */
+    if (timecmp == UINT64_MAX) {
+        return;
+    }
+
     /* otherwise, set up the future timer interrupt */
     diff = timecmp - rtc_r;
     /* back to ns (note args switched in muldiv64) */
-- 
2.34.1

Re: [PATCH 4/5] target/riscv: No need to re-start QEMU timer when timecmp == UINT64_MAX

[Alistair Francis]

On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> The time CSR will wrap-around immediately after reaching UINT64_MAX
> so we don't need to re-start QEMU timer when timecmp == UINT64_MAX
> in riscv_timer_write_timecmp().

I'm not clear what this is fixing?

If the guest sets a timer for UINT64_MAX shouldn't that still trigger
an event at some point?

Alistair

>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  target/riscv/time_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> index 4fb2a471a9..1ee9f94813 100644
> --- a/target/riscv/time_helper.c
> +++ b/target/riscv/time_helper.c
> @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer,
>          riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
>      }
>
> +    /*
> +     * Don't re-start the QEMU timer when timecmp == UINT64_MAX because
> +     * time CSR will wrap-around immediately after reaching UINT64_MAX.
> +     */
> +    if (timecmp == UINT64_MAX) {
> +        return;
> +    }
> +
>      /* otherwise, set up the future timer interrupt */
>      diff = timecmp - rtc_r;
>      /* back to ns (note args switched in muldiv64) */
> --
> 2.34.1
>
>

Re: [PATCH 4/5] target/riscv: No need to re-start QEMU timer when timecmp == UINT64_MAX

[Anup Patel]

On Mon, Oct 31, 2022 at 6:25 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > The time CSR will wrap-around immediately after reaching UINT64_MAX
> > so we don't need to re-start QEMU timer when timecmp == UINT64_MAX
> > in riscv_timer_write_timecmp().
>
> I'm not clear what this is fixing?
>
> If the guest sets a timer for UINT64_MAX shouldn't that still trigger
> an event at some point?

Here's what Sstc says about timer interrupt using Sstc:
"A supervisor timer interrupt becomes pending - as reflected in the
STIP bit in the mip and sip registers - whenever time contains a
value greater than or equal to stimecmp, treating the values as
unsigned integers. Writes to stimecmp are guaranteed to be
reflected in STIP eventually, but not necessarily immediately.
The interrupt remains posted until stimecmp becomes greater
than time - typically as a result of writing stimecmp."

When timecmp = UINT64_MAX, the time CSR will eventually reach
timecmp value but on next timer tick the time CSR will wrap-around
and become zero which is less than UINT64_MAX. Now, the timer
interrupt behaves like a level triggered interrupt so it will become 1
when time = timecmp = UINT64_MAX and next timer tick it will
become 0 again because time = 0 < timecmp = UINT64_MAX.

This time CSR wrap-around comparison with timecmp is natural
to implement in HW but not straight forward in QEMU hence this
patch.

Software can potentially use timecmp = UINT64_MAX as a way
to clear the timer interrupt and keep timer disabled instead of
enabling/disabling sie.STIP. This timecmp = UINT64_MAX helps:
1) Linux RISC-V timer driver keep timer interrupt enable/disable
    state in-sync with Linux interrupt subsystem.
2) Reduce number of traps taken when emulating Sstc for the
    "Nested Guest" (i.e. Guest running under some "Guest Hypervisor"
    which in-turn runs under a "Host Hypervisor").

In fact, the SBI set_timer() call also defines similar mechanism to
disable timer: "If the supervisor wishes to clear the timer interrupt
without scheduling the next timer event, it can either request a timer
interrupt infinitely far into the future (i.e., (uint64_t)-1), ...".

Regards,
Anup

>
> Alistair
>
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  target/riscv/time_helper.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> > index 4fb2a471a9..1ee9f94813 100644
> > --- a/target/riscv/time_helper.c
> > +++ b/target/riscv/time_helper.c
> > @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer,
> >          riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
> >      }
> >
> > +    /*
> > +     * Don't re-start the QEMU timer when timecmp == UINT64_MAX because
> > +     * time CSR will wrap-around immediately after reaching UINT64_MAX.
> > +     */
> > +    if (timecmp == UINT64_MAX) {
> > +        return;
> > +    }
> > +
> >      /* otherwise, set up the future timer interrupt */
> >      diff = timecmp - rtc_r;
> >      /* back to ns (note args switched in muldiv64) */
> > --
> > 2.34.1
> >
> >

Re: [PATCH 4/5] target/riscv: No need to re-start QEMU timer when timecmp == UINT64_MAX

[Alistair Francis]

On Mon, Oct 31, 2022 at 1:49 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Mon, Oct 31, 2022 at 6:25 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > The time CSR will wrap-around immediately after reaching UINT64_MAX
> > > so we don't need to re-start QEMU timer when timecmp == UINT64_MAX
> > > in riscv_timer_write_timecmp().
> >
> > I'm not clear what this is fixing?
> >
> > If the guest sets a timer for UINT64_MAX shouldn't that still trigger
> > an event at some point?
>
> Here's what Sstc says about timer interrupt using Sstc:
> "A supervisor timer interrupt becomes pending - as reflected in the
> STIP bit in the mip and sip registers - whenever time contains a
> value greater than or equal to stimecmp, treating the values as
> unsigned integers. Writes to stimecmp are guaranteed to be
> reflected in STIP eventually, but not necessarily immediately.
> The interrupt remains posted until stimecmp becomes greater
> than time - typically as a result of writing stimecmp."
>
> When timecmp = UINT64_MAX, the time CSR will eventually reach
> timecmp value but on next timer tick the time CSR will wrap-around
> and become zero which is less than UINT64_MAX. Now, the timer
> interrupt behaves like a level triggered interrupt so it will become 1
> when time = timecmp = UINT64_MAX and next timer tick it will
> become 0 again because time = 0 < timecmp = UINT64_MAX.

Ah, I didn't realise this. Can you add this to the code comment and
maybe add this description to the commit message. Otherwise:

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

Alistair

>
> This time CSR wrap-around comparison with timecmp is natural
> to implement in HW but not straight forward in QEMU hence this
> patch.
>
> Software can potentially use timecmp = UINT64_MAX as a way
> to clear the timer interrupt and keep timer disabled instead of
> enabling/disabling sie.STIP. This timecmp = UINT64_MAX helps:
> 1) Linux RISC-V timer driver keep timer interrupt enable/disable
>     state in-sync with Linux interrupt subsystem.
> 2) Reduce number of traps taken when emulating Sstc for the
>     "Nested Guest" (i.e. Guest running under some "Guest Hypervisor"
>     which in-turn runs under a "Host Hypervisor").
>
> In fact, the SBI set_timer() call also defines similar mechanism to
> disable timer: "If the supervisor wishes to clear the timer interrupt
> without scheduling the next timer event, it can either request a timer
> interrupt infinitely far into the future (i.e., (uint64_t)-1), ...".
>
> Regards,
> Anup
>
> >
> > Alistair
> >
> > >
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  target/riscv/time_helper.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> > > index 4fb2a471a9..1ee9f94813 100644
> > > --- a/target/riscv/time_helper.c
> > > +++ b/target/riscv/time_helper.c
> > > @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer,
> > >          riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
> > >      }
> > >
> > > +    /*
> > > +     * Don't re-start the QEMU timer when timecmp == UINT64_MAX because
> > > +     * time CSR will wrap-around immediately after reaching UINT64_MAX.
> > > +     */
> > > +    if (timecmp == UINT64_MAX) {
> > > +        return;
> > > +    }
> > > +
> > >      /* otherwise, set up the future timer interrupt */
> > >      diff = timecmp - rtc_r;
> > >      /* back to ns (note args switched in muldiv64) */
> > > --
> > > 2.34.1
> > >
> > >

Re: [PATCH 4/5] target/riscv: No need to re-start QEMU timer when timecmp == UINT64_MAX

[Anup Patel]

On Wed, Nov 2, 2022 at 5:40 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Oct 31, 2022 at 1:49 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Mon, Oct 31, 2022 at 6:25 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Fri, Oct 28, 2022 at 2:53 AM Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > The time CSR will wrap-around immediately after reaching UINT64_MAX
> > > > so we don't need to re-start QEMU timer when timecmp == UINT64_MAX
> > > > in riscv_timer_write_timecmp().
> > >
> > > I'm not clear what this is fixing?
> > >
> > > If the guest sets a timer for UINT64_MAX shouldn't that still trigger
> > > an event at some point?
> >
> > Here's what Sstc says about timer interrupt using Sstc:
> > "A supervisor timer interrupt becomes pending - as reflected in the
> > STIP bit in the mip and sip registers - whenever time contains a
> > value greater than or equal to stimecmp, treating the values as
> > unsigned integers. Writes to stimecmp are guaranteed to be
> > reflected in STIP eventually, but not necessarily immediately.
> > The interrupt remains posted until stimecmp becomes greater
> > than time - typically as a result of writing stimecmp."
> >
> > When timecmp = UINT64_MAX, the time CSR will eventually reach
> > timecmp value but on next timer tick the time CSR will wrap-around
> > and become zero which is less than UINT64_MAX. Now, the timer
> > interrupt behaves like a level triggered interrupt so it will become 1
> > when time = timecmp = UINT64_MAX and next timer tick it will
> > become 0 again because time = 0 < timecmp = UINT64_MAX.
>
> Ah, I didn't realise this. Can you add this to the code comment and
> maybe add this description to the commit message. Otherwise:
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Sure, I will add a detailed comment block in the code itself.

Thanks,
Anup

>
> Alistair
>
> >
> > This time CSR wrap-around comparison with timecmp is natural
> > to implement in HW but not straight forward in QEMU hence this
> > patch.
> >
> > Software can potentially use timecmp = UINT64_MAX as a way
> > to clear the timer interrupt and keep timer disabled instead of
> > enabling/disabling sie.STIP. This timecmp = UINT64_MAX helps:
> > 1) Linux RISC-V timer driver keep timer interrupt enable/disable
> >     state in-sync with Linux interrupt subsystem.
> > 2) Reduce number of traps taken when emulating Sstc for the
> >     "Nested Guest" (i.e. Guest running under some "Guest Hypervisor"
> >     which in-turn runs under a "Host Hypervisor").
> >
> > In fact, the SBI set_timer() call also defines similar mechanism to
> > disable timer: "If the supervisor wishes to clear the timer interrupt
> > without scheduling the next timer event, it can either request a timer
> > interrupt infinitely far into the future (i.e., (uint64_t)-1), ...".
> >
> > Regards,
> > Anup
> >
> > >
> > > Alistair
> > >
> > > >
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > ---
> > > >  target/riscv/time_helper.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/target/riscv/time_helper.c b/target/riscv/time_helper.c
> > > > index 4fb2a471a9..1ee9f94813 100644
> > > > --- a/target/riscv/time_helper.c
> > > > +++ b/target/riscv/time_helper.c
> > > > @@ -72,6 +72,14 @@ void riscv_timer_write_timecmp(RISCVCPU *cpu, QEMUTimer *timer,
> > > >          riscv_cpu_update_mip(cpu, timer_irq, BOOL_TO_MASK(0));
> > > >      }
> > > >
> > > > +    /*
> > > > +     * Don't re-start the QEMU timer when timecmp == UINT64_MAX because
> > > > +     * time CSR will wrap-around immediately after reaching UINT64_MAX.
> > > > +     */
> > > > +    if (timecmp == UINT64_MAX) {
> > > > +        return;
> > > > +    }
> > > > +
> > > >      /* otherwise, set up the future timer interrupt */
> > > >      diff = timecmp - rtc_r;
> > > >      /* back to ns (note args switched in muldiv64) */
> > > > --
> > > > 2.34.1
> > > >
> > > >

[PATCH 5/5] target/riscv: Ensure opcode is saved for all relevant instructions

[Anup Patel]

We should call decode_save_opc() for all relevant instructions which
can potentially generate a virtual instruction fault or a guest page
fault because generating transformed instruction upon guest page fault
expects opcode to be available. Without this, hypervisor will see
transformed instruction as zero in htinst CSR for guest MMIO emulation
which makes MMIO emulation in hypervisor slow and also breaks nested
virtualization.

Fixes: a9814e3e08d2 ("target/riscv: Minimize the calls to decode_save_opc")
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 target/riscv/insn_trans/trans_rva.c.inc     | 10 +++++++---
 target/riscv/insn_trans/trans_rvd.c.inc     |  2 ++
 target/riscv/insn_trans/trans_rvf.c.inc     |  2 ++
 target/riscv/insn_trans/trans_rvh.c.inc     |  3 +++
 target/riscv/insn_trans/trans_rvi.c.inc     |  2 ++
 target/riscv/insn_trans/trans_rvzfh.c.inc   |  2 ++
 target/riscv/insn_trans/trans_svinval.c.inc |  3 +++
 7 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rva.c.inc b/target/riscv/insn_trans/trans_rva.c.inc
index 45db82c9be..5f194a447b 100644
--- a/target/riscv/insn_trans/trans_rva.c.inc
+++ b/target/riscv/insn_trans/trans_rva.c.inc
@@ -20,8 +20,10 @@
 
 static bool gen_lr(DisasContext *ctx, arg_atomic *a, MemOp mop)
 {
-    TCGv src1 = get_address(ctx, a->rs1, 0);
+    TCGv src1;
 
+    decode_save_opc(ctx);
+    src1 = get_address(ctx, a->rs1, 0);
     if (a->rl) {
         tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
     }
@@ -43,6 +45,7 @@ static bool gen_sc(DisasContext *ctx, arg_atomic *a, MemOp mop)
     TCGLabel *l1 = gen_new_label();
     TCGLabel *l2 = gen_new_label();
 
+    decode_save_opc(ctx);
     src1 = get_address(ctx, a->rs1, 0);
     tcg_gen_brcond_tl(TCG_COND_NE, load_res, src1, l1);
 
@@ -81,9 +84,10 @@ static bool gen_amo(DisasContext *ctx, arg_atomic *a,
                     MemOp mop)
 {
     TCGv dest = dest_gpr(ctx, a->rd);
-    TCGv src1 = get_address(ctx, a->rs1, 0);
-    TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
+    TCGv src1, src2 = get_gpr(ctx, a->rs2, EXT_NONE);
 
+    decode_save_opc(ctx);
+    src1 = get_address(ctx, a->rs1, 0);
     func(dest, src1, src2, ctx->mem_idx, mop);
 
     gen_set_gpr(ctx, a->rd, dest);
diff --git a/target/riscv/insn_trans/trans_rvd.c.inc b/target/riscv/insn_trans/trans_rvd.c.inc
index 1397c1ce1c..6e3159b797 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -38,6 +38,7 @@ static bool trans_fld(DisasContext *ctx, arg_fld *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVD);
 
+    decode_save_opc(ctx);
     addr = get_address(ctx, a->rs1, a->imm);
     tcg_gen_qemu_ld_i64(cpu_fpr[a->rd], addr, ctx->mem_idx, MO_TEUQ);
 
@@ -52,6 +53,7 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVD);
 
+    decode_save_opc(ctx);
     addr = get_address(ctx, a->rs1, a->imm);
     tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], addr, ctx->mem_idx, MO_TEUQ);
     return true;
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc
index a1d3eb52ad..965e1f8d11 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -38,6 +38,7 @@ static bool trans_flw(DisasContext *ctx, arg_flw *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVF);
 
+    decode_save_opc(ctx);
     addr = get_address(ctx, a->rs1, a->imm);
     dest = cpu_fpr[a->rd];
     tcg_gen_qemu_ld_i64(dest, addr, ctx->mem_idx, MO_TEUL);
@@ -54,6 +55,7 @@ static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
     REQUIRE_FPU;
     REQUIRE_EXT(ctx, RVF);
 
+    decode_save_opc(ctx);
     addr = get_address(ctx, a->rs1, a->imm);
     tcg_gen_qemu_st_i64(cpu_fpr[a->rs2], addr, ctx->mem_idx, MO_TEUL);
     return true;
diff --git a/target/riscv/insn_trans/trans_rvh.c.inc b/target/riscv/insn_trans/trans_rvh.c.inc
index 4f8aecddc7..9248b48c36 100644
--- a/target/riscv/insn_trans/trans_rvh.c.inc
+++ b/target/riscv/insn_trans/trans_rvh.c.inc
@@ -36,6 +36,7 @@ static bool do_hlv(DisasContext *ctx, arg_r2 *a, MemOp mop)
 #ifdef CONFIG_USER_ONLY
     return false;
 #else
+    decode_save_opc(ctx);
     if (check_access(ctx)) {
         TCGv dest = dest_gpr(ctx, a->rd);
         TCGv addr = get_gpr(ctx, a->rs1, EXT_NONE);
@@ -82,6 +83,7 @@ static bool do_hsv(DisasContext *ctx, arg_r2_s *a, MemOp mop)
 #ifdef CONFIG_USER_ONLY
     return false;
 #else
+    decode_save_opc(ctx);
     if (check_access(ctx)) {
         TCGv addr = get_gpr(ctx, a->rs1, EXT_NONE);
         TCGv data = get_gpr(ctx, a->rs2, EXT_NONE);
@@ -135,6 +137,7 @@ static bool trans_hsv_d(DisasContext *ctx, arg_hsv_d *a)
 static bool do_hlvx(DisasContext *ctx, arg_r2 *a,
                     void (*func)(TCGv, TCGv_env, TCGv))
 {
+    decode_save_opc(ctx);
     if (check_access(ctx)) {
         TCGv dest = dest_gpr(ctx, a->rd);
         TCGv addr = get_gpr(ctx, a->rs1, EXT_NONE);
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index c49dbec0eb..1665efb639 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -261,6 +261,7 @@ static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
 
 static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop)
 {
+    decode_save_opc(ctx);
     if (get_xl(ctx) == MXL_RV128) {
         return gen_load_i128(ctx, a, memop);
     } else {
@@ -350,6 +351,7 @@ static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
 
 static bool gen_store(DisasContext *ctx, arg_sb *a, MemOp memop)
 {
+    decode_save_opc(ctx);
     if (get_xl(ctx) == MXL_RV128) {
         return gen_store_i128(ctx, a, memop);
     } else {
diff --git a/target/riscv/insn_trans/trans_rvzfh.c.inc b/target/riscv/insn_trans/trans_rvzfh.c.inc
index 5d07150cd0..2ad5716312 100644
--- a/target/riscv/insn_trans/trans_rvzfh.c.inc
+++ b/target/riscv/insn_trans/trans_rvzfh.c.inc
@@ -49,6 +49,7 @@ static bool trans_flh(DisasContext *ctx, arg_flh *a)
     REQUIRE_FPU;
     REQUIRE_ZFH_OR_ZFHMIN(ctx);
 
+    decode_save_opc(ctx);
     t0 = get_gpr(ctx, a->rs1, EXT_NONE);
     if (a->imm) {
         TCGv temp = temp_new(ctx);
@@ -71,6 +72,7 @@ static bool trans_fsh(DisasContext *ctx, arg_fsh *a)
     REQUIRE_FPU;
     REQUIRE_ZFH_OR_ZFHMIN(ctx);
 
+    decode_save_opc(ctx);
     t0 = get_gpr(ctx, a->rs1, EXT_NONE);
     if (a->imm) {
         TCGv temp = tcg_temp_new();
diff --git a/target/riscv/insn_trans/trans_svinval.c.inc b/target/riscv/insn_trans/trans_svinval.c.inc
index 2682bd969f..f3cd7d5c0b 100644
--- a/target/riscv/insn_trans/trans_svinval.c.inc
+++ b/target/riscv/insn_trans/trans_svinval.c.inc
@@ -28,6 +28,7 @@ static bool trans_sinval_vma(DisasContext *ctx, arg_sinval_vma *a)
     /* Do the same as sfence.vma currently */
     REQUIRE_EXT(ctx, RVS);
 #ifndef CONFIG_USER_ONLY
+    decode_save_opc(ctx);
     gen_helper_tlb_flush(cpu_env);
     return true;
 #endif
@@ -56,6 +57,7 @@ static bool trans_hinval_vvma(DisasContext *ctx, arg_hinval_vvma *a)
     /* Do the same as hfence.vvma currently */
     REQUIRE_EXT(ctx, RVH);
 #ifndef CONFIG_USER_ONLY
+    decode_save_opc(ctx);
     gen_helper_hyp_tlb_flush(cpu_env);
     return true;
 #endif
@@ -68,6 +70,7 @@ static bool trans_hinval_gvma(DisasContext *ctx, arg_hinval_gvma *a)
     /* Do the same as hfence.gvma currently */
     REQUIRE_EXT(ctx, RVH);
 #ifndef CONFIG_USER_ONLY
+    decode_save_opc(ctx);
     gen_helper_hyp_gvma_tlb_flush(cpu_env);
     return true;
 #endif
-- 
2.34.1