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

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

On 3/8/22 10:24, Peter Maydell wrote:
> 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.

That's patch 19.

> 
>> +    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 ?

Present with MPU or ECC, and we implement neither.  Perhaps these should be listed below 
with the TODO?

> 
>> +
>> +    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...

Yeah, I noticed this when cleaning things up in patch 28, but didn't want to change things 
too much in that patch.  I also don't have a no-mmu kernel to test against.

>> +        WR_FIELD(CR_TLBMISC, WR);
> 
> (the docs call this field 'WE', incidentally)

Yeah, noticed that and meant to fix it, but forgot.

>> +        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.)

Mm.  Perhaps calling it reserved is the easiest way out of that.  For these mmu registers, 
we don't apply the two masks, but pass it all to the helper.  I'm open to ideas there.

>> +        WR_FIELD(CR_TLBMISC, WAY);
> 
> Missing PID field ?

Yep, thanks.

>> +        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.)

Yep.  I could move the masking back to this patch, leave only the estatus/sstatus change 
to 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...

Hah, thanks, yes.


r~