Re: [PATCH v2 2/5] [RISCV_PM] Support CSRs required for RISC-V PM extension except for ones in hypervisor mode

[Richard Henderson <richard.henderson@linaro.org>]

On 10/15/20 8:21 AM, Alexey Baturo wrote:
> +/* Functions to access Pointer Masking feature registers 
> + * We have to check if current priv lvl could modify
> + * csr in given mode
> + */
> +static int check_pm_current_disabled(CPURISCVState *env, int csrno)
> +{
> +    /* m-mode is always allowed to modify registers, so allow */
> +    if (env->priv == PRV_M) {
> +        return 0;
> +    }
> +    int csr_priv = get_field(csrno, 0xC00);
> +    /* If priv lvls differ that means we're accessing csr from higher priv lvl, so allow */
> +    if (env->priv != csr_priv) {
> +        return 0;
> +    }
> +    int cur_bit_pos = (env->priv == PRV_U) ? U_PM_CURRENT :
> +                      (env->priv == PRV_S) ? S_PM_CURRENT : -1;
> +    assert(cur_bit_pos != -1);

This is a bit clumsy.  I suggest

    int cur_bit_pos;
    switch (env->priv) {
    case PRV_M:
        return 0;
    case PRV_S:
        cur_bit_pos = S_PM_CURRENT;
        break;
    case PRV_U:
        cur_bit_pos = U_PM_CURRENT;
        break;
    default:
        g_assert_not_reached();
    }

> +static int read_mmte(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!riscv_has_ext(env, RVJ)) {
> +        *val = 0;
> +        return 0;
> +    }
> +#endif

This ifdef is wrong.  CONFIG_USER_ONLY doesn't have anything to do with what
features the cpu supports.  Nor can it be correct.  If you try to read this on
current hardware, without J, then you get an exception not zero.

I would have expected this check would actually go into the csr predicate function.

> +    *val = env->mmte & MMTE_MASK;

Surely you don't need MMTE_MASK here because you used it when writing.

That said, don't you also need to mask vs the current priv level?  There's
language in your doc that says "XS bits are available in..." and "PM bits are
available in...".

I'll also note that it says "by default only higher priv code can set the value
for PM bits", while surely it's a security bug for lower priv code to read
anything related to a higher priv.


> +    target_ulong wpri_val = val & SMTE_MASK;
> +    assert(val == wpri_val);

Incorrect assert.  This value is under guest control.  Do not crash qemu
because the guest is doing silly things.  Raise an exception if you like, and
perhaps qemu_log_mask with LOG_GUEST_ERROR.

> +    if (check_pm_current_disabled(env, csrno))
> +        return 0;

Missing braces.


r~