Re: [PATCH v4 17/33] target/nios2: Prevent writes to read-only or reserved control fields

[Peter Maydell]

On Tue, 8 Mar 2022 at 07:20, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Create an array of masks which detail the writable and readonly
> bits for each control register.  Apply them when writing to
> control registers.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index f2813d3b47..189adf111c 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -88,6 +88,55 @@ static void nios2_cpu_initfn(Object *obj)
>
>      cpu_set_cpustate_pointers(cpu);
>
> +    /* Begin with all fields of all registers are reserved. */
> +    memset(cpu->cr_state, 0, sizeof(cpu->cr_state));
> +
> +    /*
> +     * The combination of writable and readonly is the set of all
> +     * non-reserved fields.  We apply writable as a mask to bits,
> +     * and merge in existing readonly bits, before storing.
> +     */
> +#define WR_REG(C)       cpu->cr_state[C].writable = -1
> +#define RO_REG(C)       cpu->cr_state[C].readonly = -1
> +#define WR_FIELD(C, F)  cpu->cr_state[C].writable |= R_##C##_##F##_MASK
> +#define RO_FIELD(C, F)  cpu->cr_state[C].readonly |= R_##C##_##F##_MASK
> +
> +    WR_FIELD(CR_STATUS, PIE);

I think you need to claim (CR_STATUS, RSIE) is a RO bit, because without
EIC it's should-be-one.

> +    WR_REG(CR_ESTATUS);
> +    WR_REG(CR_BSTATUS);
> +    RO_REG(CR_CPUID);
> +    WR_FIELD(CR_EXCEPTION, CAUSE);
> +    WR_REG(CR_BADADDR);
> +
> +    /* TODO: These control registers are not present with the EIC. */
> +    WR_REG(CR_IENABLE);
> +    RO_REG(CR_IPENDING);

Missing CR_CONFIG register ?

> +
> +    if (cpu->mmu_present) {
> +        WR_FIELD(CR_STATUS, U);
> +        WR_FIELD(CR_STATUS, EH);

True by the documentation, but we don't seem to prevent EH from
being set to 1 when we take an exception on the no-MMU config...

> +
> +        WR_FIELD(CR_PTEADDR, VPN);
> +        WR_FIELD(CR_PTEADDR, PTBASE);
> +
> +        RO_FIELD(CR_TLBMISC, D);
> +        RO_FIELD(CR_TLBMISC, PERM);
> +        RO_FIELD(CR_TLBMISC, BAD);
> +        RO_FIELD(CR_TLBMISC, DBL);
> +        WR_FIELD(CR_TLBMISC, WR);

(the docs call this field 'WE', incidentally)

> +        WR_FIELD(CR_TLBMISC, RD);

If you claim this bit to be writable you'll allow the gdbstub
to set it, which is probably not what you want. (Actual writes to
this register are handled via the helper function.)

> +        WR_FIELD(CR_TLBMISC, WAY);

Missing PID field ?

> +
> +        WR_REG(CR_TLBACC);

> +    }

You don't enforce the reserved/readonly bits on status when
we copy it from estatus during eret. (That change appears later,
in patch 26.)

The same *ought* to apply for bret, except that we have a bug in
our implementation of it, where we fail to copy bstatus into status...

The machinery itself looks OK.

thanks
-- PMM