[PATCH 0/2] gdb invalid memory access handling improvements

[PATCH 0/2] gdb invalid memory access handling improvements

[Nicholas Piggin]

This adds .debug=1 attribute for GDB's phys mem access mode, adds
memory transaction error handling for it so it reports cannot access
memory instead of silent success, and silences warning logs for
invalid memory access coming from the debugger.

Thanks,
Nick

Nicholas Piggin (2):
  gdbstub: Add phys_memory_rw_debug for physical memory access
  memory: suppress INVALID_MEM logs caused by debug access

 docs/devel/loads-stores.rst | 11 +++++++++++
 include/exec/cpu-common.h   |  3 +++
 gdbstub/system.c            |  7 +------
 system/memory.c             | 37 ++++++++++++++++++++++---------------
 system/physmem.c            | 16 ++++++++++++++++
 5 files changed, 53 insertions(+), 21 deletions(-)

-- 
2.47.1

[PATCH 1/2] gdbstub: Add phys_memory_rw_debug for physical memory access

[Nicholas Piggin]

Add an accessor for gdb physical memory access mode which sets the
the .debug attribute for the MemTxAttribute, and also returns success
to the caller.

GDB with PhyMemMode will now report failure from memory accesses outside
valid system memory addresses, and it is also able to write to ROMs as
GDB virtual memory access can.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 docs/devel/loads-stores.rst | 11 +++++++++++
 include/exec/cpu-common.h   |  3 +++
 gdbstub/system.c            |  7 +------
 system/physmem.c            | 16 ++++++++++++++++
 4 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
index 9471bac8599..ac2e0d34d67 100644
--- a/docs/devel/loads-stores.rst
+++ b/docs/devel/loads-stores.rst
@@ -481,6 +481,17 @@ would ignore the write attempt).
 
 ``cpu_memory_rw_debug``
 
+``phys_memory_rw_debug``
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Access system memory by physical address for debug purposes.
+
+This function is intended for use by the GDB stub and similar code.
+It takes a physical address and operates on the system address space.
+Access is performed as in cpu_memory_rw_debug().
+
+``phys_memory_rw_debug``
+
 ``dma_memory_*``
 ~~~~~~~~~~~~~~~~
 
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 3771b2130c2..6429dc2331e 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -181,6 +181,9 @@ int ram_block_discard_guest_memfd_range(RAMBlock *rb, uint64_t start,
 /* Returns: 0 on success, -1 on error */
 int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
                         void *ptr, size_t len, bool is_write);
+/* Returns: 0 on success, -1 on error */
+int phys_memory_rw_debug(hwaddr addr, void *buf,
+                         hwaddr len, bool is_write);
 
 /* vl.c */
 void list_cpus(void);
