| Message ID | 20190926162615.31168-15-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/s390: Use tcg unwinding for ilen | expand |
On 26.09.19 18:26, Richard Henderson wrote: > We currently set ilen to AUTO, then overwrite that during > unwinding, then overwrite that for the code access case. > > This can be simplified to setting ilen to our arbitrary > value for the (undefined) code access case, then rely on > unwinding to overwrite that with the correct value for > the data access case. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/s390x/excp_helper.c | 23 +++++++---------------- > 1 file changed, 7 insertions(+), 16 deletions(-) > > diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c > index 98a1ee8317..8ce992e639 100644 > --- a/target/s390x/excp_helper.c > +++ b/target/s390x/excp_helper.c > @@ -96,7 +96,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > { > S390CPU *cpu = S390_CPU(cs); > > - trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO); > + trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_UNWIND); Hmm, we always trigger a pgm exceptions, meaning we set cs->exception_index even if we have probe = true. Confused by that. > /* On real machines this value is dropped into LowMem. Since this > is userland, simply put this someplace that cpu_loop can find it. */ > cpu->env.__excp_addr = address; > @@ -179,24 +179,15 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > stq_phys(env_cpu(env)->as, > env->psa + offsetof(LowCore, trans_exc_code), tec); > } > - trigger_pgm_exception(env, excp, ILEN_AUTO); > - cpu_restore_state(cs, retaddr, true); > > /* > - * The ILC value for code accesses is undefined. The important > - * thing here is to *not* leave env->int_pgm_ilen set to ILEN_AUTO, > - * which would cause do_program_interrupt to attempt to read from > - * env->psw.addr again. C.f. the condition in trigger_page_fault, > - * but is not universally applied. > - * > - * ??? If we remove ILEN_AUTO, by moving the computation of ILEN > - * into cpu_restore_state, then we may remove this entirely. > + * For data accesses, ILEN will be filled in from the unwind info, > + * within cpu_loop_exit_restore. For code accesses, retaddr == 0, > + * and so unwinding will not occur. However, ILEN is also undefined > + * for that case -- we choose to set ILEN = 2. > */ > - if (access_type == MMU_INST_FETCH) { > - env->int_pgm_ilen = 2; > - } > - > - cpu_loop_exit(cs); > + trigger_pgm_exception(env, excp, 2); I wonder if it is still worth setting this only conditionally. Most probably not. > + cpu_loop_exit_restore(cs, retaddr); > } > > static void do_program_interrupt(CPUS390XState *env) > -- Thanks, David / dhildenb
On 9/27/19 4:02 AM, David Hildenbrand wrote: > On 26.09.19 18:26, Richard Henderson wrote: >> We currently set ilen to AUTO, then overwrite that during >> unwinding, then overwrite that for the code access case. >> >> This can be simplified to setting ilen to our arbitrary >> value for the (undefined) code access case, then rely on >> unwinding to overwrite that with the correct value for >> the data access case. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/s390x/excp_helper.c | 23 +++++++---------------- >> 1 file changed, 7 insertions(+), 16 deletions(-) >> >> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c >> index 98a1ee8317..8ce992e639 100644 >> --- a/target/s390x/excp_helper.c >> +++ b/target/s390x/excp_helper.c >> @@ -96,7 +96,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >> { >> S390CPU *cpu = S390_CPU(cs); >> >> - trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO); >> + trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_UNWIND); > > Hmm, we always trigger a pgm exceptions, meaning we set > cs->exception_index even if we have probe = true. Confused by that. This is the CONFIG_USER_ONLY version, for which probe is always false. Perhaps I shouldn't have made the function interface identical, but it did appear to make things cleaner for most targets. >> + trigger_pgm_exception(env, excp, 2); > > I wonder if it is still worth setting this only conditionally. Most > probably not. I don't see that it would be. I hope the comment is clear about this arbitrary value is overwritten during unwinding. r~
On 27.09.19 18:16, Richard Henderson wrote: > On 9/27/19 4:02 AM, David Hildenbrand wrote: >> On 26.09.19 18:26, Richard Henderson wrote: >>> We currently set ilen to AUTO, then overwrite that during >>> unwinding, then overwrite that for the code access case. >>> >>> This can be simplified to setting ilen to our arbitrary >>> value for the (undefined) code access case, then rely on >>> unwinding to overwrite that with the correct value for >>> the data access case. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> target/s390x/excp_helper.c | 23 +++++++---------------- >>> 1 file changed, 7 insertions(+), 16 deletions(-) >>> >>> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c >>> index 98a1ee8317..8ce992e639 100644 >>> --- a/target/s390x/excp_helper.c >>> +++ b/target/s390x/excp_helper.c >>> @@ -96,7 +96,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, >>> { >>> S390CPU *cpu = S390_CPU(cs); >>> >>> - trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO); >>> + trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_UNWIND); >> >> Hmm, we always trigger a pgm exceptions, meaning we set >> cs->exception_index even if we have probe = true. Confused by that. > > This is the CONFIG_USER_ONLY version, for which probe is always false. Perhaps > I shouldn't have made the function interface identical, but it did appear to > make things cleaner for most targets. > >>> + trigger_pgm_exception(env, excp, 2); >> >> I wonder if it is still worth setting this only conditionally. Most >> probably not. > > I don't see that it would be. I hope the comment is clear about this arbitrary > value is overwritten during unwinding. It's confusing, but I get it :) -- Thanks, David / dhildenb
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c index 98a1ee8317..8ce992e639 100644 --- a/target/s390x/excp_helper.c +++ b/target/s390x/excp_helper.c @@ -96,7 +96,7 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, { S390CPU *cpu = S390_CPU(cs); - trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_AUTO); + trigger_pgm_exception(&cpu->env, PGM_ADDRESSING, ILEN_UNWIND); /* On real machines this value is dropped into LowMem. Since this is userland, simply put this someplace that cpu_loop can find it. */ cpu->env.__excp_addr = address; @@ -179,24 +179,15 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, stq_phys(env_cpu(env)->as, env->psa + offsetof(LowCore, trans_exc_code), tec); } - trigger_pgm_exception(env, excp, ILEN_AUTO); - cpu_restore_state(cs, retaddr, true); /* - * The ILC value for code accesses is undefined. The important - * thing here is to *not* leave env->int_pgm_ilen set to ILEN_AUTO, - * which would cause do_program_interrupt to attempt to read from - * env->psw.addr again. C.f. the condition in trigger_page_fault, - * but is not universally applied. - * - * ??? If we remove ILEN_AUTO, by moving the computation of ILEN - * into cpu_restore_state, then we may remove this entirely. + * For data accesses, ILEN will be filled in from the unwind info, + * within cpu_loop_exit_restore. For code accesses, retaddr == 0, + * and so unwinding will not occur. However, ILEN is also undefined + * for that case -- we choose to set ILEN = 2. */ - if (access_type == MMU_INST_FETCH) { - env->int_pgm_ilen = 2; - } - - cpu_loop_exit(cs); + trigger_pgm_exception(env, excp, 2); + cpu_loop_exit_restore(cs, retaddr); } static void do_program_interrupt(CPUS390XState *env)
We currently set ilen to AUTO, then overwrite that during unwinding, then overwrite that for the code access case. This can be simplified to setting ilen to our arbitrary value for the (undefined) code access case, then rely on unwinding to overwrite that with the correct value for the data access case. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/s390x/excp_helper.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) -- 2.17.1