[Qemu-devel] [PATCH 1/2] PPC: Add dump_mmu() for 6xx

[Qemu-devel] [PATCH 1/2] PPC: Add dump_mmu() for 6xx

[Fabien Chouteau]

"(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) {
+        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:
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/2] PPC: Fix GDB read on code area for PPC6xx

[Fabien Chouteau]

On PPC 6xx, data and code have separated TLBs. Until now QEMU was only
looking at data TLBs, which is not good when GDB wants to read code.

This patch adds a second call to get_physical_address() with an
ACCESS_CODE type of access when the first call with ACCESS_INT fails.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 target-ppc/mmu_helper.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 910e022..19f0b8c 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -1378,7 +1378,15 @@ hwaddr cpu_get_phys_page_debug(CPUPPCState *env, target_ulong addr)
     }
 
     if (unlikely(get_physical_address(env, &ctx, addr, 0, ACCESS_INT) != 0)) {
-        return -1;
+
+        /* Some MMUs have separate TLBs for code and data. If we only try an
+         * ACCESS_INT, we may not be able to read instructions mapped by code
+         * TLBs, so we also try a ACCESS_CODE.
+         */
+        if (unlikely(get_physical_address(env, &ctx, addr, 0,
+                                          ACCESS_CODE) != 0)) {
+            return -1;
+        }
     }
 
     return ctx.raddr & TARGET_PAGE_MASK;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/2] PPC: Fix GDB read on code area for PPC6xx

[Alexander Graf]

On 18.06.2013, at 16:53, Fabien Chouteau wrote:

> On PPC 6xx, data and code have separated TLBs. Until now QEMU was only
> looking at data TLBs, which is not good when GDB wants to read code.
> 
> This patch adds a second call to get_physical_address() with an
> ACCESS_CODE type of access when the first call with ACCESS_INT fails.
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
> target-ppc/mmu_helper.c |   10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 910e022..19f0b8c 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -1378,7 +1378,15 @@ hwaddr cpu_get_phys_page_debug(CPUPPCState *env, target_ulong addr)
>     }
> 
>     if (unlikely(get_physical_address(env, &ctx, addr, 0, ACCESS_INT) != 0)) {
> -        return -1;
> +
> +        /* Some MMUs have separate TLBs for code and data. If we only try an
> +         * ACCESS_INT, we may not be able to read instructions mapped by code
> +         * TLBs, so we also try a ACCESS_CODE.

This is pretty ugly, but I don't see any other way to conveniently give gdb the information it needs. Let's ask Jan whether he has an idea. Maybe a monitor command to switch memory access modes?


Alex

> +         */
> +        if (unlikely(get_physical_address(env, &ctx, addr, 0,
> +                                          ACCESS_CODE) != 0)) {
> +            return -1;
> +        }
>     }
> 
>     return ctx.raddr & TARGET_PAGE_MASK;
> -- 
> 1.7.9.5
> 

Re: [Qemu-devel] [PATCH 2/2] PPC: Fix GDB read on code area for PPC6xx

[Jan Kiszka]

On 2013-06-18 17:34, Alexander Graf wrote:
> 
> On 18.06.2013, at 16:53, Fabien Chouteau wrote:
> 
>> On PPC 6xx, data and code have separated TLBs. Until now QEMU was only
>> looking at data TLBs, which is not good when GDB wants to read code.
>>
>> This patch adds a second call to get_physical_address() with an
>> ACCESS_CODE type of access when the first call with ACCESS_INT fails.
>>
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>> ---
>> target-ppc/mmu_helper.c |   10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
>> index 910e022..19f0b8c 100644
>> --- a/target-ppc/mmu_helper.c
>> +++ b/target-ppc/mmu_helper.c
>> @@ -1378,7 +1378,15 @@ hwaddr cpu_get_phys_page_debug(CPUPPCState *env, target_ulong addr)
>>     }
>>
>>     if (unlikely(get_physical_address(env, &ctx, addr, 0, ACCESS_INT) != 0)) {
>> -        return -1;
>> +
>> +        /* Some MMUs have separate TLBs for code and data. If we only try an
>> +         * ACCESS_INT, we may not be able to read instructions mapped by code
>> +         * TLBs, so we also try a ACCESS_CODE.
> 
> This is pretty ugly, but I don't see any other way to conveniently give gdb the information it needs. Let's ask Jan whether he has an idea. Maybe a monitor command to switch memory access modes?

I suppose the gdb frontend is not willing to tell us what kind of memory
it accesses (code disassembling or data access etc.), right? So we can
only guess here, ie. try both. I don't see how a monitor-based side
channel could help. If we touched gdb, we could also touch the remote
protocol (for memory accesses).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/2] PPC: Fix GDB read on code area for PPC6xx

[Alexander Graf]

On 21.06.2013, at 20:10, Jan Kiszka wrote:

> On 2013-06-18 17:34, Alexander Graf wrote:
>> 
>> On 18.06.2013, at 16:53, Fabien Chouteau wrote:
>> 
>>> On PPC 6xx, data and code have separated TLBs. Until now QEMU was only
>>> looking at data TLBs, which is not good when GDB wants to read code.
>>> 
>>> This patch adds a second call to get_physical_address() with an
>>> ACCESS_CODE type of access when the first call with ACCESS_INT fails.
>>> 
>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>> ---
>>> target-ppc/mmu_helper.c |   10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
>>> index 910e022..19f0b8c 100644
>>> --- a/target-ppc/mmu_helper.c
>>> +++ b/target-ppc/mmu_helper.c
>>> @@ -1378,7 +1378,15 @@ hwaddr cpu_get_phys_page_debug(CPUPPCState *env, target_ulong addr)
>>>    }
>>> 
>>>    if (unlikely(get_physical_address(env, &ctx, addr, 0, ACCESS_INT) != 0)) {
>>> -        return -1;
>>> +
>>> +        /* Some MMUs have separate TLBs for code and data. If we only try an
>>> +         * ACCESS_INT, we may not be able to read instructions mapped by code
>>> +         * TLBs, so we also try a ACCESS_CODE.
>> 
>> This is pretty ugly, but I don't see any other way to conveniently give gdb the information it needs. Let's ask Jan whether he has an idea. Maybe a monitor command to switch memory access modes?
> 
> I suppose the gdb frontend is not willing to tell us what kind of memory
> it accesses (code disassembling or data access etc.), right? So we can
> only guess here, ie. try both. I don't see how a monitor-based side
> channel could help. If we touched gdb, we could also touch the remote
> protocol (for memory accesses).

Thanks, applied to ppc-next. Teaching gdb a new protocol is way out of scope for these rare cases :).


Alex

Re: [Qemu-devel] [PATCH 2/2] PPC: Fix GDB read on code area for PPC6xx

[Fabien Chouteau]

On 06/22/2013 02:26 AM, Alexander Graf wrote:
> Thanks, applied to ppc-next. Teaching gdb a new protocol is way out of scope for these rare cases :).
> 

Thanks guys,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 1/2] PPC: Add dump_mmu() for 6xx

[Alexander Graf]

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
> 

Re: [Qemu-devel] [PATCH 1/2] PPC: Add dump_mmu() for 6xx

[Fabien Chouteau]

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,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 1/2] PPC: Add dump_mmu() for 6xx

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH 1/2] PPC: Add dump_mmu() for 6xx

[Fabien Chouteau]

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.




-- 
Fabien Chouteau