diff --git a/gdbstub/system.c b/gdbstub/system.c
index dd22ff0fb3a..79fcb30f6f0 100644
--- a/gdbstub/system.c
+++ b/gdbstub/system.c
@@ -457,12 +457,7 @@ int gdb_target_memory_rw_debug(CPUState *cpu, hwaddr addr,
                                uint8_t *buf, int len, bool is_write)
 {
     if (phy_memory_mode) {
-        if (is_write) {
-            cpu_physical_memory_write(addr, buf, len);
-        } else {
-            cpu_physical_memory_read(addr, buf, len);
-        }
-        return 0;
+        return phys_memory_rw_debug(addr, buf, len, is_write);
     }
 
     if (cpu->cc->memory_rw_debug) {
diff --git a/system/physmem.c b/system/physmem.c
index e97de3ef65c..aa78b0d514b 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3743,6 +3743,22 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
     return 0;
 }
 
+/* physical memory access for debug (includes writing to ROM) */
+int phys_memory_rw_debug(hwaddr addr, void *buf,
+                         hwaddr len, bool is_write)
+{
+    MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+    MemTxResult res;
+
+    attrs.debug = 1;
+    res = address_space_rw(&address_space_memory, addr, attrs,
+                           buf, len, is_write);
+    if (res != MEMTX_OK) {
+        return -1;
+    }
+    return 0;
+}
+
 bool cpu_physical_memory_is_io(hwaddr phys_addr)
 {
     MemoryRegion*mr;
-- 
2.47.1

Re: [PATCH 1/2] gdbstub: Add phys_memory_rw_debug for physical memory access

[Richard Henderson]

On 3/14/25 00:41, Nicholas Piggin wrote:
> Add an accessor for gdb physical memory access mode which sets the
> the .debug attribute for the MemTxAttribute, and also returns success
> to the caller.
> 
> GDB with PhyMemMode will now report failure from memory accesses outside
> valid system memory addresses, and it is also able to write to ROMs as
> GDB virtual memory access can.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   docs/devel/loads-stores.rst | 11 +++++++++++
>   include/exec/cpu-common.h   |  3 +++
>   gdbstub/system.c            |  7 +------
>   system/physmem.c            | 16 ++++++++++++++++
>   4 files changed, 31 insertions(+), 6 deletions(-)
> 

I think you might as well put this function in gdbstub/system.c
and not export (or document) it.


r~

Re: [PATCH 1/2] gdbstub: Add phys_memory_rw_debug for physical memory access

[Nicholas Piggin]

On Sat Mar 15, 2025 at 7:19 AM AEST, Richard Henderson wrote:
> On 3/14/25 00:41, Nicholas Piggin wrote:
>> Add an accessor for gdb physical memory access mode which sets the
>> the .debug attribute for the MemTxAttribute, and also returns success
>> to the caller.
>> 
>> GDB with PhyMemMode will now report failure from memory accesses outside
>> valid system memory addresses, and it is also able to write to ROMs as
>> GDB virtual memory access can.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   docs/devel/loads-stores.rst | 11 +++++++++++
>>   include/exec/cpu-common.h   |  3 +++
>>   gdbstub/system.c            |  7 +------
>>   system/physmem.c            | 16 ++++++++++++++++
>>   4 files changed, 31 insertions(+), 6 deletions(-)
>> 
>
> I think you might as well put this function in gdbstub/system.c
> and not export (or document) it.

A possible advantage this way is gdbstub not knowing precise
details of the memory transaction (i.e., .debug = 1), but I
can do that. Will submit a v2.

Thanks,
Nick

[PATCH 2/2] memory: suppress INVALID_MEM logs caused by debug access

[Nicholas Piggin]

Debugger-driven invalid memory accesses are not guest errors, so should
not cause these error logs.

Debuggers can access memory wildly, including access to addresses not
specified by the user (e.g., gdb it might try to walk the stack or load
target addresses to display disassembly). Failure is reported
synchronously by the GDB protcol so the user can be notified via the
debugger client.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 system/memory.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 4c829793a0a..960f66e8d7e 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1412,18 +1412,23 @@ bool memory_region_access_valid(MemoryRegion *mr,
 {
     if (mr->ops->valid.accepts
         && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
-        qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
-                      ", size %u, region '%s', reason: rejected\n",
-                      is_write ? "write" : "read",
-                      addr, size, memory_region_name(mr));
+        if (attrs.debug) {
+            /* Don't log memory errors due to debugger accesses */
+            qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
+                          ", size %u, region '%s', reason: rejected\n",
+                          is_write ? "write" : "read",
+                          addr, size, memory_region_name(mr));
+        }
         return false;
     }
 
     if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
-        qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
-                      ", size %u, region '%s', reason: unaligned\n",
-                      is_write ? "write" : "read",
-                      addr, size, memory_region_name(mr));
+        if (attrs.debug) {
+            qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
+                          ", size %u, region '%s', reason: unaligned\n",
+                          is_write ? "write" : "read",
+                          addr, size, memory_region_name(mr));
+        }
         return false;
     }
 
@@ -1434,13 +1439,15 @@ bool memory_region_access_valid(MemoryRegion *mr,
 
     if (size > mr->ops->valid.max_access_size
         || size < mr->ops->valid.min_access_size) {
-        qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
-                      ", size %u, region '%s', reason: invalid size "
-                      "(min:%u max:%u)\n",
-                      is_write ? "write" : "read",
-                      addr, size, memory_region_name(mr),
-                      mr->ops->valid.min_access_size,
-                      mr->ops->valid.max_access_size);
+        if (attrs.debug) {
+            qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
+                          ", size %u, region '%s', reason: invalid size "
+                          "(min:%u max:%u)\n",
+                          is_write ? "write" : "read",
+                          addr, size, memory_region_name(mr),
+                          mr->ops->valid.min_access_size,
+                          mr->ops->valid.max_access_size);
+        }
         return false;
     }
     return true;
