| Message ID | 5074c129ebfea0e1dfc22ef4691d8b62038d59b2.1715125376.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Misc PPC exception and BookE MMU clean ups | expand |
On Wed May 8, 2024 at 10:15 AM AEST, BALATON Zoltan wrote: > Move setting error_code that appears in every case out in front and > hoist the common fall through case for BOOKE206 as well which allows > removing the nested switches. > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > target/ppc/mmu_common.c | 41 ++++++++++++----------------------------- > 1 file changed, 12 insertions(+), 29 deletions(-) > > diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c > index 788e2bebd5..c725a7932f 100644 > --- a/target/ppc/mmu_common.c > +++ b/target/ppc/mmu_common.c > @@ -1205,58 +1205,41 @@ static bool ppc_booke_xlate(PowerPCCPU *cpu, vaddr eaddr, > } > > log_cpu_state_mask(CPU_LOG_MMU, cs, 0); > + env->error_code = 0; > + if (ret == -1) { > + if (env->mmu_model == POWERPC_MMU_BOOKE206) { > + booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); > + } > + } > if (access_type == MMU_INST_FETCH) { > switch (ret) { > case -1: > /* No matches in page tables or TLB */ > - switch (env->mmu_model) { > - case POWERPC_MMU_BOOKE206: > - booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); > - /* fall through */ > - case POWERPC_MMU_BOOKE: > - cs->exception_index = POWERPC_EXCP_ITLB; > - env->error_code = 0; > - env->spr[SPR_BOOKE_DEAR] = eaddr; > - env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); > - break; > - default: > - g_assert_not_reached(); > - } > + cs->exception_index = POWERPC_EXCP_ITLB; > + env->spr[SPR_BOOKE_DEAR] = eaddr; > + env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); > break; > case -2: > /* Access rights violation */ > cs->exception_index = POWERPC_EXCP_ISI; > - env->error_code = 0; > break; > case -3: > /* No execute protection violation */ > cs->exception_index = POWERPC_EXCP_ISI; > env->spr[SPR_BOOKE_ESR] = 0; I don't know BookE well but AFAIKS it says ESR if not set explicitly is generally cleared to 0 by interrupts which I guess is the case here. I don't see why the same would not apply to the -2 case either. That would reduce special cases. Although that's a behaviour change. It's possible current beahviour is deliberate or matches some particular CPU. Not something for this series. Thanks, Nick > - env->error_code = 0; > break; > } > } else { > switch (ret) { > case -1: > /* No matches in page tables or TLB */ > - switch (env->mmu_model) { > - case POWERPC_MMU_BOOKE206: > - booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); > - /* fall through */ > - case POWERPC_MMU_BOOKE: > - cs->exception_index = POWERPC_EXCP_DTLB; > - env->error_code = 0; > - env->spr[SPR_BOOKE_DEAR] = eaddr; > - env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); > - break; > - default: > - g_assert_not_reached(); > - } > + cs->exception_index = POWERPC_EXCP_DTLB; > + env->spr[SPR_BOOKE_DEAR] = eaddr; > + env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); > break; > case -2: > /* Access rights violation */ > cs->exception_index = POWERPC_EXCP_DSI; > - env->error_code = 0; > env->spr[SPR_BOOKE_DEAR] = eaddr; > env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); > break;
On Wed, 8 May 2024, Nicholas Piggin wrote: > On Wed May 8, 2024 at 10:15 AM AEST, BALATON Zoltan wrote: >> Move setting error_code that appears in every case out in front and >> hoist the common fall through case for BOOKE206 as well which allows >> removing the nested switches. >> > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> target/ppc/mmu_common.c | 41 ++++++++++++----------------------------- >> 1 file changed, 12 insertions(+), 29 deletions(-) >> >> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c >> index 788e2bebd5..c725a7932f 100644 >> --- a/target/ppc/mmu_common.c >> +++ b/target/ppc/mmu_common.c >> @@ -1205,58 +1205,41 @@ static bool ppc_booke_xlate(PowerPCCPU *cpu, vaddr eaddr, >> } >> >> log_cpu_state_mask(CPU_LOG_MMU, cs, 0); >> + env->error_code = 0; >> + if (ret == -1) { >> + if (env->mmu_model == POWERPC_MMU_BOOKE206) { >> + booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); >> + } >> + } >> if (access_type == MMU_INST_FETCH) { >> switch (ret) { >> case -1: >> /* No matches in page tables or TLB */ >> - switch (env->mmu_model) { >> - case POWERPC_MMU_BOOKE206: >> - booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); >> - /* fall through */ >> - case POWERPC_MMU_BOOKE: >> - cs->exception_index = POWERPC_EXCP_ITLB; >> - env->error_code = 0; >> - env->spr[SPR_BOOKE_DEAR] = eaddr; >> - env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); >> - break; >> - default: >> - g_assert_not_reached(); >> - } >> + cs->exception_index = POWERPC_EXCP_ITLB; >> + env->spr[SPR_BOOKE_DEAR] = eaddr; >> + env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); >> break; >> case -2: >> /* Access rights violation */ >> cs->exception_index = POWERPC_EXCP_ISI; >> - env->error_code = 0; >> break; >> case -3: >> /* No execute protection violation */ >> cs->exception_index = POWERPC_EXCP_ISI; >> env->spr[SPR_BOOKE_ESR] = 0; > > I don't know BookE well but AFAIKS it says ESR if not set explicitly > is generally cleared to 0 by interrupts which I guess is the case here. > I don't see why the same would not apply to the -2 case either. That > would reduce special cases. > > Although that's a behaviour change. It's possible current beahviour is > deliberate or matches some particular CPU. Not something for this > series. I don't know what the correct behaviour should be so I just tried to keep what was there. After this clean it should be simpler to find out and correct this later. Regards, BALATON Zoltan > Thanks, > Nick > >> - env->error_code = 0; >> break; >> } >> } else { >> switch (ret) { >> case -1: >> /* No matches in page tables or TLB */ >> - switch (env->mmu_model) { >> - case POWERPC_MMU_BOOKE206: >> - booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); >> - /* fall through */ >> - case POWERPC_MMU_BOOKE: >> - cs->exception_index = POWERPC_EXCP_DTLB; >> - env->error_code = 0; >> - env->spr[SPR_BOOKE_DEAR] = eaddr; >> - env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); >> - break; >> - default: >> - g_assert_not_reached(); >> - } >> + cs->exception_index = POWERPC_EXCP_DTLB; >> + env->spr[SPR_BOOKE_DEAR] = eaddr; >> + env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); >> break; >> case -2: >> /* Access rights violation */ >> cs->exception_index = POWERPC_EXCP_DSI; >> - env->error_code = 0; >> env->spr[SPR_BOOKE_DEAR] = eaddr; >> env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); >> break; > > >
On Thu May 9, 2024 at 1:25 AM AEST, BALATON Zoltan wrote: > On Wed, 8 May 2024, Nicholas Piggin wrote: > > On Wed May 8, 2024 at 10:15 AM AEST, BALATON Zoltan wrote: > >> Move setting error_code that appears in every case out in front and > >> hoist the common fall through case for BOOKE206 as well which allows > >> removing the nested switches. > >> > > > > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > > >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > >> --- > >> target/ppc/mmu_common.c | 41 ++++++++++++----------------------------- > >> 1 file changed, 12 insertions(+), 29 deletions(-) > >> > >> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c > >> index 788e2bebd5..c725a7932f 100644 > >> --- a/target/ppc/mmu_common.c > >> +++ b/target/ppc/mmu_common.c > >> @@ -1205,58 +1205,41 @@ static bool ppc_booke_xlate(PowerPCCPU *cpu, vaddr eaddr, > >> } > >> > >> log_cpu_state_mask(CPU_LOG_MMU, cs, 0); > >> + env->error_code = 0; > >> + if (ret == -1) { > >> + if (env->mmu_model == POWERPC_MMU_BOOKE206) { > >> + booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); > >> + } > >> + } > >> if (access_type == MMU_INST_FETCH) { > >> switch (ret) { > >> case -1: > >> /* No matches in page tables or TLB */ > >> - switch (env->mmu_model) { > >> - case POWERPC_MMU_BOOKE206: > >> - booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); > >> - /* fall through */ > >> - case POWERPC_MMU_BOOKE: > >> - cs->exception_index = POWERPC_EXCP_ITLB; > >> - env->error_code = 0; > >> - env->spr[SPR_BOOKE_DEAR] = eaddr; > >> - env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); > >> - break; > >> - default: > >> - g_assert_not_reached(); > >> - } > >> + cs->exception_index = POWERPC_EXCP_ITLB; > >> + env->spr[SPR_BOOKE_DEAR] = eaddr; > >> + env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); > >> break; > >> case -2: > >> /* Access rights violation */ > >> cs->exception_index = POWERPC_EXCP_ISI; > >> - env->error_code = 0; > >> break; > >> case -3: > >> /* No execute protection violation */ > >> cs->exception_index = POWERPC_EXCP_ISI; > >> env->spr[SPR_BOOKE_ESR] = 0; > > > > I don't know BookE well but AFAIKS it says ESR if not set explicitly > > is generally cleared to 0 by interrupts which I guess is the case here. > > I don't see why the same would not apply to the -2 case either. That > > would reduce special cases. > > > > Although that's a behaviour change. It's possible current beahviour is > > deliberate or matches some particular CPU. Not something for this > > series. > > I don't know what the correct behaviour should be so I just tried to keep > what was there. After this clean it should be simpler to find out and > correct this later. Right. Keeping exact behaviour is the right thing to do for such a series, so it's good you have been doing it. It was just an offhand comment because the special case annoyed me :) Thanks, Nick
diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c index 788e2bebd5..c725a7932f 100644 --- a/target/ppc/mmu_common.c +++ b/target/ppc/mmu_common.c @@ -1205,58 +1205,41 @@ static bool ppc_booke_xlate(PowerPCCPU *cpu, vaddr eaddr, } log_cpu_state_mask(CPU_LOG_MMU, cs, 0); + env->error_code = 0; + if (ret == -1) { + if (env->mmu_model == POWERPC_MMU_BOOKE206) { + booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); + } + } if (access_type == MMU_INST_FETCH) { switch (ret) { case -1: /* No matches in page tables or TLB */ - switch (env->mmu_model) { - case POWERPC_MMU_BOOKE206: - booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); - /* fall through */ - case POWERPC_MMU_BOOKE: - cs->exception_index = POWERPC_EXCP_ITLB; - env->error_code = 0; - env->spr[SPR_BOOKE_DEAR] = eaddr; - env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); - break; - default: - g_assert_not_reached(); - } + cs->exception_index = POWERPC_EXCP_ITLB; + env->spr[SPR_BOOKE_DEAR] = eaddr; + env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); break; case -2: /* Access rights violation */ cs->exception_index = POWERPC_EXCP_ISI; - env->error_code = 0; break; case -3: /* No execute protection violation */ cs->exception_index = POWERPC_EXCP_ISI; env->spr[SPR_BOOKE_ESR] = 0; - env->error_code = 0; break; } } else { switch (ret) { case -1: /* No matches in page tables or TLB */ - switch (env->mmu_model) { - case POWERPC_MMU_BOOKE206: - booke206_update_mas_tlb_miss(env, eaddr, access_type, mmu_idx); - /* fall through */ - case POWERPC_MMU_BOOKE: - cs->exception_index = POWERPC_EXCP_DTLB; - env->error_code = 0; - env->spr[SPR_BOOKE_DEAR] = eaddr; - env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); - break; - default: - g_assert_not_reached(); - } + cs->exception_index = POWERPC_EXCP_DTLB; + env->spr[SPR_BOOKE_DEAR] = eaddr; + env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); break; case -2: /* Access rights violation */ cs->exception_index = POWERPC_EXCP_DSI; - env->error_code = 0; env->spr[SPR_BOOKE_DEAR] = eaddr; env->spr[SPR_BOOKE_ESR] = mmubooke206_esr(mmu_idx, access_type); break;
Move setting error_code that appears in every case out in front and hoist the common fall through case for BOOKE206 as well which allows removing the nested switches. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- target/ppc/mmu_common.c | 41 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 29 deletions(-)