| Message ID | d059a3b3-ac6c-27d2-5270-b6d5b32cc41d@twiddle.net |
|---|---|
| State | New |
| Headers | show |
On 2017-05-23 08:54, Richard Henderson wrote: > On 05/23/2017 03:48 AM, Aurelien Jarno wrote: > > On 2017-05-22 20:02, Richard Henderson wrote: > > > Previously, helper_ex would construct the insn and then implement > > > the insn via direct calls other helpers. This was sufficient to > > > boot Linux but that is all. > > > > > > It is easy enough to go the whole nine yards by stashing state for > > > EXECUTE within the cpu, and then relying on a new TB to be created > > > that properly and completely interprets the insn. > > > > > > Signed-off-by: Richard Henderson <rth@twiddle.net> > > > --- > > > target/s390x/cpu.h | 4 +- > > > target/s390x/helper.h | 2 +- > > > target/s390x/insn-data.def | 4 +- > > > target/s390x/machine.c | 19 +++++++ > > > target/s390x/mem_helper.c | 136 +++++++++++---------------------------------- > > > target/s390x/translate.c | 124 +++++++++++++++++++++++++---------------- > > > 6 files changed, 133 insertions(+), 156 deletions(-) > > > > This looks good on the principle, and finally removes a big hack. That > > said it prevent my test system to boot. I haven't investigated why yet. > > Hmm. I've not got a complete environment -- merely booting a kernel up to > the point it fails to find a rootfs. Which did find several problems with > my first attempts at this, but wouldn't have exercised paging. I'll try > again to get a full install working... > > I wonder if I needed to adjust s390_cpu_handle_mmu_fault (and its myriad > subroutines) to handle setting ILEN correctly. > > There might be a simpler fix though. Currently I advance the PC and > remember the ilen of the EX(RL). Maybe better to *not* advance the PC so as > to have the original EX(RL) right there for ILEN_LATER and ILEN_LATER_INC to > operate on. > > Something like this, as a delta patch. Unfortunately it doesn't work. So far I have no real idea what could be the root cause of the issue. I have just determined that up to the crash, only a very limited set of instructions are being executed. They are the 4 bytes long versions of MVC, CLC, XC, TR.
On 23.05.2017 17:54, Richard Henderson wrote: > On 05/23/2017 03:48 AM, Aurelien Jarno wrote: >> On 2017-05-22 20:02, Richard Henderson wrote: >>> Previously, helper_ex would construct the insn and then implement >>> the insn via direct calls other helpers. This was sufficient to >>> boot Linux but that is all. >>> >>> It is easy enough to go the whole nine yards by stashing state for >>> EXECUTE within the cpu, and then relying on a new TB to be created >>> that properly and completely interprets the insn. >>> >>> Signed-off-by: Richard Henderson <rth@twiddle.net> >>> --- >>> target/s390x/cpu.h | 4 +- >>> target/s390x/helper.h | 2 +- >>> target/s390x/insn-data.def | 4 +- >>> target/s390x/machine.c | 19 +++++++ >>> target/s390x/mem_helper.c | 136 >>> +++++++++++---------------------------------- >>> target/s390x/translate.c | 124 >>> +++++++++++++++++++++++++---------------- >>> 6 files changed, 133 insertions(+), 156 deletions(-) >> >> This looks good on the principle, and finally removes a big hack. That >> said it prevent my test system to boot. I haven't investigated why yet. > > Hmm. I've not got a complete environment -- merely booting a kernel up > to the point it fails to find a rootfs. Which did find several problems > with my first attempts at this, but wouldn't have exercised paging. > I'll try again to get a full install working... Something nice for a quick test is also: http://www.qemu-advent-calendar.org/2014/download/s390-moon-buggy.tar.xz Not sure whether it will trigger your EXECUTE problem, though. Thomas
On 05/23/2017 10:28 AM, Aurelien Jarno wrote: >> Something like this, as a delta patch. > > Unfortunately it doesn't work. So far I have no real idea what could be > the root cause of the issue. I have just determined that up to the crash, > only a very limited set of instructions are being executed. They are the > 4 bytes long versions of MVC, CLC, XC, TR. Yeah, it appears XC is the culprit, though I have not yet determined exactly what's going wrong. That said, perhaps I'll delay this for later and just add some extra helpers for now. It does seem slightly wasteful to create a TB for at least these common cases. r~
diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 67c85f0..5773f92 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -2206,8 +2206,10 @@ static ExitStatus op_ex(DisasContext *s, DisasOps *o) tcg_temp_free_i64(v1); } - /* End the TB; a new TB will be created for modified insn. */ - return EXIT_PC_STALE; + /* End the TB; a new TB will be created for modified insn. + Note that the modified insn runs with this same PC. */ + update_psw_addr(s); + return EXIT_PC_UPDATED; } static ExitStatus op_fieb(DisasContext *s, DisasOps *o) @@ -5189,14 +5191,10 @@ static const DisasInsn *extract_insn insn = s->ex_value & 0xffffffffffff0000ull; ilen = s->ex_value & 0xff; op = insn >> 56; - s->ilen = ilen; - s->next_pc = s->pc; } else { insn = ld_code2(env, pc); op = (insn >> 8) & 0xff; ilen = get_ilen(op); - s->ilen = ilen; - s->next_pc = s->pc + ilen; switch (ilen) { case 2: @@ -5212,6 +5210,8 @@ static const DisasInsn *extract_insn g_assert_not_reached(); } } + s->next_pc = s->pc + ilen; + s->ilen = ilen; /* We can't actually determine the insn format until we've looked up the full insn opcode. Which we can't do without locating the @@ -5470,17 +5470,14 @@ void gen_intermediate_code /* If we reach a page boundary, are single stepping, or exhaust instruction count, stop generation. */ - if (status == NO_EXIT) { - if (unlikely(dc.ex_value)) { - /* The PC on entry is already advanced. */ - status = EXIT_PC_UPDATED; - } else if (dc.pc >= next_page_start - || tcg_op_buf_full() - || num_insns >= max_insns - || singlestep - || cs->singlestep_enabled) { - status = EXIT_PC_STALE; - } + if (status == NO_EXIT + && (dc.pc >= next_page_start + || tcg_op_buf_full() + || num_insns >= max_insns + || singlestep + || cs->singlestep_enabled + || dc.ex_value)) { + status = EXIT_PC_STALE; } } while (status == NO_EXIT);