1. Patchew
  2. QEMU
  3. [PATCH 0/2] gdb invalid memory access handling improvements
  4. View patch

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

Nicholas Piggin posted 2 patches 2 weeks, 6 days ago
There is a newer version of this series
[PATCH 2/2] memory: suppress INVALID_MEM logs caused by debug access
Posted by Nicholas Piggin 2 weeks, 6 days ago 
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
Posted by Philippe Mathieu-Daudé 2 weeks, 3 days ago 
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
Posted by Nicholas Piggin 2 weeks, 3 days ago 
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 2/2] memory: suppress INVALID_MEM logs caused by debug access
Posted by Philippe Mathieu-Daudé 2 weeks, 6 days ago 
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
Posted by Richard Henderson 2 weeks, 6 days ago 
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~

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.