| Message ID | 20190926162615.31168-3-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/s390: Use tcg unwinding for ilen | expand |
On 26.09.19 18:25, Richard Henderson wrote: > From: Richard Henderson <rth@twiddle.net> > > Use ILEN_UNWIND to signal that we have in fact that > cpu_restore_state will have been called by the time > we arrive in do_program_interrupt. > > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > target/s390x/cpu.h | 4 +++- > target/s390x/interrupt.c | 5 ++++- > target/s390x/translate.c | 21 ++++++++++++++++++--- > 3 files changed, 25 insertions(+), 5 deletions(-) > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index ce20dafd23..080ebcd6bb 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -30,7 +30,7 @@ > /* The z/Architecture has a strong memory model with some store-after-load re-ordering */ > #define TCG_GUEST_DEFAULT_MO (TCG_MO_ALL & ~TCG_MO_ST_LD) > > -#define TARGET_INSN_START_EXTRA_WORDS 1 > +#define TARGET_INSN_START_EXTRA_WORDS 2 > > #define MMU_MODE0_SUFFIX _primary > #define MMU_MODE1_SUFFIX _secondary > @@ -814,6 +814,8 @@ int cpu_s390x_signal_handler(int host_signum, void *pinfo, void *puc); > void s390_crw_mchk(void); > void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr, > uint32_t io_int_parm, uint32_t io_int_word); > +/* instruction length set by unwind info */ > +#define ILEN_UNWIND 0 > /* automatically detect the instruction length */ > #define ILEN_AUTO 0xff > #define RA_IGNORED 0 > diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c > index a841f7187d..30a9fb8852 100644 > --- a/target/s390x/interrupt.c > +++ b/target/s390x/interrupt.c > @@ -28,7 +28,10 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen) > > cs->exception_index = EXCP_PGM; > env->int_pgm_code = code; > - env->int_pgm_ilen = ilen; > + /* If ILEN_UNWIND, int_pgm_ilen already has the correct value. */ > + if (ilen != ILEN_UNWIND) { > + env->int_pgm_ilen = ilen; > + } > } > > void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen, > diff --git a/target/s390x/translate.c b/target/s390x/translate.c > index e1c54ab03b..08f99454de 100644 > --- a/target/s390x/translate.c > +++ b/target/s390x/translate.c > @@ -6309,6 +6309,9 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s) > /* Search for the insn in the table. */ > insn = extract_insn(env, s, &f); > > + /* Emit insn_start now that we know the ILEN. */ > + tcg_gen_insn_start(s->base.pc_next, s->cc_op, s->ilen); > + > /* Not found means unimplemented/illegal opcode. */ > if (insn == NULL) { > qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n", > @@ -6457,9 +6460,6 @@ static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs) > > static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs) > { > - DisasContext *dc = container_of(dcbase, DisasContext, base); > - > - tcg_gen_insn_start(dc->base.pc_next, dc->cc_op); > } > > static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs, > @@ -6467,6 +6467,12 @@ static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs, > { > DisasContext *dc = container_of(dcbase, DisasContext, base); > > + /* > + * Emit an insn_start to accompany the breakpoint exception. > + * The ILEN value is a dummy, since we didn't actually read an insn. > + */ > + tcg_gen_insn_start(dc->base.pc_next, dc->cc_op, 0); > + > dc->base.is_jmp = DISAS_PC_STALE; > dc->do_debug = true; > /* The address covered by the breakpoint must be included in > @@ -6561,8 +6567,17 @@ void restore_state_to_opc(CPUS390XState *env, TranslationBlock *tb, > target_ulong *data) > { > int cc_op = data[1]; > + int ilen = data[2]; > + > env->psw.addr = data[0]; > + > + /* Update the CC opcode if it is not already up-to-date. */ > if ((cc_op != CC_OP_DYNAMIC) && (cc_op != CC_OP_STATIC)) { > env->cc_op = cc_op; > } > + > + /* Update ILEN, except for breakpoint, where we didn't load an insn. */ > + if (ilen) { > + env->int_pgm_ilen = ilen; > + } I am not completely sure about breakpoint handling and which implications we'll have when not setting int_pgm_ilen ... > } > I wonder if that change can help to better handle exceptions during EXECUTE, whereby we have to indicate the EXECUTE instruction and the ilen of the EXECUTE instruction (so the pc and ilen of the original EXECUTE function, not of the EXECUTE target). I don't completely like the current interrupt handling when we have "env->ex_value" in "s390_cpu_exec_interrupt()". I'd love to see that check go, then we can reuse that function easily e.g., in MVC to test and inject exceptions while processing such an interruptible instruction - like MVC. -- Thanks, David / dhildenb
On 9/27/19 3:30 AM, David Hildenbrand wrote: >> + /* Update ILEN, except for breakpoint, where we didn't load an insn. */ >> + if (ilen) { >> + env->int_pgm_ilen = ilen; >> + } > > I am not completely sure about breakpoint handling and which > implications we'll have when not setting int_pgm_ilen ... Yeah. Possibly to make it simple I should simply assign 2 as the length of a breakpoint, bearing in mind that there is no a s390 exception to be delivered -- this is purely a qemu-internal thing, raising EXCP_DEBUG to get back to the gdbstub interface. > I wonder if that change can help to better handle exceptions during > EXECUTE, whereby we have to indicate the EXECUTE instruction and the > ilen of the EXECUTE instruction (so the pc and ilen of the original > EXECUTE function, not of the EXECUTE target). Yes, that's already there. The ilen of the execute insn is placed in the low 4 bits of env->ex_value, and that's what we record as ilen within extract_insn(). > I don't completely like the current interrupt handling when we have > "env->ex_value" in "s390_cpu_exec_interrupt()". I'd love to see that > check go, then we can reuse that function easily e.g., in MVC to test > and inject exceptions while processing such an interruptible instruction > - like MVC. I don't think that reusing s390_cpu_exec_interrupt directly is a good idea. There's other cleanup that needs to happen when exiting a TB. What I think you should do instead is check env_neg(env)->icount_decr, exactly like we do at the start of every basic block, and use that as an indication that you should exit back to the main loop. r~
On 27.09.19 18:02, Richard Henderson wrote: > On 9/27/19 3:30 AM, David Hildenbrand wrote: >>> + /* Update ILEN, except for breakpoint, where we didn't load an insn. */ >>> + if (ilen) { >>> + env->int_pgm_ilen = ilen; >>> + } >> >> I am not completely sure about breakpoint handling and which >> implications we'll have when not setting int_pgm_ilen ... > > Yeah. Possibly to make it simple I should simply assign 2 as the length of a > breakpoint, bearing in mind that there is no a s390 exception to be delivered > -- this is purely a qemu-internal thing, raising EXCP_DEBUG to get back to the > gdbstub interface. > >> I wonder if that change can help to better handle exceptions during >> EXECUTE, whereby we have to indicate the EXECUTE instruction and the >> ilen of the EXECUTE instruction (so the pc and ilen of the original >> EXECUTE function, not of the EXECUTE target). > > Yes, that's already there. The ilen of the execute insn is placed in the low 4 > bits of env->ex_value, and that's what we record as ilen within extract_insn(). Ah, good to know. I'll have to review PGM injection, which ILEN we currently indicate and which one we are supposed to indicate. > >> I don't completely like the current interrupt handling when we have >> "env->ex_value" in "s390_cpu_exec_interrupt()". I'd love to see that >> check go, then we can reuse that function easily e.g., in MVC to test >> and inject exceptions while processing such an interruptible instruction >> - like MVC. > > I don't think that reusing s390_cpu_exec_interrupt directly is a good idea. > There's other cleanup that needs to happen when exiting a TB. > > What I think you should do instead is check env_neg(env)->icount_decr, exactly > like we do at the start of every basic block, and use that as an indication > that you should exit back to the main loop. The issue is that when we return to the main loop we really have to inject an interrupt - otherwise we might simply skip parts of the (interruptible) instruction and continue with the next one. However, with I/O interrupts, we can actually race against other VCPUs. So the I/O interrupt might be gone by the time we arrive in the main loop. -- Thanks, David / dhildenb
On 9/30/19 12:55 AM, David Hildenbrand wrote: >> What I think you should do instead is check env_neg(env)->icount_decr, exactly >> like we do at the start of every basic block, and use that as an indication >> that you should exit back to the main loop. > > The issue is that when we return to the main loop we really have to > inject an interrupt - otherwise we might simply skip parts of the > (interruptible) instruction and continue with the next one. Do we? We will return to the main loop with psw.addr still pointing at MVCL, so the interrupt is delivered, and when the exception returns we restart the MVCL. Or, an interrupt is not delivered and we restart from psw.addr and still restart the MVCL. We probably would have to take a hard look at EXECUTE of MVCL to see if that is actually restartable. Not that it makes particular sense to want that combo, but it's required to work. > However, with I/O interrupts, we can actually race against other VCPUs. > So the I/O interrupt might be gone by the time we arrive in the main loop. Of course, but it's no different with this case than any other. If the interrupt has already been handled, then we will simply restart the next TB as per normal. r~
On 30.09.19 17:03, Richard Henderson wrote: > On 9/30/19 12:55 AM, David Hildenbrand wrote: >>> What I think you should do instead is check env_neg(env)->icount_decr, exactly >>> like we do at the start of every basic block, and use that as an indication >>> that you should exit back to the main loop. >> >> The issue is that when we return to the main loop we really have to >> inject an interrupt - otherwise we might simply skip parts of the >> (interruptible) instruction and continue with the next one. > > Do we? We will return to the main loop with psw.addr still pointing at MVCL, > so the interrupt is delivered, and when the exception returns we restart the > MVCL. Or, an interrupt is not delivered and we restart from psw.addr and still > restart the MVCL. If that's the case, then this should indeed work. > > We probably would have to take a hard look at EXECUTE of MVCL to see if that is > actually restartable. Not that it makes particular sense to want that combo, > but it's required to work. > Yes, this has to work. I'll look into that once I have some time. >> However, with I/O interrupts, we can actually race against other VCPUs. >> So the I/O interrupt might be gone by the time we arrive in the main loop. > > Of course, but it's no different with this case than any other. If the > interrupt has already been handled, then we will simply restart the next TB as > per normal. Yeah, I was mostly concerned that "the next TB" will be "the next instruction" and not "the original instruction again". -- Thanks, David / dhildenb
On 9/30/19 8:42 AM, David Hildenbrand wrote: >> Of course, but it's no different with this case than any other. If the >> interrupt has already been handled, then we will simply restart the next TB as >> per normal. > Yeah, I was mostly concerned that "the next TB" will be "the next > instruction" and not "the original instruction again". Ah, well, that's easy. The next TB will start from psw.addr, and when we cpu_loop_exit_restore(), psw.addr will be set to the current instruction. This is exactly the same as when we raise a PGM_ADDRESSING exception which needs to be restarted after the kernel swaps in the page. r~
On 30.09.19 18:15, Richard Henderson wrote: > On 9/30/19 8:42 AM, David Hildenbrand wrote: >>> Of course, but it's no different with this case than any other. If the >>> interrupt has already been handled, then we will simply restart the next TB as >>> per normal. >> Yeah, I was mostly concerned that "the next TB" will be "the next >> instruction" and not "the original instruction again". > > Ah, well, that's easy. The next TB will start from psw.addr, and when we > cpu_loop_exit_restore(), psw.addr will be set to the current instruction. > > This is exactly the same as when we raise a PGM_ADDRESSING exception which > needs to be restarted after the kernel swaps in the page. > Now that I think about it, this makes perfect sense :) So it's mostly checking if there is an exception (like you said) and then doing a cpu_loop_exit_restore() (if I'm not wrong, but I'll have to look at the details). -- Thanks, David / dhildenb
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index ce20dafd23..080ebcd6bb 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -30,7 +30,7 @@ /* The z/Architecture has a strong memory model with some store-after-load re-ordering */ #define TCG_GUEST_DEFAULT_MO (TCG_MO_ALL & ~TCG_MO_ST_LD) -#define TARGET_INSN_START_EXTRA_WORDS 1 +#define TARGET_INSN_START_EXTRA_WORDS 2 #define MMU_MODE0_SUFFIX _primary #define MMU_MODE1_SUFFIX _secondary @@ -814,6 +814,8 @@ int cpu_s390x_signal_handler(int host_signum, void *pinfo, void *puc); void s390_crw_mchk(void); void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr, uint32_t io_int_parm, uint32_t io_int_word); +/* instruction length set by unwind info */ +#define ILEN_UNWIND 0 /* automatically detect the instruction length */ #define ILEN_AUTO 0xff #define RA_IGNORED 0 diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c index a841f7187d..30a9fb8852 100644 --- a/target/s390x/interrupt.c +++ b/target/s390x/interrupt.c @@ -28,7 +28,10 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen) cs->exception_index = EXCP_PGM; env->int_pgm_code = code; - env->int_pgm_ilen = ilen; + /* If ILEN_UNWIND, int_pgm_ilen already has the correct value. */ + if (ilen != ILEN_UNWIND) { + env->int_pgm_ilen = ilen; + } } void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen, diff --git a/target/s390x/translate.c b/target/s390x/translate.c index e1c54ab03b..08f99454de 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -6309,6 +6309,9 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s) /* Search for the insn in the table. */ insn = extract_insn(env, s, &f); + /* Emit insn_start now that we know the ILEN. */ + tcg_gen_insn_start(s->base.pc_next, s->cc_op, s->ilen); + /* Not found means unimplemented/illegal opcode. */ if (insn == NULL) { qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n", @@ -6457,9 +6460,6 @@ static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs) static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs) { - DisasContext *dc = container_of(dcbase, DisasContext, base); - - tcg_gen_insn_start(dc->base.pc_next, dc->cc_op); } static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs, @@ -6467,6 +6467,12 @@ static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs, { DisasContext *dc = container_of(dcbase, DisasContext, base); + /* + * Emit an insn_start to accompany the breakpoint exception. + * The ILEN value is a dummy, since we didn't actually read an insn. + */ + tcg_gen_insn_start(dc->base.pc_next, dc->cc_op, 0); + dc->base.is_jmp = DISAS_PC_STALE; dc->do_debug = true; /* The address covered by the breakpoint must be included in @@ -6561,8 +6567,17 @@ void restore_state_to_opc(CPUS390XState *env, TranslationBlock *tb, target_ulong *data) { int cc_op = data[1]; + int ilen = data[2]; + env->psw.addr = data[0]; + + /* Update the CC opcode if it is not already up-to-date. */ if ((cc_op != CC_OP_DYNAMIC) && (cc_op != CC_OP_STATIC)) { env->cc_op = cc_op; } + + /* Update ILEN, except for breakpoint, where we didn't load an insn. */ + if (ilen) { + env->int_pgm_ilen = ilen; + } }