| Message ID | 20230728195538.488932-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/s390x: Move trans_exc_code update to do_program_interrupt | expand |
On 7/28/23 21:55, Richard Henderson wrote: > This solves a problem in which the store to LowCore during tlb_fill > triggers a clean-page TB invalidation for page0 during translation, > which results in an assertion failure for locked pages. > > By delaying the store until after the exception has been raised, > we will have unwound the pages locked for translation and the > problem does not arise. There are plenty of other updates to > LowCore while delivering an interrupt/exception; trans_exc_code > does not need to be special. > > Reported-by: Claudio Fontana <cfontana@suse.de> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Verified, fixes the TCG assertions I encountered with qemu-system-s390x on x86 host. Tested-by: Claudio Fontana <cfontana@suse.de> > --- > target/s390x/tcg/excp_helper.c | 42 +++++++++++++++++++++++----------- > 1 file changed, 29 insertions(+), 13 deletions(-) > > diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c > index 3da337f7c7..b260bf7331 100644 > --- a/target/s390x/tcg/excp_helper.c > +++ b/target/s390x/tcg/excp_helper.c > @@ -190,11 +190,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, > return false; > } > > - if (excp != PGM_ADDRESSING) { > - stq_phys(env_cpu(env)->as, > - env->psa + offsetof(LowCore, trans_exc_code), tec); > - } > - > /* > * For data accesses, ILEN will be filled in from the unwind info, > * within cpu_loop_exit_restore. For code accesses, retaddr == 0, > @@ -211,20 +206,34 @@ static void do_program_interrupt(CPUS390XState *env) > uint64_t mask, addr; > LowCore *lowcore; > int ilen = env->int_pgm_ilen; > + bool set_trans_exc_code = false; > + bool advance = false; > > assert((env->int_pgm_code == PGM_SPECIFICATION && ilen == 0) || > ilen == 2 || ilen == 4 || ilen == 6); > > switch (env->int_pgm_code) { > case PGM_PER: > - if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) { > - break; > - } > - /* FALL THROUGH */ > + advance = !(env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION); > + break; > + case PGM_ASCE_TYPE: > + case PGM_REG_FIRST_TRANS: > + case PGM_REG_SEC_TRANS: > + case PGM_REG_THIRD_TRANS: > + case PGM_SEGMENT_TRANS: > + case PGM_PAGE_TRANS: > + assert(env->int_pgm_code == env->tlb_fill_exc); > + set_trans_exc_code = true; > + break; > + case PGM_PROTECTION: > + case PGM_TRANS_SPEC: > + assert(env->int_pgm_code == env->tlb_fill_exc); > + set_trans_exc_code = true; > + advance = true; > + break; > case PGM_OPERATION: > case PGM_PRIVILEGED: > case PGM_EXECUTE: > - case PGM_PROTECTION: > case PGM_ADDRESSING: > case PGM_SPECIFICATION: > case PGM_DATA: > @@ -236,18 +245,21 @@ static void do_program_interrupt(CPUS390XState *env) > case PGM_HFP_EXP_UNDERFLOW: > case PGM_HFP_SIGNIFICANCE: > case PGM_HFP_DIVIDE: > - case PGM_TRANS_SPEC: > case PGM_SPECIAL_OP: > case PGM_OPERAND: > case PGM_HFP_SQRT: > case PGM_PC_TRANS_SPEC: > case PGM_ALET_SPEC: > case PGM_MONITOR: > - /* advance the PSW if our exception is not nullifying */ > - env->psw.addr += ilen; > + advance = true; > break; > } > > + /* advance the PSW if our exception is not nullifying */ > + if (advance) { > + env->psw.addr += ilen; > + } > + > qemu_log_mask(CPU_LOG_INT, > "%s: code=0x%x ilen=%d psw: %" PRIx64 " %" PRIx64 "\n", > __func__, env->int_pgm_code, ilen, env->psw.mask, > @@ -263,6 +275,10 @@ static void do_program_interrupt(CPUS390XState *env) > env->per_perc_atmid = 0; > } > > + if (set_trans_exc_code) { > + lowcore->trans_exc_code = cpu_to_be64(env->tlb_fill_tec); > + } > + > lowcore->pgm_ilen = cpu_to_be16(ilen); > lowcore->pgm_code = cpu_to_be16(env->int_pgm_code); > lowcore->program_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
On Fri, 2023-07-28 at 13:02 -0700, Richard Henderson wrote: > On 7/28/23 12:55, Richard Henderson wrote: > > This solves a problem in which the store to LowCore during tlb_fill > > triggers a clean-page TB invalidation for page0 during translation, > > which results in an assertion failure for locked pages. > > > > By delaying the store until after the exception has been raised, > > we will have unwound the pages locked for translation and the > > problem does not arise. There are plenty of other updates to > > LowCore while delivering an interrupt/exception; trans_exc_code > > does not need to be special. > > > > Reported-by: Claudio Fontana <cfontana@suse.de> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > --- > > target/s390x/tcg/excp_helper.c | 42 +++++++++++++++++++++++------ > > ----- > > 1 file changed, 29 insertions(+), 13 deletions(-) [...] > > switch (env->int_pgm_code) { > > case PGM_PER: > > - if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) { > > - break; > > - } > > - /* FALL THROUGH */ > > + advance = !(env->per_perc_atmid & > > PER_CODE_EVENT_NULLIFICATION); > > + break; > > + case PGM_ASCE_TYPE: > > + case PGM_REG_FIRST_TRANS: > > + case PGM_REG_SEC_TRANS: > > + case PGM_REG_THIRD_TRANS: > > + case PGM_SEGMENT_TRANS: > > + case PGM_PAGE_TRANS: > > + assert(env->int_pgm_code == env->tlb_fill_exc); > > + set_trans_exc_code = true; > > + break; > > I should have mentioned that this block of exceptions came from page > 3-76 > (Translation-Exception Identification for DAT Exceptions) of the 13th > edition of the PoO. > > > + case PGM_PROTECTION: > > + case PGM_TRANS_SPEC: > > + assert(env->int_pgm_code == env->tlb_fill_exc); > > + set_trans_exc_code = true; > > + advance = true; > > + break; > > These exceptions came from seeing an early kernel fault, grepping for > the set of > exceptions raised in mmu_helper.c, and eliminating PGM_ADDRESSING per > the first hunk. Does POp specify that the CPU stores Translation-Exception Identification on Translation-Specification Exceptions (PGM_TRANS_SPEC)? I re-read the 0xA8 documentation a few times, but could not find it. It's also interesting what the kernel was attempting when it got PGM_TRANS_SPEC and recovered from it. Maybe something else is wrong? Other than the POp question: Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com> > > I wasn't sure where to look for the full specification of exception > effects, but this did > solve the kernel fault. > > > r~
On 7/31/23 01:26, Ilya Leoshkevich wrote: >>> + case PGM_PROTECTION: >>> + case PGM_TRANS_SPEC: >>> + assert(env->int_pgm_code == env->tlb_fill_exc); >>> + set_trans_exc_code = true; >>> + advance = true; >>> + break; >> >> These exceptions came from seeing an early kernel fault, grepping for >> the set of >> exceptions raised in mmu_helper.c, and eliminating PGM_ADDRESSING per >> the first hunk. > > Does POp specify that the CPU stores Translation-Exception > Identification on Translation-Specification Exceptions > (PGM_TRANS_SPEC)? I re-read the 0xA8 documentation a few times, but > could not find it. Neither could I. > It's also interesting what the kernel was attempting when it got > PGM_TRANS_SPEC and recovered from it. Maybe something else is wrong? I think the kernel was testing PGM_PROTECTION, to see if LowCore protection worked. But since that one wasn't listed for 0xA8 either, I preserved previous behaviour in setting trans_exc_code for all mmu exceptions except for PGM_ADDRESSING. r~
On Mon, 2023-07-31 at 07:55 -0700, Richard Henderson wrote: > On 7/31/23 01:26, Ilya Leoshkevich wrote: > > > > + case PGM_PROTECTION: > > > > + case PGM_TRANS_SPEC: > > > > + assert(env->int_pgm_code == env->tlb_fill_exc); > > > > + set_trans_exc_code = true; > > > > + advance = true; > > > > + break; > > > > > > These exceptions came from seeing an early kernel fault, grepping > > > for > > > the set of > > > exceptions raised in mmu_helper.c, and eliminating PGM_ADDRESSING > > > per > > > the first hunk. > > > > Does POp specify that the CPU stores Translation-Exception > > Identification on Translation-Specification Exceptions > > (PGM_TRANS_SPEC)? I re-read the 0xA8 documentation a few times, but > > could not find it. > > Neither could I. > > > It's also interesting what the kernel was attempting when it got > > PGM_TRANS_SPEC and recovered from it. Maybe something else is > > wrong? > > I think the kernel was testing PGM_PROTECTION, to see if LowCore > protection worked. > > But since that one wasn't listed for 0xA8 either, I preserved > previous behaviour in > setting trans_exc_code for all mmu exceptions except for > PGM_ADDRESSING. > > > r~ Actually, PGM_PROTECTION is there. If you scroll a few pages down, you will see: "Translation-Exception Identification for Protection Exceptions". But nothing like this for PGM_TRANS_SPEC.
diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c index 3da337f7c7..b260bf7331 100644 --- a/target/s390x/tcg/excp_helper.c +++ b/target/s390x/tcg/excp_helper.c @@ -190,11 +190,6 @@ bool s390_cpu_tlb_fill(CPUState *cs, vaddr address, int size, return false; } - if (excp != PGM_ADDRESSING) { - stq_phys(env_cpu(env)->as, - env->psa + offsetof(LowCore, trans_exc_code), tec); - } - /* * For data accesses, ILEN will be filled in from the unwind info, * within cpu_loop_exit_restore. For code accesses, retaddr == 0, @@ -211,20 +206,34 @@ static void do_program_interrupt(CPUS390XState *env) uint64_t mask, addr; LowCore *lowcore; int ilen = env->int_pgm_ilen; + bool set_trans_exc_code = false; + bool advance = false; assert((env->int_pgm_code == PGM_SPECIFICATION && ilen == 0) || ilen == 2 || ilen == 4 || ilen == 6); switch (env->int_pgm_code) { case PGM_PER: - if (env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION) { - break; - } - /* FALL THROUGH */ + advance = !(env->per_perc_atmid & PER_CODE_EVENT_NULLIFICATION); + break; + case PGM_ASCE_TYPE: + case PGM_REG_FIRST_TRANS: + case PGM_REG_SEC_TRANS: + case PGM_REG_THIRD_TRANS: + case PGM_SEGMENT_TRANS: + case PGM_PAGE_TRANS: + assert(env->int_pgm_code == env->tlb_fill_exc); + set_trans_exc_code = true; + break; + case PGM_PROTECTION: + case PGM_TRANS_SPEC: + assert(env->int_pgm_code == env->tlb_fill_exc); + set_trans_exc_code = true; + advance = true; + break; case PGM_OPERATION: case PGM_PRIVILEGED: case PGM_EXECUTE: - case PGM_PROTECTION: case PGM_ADDRESSING: case PGM_SPECIFICATION: case PGM_DATA: @@ -236,18 +245,21 @@ static void do_program_interrupt(CPUS390XState *env) case PGM_HFP_EXP_UNDERFLOW: case PGM_HFP_SIGNIFICANCE: case PGM_HFP_DIVIDE: - case PGM_TRANS_SPEC: case PGM_SPECIAL_OP: case PGM_OPERAND: case PGM_HFP_SQRT: case PGM_PC_TRANS_SPEC: case PGM_ALET_SPEC: case PGM_MONITOR: - /* advance the PSW if our exception is not nullifying */ - env->psw.addr += ilen; + advance = true; break; } + /* advance the PSW if our exception is not nullifying */ + if (advance) { + env->psw.addr += ilen; + } + qemu_log_mask(CPU_LOG_INT, "%s: code=0x%x ilen=%d psw: %" PRIx64 " %" PRIx64 "\n", __func__, env->int_pgm_code, ilen, env->psw.mask, @@ -263,6 +275,10 @@ static void do_program_interrupt(CPUS390XState *env) env->per_perc_atmid = 0; } + if (set_trans_exc_code) { + lowcore->trans_exc_code = cpu_to_be64(env->tlb_fill_tec); + } + lowcore->pgm_ilen = cpu_to_be16(ilen); lowcore->pgm_code = cpu_to_be16(env->int_pgm_code); lowcore->program_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
This solves a problem in which the store to LowCore during tlb_fill triggers a clean-page TB invalidation for page0 during translation, which results in an assertion failure for locked pages. By delaying the store until after the exception has been raised, we will have unwound the pages locked for translation and the problem does not arise. There are plenty of other updates to LowCore while delivering an interrupt/exception; trans_exc_code does not need to be special. Reported-by: Claudio Fontana <cfontana@suse.de> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/s390x/tcg/excp_helper.c | 42 +++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 13 deletions(-)