| Message ID | 1371567181-4917-1-git-send-email-chouteau@adacore.com |
|---|---|
| State | New |
| Headers | show |
On 18.06.2013, at 16:53, Fabien Chouteau wrote: > "(qemu) info tlb" is a very useful tool for debugging, so I implemented > the missing 6xx version. > > Signed-off-by: Fabien Chouteau <chouteau@adacore.com> > --- > target-ppc/mmu_helper.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c > index 68d5415..910e022 100644 > --- a/target-ppc/mmu_helper.c > +++ b/target-ppc/mmu_helper.c > @@ -1176,6 +1176,41 @@ static void mmubooke206_dump_mmu(FILE *f, fprintf_function cpu_fprintf, > } > } > > +static void mmu6xx_dump_mmu(FILE *f, fprintf_function cpu_fprintf, > + CPUPPCState *env) > +{ > + ppc6xx_tlb_t *tlb; > + > + int type, way, entry; > + > + if (kvm_enabled() && !env->kvm_sw_tlb) { Please just drop the check for kvm_sw_tlb. Today it's semantically only used on e500 CPUs. I wouldn't like to draw any assumptions into QEMU code that KVM doesn't fulfill yet :). if (kvm_enabled()) { > + cpu_fprintf(f, "Cannot access KVM TLB\n"); > + return; > + } > + > + if (env->id_tlbs != 1) { > + cpu_fprintf(f, "ERROR: 6xx MMU should have separated TLB" > + " for code and data\n"); > + } > + > + cpu_fprintf(f, " [EPN EPN + SIZE]\n"); > + > + for (type = 0; type < 2; type++) You need braces on these. Please run your patch through checkpatch.pl :). > + for (way = 0; way < env->nb_ways; way++) > + for (entry = env->nb_tlb * type + env->tlb_per_way * way; > + entry < (env->nb_tlb * type + env->tlb_per_way * (way + 1)); > + entry++) { > + > + tlb = &env->tlb.tlb6[entry]; > + cpu_fprintf(f, "TLB %02d/%02d %s way:%d %s [" > + TARGET_FMT_lx " " TARGET_FMT_lx "]\n", > + entry % env->nb_tlb, env->nb_tlb, > + type ? "code" : "data", way, > + pte_is_valid(tlb->pte0) ? "valid" : "inval", > + tlb->EPN, tlb->EPN + TARGET_PAGE_SIZE); > + } I thought 6xx and 74xx also support HTAB and SRs? Shouldn't we dump those as well? Alex > +} > + > void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env) > { > switch (env->mmu_model) { > @@ -1185,6 +1220,10 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env) > case POWERPC_MMU_BOOKE206: > mmubooke206_dump_mmu(f, cpu_fprintf, env); > break; > + case POWERPC_MMU_SOFT_6xx: > + case POWERPC_MMU_SOFT_74xx: > + mmu6xx_dump_mmu(f, cpu_fprintf, env); > + break; > #if defined(TARGET_PPC64) > case POWERPC_MMU_64B: > case POWERPC_MMU_2_06: > -- > 1.7.9.5 >
On 06/18/2013 05:31 PM, Alexander Graf wrote: >> + for (type = 0; type < 2; type++) > > You need braces on these. Please run your patch through checkpatch.pl :). I did ;) -> ./scripts/checkpatch.pl 0001-PPC-Add-dump_mmu-for-6xx.patch total: 0 errors, 0 warnings, 51 lines checked 0001-PPC-Add-dump_mmu-for-6xx.patch has no obvious style problems and is ready for submission. > >> + for (way = 0; way < env->nb_ways; way++) >> + for (entry = env->nb_tlb * type + env->tlb_per_way * way; >> + entry < (env->nb_tlb * type + env->tlb_per_way * (way + 1)); >> + entry++) { >> + >> + tlb = &env->tlb.tlb6[entry]; >> + cpu_fprintf(f, "TLB %02d/%02d %s way:%d %s [" >> + TARGET_FMT_lx " " TARGET_FMT_lx "]\n", >> + entry % env->nb_tlb, env->nb_tlb, >> + type ? "code" : "data", way, >> + pte_is_valid(tlb->pte0) ? "valid" : "inval", >> + tlb->EPN, tlb->EPN + TARGET_PAGE_SIZE); >> + } > > I thought 6xx and 74xx also support HTAB and SRs? Shouldn't we dump those as well? > I don't know what that is, can you send me an example of what the printf line should be? Thanks for the review,
On 18.06.2013, at 18:04, Fabien Chouteau wrote: > On 06/18/2013 05:31 PM, Alexander Graf wrote: >>> + for (type = 0; type < 2; type++) >> >> You need braces on these. Please run your patch through checkpatch.pl :). > > I did ;) > > -> ./scripts/checkpatch.pl 0001-PPC-Add-dump_mmu-for-6xx.patch > total: 0 errors, 0 warnings, 51 lines checked > > 0001-PPC-Add-dump_mmu-for-6xx.patch has no obvious style problems and is ready for submission. Meh - broken script :). According to the CODING_STYLE convention all of the above need to be cluttered with braces ;). > >> >>> + for (way = 0; way < env->nb_ways; way++) >>> + for (entry = env->nb_tlb * type + env->tlb_per_way * way; >>> + entry < (env->nb_tlb * type + env->tlb_per_way * (way + 1)); >>> + entry++) { >>> + >>> + tlb = &env->tlb.tlb6[entry]; >>> + cpu_fprintf(f, "TLB %02d/%02d %s way:%d %s [" >>> + TARGET_FMT_lx " " TARGET_FMT_lx "]\n", >>> + entry % env->nb_tlb, env->nb_tlb, >>> + type ? "code" : "data", way, >>> + pte_is_valid(tlb->pte0) ? "valid" : "inval", >>> + tlb->EPN, tlb->EPN + TARGET_PAGE_SIZE); >>> + } >> >> I thought 6xx and 74xx also support HTAB and SRs? Shouldn't we dump those as well? >> > > I don't know what that is, can you send me an example of what the printf line should be? SRs are similar to the SLB that book3s_64 print out. Just that there are a fixed smaller number of them (16). Basically you'd dump the env->sr array, similar to how the debug functions in get_segment_6xx_tlb() dump it. For the HTAB I think SDR1 should be enough, so you don't need to do too much here. If you like, you can just dump the decoded fields env->htab_base and env->htab_mask. Dumping the whole HTAB would just explode the output. However, you also should definitely dump all (valid) BATs. Check out get_bat_6xx_tlb() for debug code that dumps BATs. Alex
On 06/20/2013 01:16 PM, Alexander Graf wrote: > > On 18.06.2013, at 18:04, Fabien Chouteau wrote: > >> On 06/18/2013 05:31 PM, Alexander Graf wrote: >>>> + for (type = 0; type < 2; type++) >>> >>> You need braces on these. Please run your patch through checkpatch.pl :). >> >> I did ;) >> >> -> ./scripts/checkpatch.pl 0001-PPC-Add-dump_mmu-for-6xx.patch >> total: 0 errors, 0 warnings, 51 lines checked >> >> 0001-PPC-Add-dump_mmu-for-6xx.patch has no obvious style problems and is ready for submission. > > Meh - broken script :). According to the CODING_STYLE convention all of the above need to be cluttered with braces ;). Will do. >>>> + for (way = 0; way < env->nb_ways; way++) >>>> + for (entry = env->nb_tlb * type + env->tlb_per_way * way; >>>> + entry < (env->nb_tlb * type + env->tlb_per_way * (way + 1)); >>>> + entry++) { >>>> + >>>> + tlb = &env->tlb.tlb6[entry]; >>>> + cpu_fprintf(f, "TLB %02d/%02d %s way:%d %s [" >>>> + TARGET_FMT_lx " " TARGET_FMT_lx "]\n", >>>> + entry % env->nb_tlb, env->nb_tlb, >>>> + type ? "code" : "data", way, >>>> + pte_is_valid(tlb->pte0) ? "valid" : "inval", >>>> + tlb->EPN, tlb->EPN + TARGET_PAGE_SIZE); >>>> + } >>> >>> I thought 6xx and 74xx also support HTAB and SRs? Shouldn't we dump those as well? >>> >> >> I don't know what that is, can you send me an example of what the printf line should be? > > SRs are similar to the SLB that book3s_64 print out. Just that there are a fixed smaller number of them (16). Basically you'd dump the env->sr array, similar to how the debug functions in get_segment_6xx_tlb() dump it. > > For the HTAB I think SDR1 should be enough, so you don't need to do too much here. If you like, you can just dump the decoded fields env->htab_base and env->htab_mask. Dumping the whole HTAB would just explode the output. > > However, you also should definitely dump all (valid) BATs. Check out get_bat_6xx_tlb() for debug code that dumps BATs. > Ok I'll have a look at that, and BATs should definitely be printed out.
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c index 68d5415..910e022 100644 --- a/target-ppc/mmu_helper.c +++ b/target-ppc/mmu_helper.c @@ -1176,6 +1176,41 @@ static void mmubooke206_dump_mmu(FILE *f, fprintf_function cpu_fprintf, } } +static void mmu6xx_dump_mmu(FILE *f, fprintf_function cpu_fprintf, + CPUPPCState *env) +{ + ppc6xx_tlb_t *tlb; + + int type, way, entry; + + if (kvm_enabled() && !env->kvm_sw_tlb) { + cpu_fprintf(f, "Cannot access KVM TLB\n"); + return; + } + + if (env->id_tlbs != 1) { + cpu_fprintf(f, "ERROR: 6xx MMU should have separated TLB" + " for code and data\n"); + } + + cpu_fprintf(f, " [EPN EPN + SIZE]\n"); + + for (type = 0; type < 2; type++) + for (way = 0; way < env->nb_ways; way++) + for (entry = env->nb_tlb * type + env->tlb_per_way * way; + entry < (env->nb_tlb * type + env->tlb_per_way * (way + 1)); + entry++) { + + tlb = &env->tlb.tlb6[entry]; + cpu_fprintf(f, "TLB %02d/%02d %s way:%d %s [" + TARGET_FMT_lx " " TARGET_FMT_lx "]\n", + entry % env->nb_tlb, env->nb_tlb, + type ? "code" : "data", way, + pte_is_valid(tlb->pte0) ? "valid" : "inval", + tlb->EPN, tlb->EPN + TARGET_PAGE_SIZE); + } +} + void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env) { switch (env->mmu_model) { @@ -1185,6 +1220,10 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env) case POWERPC_MMU_BOOKE206: mmubooke206_dump_mmu(f, cpu_fprintf, env); break; + case POWERPC_MMU_SOFT_6xx: + case POWERPC_MMU_SOFT_74xx: + mmu6xx_dump_mmu(f, cpu_fprintf, env); + break; #if defined(TARGET_PPC64) case POWERPC_MMU_64B: case POWERPC_MMU_2_06:
"(qemu) info tlb" is a very useful tool for debugging, so I implemented the missing 6xx version. Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- target-ppc/mmu_helper.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)