diff mbox series

target/arm: Fix debugging of ARMv8M Secure code

Message ID 20230408000118.50854-1-pbartell@amazon.com
State New
Headers show
Series target/arm: Fix debugging of ARMv8M Secure code | expand

Commit Message

Bartell, Paul April 8, 2023, 12:01 a.m. UTC 
From: Paul Bartell <pbartell@amazon.com>

Revert changes to arm_cpu_get_phys_page_attrs_debug made in commit
4a35855682cebb89f9630b07aa9fd37c4e8c733b.

Commit 4a35855682 modifies the arm_cpu_get_phys_page_attrs_debug function
so that it calls get_phys_addr_with_struct rather than get_phys_addr, which
leads to a variety of memory access errors when debugging secure state
code on qemu ARMv8M targets with gdb.

This commit fixes a variety of gdb memory access errors including:
"error reading variable" and "Cannot access memory at address" when
attempting to read any memory address via gdb.

Signed-off-by: Paul Bartell <pbartell@amazon.com>
---
 target/arm/ptw.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Richard Henderson April 8, 2023, 4:17 a.m. UTC | #1 
On 4/7/23 17:01, pbartell@amazon.com wrote:
> From: Paul Bartell <pbartell@amazon.com>
> 
> Revert changes to arm_cpu_get_phys_page_attrs_debug made in commit
> 4a35855682cebb89f9630b07aa9fd37c4e8c733b.
> 
> Commit 4a35855682 modifies the arm_cpu_get_phys_page_attrs_debug function
> so that it calls get_phys_addr_with_struct rather than get_phys_addr, which
> leads to a variety of memory access errors when debugging secure state
> code on qemu ARMv8M targets with gdb.
> 
> This commit fixes a variety of gdb memory access errors including:
> "error reading variable" and "Cannot access memory at address" when
> attempting to read any memory address via gdb.
> 
> Signed-off-by: Paul Bartell <pbartell@amazon.com>
> ---
>   target/arm/ptw.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index ec3f51782a..5a1339d38f 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2999,16 +2999,12 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
>   {
>       ARMCPU *cpu = ARM_CPU(cs);
>       CPUARMState *env = &cpu->env;
> -    S1Translate ptw = {
> -        .in_mmu_idx = arm_mmu_idx(env),
> -        .in_secure = arm_is_secure(env),
> -        .in_debug = true,

Nack.  This will now affect vcpu state by changing the contents of the softmmu tlb, as 
well as changing the contents of memory (!) via PTE access/dirty bit updates.

A more complete description of "a variety of ... errors", and the conditions under which 
they are produced, would be appreciated.

r~
Bartell, Paul April 10, 2023, 2:38 p.m. UTC | #2 
You can reproduce the problem by running gdb against an ARMv8M target running secure mode code (the default).

Running qemu with the following arguments : qemu-system-arm -machine mps2-an505 -m 16M -cpu cortex-m33 -nographic -semihosting -monitor null --semihosting-config enable=on,target=native -d guest_errors -kernel /path/to/binary.elf

With the following .gdbinit file:
target extended-remote :1234
compare-sections

Upon startup, every symbol in the elf file reports the following error:

Section .text, range 0x10000000 -- 0x10009298: MIS-MATCHED!
Section .text.main, range 0x10009298 -- 0x10009300: MIS-MATCHED!
Section .text.prvQueueSendTask, range 0x10009300 -- 0x10009338: MIS-MATCHED!

Attempting to debug results in errors constantly (unable to read or write memory at all).

init_data_sections () at /project/Demo/ARM_MPS/startup.c:95
95      {
(gdb) info locals
pCopyTable = <error reading variable pCopyTable (Cannot access memory at address 0x381fffec)>
dataIndex = <error reading variable dataIndex (Cannot access memory at address 0x381fffe8)>

Does that clarify my report sufficiently?

On 4/7/23, 9:18 PM, "Richard Henderson" <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>> wrote:

On 4/7/23 17:01, pbartell@amazon.com <mailto:pbartell@amazon.com> wrote:
> From: Paul Bartell <pbartell@amazon.com <mailto:pbartell@amazon.com>>
>
> Revert changes to arm_cpu_get_phys_page_attrs_debug made in commit
> 4a35855682cebb89f9630b07aa9fd37c4e8c733b.
>
> Commit 4a35855682 modifies the arm_cpu_get_phys_page_attrs_debug function
> so that it calls get_phys_addr_with_struct rather than get_phys_addr, which
> leads to a variety of memory access errors when debugging secure state
> code on qemu ARMv8M targets with gdb.
>
> This commit fixes a variety of gdb memory access errors including:
> "error reading variable" and "Cannot access memory at address" when
> attempting to read any memory address via gdb.
>
> Signed-off-by: Paul Bartell <pbartell@amazon.com <mailto:pbartell@amazon.com>>
> ---
> target/arm/ptw.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index ec3f51782a..5a1339d38f 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -2999,16 +2999,12 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
> {
> ARMCPU *cpu = ARM_CPU(cs);
> CPUARMState *env = &cpu->env;
> - S1Translate ptw = {
> - .in_mmu_idx = arm_mmu_idx(env),
> - .in_secure = arm_is_secure(env),
> - .in_debug = true,


Nack. This will now affect vcpu state by changing the contents of the softmmu tlb, as
well as changing the contents of memory (!) via PTE access/dirty bit updates.


A more complete description of "a variety of ... errors", and the conditions under which
they are produced, would be appreciated.


r~
Peter Maydell April 11, 2023, 3:24 p.m. UTC | #3 
On Mon, 10 Apr 2023 at 15:38, Bartell, Paul <pbartell@amazon.com> wrote:
>
> You can reproduce the problem by running gdb against an ARMv8M target running secure mode code (the default).
>
> Running qemu with the following arguments : qemu-system-arm -machine mps2-an505 -m 16M -cpu cortex-m33 -nographic -semihosting -monitor null --semihosting-config enable=on,target=native -d guest_errors -kernel /path/to/binary.elf
>
> With the following .gdbinit file:
> target extended-remote :1234
> compare-sections
>
> Upon startup, every symbol in the elf file reports the following error:
>
> Section .text, range 0x10000000 -- 0x10009298: MIS-MATCHED!
> Section .text.main, range 0x10009298 -- 0x10009300: MIS-MATCHED!
> Section .text.prvQueueSendTask, range 0x10009300 -- 0x10009338: MIS-MATCHED!
>
> Attempting to debug results in errors constantly (unable to read or write memory at all).
>
> init_data_sections () at /project/Demo/ARM_MPS/startup.c:95
> 95      {
> (gdb) info locals
> pCopyTable = <error reading variable pCopyTable (Cannot access memory at address 0x381fffec)>
> dataIndex = <error reading variable dataIndex (Cannot access memory at address 0x381fffe8)>
>
> Does that clarify my report sufficiently?

Could you (a) file a bug and (b) attach a sample test executable
that demonstrates the problem, please?

thanks
-- PMM
Bartell, Paul April 11, 2023, 6:12 p.m. UTC | #4 
> On Apr 11, 2023, at 8:24 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Mon, 10 Apr 2023 at 15:38, Bartell, Paul <pbartell@amazon.com> wrote:
>> 
>> You can reproduce the problem by running gdb against an ARMv8M target running secure mode code (the default).
>> 
>> Running qemu with the following arguments : qemu-system-arm -machine mps2-an505 -m 16M -cpu cortex-m33 -nographic -semihosting -monitor null --semihosting-config enable=on,target=native -d guest_errors -kernel /path/to/binary.elf
>> 
>> With the following .gdbinit file:
>> target extended-remote :1234
>> compare-sections
>> 
>> Upon startup, every symbol in the elf file reports the following error:
>> 
>> Section .text, range 0x10000000 -- 0x10009298: MIS-MATCHED!
>> Section .text.main, range 0x10009298 -- 0x10009300: MIS-MATCHED!
>> Section .text.prvQueueSendTask, range 0x10009300 -- 0x10009338: MIS-MATCHED!
>> 
>> Attempting to debug results in errors constantly (unable to read or write memory at all).
>> 
>> init_data_sections () at /project/Demo/ARM_MPS/startup.c:95
>> 95      {
>> (gdb) info locals
>> pCopyTable = <error reading variable pCopyTable (Cannot access memory at address 0x381fffec)>
>> dataIndex = <error reading variable dataIndex (Cannot access memory at address 0x381fffe8)>
>> 
>> Does that clarify my report sufficiently?
> 
> Could you (a) file a bug and (b) attach a sample test executable
> that demonstrates the problem, please?
> 
> thanks
> -- PMM

Bug filed at https://gitlab.com/qemu-project/qemu/-/issues/1590 with binary attached and some additional logs.

Adding the qemu-stable list since semihosting and gdb debugging for all ARMv8M targets is broken in the current stable release (v7.2.1) and previous stable (v7.2.0). v7.1.0 is not affected.
Peter Maydell April 12, 2023, 11:51 a.m. UTC | #5 
On Tue, 11 Apr 2023 at 19:12, Bartell, Paul <pbartell@amazon.com> wrote:
> Bug filed at https://gitlab.com/qemu-project/qemu/-/issues/1590 with binary attached and some additional logs.
>
> Adding the qemu-stable list since semihosting and gdb debugging for all ARMv8M targets is broken in the current stable release (v7.2.1) and previous stable (v7.2.0). v7.1.0 is not affected.

For qemu-stable: if we do another v7.2.x then the commit we should
cherry-pick to fix this is 9094f955 (which unfortunately didn't get
the Cc: stable tag when it was applied).

(Thanks to Paul for tracking down this commit.)

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index ec3f51782a..5a1339d38f 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2999,16 +2999,12 @@  hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    S1Translate ptw = {
-        .in_mmu_idx = arm_mmu_idx(env),
-        .in_secure = arm_is_secure(env),
-        .in_debug = true,
-    };
     GetPhysAddrResult res = {};
     ARMMMUFaultInfo fi = {};
+    ARMMMUIdx mmu_idx = arm_mmu_idx(env);
     bool ret;
 
-    ret = get_phys_addr_with_struct(env, &ptw, addr, MMU_DATA_LOAD, &res, &fi);
+    ret = get_phys_addr(env, addr, MMU_DATA_LOAD, mmu_idx, &res, &fi);
     *attrs = res.f.attrs;
 
     if (ret) {