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
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.
thanks
-- PMM
