| Message ID | 20220308072005.307955-18-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/nios2: Shadow register set, EIC and VIC | expand |
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> What's the justification for this extra machinery? Does existing guest code rely on writes to r/o bits being ignored ? -- PMM
On 3/8/22 00:57, 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> > > What's the justification for this extra machinery? Does > existing guest code rely on writes to r/o bits being ignored ? During review of a previous edition of this patch set, I asked myself: why isn't Amir changing the shadow register set on WRCTL to STATUS, as the CRS field could change. The answer is that the architecture disallows this, by making the CRS field read-only from software. CRS can only be modified by interrupt entry and return. Then I looked further and found more read-only fields, and lots of fields that are reserved unless the appropriate cpu options are enabled. Again thining of CRS more so to PRS when shadow register sets are *not* enabled -- we don't want software to put us into some weird state. I probably should have put all that in the patch description. :-) r~
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
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~
diff --git a/target/nios2/cpu.h b/target/nios2/cpu.h index 927c4aaa80..7faec97d77 100644 --- a/target/nios2/cpu.h +++ b/target/nios2/cpu.h @@ -207,6 +207,11 @@ struct CPUNios2State { int error_code; }; +typedef struct { + uint32_t writable; + uint32_t readonly; +} ControlRegState; + /** * Nios2CPU: * @env: #CPUNios2State @@ -230,9 +235,17 @@ struct Nios2CPU { uint32_t reset_addr; uint32_t exception_addr; uint32_t fast_tlb_miss_addr; + + /* Bits within each control register which are reserved or readonly. */ + ControlRegState cr_state[NUM_CR_REGS]; }; +static inline bool nios2_cr_reserved(const ControlRegState *s) +{ + return (s->writable | s->readonly) == 0; +} + void nios2_tcg_init(void); void nios2_cpu_do_interrupt(CPUState *cs); void dump_mmu(CPUNios2State *env); 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); + 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); + + if (cpu->mmu_present) { + WR_FIELD(CR_STATUS, U); + WR_FIELD(CR_STATUS, EH); + + 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); + WR_FIELD(CR_TLBMISC, RD); + WR_FIELD(CR_TLBMISC, WAY); + + WR_REG(CR_TLBACC); + } + + /* TODO: ECC and MPU not implemented. */ + +#undef WR_REG +#undef RO_REG +#undef WR_FIELD +#undef RO_FIELD + #if !defined(CONFIG_USER_ONLY) mmu_init(&cpu->env); @@ -152,23 +201,26 @@ static void nios2_cpu_disas_set_info(CPUState *cpu, disassemble_info *info) static int nios2_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n) { Nios2CPU *cpu = NIOS2_CPU(cs); - CPUClass *cc = CPU_GET_CLASS(cs); CPUNios2State *env = &cpu->env; + uint32_t val; - if (n > cc->gdb_num_core_regs) { + if (n < 32) { /* GP regs */ + val = env->regs[n]; + } else if (n == 32) { /* PC */ + val = env->pc; + } else if (n < 49) { /* Status regs */ + unsigned cr = n - 33; + if (nios2_cr_reserved(&cpu->cr_state[cr])) { + val = 0; + } else { + val = env->ctrl[n - 33]; + } + } else { + /* Invalid regs */ return 0; } - if (n < 32) { /* GP regs */ - return gdb_get_reg32(mem_buf, env->regs[n]); - } else if (n == 32) { /* PC */ - return gdb_get_reg32(mem_buf, env->pc); - } else if (n < 49) { /* Status regs */ - return gdb_get_reg32(mem_buf, env->ctrl[n - 33]); - } - - /* Invalid regs */ - return 0; + return gdb_get_reg32(mem_buf, val); } static int nios2_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) @@ -176,17 +228,25 @@ static int nios2_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n) Nios2CPU *cpu = NIOS2_CPU(cs); CPUClass *cc = CPU_GET_CLASS(cs); CPUNios2State *env = &cpu->env; + uint32_t val; if (n > cc->gdb_num_core_regs) { return 0; } + val = ldl_p(mem_buf); if (n < 32) { /* GP regs */ - env->regs[n] = ldl_p(mem_buf); + env->regs[n] = val; } else if (n == 32) { /* PC */ - env->pc = ldl_p(mem_buf); + env->pc = val; } else if (n < 49) { /* Status regs */ - env->ctrl[n - 33] = ldl_p(mem_buf); + unsigned cr = n - 33; + /* ??? Maybe allow the debugger to write to readonly fields. */ + val &= cpu->cr_state[cr].writable; + val |= cpu->cr_state[cr].readonly & env->ctrl[cr]; + env->ctrl[cr] = val; + } else { + g_assert_not_reached(); } return 4; diff --git a/target/nios2/translate.c b/target/nios2/translate.c index 77b3bf05f3..38e16df459 100644 --- a/target/nios2/translate.c +++ b/target/nios2/translate.c @@ -101,6 +101,7 @@ typedef struct DisasContext { TCGv_i32 zero; target_ulong pc; int mem_idx; + const ControlRegState *cr_state; } DisasContext; static TCGv cpu_R[NUM_GP_REGS]; @@ -452,17 +453,26 @@ static void callr(DisasContext *dc, uint32_t code, uint32_t flags) /* rC <- ctlN */ static void rdctl(DisasContext *dc, uint32_t code, uint32_t flags) { - R_TYPE(instr, code); - TCGv t1, t2; - if (!gen_check_supervisor(dc)) { return; } +#ifdef CONFIG_USER_ONLY + g_assert_not_reached(); +#else + R_TYPE(instr, code); + TCGv t1, t2; + if (unlikely(instr.c == R_ZERO)) { return; } + /* Reserved registers read as zero. */ + if (nios2_cr_reserved(&dc->cr_state[instr.imm5])) { + tcg_gen_movi_tl(cpu_R[instr.c], 0); + return; + } + switch (instr.imm5) { case CR_IPENDING: /* @@ -486,6 +496,7 @@ static void rdctl(DisasContext *dc, uint32_t code, uint32_t flags) offsetof(CPUNios2State, ctrl[instr.imm5])); break; } +#endif } /* ctlN <- rA */ @@ -500,6 +511,14 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags) #else R_TYPE(instr, code); TCGv v = load_gpr(dc, instr.a); + uint32_t ofs = offsetof(CPUNios2State, ctrl[instr.imm5]); + uint32_t wr = dc->cr_state[instr.imm5].writable; + uint32_t ro = dc->cr_state[instr.imm5].readonly; + + /* Skip reserved or readonly registers. */ + if (wr == 0) { + return; + } switch (instr.imm5) { case CR_PTEADDR: @@ -511,17 +530,35 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags) case CR_TLBMISC: gen_helper_mmu_write_tlbmisc(cpu_env, v); break; - case CR_IPENDING: - /* ipending is read only, writes ignored. */ - break; case CR_STATUS: case CR_IENABLE: /* If interrupts were enabled using WRCTL, trigger them. */ dc->base.is_jmp = DISAS_UPDATE; /* fall through */ default: - tcg_gen_st_tl(v, cpu_env, - offsetof(CPUNios2State, ctrl[instr.imm5])); + if (wr == -1) { + /* The register is entirely writable. */ + tcg_gen_st_tl(v, cpu_env, ofs); + } else { + /* + * The register is partially read-only or reserved: + * merge the value. + */ + TCGv n = tcg_temp_new(); + + tcg_gen_andi_tl(n, v, wr); + + if (ro != 0) { + TCGv o = tcg_temp_new(); + tcg_gen_ld_tl(o, cpu_env, ofs); + tcg_gen_andi_tl(o, o, ro); + tcg_gen_or_tl(n, n, o); + tcg_temp_free(o); + } + + tcg_gen_st_tl(n, cpu_env, ofs); + tcg_temp_free(n); + } break; } #endif @@ -785,9 +822,11 @@ static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) { DisasContext *dc = container_of(dcbase, DisasContext, base); CPUNios2State *env = cs->env_ptr; + Nios2CPU *cpu = env_archcpu(env); int page_insns; dc->mem_idx = cpu_mmu_index(env, false); + dc->cr_state = cpu->cr_state; /* Bound the number of insns to execute to those left on the page. */ page_insns = -(dc->base.pc_first | TARGET_PAGE_MASK) / 4; @@ -902,16 +941,25 @@ void nios2_cpu_dump_state(CPUState *cs, FILE *f, int flags) } #if !defined(CONFIG_USER_ONLY) - for (i = 0; i < NUM_CR_REGS; i++) { - qemu_fprintf(f, "%9s=%8.8x ", cr_regnames[i], env->ctrl[i]); - if ((i + 1) % 4 == 0) { - qemu_fprintf(f, "\n"); + int j; + + for (i = j = 0; i < NUM_CR_REGS; i++) { + if (!nios2_cr_reserved(&cpu->cr_state[i])) { + qemu_fprintf(f, "%9s=%8.8x ", cr_regnames[i], env->ctrl[i]); + if (++j % 4 == 0) { + qemu_fprintf(f, "\n"); + } } } - qemu_fprintf(f, " mmu write: VPN=%05X PID %02X TLBACC %08X\n", - env->mmu.pteaddr_wr & R_CR_PTEADDR_VPN_MASK, - FIELD_EX32(env->mmu.tlbmisc_wr, CR_TLBMISC, PID), - env->mmu.tlbacc_wr); + if (j % 4 != 0) { + qemu_fprintf(f, "\n"); + } + if (cpu->mmu_present) { + qemu_fprintf(f, " mmu write: VPN=%05X PID %02X TLBACC %08X\n", + env->mmu.pteaddr_wr & R_CR_PTEADDR_VPN_MASK, + FIELD_EX32(env->mmu.tlbmisc_wr, CR_TLBMISC, PID), + env->mmu.tlbacc_wr); + } #endif qemu_fprintf(f, "\n\n"); }
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> --- target/nios2/cpu.h | 13 ++++++ target/nios2/cpu.c | 90 +++++++++++++++++++++++++++++++++------- target/nios2/translate.c | 80 ++++++++++++++++++++++++++++------- 3 files changed, 152 insertions(+), 31 deletions(-)