Patchwork [1/2] PPC: Add dump_mmu() for 6xx

login
register
mail settings
Submitter Fabien Chouteau
Date June 18, 2013, 2:53 p.m.
Message ID <1371567181-4917-1-git-send-email-chouteau@adacore.com>
Download mbox | patch
Permalink /patch/252368/
State New
Headers show

Comments

Fabien Chouteau - June 18, 2013, 2:53 p.m.
"(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(+)
Alexander Graf - June 18, 2013, 3:31 p.m.
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
>
Fabien Chouteau - June 18, 2013, 4:04 p.m.
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,
Alexander Graf - June 20, 2013, 11:16 a.m.
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
Fabien Chouteau - June 20, 2013, 1:06 p.m.
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.

Patch

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: