Re: [PATCH 1/3] target/riscv: Save counter values during countinhibit update

[Atish Kumar Patra]

On Thu, May 2, 2024 at 5:39 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Tue, Apr 30, 2024 at 03:00:45PM GMT, Daniel Henrique Barboza wrote:
> >
> >
> > On 4/29/24 16:28, Atish Patra wrote:
> > > Currently, if a counter monitoring cycle/instret is stopped via
> > > mcountinhibit we just update the state while the value is saved
> > > during the next read. This is not accurate as the read may happen
> > > many cycles after the counter is stopped. Ideally, the read should
> > > return the value saved when the counter is stopped.
> > >
> > > Thus, save the value of the counter during the inhibit update
> > > operation and return that value during the read if corresponding bit
> > > in mcountihibit is set.
> > >
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > >   target/riscv/cpu.h     |  1 -
> > >   target/riscv/csr.c     | 32 ++++++++++++++++++++------------
> > >   target/riscv/machine.c |  1 -
> > >   3 files changed, 20 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 3b1a02b9449a..09bbf7ce9880 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -153,7 +153,6 @@ typedef struct PMUCTRState {
> > >       target_ulong mhpmcounter_prev;
> > >       /* Snapshort value of a counter in RV32 */
> > >       target_ulong mhpmcounterh_prev;
> > > -    bool started;
> > >       /* Value beyond UINT32_MAX/UINT64_MAX before overflow interrupt 
> > > trigger */
> > >       target_ulong irq_overflow_left;
> > >   } PMUCTRState;
> > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > > index 726096444fae..68ca31aff47d 100644
> > > --- a/target/riscv/csr.c
> > > +++ b/target/riscv/csr.c
> > > @@ -929,17 +929,11 @@ static RISCVException 
> > > riscv_pmu_read_ctr(CPURISCVState *env, target_ulong *val,
> > >       if (get_field(env->mcountinhibit, BIT(ctr_idx))) {
> > >           /*
> > > -         * Counter should not increment if inhibit bit is set. We can't 
> > > really
> > > -         * stop the icount counting. Just return the counter value 
> > > written by
> > > -         * the supervisor to indicate that counter was not incremented.
> > > +         * Counter should not increment if inhibit bit is set. Just 
> > > return the
> > > +         * current counter value.
> > >            */
> > > -        if (!counter->started) {
> > > -            *val = ctr_val;
> > > -            return RISCV_EXCP_NONE;
> > > -        } else {
> > > -            /* Mark that the counter has been stopped */
> > > -            counter->started = false;
> > > -        }
> > > +         *val = ctr_val;
> > > +         return RISCV_EXCP_NONE;
> > >       }
> > >       /*
> > > @@ -1973,9 +1967,23 @@ static RISCVException 
> > > write_mcountinhibit(CPURISCVState *env, int csrno,
> > >       /* Check if any other counter is also monitoring 
> > > cycles/instructions */
> > >       for (cidx = 0; cidx < RV_MAX_MHPMCOUNTERS; cidx++) {
> > > -        if (!get_field(env->mcountinhibit, BIT(cidx))) {
> > >               counter = &env->pmu_ctrs[cidx];
> > > -            counter->started = true;
> > > +        if (get_field(env->mcountinhibit, BIT(cidx)) && (val & 
> > > BIT(cidx))) {
> > > +       /*
> > > +             * Update the counter value for cycle/instret as we can't 
> > > stop the
> > > +             * host ticks. But we should show the current value at this 
> > > moment.
> > > +             */
> > > +            if (riscv_pmu_ctr_monitor_cycles(env, cidx) ||
> > > +                riscv_pmu_ctr_monitor_instructions(env, cidx)) {
> > > +                counter->mhpmcounter_val = get_ticks(false) -
> > > +                                           counter->mhpmcounter_prev +
> > > +                                           counter->mhpmcounter_val;
> > > +                if (riscv_cpu_mxl(env) == MXL_RV32) {
> > > +                    counter->mhpmcounterh_val = get_ticks(false) -
> > > +                                                
> > > counter->mhpmcounterh_prev +
> > > +                                                
> > > counter->mhpmcounterh_val;
> > > +           }
> > > +            }
> > >           }
> > >       }
> > > diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> > > index 76f2150f78b5..3e0f2dd2ce2a 100644
> > > --- a/target/riscv/machine.c
> > > +++ b/target/riscv/machine.c
> > > @@ -328,7 +328,6 @@ static const VMStateDescription vmstate_pmu_ctr_state 
> > > = {
> > >           VMSTATE_UINTTL(mhpmcounterh_val, PMUCTRState),
> > >           VMSTATE_UINTTL(mhpmcounter_prev, PMUCTRState),
> > >           VMSTATE_UINTTL(mhpmcounterh_prev, PMUCTRState),
> > > -        VMSTATE_BOOL(started, PMUCTRState),
> >
> > Unfortunately we can't remove fields from the VMStateDescription without 
> > breaking
> > migration backward compatibility. Older QEMUs will attempt to read a field 
> > that
> > doesn't exist and migration will fail.
> >
> > I'm assuming that we care about backward compat. If we're not up to this 
> > point yet
> > then we can just bump the version_id of vmstate_pmu_ctr_state and be done 
> > with it.
> > This is fine to do unless someone jumps in and complains that we broke a 
> > migration
> > case for the 'virt' board. Granted, we don't have versioned boards yet so 
> > I'm unsure
> > if someone would actually have a base to complain. Alistair, Drew, care to 
> > comment?
>
> Without versioning boards, then we shouldn't expect migrations to work for
> anything other than between QEMUs of the same version. We're delaying the
> versioning until it's reasonable to expect users to prefer to migrate
> their guests, rather than reboot them, when updating the QEMU the guests
> are running on. I'm not sure how we'll know when that is, but I think we
> can wait until somebody shouts or at least until we see that the tooling
> which makes migration easy (libvirt, etc.) is present.
>
> Regarding this patch, I'm curious what the current status is of migration.
> If we can currently migrate from a QEMU with the latest released version
> to a QEMU built from the current upstream, and then back again, then I

I haven't heard of anyone who actually uses migration in Qemu.
There is only one way to know about it when somebody complains.

I think we should just keep it simple and bump up the version  of
vmstate_pmu_ctr_state.
If somebody complains about backward compatibility, we can implement
compat code.
Otherwise, I don't see the point.

> think this patch should be written in a way to preserve that. If we
> already fail that ping-pong migration, then, as this patch doesn't make
> things worse, we might as well save ourselves from the burden of the
> compat code.
>
> Thanks,
> drew