On Sat, Jul 20, 2024 at 7:44 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Thu, 18 Jul 2024 at 03:12, Alistair Francis <alistai...@gmail.com> wrote:
> >
> > From: Atish Patra <ati...@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
