Richard, again thanks for the review!

> This is a bit clumsy.  I suggest
Sure, will fix.

> If you try to read this on current hardware, without J, then you get an
exception not zero.
If I get the spec right, the idea is to read 0 from PM CSRs even if this
extension is not present.

>I would have expected this check would actually go into the csr predicate
function.
If I understand your proposal correctly, in this case I'd have to introduce
3 new functions for S/M/U PM CSRs as a predicate. I'm okay with that if you
suggest so

>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?
I suppose that applying these masks helps to prepare the correct value for
the given priv level. So if I understand correctly if you're in *M-mode*
and try to read *UMTE*, you'll get only *U.Enable* and *U.Current*, while
if you'd try to read *SMTE*, you'll also get *XS* bits, *S.Enable* and
*S.Current*.

> it's a security bug for lower priv code to read anything related to a
higher priv.
Could you please elaborate a bit more here? I don't see how this scenario
is valid: in *U-mode *you're supposed to be able to read only *U* *registers,
while *M*/*S *mode could allow *U-mode *to write to its *U** registers.
As for *S-mode*, I believe it's allowed to write to any *U** registers and
it's available to write to *S** registers if *S.Current* is set.

> Do not crash qemu because the guest is doing silly things
Sure, you're right. Will fix.

>Raise an exception if you like, and perhaps qemu_log_mask with
LOG_GUEST_ERROR
I think raising exception here might be too much, but logging the error is
a great idea, thanks!

Thanks!

чт, 15 окт. 2020 г. в 19:48, 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~
>