| Message ID | 20200825205950.730499-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | target/microblaze improvements | expand |
On Tue, Aug 25, 2020 at 01:58:33PM -0700, Richard Henderson wrote: > Well, this is larger than I expected. > Wow, thanks for working on this Richard! I'll run some tests on it and go through the patches. Thanks, Edgar > I started off thinking conversion to decodetree would be quick, > after I reviewed the mttcg patches last week. Then I realized > that this could also use conversion to the generic translation loop. > Then I realized that there were a number of bugs, and some > inefficiencies, that could be fixed by using tcg exception unwind > properly. > > Hopefully most of these are small and easy to understand. > > I begin by adding enough stuff to test/tcg to make use of a > self-built musl cross-environment. I'll note that linuxtest > does not pass before or after this patch set -- I think that > linux-user/microblaze/signal.c needs work -- but that the > other tests do work. > > I also have an old copy of a microblaze system test image, > which I think used to be hosted on our wiki. After basic kernel > boot, it contains a "selftest" script which runs a bunch of > user-land isa tests. That still works fine too. > > HOWEVER: That's not nearly complete. There are cpu config options > that aren't default and I don't know how to change or test. > > In addition, the manual is really not clear on what's supposed to > happen under various edge conditions, especially with MSR[EE] unset. > E.g. unaligned access: Does the insn get nop-ed out? Do the low > bits of the address get ignored? Right now (before and after) the > access simply happens unaligned, which doesn't seem correct. > I assume the reason for having this configure option is to reduce > the size of the FPGA so that the unaligned access handling hw > simply isn't present. > > Lemme know what you think. > > > r~ > > > Richard Henderson (77): > tests/tcg: Add microblaze to arches filter > tests/tcg: Do not require FE_TOWARDZERO > tests/tcg: Do not require FE_* exception bits > target/microblaze: Tidy gdbstub > target/microblaze: Split out PC from env->sregs > target/microblaze: Split out MSR from env->sregs > target/microblaze: Split out EAR from env->sregs > target/microblaze: Split out ESR from env->sregs > target/microblaze: Split out FSR from env->sregs > target/microblaze: Split out BTR from env->sregs > target/microblaze: Split out EDR from env->sregs > target/microblaze: Split the cpu_SR array > target/microblaze: Fix width of PC and BTARGET > target/microblaze: Fix width of MSR > target/microblaze: Fix width of ESR > target/microblaze: Fix width of FSR > target/microblaze: Fix width of BTR > target/microblaze: Fix width of EDR > target/microblaze: Remove cpu_ear > target/microblaze: Tidy raising of exceptions > target/microblaze: Mark raise_exception as noreturn > target/microblaze: Remove helper_debug and env->debug > target/microblaze: Rename env_* tcg variables to cpu_* > target/microblaze: Tidy mb_tcg_init > target/microblaze: Split out MSR[C] to its own variable > target/microblaze: Use DISAS_NORETURN > target/microblaze: Check singlestep_enabled in gen_goto_tb > target/microblaze: Convert to DisasContextBase > target/microblaze: Convert to translator_loop > target/microblaze: Remove SIM_COMPAT > target/microblaze: Remove DISAS_GNU > target/microblaze: Remove empty D macros > target/microblaze: Remove LOG_DIS > target/microblaze: Ensure imm constant is always available > target/microblaze: Add decodetree infrastructure > target/microblaze: Convert dec_add to decodetree > target/microblaze: Convert dec_sub to decodetree > target/microblaze: Implement cmp and cmpu inline > target/microblaze: Convert dec_pattern to decodetree > target/microblaze: Convert dec_and, dec_or, dec_xor to decodetree > target/microblaze: Convert dec_mul to decodetree > target/microblaze: Convert dec_div to decodetree > target/microblaze: Unwind properly when raising divide-by-zero > target/microblaze: Convert dec_bit to decodetree > target/microblaze: Convert dec_barrel to decodetree > target/microblaze: Convert dec_imm to decodetree > target/microblaze: Convert dec_fpu to decodetree > target/microblaze: Fix cpu unwind for fpu exceptions > target/microblaze: Mark fpu helpers TCG_CALL_NO_WG > target/microblaze: Replace MSR_EE_FLAG with MSR_EE > target/microblaze: Cache mem_index in DisasContext > target/microblaze: Fix cpu unwind for stackprot > target/microblaze: Convert dec_load and dec_store to decodetree > target/microblaze: Assert no overlap in flags making up tb_flags > target/microblaze: Move bimm to BIMM_FLAG > target/microblaze: Store "current" iflags in insn_start > tcg: Add tcg_get_insn_start_param > target/microblaze: Use cc->do_unaligned_access > target/microblaze: Replace clear_imm with tb_flags_to_set > target/microblaze: Replace delayed_branch with tb_flags_to_set > target/microblaze: Tidy mb_cpu_dump_state > target/microblaze: Try to keep imm and delay slot together > target/microblaze: Convert brk and brki to decodetree > target/microblaze: Convert mbar to decodetree > target/microblaze: Reorganize branching > target/microblaze: Use tcg_gen_lookup_and_goto_ptr > target/microblaze: Convert dec_br to decodetree > target/microblaze: Convert dec_bcc to decodetree > target/microblaze: Convert dec_rts to decodetree > target/microblaze: Tidy do_rti, do_rtb, do_rte > target/microblaze: Convert msrclr, msrset to decodetree > target/microblaze: Convert dec_msr to decodetree > target/microblaze: Convert dec_stream to decodetree > target/microblaze: Remove last of old decoder > target/microblaze: Remove cpu_R[0] > target/microblaze: Add flags markup to some helpers > target/microblaze: Reduce linux-user address space to 32-bit > > include/tcg/tcg.h | 15 + > target/microblaze/cpu-param.h | 15 + > target/microblaze/cpu.h | 67 +- > target/microblaze/helper.h | 49 +- > tests/tcg/multiarch/float_helpers.h | 17 + > target/microblaze/insns.decode | 253 +++ > linux-user/elfload.c | 9 +- > linux-user/microblaze/cpu_loop.c | 26 +- > linux-user/microblaze/signal.c | 8 +- > target/microblaze/cpu.c | 9 +- > target/microblaze/gdbstub.c | 193 +- > target/microblaze/helper.c | 164 +- > target/microblaze/mmu.c | 4 +- > target/microblaze/op_helper.c | 175 +- > target/microblaze/translate.c | 3250 ++++++++++++++------------- > tests/tcg/multiarch/float_convs.c | 2 + > tests/tcg/multiarch/float_madds.c | 2 + > target/microblaze/meson.build | 3 + > tests/tcg/configure.sh | 2 +- > 19 files changed, 2309 insertions(+), 1954 deletions(-) > create mode 100644 target/microblaze/insns.decode > > -- > 2.25.1 >
On Tue, Aug 25, 2020 at 01:58:33PM -0700, Richard Henderson wrote: > Well, this is larger than I expected. > > I started off thinking conversion to decodetree would be quick, > after I reviewed the mttcg patches last week. Then I realized > that this could also use conversion to the generic translation loop. > Then I realized that there were a number of bugs, and some > inefficiencies, that could be fixed by using tcg exception unwind > properly. > > Hopefully most of these are small and easy to understand. > > I begin by adding enough stuff to test/tcg to make use of a > self-built musl cross-environment. I'll note that linuxtest > does not pass before or after this patch set -- I think that > linux-user/microblaze/signal.c needs work -- but that the > other tests do work. > > I also have an old copy of a microblaze system test image, > which I think used to be hosted on our wiki. After basic kernel > boot, it contains a "selftest" script which runs a bunch of > user-land isa tests. That still works fine too. > > HOWEVER: That's not nearly complete. There are cpu config options > that aren't default and I don't know how to change or test. > > In addition, the manual is really not clear on what's supposed to > happen under various edge conditions, especially with MSR[EE] unset. > E.g. unaligned access: Does the insn get nop-ed out? Do the low > bits of the address get ignored? Right now (before and after) the > access simply happens unaligned, which doesn't seem correct. > I assume the reason for having this configure option is to reduce > the size of the FPGA so that the unaligned access handling hw > simply isn't present. Yes, I verified with the RTL designers and this particular case will result in the core issuing the load/store with the lower address bits ignored. Cheers, Edgar
On Tue, Aug 25, 2020 at 01:58:33PM -0700, Richard Henderson wrote: > Well, this is larger than I expected. > > I started off thinking conversion to decodetree would be quick, > after I reviewed the mttcg patches last week. Then I realized > that this could also use conversion to the generic translation loop. > Then I realized that there were a number of bugs, and some > inefficiencies, that could be fixed by using tcg exception unwind > properly. > > Hopefully most of these are small and easy to understand. > > I begin by adding enough stuff to test/tcg to make use of a > self-built musl cross-environment. I'll note that linuxtest > does not pass before or after this patch set -- I think that > linux-user/microblaze/signal.c needs work -- but that the > other tests do work. > > I also have an old copy of a microblaze system test image, > which I think used to be hosted on our wiki. After basic kernel > boot, it contains a "selftest" script which runs a bunch of > user-land isa tests. That still works fine too. > > HOWEVER: That's not nearly complete. There are cpu config options > that aren't default and I don't know how to change or test. > > In addition, the manual is really not clear on what's supposed to > happen under various edge conditions, especially with MSR[EE] unset. > E.g. unaligned access: Does the insn get nop-ed out? Do the low > bits of the address get ignored? Right now (before and after) the > access simply happens unaligned, which doesn't seem correct. > I assume the reason for having this configure option is to reduce > the size of the FPGA so that the unaligned access handling hw > simply isn't present. > > Lemme know what you think. I merged this to our internal tree and run some of our tests, unfortunately it causes quite a few regressions. I'll try to get you details as I go and respond to individual patches if related otherwise here. So a first general regression is that opcode 0 no longer traps as an illegal instruction (seems to be treated as an add with all r0). Cheers, Edgar
On 8/27/20 2:11 AM, Edgar E. Iglesias wrote: > So a first general regression is that opcode 0 no longer > traps as an illegal instruction (seems to be treated as an > add with all r0). Oops, will fix. r~
On Thu, Aug 27, 2020 at 03:01:57AM -0700, Richard Henderson wrote: > On 8/27/20 2:11 AM, Edgar E. Iglesias wrote: > > So a first general regression is that opcode 0 no longer > > traps as an illegal instruction (seems to be treated as an > > add with all r0). > > Oops, will fix. Thanks. Here's another issue, it seems some branches are jumping to the wrong address. This is a disasm from a failing case: 0x00000000ffd033a0: brlid r15, -636 // 0xffffffffffd03124 0x00000000ffd033a4: or r0, r0, r0 0x00000000ffa73124: Address 0xffa73124 is out of bounds. 0x00000000ffa73128: Address 0xffa73128 is out of bounds. This one is from a working one: 0x00000000ffd033a0: brlid r15, -636 // 0xffffffffffd03124 0x00000000ffd033a4: or r0, r0, r0 -------------- 0x00000000ffd03124: imm -40 0x00000000ffd03128: lwi r3, r0, 268 0x00000000ffd0312c: imm -40 0x00000000ffd03130: lwi r4, r0, 256 0x00000000ffd03134: srl r3, r3 0x00000000ffd03138: bsrli r4, r4, 23 0x00000000ffd0313c: andi r3, r3, 1 0x00000000ffd03140: rtsd r15, 8 0x00000000ffd03144: and r3, r4, r3
On 8/27/20 3:22 AM, Edgar E. Iglesias wrote: > Thanks. Here's another issue, it seems some branches are jumping > to the wrong address. > > This is a disasm from a failing case: > > 0x00000000ffd033a0: brlid r15, -636 // 0xffffffffffd03124 > 0x00000000ffd033a4: or r0, r0, r0 > > 0x00000000ffa73124: Address 0xffa73124 is out of bounds. That's a weird one. My guess is that IMM_FLAG is set in iflags incorrectly. Can you verify this with -d in_asm,op,exec? When IMM_FLAG is set, you'll see in in iflags: bit 0 will be set in the second word of the insn_data. E.g.: ---- 00000000ffd033a0 0000000000000001 It would also show up in the tb_flags of the exec lines. E.g. Trace 0: 0x7f38a4000940 [0000000000000000/0000000090000058/0] where the format is host_pc [cs_base/pc/tb_flags]. If so, then we'll need to check where iflags got out of sync. r~
On Thu, Aug 27, 2020 at 04:19:44AM -0700, Richard Henderson wrote: > On 8/27/20 3:22 AM, Edgar E. Iglesias wrote: > > Thanks. Here's another issue, it seems some branches are jumping > > to the wrong address. > > > > This is a disasm from a failing case: > > > > 0x00000000ffd033a0: brlid r15, -636 // 0xffffffffffd03124 > > 0x00000000ffd033a4: or r0, r0, r0 > > > > 0x00000000ffa73124: Address 0xffa73124 is out of bounds. > > That's a weird one. > > My guess is that IMM_FLAG is set in iflags incorrectly. > Can you verify this with -d in_asm,op,exec? > > When IMM_FLAG is set, you'll see in in iflags: bit 0 will be set in the second > word of the insn_data. E.g.: > > ---- 00000000ffd033a0 0000000000000001 > > It would also show up in the tb_flags of the exec lines. E.g. > > Trace 0: 0x7f38a4000940 [0000000000000000/0000000090000058/0] > > where the format is host_pc [cs_base/pc/tb_flags]. > > > If so, then we'll need to check where iflags got out of sync. > It seems to be getting out of sync when getting a slave-error and the core is not setup to take exceptions for slave errors. Looks like a pre-existing bug where we're restoring CPU state without taking the exception. The following fixes that particular case in my runs. I'm on a backported QEMU 5.1 so thing may look different in master. diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c index 831ff2cac1..0cae51c2df 100644 --- a/target/microblaze/op_helper.c +++ b/target/microblaze/op_helper.c @@ -432,22 +432,19 @@ void mb_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr, cpu = MICROBLAZE_CPU(cs); env = &cpu->env; - cpu_restore_state(cs, retaddr, true); - if (!(env->msr & MSR_EE)) { + if (!cpu->cfg.iopb_bus_exception || !(env->msr & MSR_EE)) { return; } + cpu_restore_state(cs, retaddr, true); + env->ear = addr; if (access_type == MMU_INST_FETCH) { - if ((env->pvr.regs[2] & PVR2_IOPB_BUS_EXC_MASK)) { - env->esr = ESR_EC_INSN_BUS; - helper_raise_exception(env, EXCP_HW_EXCP); - } + env->esr = ESR_EC_INSN_BUS; + helper_raise_exception(env, EXCP_HW_EXCP); } else { - if ((env->pvr.regs[2] & PVR2_DOPB_BUS_EXC_MASK)) { - env->esr = ESR_EC_DATA_BUS; - helper_raise_exception(env, EXCP_HW_EXCP); - } + env->esr = ESR_EC_DATA_BUS; + helper_raise_exception(env, EXCP_HW_EXCP); } } #endif
On Tue, Aug 25, 2020 at 01:58:33PM -0700, Richard Henderson wrote: > Well, this is larger than I expected. > > I started off thinking conversion to decodetree would be quick, > after I reviewed the mttcg patches last week. Then I realized > that this could also use conversion to the generic translation loop. > Then I realized that there were a number of bugs, and some > inefficiencies, that could be fixed by using tcg exception unwind > properly. > > Hopefully most of these are small and easy to understand. > > I begin by adding enough stuff to test/tcg to make use of a > self-built musl cross-environment. I'll note that linuxtest > does not pass before or after this patch set -- I think that > linux-user/microblaze/signal.c needs work -- but that the > other tests do work. > > I also have an old copy of a microblaze system test image, > which I think used to be hosted on our wiki. After basic kernel > boot, it contains a "selftest" script which runs a bunch of > user-land isa tests. That still works fine too. > > HOWEVER: That's not nearly complete. There are cpu config options > that aren't default and I don't know how to change or test. > > In addition, the manual is really not clear on what's supposed to > happen under various edge conditions, especially with MSR[EE] unset. > E.g. unaligned access: Does the insn get nop-ed out? Do the low > bits of the address get ignored? Right now (before and after) the > access simply happens unaligned, which doesn't seem correct. > I assume the reason for having this configure option is to reduce > the size of the FPGA so that the unaligned access handling hw > simply isn't present. > > Lemme know what you think. > It looks like our tests pass after adressing the issues I've mentioned so far. Don't know whats going on with the tcg_gen_lookup_and_goto_ptr issue, I'll double-check next week to make sure I didn't mess something up. Thanks, Edgar
On 8/27/20 10:09 AM, Edgar E. Iglesias wrote: > It seems to be getting out of sync when getting a slave-error and the core > is not setup to take exceptions for slave errors. Looks like a pre-existing > bug where we're restoring CPU state without taking the exception. > The following fixes that particular case in my runs. > > > I'm on a backported QEMU 5.1 so thing may look different in master. > > diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c > index 831ff2cac1..0cae51c2df 100644 > --- a/target/microblaze/op_helper.c > +++ b/target/microblaze/op_helper.c > @@ -432,22 +432,19 @@ void mb_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr, > cpu = MICROBLAZE_CPU(cs); > env = &cpu->env; > > - cpu_restore_state(cs, retaddr, true); > - if (!(env->msr & MSR_EE)) { > + if (!cpu->cfg.iopb_bus_exception || !(env->msr & MSR_EE)) { > return; > } > > + cpu_restore_state(cs, retaddr, true); > + > env->ear = addr; > if (access_type == MMU_INST_FETCH) { > - if ((env->pvr.regs[2] & PVR2_IOPB_BUS_EXC_MASK)) { > - env->esr = ESR_EC_INSN_BUS; > - helper_raise_exception(env, EXCP_HW_EXCP); > - } > + env->esr = ESR_EC_INSN_BUS; > + helper_raise_exception(env, EXCP_HW_EXCP); > } else { > - if ((env->pvr.regs[2] & PVR2_DOPB_BUS_EXC_MASK)) { > - env->esr = ESR_EC_DATA_BUS; > - helper_raise_exception(env, EXCP_HW_EXCP); > - } > + env->esr = ESR_EC_DATA_BUS; > + helper_raise_exception(env, EXCP_HW_EXCP); > } > } Thanks for the pointer. I've re-written this section to use cpu_loop_exit_restore(), so that the restore is at the end. The new patch will appear in v2, just before iflags is added to the restore state. r~
On Fri, 28 Aug 2020, 15:36 Richard Henderson, <richard.henderson@linaro.org> wrote: > On 8/27/20 10:09 AM, Edgar E. Iglesias wrote: > > It seems to be getting out of sync when getting a slave-error and the > core > > is not setup to take exceptions for slave errors. Looks like a > pre-existing > > bug where we're restoring CPU state without taking the exception. > > The following fixes that particular case in my runs. > > > > > > I'm on a backported QEMU 5.1 so thing may look different in master. > > > > diff --git a/target/microblaze/op_helper.c > b/target/microblaze/op_helper.c > > index 831ff2cac1..0cae51c2df 100644 > > --- a/target/microblaze/op_helper.c > > +++ b/target/microblaze/op_helper.c > > @@ -432,22 +432,19 @@ void mb_cpu_transaction_failed(CPUState *cs, > hwaddr physaddr, vaddr addr, > > cpu = MICROBLAZE_CPU(cs); > > env = &cpu->env; > > > > - cpu_restore_state(cs, retaddr, true); > > - if (!(env->msr & MSR_EE)) { > > + if (!cpu->cfg.iopb_bus_exception || !(env->msr & MSR_EE)) { > > return; > > } > > > > + cpu_restore_state(cs, retaddr, true); > > + > > env->ear = addr; > > if (access_type == MMU_INST_FETCH) { > > - if ((env->pvr.regs[2] & PVR2_IOPB_BUS_EXC_MASK)) { > > - env->esr = ESR_EC_INSN_BUS; > > - helper_raise_exception(env, EXCP_HW_EXCP); > > - } > > + env->esr = ESR_EC_INSN_BUS; > > + helper_raise_exception(env, EXCP_HW_EXCP); > > } else { > > - if ((env->pvr.regs[2] & PVR2_DOPB_BUS_EXC_MASK)) { > > - env->esr = ESR_EC_DATA_BUS; > > - helper_raise_exception(env, EXCP_HW_EXCP); > > - } > > + env->esr = ESR_EC_DATA_BUS; > > + helper_raise_exception(env, EXCP_HW_EXCP); > > } > > } > > Thanks for the pointer. I've re-written this section to use > cpu_loop_exit_restore(), so that the restore is at the end. The new patch > will > appear in v2, just before iflags is added to the restore state. > > > r~ > Ok, cool. I posted another fix to the list (you're on CC) but we can take your version if it makes it easier. Note that the example I gave you missed that there's two different props for selecting fetch and data access faults. Cheers, Edgar > <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 28 Aug 2020, 15:36 Richard Henderson, <<a href="mailto:richard.henderson@linaro.org">richard.henderson@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 8/27/20 10:09 AM, Edgar E. Iglesias wrote:<br> > It seems to be getting out of sync when getting a slave-error and the core<br> > is not setup to take exceptions for slave errors. Looks like a pre-existing<br> > bug where we're restoring CPU state without taking the exception.<br> > The following fixes that particular case in my runs.<br> > <br> > <br> > I'm on a backported QEMU 5.1 so thing may look different in master.<br> > <br> > diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c<br> > index 831ff2cac1..0cae51c2df 100644<br> > --- a/target/microblaze/op_helper.c<br> > +++ b/target/microblaze/op_helper.c<br> > @@ -432,22 +432,19 @@ void mb_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,<br> > cpu = MICROBLAZE_CPU(cs);<br> > env = &cpu->env;<br> > <br> > - cpu_restore_state(cs, retaddr, true);<br> > - if (!(env->msr & MSR_EE)) {<br> > + if (!cpu->cfg.iopb_bus_exception || !(env->msr & MSR_EE)) {<br> > return;<br> > }<br> > <br> > + cpu_restore_state(cs, retaddr, true);<br> > +<br> > env->ear = addr;<br> > if (access_type == MMU_INST_FETCH) {<br> > - if ((env->pvr.regs[2] & PVR2_IOPB_BUS_EXC_MASK)) {<br> > - env->esr = ESR_EC_INSN_BUS;<br> > - helper_raise_exception(env, EXCP_HW_EXCP);<br> > - }<br> > + env->esr = ESR_EC_INSN_BUS;<br> > + helper_raise_exception(env, EXCP_HW_EXCP);<br> > } else {<br> > - if ((env->pvr.regs[2] & PVR2_DOPB_BUS_EXC_MASK)) {<br> > - env->esr = ESR_EC_DATA_BUS;<br> > - helper_raise_exception(env, EXCP_HW_EXCP);<br> > - }<br> > + env->esr = ESR_EC_DATA_BUS;<br> > + helper_raise_exception(env, EXCP_HW_EXCP);<br> > }<br> > }<br> <br> Thanks for the pointer. I've re-written this section to use<br> cpu_loop_exit_restore(), so that the restore is at the end. The new patch will<br> appear in v2, just before iflags is added to the restore state.<br> <br> <br> r~<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Ok, cool. I posted another fix to the list (you're on CC) but we can take your version if it makes it easier. Note that the example I gave you missed that there's two different props for selecting fetch and data access faults. </div><div dir="auto"><br></div><div dir="auto">Cheers, </div><div dir="auto">Edgar </div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote></div></div></div>