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

[Andrew Jones <ajones@ventanamicro.com>]

On Thu, May 09, 2024 at 01:26:56PM GMT, Atish Kumar Patra wrote:
> 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.

Agreed.

Thanks,
drew

> 
> > 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