Series comparison

-[PATCH 0/2] gdb invalid memory access handling improvements
+[PATCH v2 0/2] gdb invalid memory access handling improvements
 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.
+Changes since v1:
+- Move phys_memory_rw_debug() into gdbstub/system.c and update patch
+  changelog to match.
 Thanks,
 Nick
 Nicholas Piggin (2):
-  gdbstub: Add phys_memory_rw_debug for physical memory access
+  gdbstub: Improve physical memory access handling
   memory: suppress INVALID_MEM logs caused by debug access
- docs/devel/loads-stores.rst | 11 +++++++++++
+ gdbstub/system.c | 27 +++++++++++++++++++++------
- include/exec/cpu-common.h   |  3 +++
+ system/memory.c  | 37 ++++++++++++++++++++++---------------
- gdbstub/system.c            |  7 +------
+files changed, 43 insertions(+), 21 deletions(-)
  system/memory.c             | 37 ++++++++++++++++++++++---------------
  system/physmem.c            | 16 ++++++++++++++++
 files changed, 53 insertions(+), 21 deletions(-)
 --
 .47.1

-[PATCH 1/2] gdbstub: Add phys_memory_rw_debug for physical memory access
+[PATCH v2 1/2] gdbstub: Improve physical memory access handling
-Add an accessor for gdb physical memory access mode which sets the
+Bring gdb's physical memory access handling up to speed with the CPU
-the .debug attribute for the MemTxAttribute, and also returns success
+memory access, by setting MemTxAttribute.debug=1, and by checking for
-to the caller.
+memory transaction errors.
-GDB with PhyMemMode will now report failure from memory accesses outside
+GDB with PhyMemMode will now report failure for memory access outside
 valid system memory addresses, and it is also able to write to ROMs as
-GDB virtual memory access can.
+it can with virtual memory access.
+Reviewed-by: David Hildenbrand <david@redhat.com>
 Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
 ---
- docs/devel/loads-stores.rst | 11 +++++++++++
+ gdbstub/system.c | 27 +++++++++++++++++++++------
- include/exec/cpu-common.h   |  3 +++
+file changed, 21 insertions(+), 6 deletions(-)
  gdbstub/system.c            |  7 +------
  system/physmem.c            | 16 ++++++++++++++++
 files changed, 31 insertions(+), 6 deletions(-)
-diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
-index XXXXXXX..XXXXXXX 100644
---- a/docs/devel/loads-stores.rst
-+++ b/docs/devel/loads-stores.rst
-@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
---- a/include/exec/cpu-common.h
-+++ b/include/exec/cpu-common.h
-@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
 --- a/gdbstub/system.c
 +++ b/gdbstub/system.c
-@@ -XXX,XX +XXX,XX @@ int gdb_target_memory_rw_debug(CPUState *cpu, hwaddr addr,
+@@ -XXX,XX +XXX,XX @@
-                                uint8_t *buf, int len, bool is_write)
+ #include "exec/gdbstub.h"
- {
+ #include "gdbstub/syscalls.h"
-     if (phy_memory_mode) {
+ #include "gdbstub/commands.h"
--        if (is_write) {
++#include "exec/address-spaces.h"
--            cpu_physical_memory_write(addr, buf, len);
+ #include "exec/hwaddr.h"
--        } else {
+ #include "exec/tb-flush.h"
--            cpu_physical_memory_read(addr, buf, len);
+ #include "system/accel-ops.h"
--        }
+@@ -XXX,XX +XXX,XX @@ void gdb_qemu_exit(int code)
--        return 0;
+  */
-+        return phys_memory_rw_debug(addr, buf, len, is_write);
+ static int phy_memory_mode;
-     }
++/*
-     if (cpu->cc->memory_rw_debug) {
++ * Like cpu_memory_rw_debug but it operates on the system address space
-diff --git a/system/physmem.c b/system/physmem.c
++ * rather than the CPU's view of memory.
-index XXXXXXX..XXXXXXX 100644
++ */
---- a/system/physmem.c
++static int phys_memory_rw_debug(hwaddr addr, void *buf,
-+++ b/system/physmem.c
++                                hwaddr len, bool is_write)
@@ -XXX,XX +XXX,XX @@ 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;
 ...
 +        return -1;
 +    }
 +    return 0;
 +}
 +
- bool cpu_physical_memory_is_io(hwaddr phys_addr)
+ int gdb_target_memory_rw_debug(CPUState *cpu, hwaddr addr,
                                 uint8_t *buf, int len, bool is_write)
  {
-     MemoryRegion*mr;
+     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) {
 --
 .47.1

-[PATCH 2/2] memory: suppress INVALID_MEM logs caused by debug access
+[PATCH v2 2/2] memory: suppress INVALID_MEM logs caused by debug access
 ...
 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.
+Reviewed-by: David Hildenbrand <david@redhat.com>
 Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
 ---
  system/memory.c | 37 ++++++++++++++++++++++---------------
 file changed, 22 insertions(+), 15 deletions(-)
 ...

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 XXXXXXX..XXXXXXX 100644
--- a/docs/devel/loads-stores.rst
+++ b/docs/devel/loads-stores.rst
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/gdbstub/system.c
+++ b/gdbstub/system.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -XXX,XX +XXX,XX @@ 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

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 XXXXXXX..XXXXXXX 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -XXX,XX +XXX,XX @@ 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;
     }
 
@@ -XXX,XX +XXX,XX @@ 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

Bring gdb's physical memory access handling up to speed with the CPU
memory access, by setting MemTxAttribute.debug=1, and by checking for
memory transaction errors.

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

Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 gdbstub/system.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/gdbstub/system.c b/gdbstub/system.c
index XXXXXXX..XXXXXXX 100644
--- a/gdbstub/system.c
+++ b/gdbstub/system.c
@@ -XXX,XX +XXX,XX @@
 #include "exec/gdbstub.h"
 #include "gdbstub/syscalls.h"
 #include "gdbstub/commands.h"
+#include "exec/address-spaces.h"
 #include "exec/hwaddr.h"
 #include "exec/tb-flush.h"
 #include "system/accel-ops.h"
@@ -XXX,XX +XXX,XX @@ void gdb_qemu_exit(int code)
  */
 static int phy_memory_mode;
 
+/*
+ * Like cpu_memory_rw_debug but it operates on the system address space
+ * rather than the CPU's view of memory.
+ */
+static 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;
+}
+
 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) {
-- 
2.47.1

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

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