-- 
2.47.1

Re: [PATCH 2/2] memory: suppress INVALID_MEM logs caused by debug access

[Philippe Mathieu-Daudé]

On 14/3/25 08:41, Nicholas Piggin wrote:
> Debugger-driven invalid memory accesses are not guest errors, so should
> not cause these error logs.
> 
> Debuggers can access memory wildly, including access to addresses not
> specified by the user (e.g., gdb it might try to walk the stack or load
> target addresses to display disassembly). Failure is reported
> synchronously by the GDB protcol so the user can be notified via the
> debugger client.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   system/memory.c | 37 ++++++++++++++++++++++---------------
>   1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/system/memory.c b/system/memory.c
> index 4c829793a0a..960f66e8d7e 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1412,18 +1412,23 @@ bool memory_region_access_valid(MemoryRegion *mr,
>   {

Should we instead consider debug accesses as always valid? i.e.:

         if (attrs.debug) {
             return true;
         }

>       if (mr->ops->valid.accepts
>           && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> -        qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
> -                      ", size %u, region '%s', reason: rejected\n",
> -                      is_write ? "write" : "read",
> -                      addr, size, memory_region_name(mr));
> +        if (attrs.debug) {
> +            /* Don't log memory errors due to debugger accesses */
> +            qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
> +                          ", size %u, region '%s', reason: rejected\n",
> +                          is_write ? "write" : "read",
> +                          addr, size, memory_region_name(mr));
> +        }
>           return false;
>       }
>   
>       if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> -        qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
> -                      ", size %u, region '%s', reason: unaligned\n",
> -                      is_write ? "write" : "read",
> -                      addr, size, memory_region_name(mr));
> +        if (attrs.debug) {
> +            qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
> +                          ", size %u, region '%s', reason: unaligned\n",
> +                          is_write ? "write" : "read",
> +                          addr, size, memory_region_name(mr));
> +        }
>           return false;
>       }
>   
> @@ -1434,13 +1439,15 @@ bool memory_region_access_valid(MemoryRegion *mr,
>   
>       if (size > mr->ops->valid.max_access_size
>           || size < mr->ops->valid.min_access_size) {
> -        qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
> -                      ", size %u, region '%s', reason: invalid size "
> -                      "(min:%u max:%u)\n",
> -                      is_write ? "write" : "read",
> -                      addr, size, memory_region_name(mr),
> -                      mr->ops->valid.min_access_size,
> -                      mr->ops->valid.max_access_size);
> +        if (attrs.debug) {
> +            qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
> +                          ", size %u, region '%s', reason: invalid size "
> +                          "(min:%u max:%u)\n",
> +                          is_write ? "write" : "read",
> +                          addr, size, memory_region_name(mr),
> +                          mr->ops->valid.min_access_size,
> +                          mr->ops->valid.max_access_size);
> +        }
>           return false;
>       }
>       return true;

Re: [PATCH 2/2] memory: suppress INVALID_MEM logs caused by debug access

[Richard Henderson]

