Patchwork [2/2] PPC: Fix GDB read on code area for PPC6xx

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

Comments

Fabien Chouteau - June 18, 2013, 2:53 p.m.
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(-)
Alexander Graf - June 18, 2013, 3:34 p.m.
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
>
Jan Kiszka - June 21, 2013, 6:10 p.m.
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
Alexander Graf - June 22, 2013, 12:26 a.m.
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
Fabien Chouteau - June 24, 2013, 8:40 a.m.
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,

Patch

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;