[PATCH] target/arm: Fix debugging of ARMv8M Secure code

[PATCH] target/arm: Fix debugging of ARMv8M Secure code

[pbartell]

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,
-    };
     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) {
-- 
2.37.3

Re: [PATCH] target/arm: Fix debugging of ARMv8M Secure code

[Richard Henderson]

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~

Re: [PATCH] target/arm: Fix debugging of ARMv8M Secure code

[Bartell, Paul]

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~

Re: [PATCH] target/arm: Fix debugging of ARMv8M Secure code

[Peter Maydell]

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

Re: [PATCH] target/arm: Fix debugging of ARMv8M Secure code

[Bartell, Paul]

> 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.

Re: [PATCH] target/arm: Fix debugging of ARMv8M Secure code

[Peter Maydell]

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