Re: [PULL 21/30] target/riscv: Implement privilege mode filtering for cycle/instret

[Atish Kumar Patra]

On Sat, Jul 20, 2024 at 7:44 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 18 Jul 2024 at 03:12, Alistair Francis <alistair23@gmail.com> wrote:
> >
> > From: Atish Patra <atishp@rivosinc.com>
> >
> > Privilege mode filtering can also be emulated for cycle/instret by
> > tracking host_ticks/icount during each privilege mode switch. This
> > patch implements that for both cycle/instret and mhpmcounters. The
> > first one requires Smcntrpmf while the other one requires Sscofpmf
> > to be enabled.
> >
> > The cycle/instret are still computed using host ticks when icount
> > is not enabled. Otherwise, they are computed using raw icount which
> > is more accurate in icount mode.
>
> Hi; Coverity points out some possible issues with this patch
> (CID 1558459, 1558462):
>
>
> > +typedef struct PMUFixedCtrState {
> > +        /* Track cycle and icount for each privilege mode */
> > +        uint64_t counter[4];
> > +        uint64_t counter_prev[4];
> > +        /* Track cycle and icount for each privilege mode when V = 1*/
> > +        uint64_t counter_virt[2];
> > +        uint64_t counter_virt_prev[2];
>
> These two arrays are defined with size 2...
>
>
> > +static void riscv_pmu_icount_update_priv(CPURISCVState *env,
> > +                                         target_ulong newpriv, bool 
> > new_virt)
> > +{
> > +    uint64_t *snapshot_prev, *snapshot_new;
> > +    uint64_t current_icount;
> > +    uint64_t *counter_arr;
> > +    uint64_t delta;
> > +
> > +    if (icount_enabled()) {
> > +        current_icount = icount_get_raw();
> > +    } else {
> > +        current_icount = cpu_get_host_ticks();
> > +    }
> > +
> > +    if (env->virt_enabled) {
> > +        counter_arr = env->pmu_fixed_ctrs[1].counter_virt;
> > +        snapshot_prev = env->pmu_fixed_ctrs[1].counter_virt_prev;
> > +    } else {
> > +        counter_arr = env->pmu_fixed_ctrs[1].counter;
> > +        snapshot_prev = env->pmu_fixed_ctrs[1].counter_prev;
> > +    }
> > +
> > +    if (new_virt) {
> > +        snapshot_new = env->pmu_fixed_ctrs[1].counter_virt_prev;
> > +    } else {
> > +        snapshot_new = env->pmu_fixed_ctrs[1].counter_prev;
> > +    }
> > +
> > +     /*
> > +      * new_priv can be same as env->priv. So we need to calculate
> > +      * delta first before updating snapshot_new[new_priv].
> > +      */
> > +    delta = current_icount - snapshot_prev[env->priv];
> > +    snapshot_new[newpriv] = current_icount;
> > +
> > +    counter_arr[env->priv] += delta;
>
> ...and in this function we may use those counter_virt and
> counter_virt_prev arrays with newpriv and env->priv as indexes,
> but in the callsites like riscv_cpu_set_mode() the assertions on
> newpriv etc are things like
>   g_assert(newpriv <= PRV_M && newpriv != PRV_RESERVED);
>
> so Coverity thinks newpriv might be PRV_M (because that's the only
> explicit range guard it's seen) and we will overrun the array.
>
> If this is a "can't happen" case I think we should have asserts in the


Yes. Both these arrays are used when virt is enabled which can't
happen when PRV_M.
The separate array is required to distinguish between VS and HS mode
where both priv is PRV_S.

> functions like riscv_pmu_icount_update_priv() and 
> riscv_pmu_cycle_update_priv()
> that might index into these arrays that the indexes can't be out of bounds
> for these smaller arrays.
>

Sure. I will add the asserts and send a fix patch.

>
> thanks
> -- PMM