On 3/14/25 08:24, Philippe Mathieu-Daudé wrote:
> On 14/3/25 08:41, Nicholas Piggin wrote:
>> Debugger-driven invalid memory accesses are not guest errors, so should
>> not cause these error logs.
>>
>> Debuggers can access memory wildly, including access to addresses not
>> specified by the user (e.g., gdb it might try to walk the stack or load
>> target addresses to display disassembly). Failure is reported
>> synchronously by the GDB protcol so the user can be notified via the
>> debugger client.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   system/memory.c | 37 ++++++++++++++++++++++---------------
>>   1 file changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/system/memory.c b/system/memory.c
>> index 4c829793a0a..960f66e8d7e 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -1412,18 +1412,23 @@ bool memory_region_access_valid(MemoryRegion *mr,
>>   {
> 
> Should we instead consider debug accesses as always valid? i.e.:
> 
>          if (attrs.debug) {
>              return true;
>          }

No.  You're likely to hit assertions in the device code.


r~

Re: [PATCH 2/2] memory: suppress INVALID_MEM logs caused by debug access

[Philippe Mathieu-Daudé]

On 14/3/25 08:41, Nicholas Piggin wrote:
> Debugger-driven invalid memory accesses are not guest errors, so should
> not cause these error logs.
> 
> Debuggers can access memory wildly, including access to addresses not
> specified by the user (e.g., gdb it might try to walk the stack or load
> target addresses to display disassembly). Failure is reported
> synchronously by the GDB protcol so the user can be notified via the
> debugger client.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   system/memory.c | 37 ++++++++++++++++++++++---------------
>   1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/system/memory.c b/system/memory.c
> index 4c829793a0a..960f66e8d7e 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1412,18 +1412,23 @@ bool memory_region_access_valid(MemoryRegion *mr,
>   {

Alternatively:

         int invalid_mem_mask = attrs.debug ? LOG_INVALID_MEM : 0;

>       if (mr->ops->valid.accepts
>           && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> -        qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
> -                      ", size %u, region '%s', reason: rejected\n",
> -                      is_write ? "write" : "read",
> -                      addr, size, memory_region_name(mr));
> +        if (attrs.debug) {
> +            /* Don't log memory errors due to debugger accesses */
> +            qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
> +                          ", size %u, region '%s', reason: rejected\n",
> +                          is_write ? "write" : "read",
> +                          addr, size, memory_region_name(mr));
> +        }
>           return false;
>       }

Re: [PATCH 2/2] memory: suppress INVALID_MEM logs caused by debug access

[Nicholas Piggin]

On Mon Mar 17, 2025 at 7:03 PM AEST, Philippe Mathieu-Daudé wrote:
> On 14/3/25 08:41, Nicholas Piggin wrote:
>> Debugger-driven invalid memory accesses are not guest errors, so should
>> not cause these error logs.
>> 
>> Debuggers can access memory wildly, including access to addresses not
>> specified by the user (e.g., gdb it might try to walk the stack or load
>> target addresses to display disassembly). Failure is reported
>> synchronously by the GDB protcol so the user can be notified via the
>> debugger client.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   system/memory.c | 37 ++++++++++++++++++++++---------------
>>   1 file changed, 22 insertions(+), 15 deletions(-)
>> 
>> diff --git a/system/memory.c b/system/memory.c
>> index 4c829793a0a..960f66e8d7e 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -1412,18 +1412,23 @@ bool memory_region_access_valid(MemoryRegion *mr,
>>   {
>
> Alternatively:
>
>          int invalid_mem_mask = attrs.debug ? LOG_INVALID_MEM : 0;

Oh that's a thing? Would save a level of indent and might look
nicer. (I guess you have x : y expressions reversed)

Thanks,
Nick

>
>>       if (mr->ops->valid.accepts
>>           && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
>> -        qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
>> -                      ", size %u, region '%s', reason: rejected\n",
>> -                      is_write ? "write" : "read",
>> -                      addr, size, memory_region_name(mr));
>> +        if (attrs.debug) {
>> +            /* Don't log memory errors due to debugger accesses */
>> +            qemu_log_mask(LOG_INVALID_MEM, "Invalid %s at addr 0x%" HWADDR_PRIX
>> +                          ", size %u, region '%s', reason: rejected\n",
>> +                          is_write ? "write" : "read",
>> +                          addr, size, memory_region_name(mr));
>> +        }
>>           return false;
>>       }

Re: [PATCH 0/2] gdb invalid memory access handling improvements

[David Hildenbrand]

On 14.03.25 08:41, Nicholas Piggin wrote:
> This adds .debug=1 attribute for GDB's phys mem access mode, adds
> memory transaction error handling for it so it reports cannot access
> memory instead of silent success, and silences warning logs for
> invalid memory access coming from the debugger.

Nothing jumped at me, with Richard's suggestion of keeping the new 
function limited in scope

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb