On Thu, May 2, 2024 at 5:39 AM Andrew Jones <ajo...@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 <ati...@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
