[PATCH v3 0/7] physmem: teach cpu_memory_rw_debug() to write to more memory regions

[PATCH v3 0/7] physmem: teach cpu_memory_rw_debug() to write to more memory regions

[David Hildenbrand]

This is a follow-up to [1], implementing it by avoiding the use of
address_space_write_rom() in cpu_memory_rw_debug() completely, and
teaching address_space_write() about debug access instead, the can also
write to ROM.

The goal is to let GDB via cpu_memory_rw_debug() to also properly write to
MMIO device regions, not just RAM/ROM.

It's worth noting that other users of address_space_write_rom() are
left unchanged. Maybe hw/core/loader.c and friends could now be converted
to to a debug access via address_space_write() instead?

Survives a basic gitlab CI build/check.

[1] https://lore.kernel.org/all/20241220195923.314208-1-git@zabka.it/

v2 -> v3:
* Rebased, only a minor conflict in the last patch.

v1 -> v2:
 * Split up "physmem: disallow direct access to RAM DEVICE in
   address_space_write_rom()" into 4 patches

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Cc: Jagannathan Raman <jag.raman@oracle.com>
Cc: "Dr. David Alan Gilbert" <dave@treblig.org>
Cc: Stefan Zabka <git@zabka.it>

David Hildenbrand (7):
  physmem: factor out memory_region_is_ram_device() check in
    memory_access_is_direct()
  physmem: factor out RAM/ROMD check in memory_access_is_direct()
  physmem: factor out direct access check into
    memory_region_supports_direct_access()
  physmem: disallow direct access to RAM DEVICE in
    address_space_write_rom()
  memory: pass MemTxAttrs to memory_access_is_direct()
  hmp: use cpu_get_phys_page_debug() in hmp_gva2gpa()
  physmem: teach cpu_memory_rw_debug() to write to more memory regions

 hw/core/cpu-system.c      | 13 +++++++++----
 hw/core/loader.c          |  2 +-
 hw/remote/vfio-user-obj.c |  2 +-
 include/exec/memattrs.h   |  5 ++++-
 include/exec/memory.h     | 35 +++++++++++++++++++++++++++--------
 monitor/hmp-cmds-target.c |  3 +--
 system/memory_ldst.c.inc  | 18 +++++++++---------
 system/physmem.c          | 24 +++++++++---------------
 8 files changed, 61 insertions(+), 41 deletions(-)

-- 
2.48.1

[PATCH v3 1/7] physmem: factor out memory_region_is_ram_device() check in memory_access_is_direct()

[David Hildenbrand]

As documented in commit 4a2e242bbb306 ("memory: Don't use memcpy for
ram_device regions"), we disallow direct access to RAM DEVICE regions.

Let's make this clearer to prepare for further changes. Note that romd
regions will never be RAM DEVICE at the same time.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/exec/memory.h | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9f73b59867..5cd7574c60 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2997,12 +2997,19 @@ bool prepare_mmio_access(MemoryRegion *mr);
 
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
+    /*
+     * RAM DEVICE regions can be accessed directly using memcpy, but it might
+     * be MMIO and access using mempy can be wrong (e.g., using instructions not
+     * intended for MMIO access). So we treat this as IO.
+     */
+    if (memory_region_is_ram_device(mr)) {
+        return false;
+    }
     if (is_write) {
         return memory_region_is_ram(mr) && !mr->readonly &&
-               !mr->rom_device && !memory_region_is_ram_device(mr);
+               !mr->rom_device;
     } else {
-        return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) ||
-               memory_region_is_romd(mr);
+        return memory_region_is_ram(mr) || memory_region_is_romd(mr);
     }
 }
 
-- 
2.48.1

[PATCH v3 2/7] physmem: factor out RAM/ROMD check in memory_access_is_direct()

[David Hildenbrand]

Let's factor more of the generic "is this directly accessible" check,
independent of the "write" condition out.

Note that the "!mr->rom_device" check in the write case essentially
disallows the memory_region_is_romd() condition again. Further note that
RAM DEVICE regions are also RAM regions, so we can check for RAM+ROMD
first.

This is a preparation for further changes.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/exec/memory.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 5cd7574c60..cb35c38402 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2997,6 +2997,10 @@ bool prepare_mmio_access(MemoryRegion *mr);
 
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
+    /* ROM DEVICE regions only allow direct access if in ROMD mode. */
+    if (!memory_region_is_ram(mr) && !memory_region_is_romd(mr)) {
+        return false;
+    }
     /*
      * RAM DEVICE regions can be accessed directly using memcpy, but it might
      * be MMIO and access using mempy can be wrong (e.g., using instructions not
@@ -3006,11 +3010,9 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
         return false;
     }
     if (is_write) {
-        return memory_region_is_ram(mr) && !mr->readonly &&
-               !mr->rom_device;
-    } else {
-        return memory_region_is_ram(mr) || memory_region_is_romd(mr);
+        return !mr->readonly && !mr->rom_device;
     }
+    return true;
 }
 
 /**
-- 
2.48.1

[PATCH v3 3/7] physmem: factor out direct access check into memory_region_supports_direct_access()

[David Hildenbrand]

Let's factor the complete "directly accessible" check independent of
the "write" condition out so we can reuse it next.

We can now split up the checks RAM and ROMD check, so we really only check
for RAM DEVICE in case of RAM -- ROM DEVICE is neither RAM not RAM DEVICE.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/exec/memory.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index cb35c38402..4e2cf95ab6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2995,10 +2995,13 @@ MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
 int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr);
 bool prepare_mmio_access(MemoryRegion *mr);
 
-static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+static inline bool memory_region_supports_direct_access(MemoryRegion *mr)
 {
     /* ROM DEVICE regions only allow direct access if in ROMD mode. */
-    if (!memory_region_is_ram(mr) && !memory_region_is_romd(mr)) {
+    if (memory_region_is_romd(mr)) {
+        return true;
+    }
+    if (!memory_region_is_ram(mr)) {
         return false;
     }
     /*
@@ -3006,7 +3009,12 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
      * be MMIO and access using mempy can be wrong (e.g., using instructions not
      * intended for MMIO access). So we treat this as IO.
      */
-    if (memory_region_is_ram_device(mr)) {
+    return !memory_region_is_ram_device(mr);
+}
+
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+{
+    if (!memory_region_supports_direct_access(mr)) {
         return false;
     }
     if (is_write) {
-- 
2.48.1

[PATCH v3 4/7] physmem: disallow direct access to RAM DEVICE in address_space_write_rom()

[David Hildenbrand]

As documented in commit 4a2e242bbb306 ("memory: Don't use memcpy for
ram_device regions"), we disallow direct access to RAM DEVICE regions.

This change implies that address_space_write_rom() and
cpu_memory_rw_debug() won't be able to write to RAM DEVICE regions. It
will also affect cpu_flush_icache_range(), but it's only used by
hw/core/loader.c after writing to ROM, so it is expected to not apply
here with RAM DEVICE.

This fixes direct access to these regions where we don't want direct
access. We'll extend cpu_memory_rw_debug() next to also be able to write to
these (and IO) regions.

This is a preparation for further changes.

Cc: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 system/physmem.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index 67c9db9daa..7cfcc6cafa 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3137,8 +3137,7 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
         l = len;
         mr = address_space_translate(as, addr, &addr1, &l, true, attrs);
 
-        if (!(memory_region_is_ram(mr) ||
-              memory_region_is_romd(mr))) {
+        if (!memory_region_supports_direct_access(mr)) {
             l = memory_access_size(mr, l, addr1);
         } else {
             /* ROM/RAM case */
-- 
2.48.1

[PATCH v3 5/7] memory: pass MemTxAttrs to memory_access_is_direct()

[David Hildenbrand]

We want to pass another flag that will be stored in MemTxAttrs. So pass
MemTxAttrs directly.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/loader.c          |  2 +-
 hw/remote/vfio-user-obj.c |  2 +-
 include/exec/memory.h     |  5 +++--
 system/memory_ldst.c.inc  | 18 +++++++++---------
 system/physmem.c          | 12 ++++++------
 5 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index fd25c5e01b..332b879a0b 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -144,7 +144,7 @@ ssize_t load_image_mr(const char *filename, MemoryRegion *mr)
 {
     ssize_t size;
 
-    if (!memory_access_is_direct(mr, false)) {
+    if (!memory_access_is_direct(mr, false, MEMTXATTRS_UNSPECIFIED)) {
         /* Can only load an image into RAM or ROM */
         return -1;
     }
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 9e5ff6d87a..6e51a92856 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -358,7 +358,7 @@ static int vfu_object_mr_rw(MemoryRegion *mr, uint8_t *buf, hwaddr offset,
     int access_size;
     uint64_t val;
 
-    if (memory_access_is_direct(mr, is_write)) {
+    if (memory_access_is_direct(mr, is_write, MEMTXATTRS_UNSPECIFIED)) {
         /**
          * Some devices expose a PCI expansion ROM, which could be buffer
          * based as compared to other regions which are primarily based on
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 4e2cf95ab6..b18ecf933e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -3012,7 +3012,8 @@ static inline bool memory_region_supports_direct_access(MemoryRegion *mr)
     return !memory_region_is_ram_device(mr);
 }
 
-static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write,
+                                           MemTxAttrs attrs)
 {
     if (!memory_region_supports_direct_access(mr)) {
         return false;
@@ -3053,7 +3054,7 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
             fv = address_space_to_flatview(as);
             l = len;
             mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
-            if (len == l && memory_access_is_direct(mr, false)) {
+            if (len == l && memory_access_is_direct(mr, false, attrs)) {
                 ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
                 memcpy(buf, ptr, len);
             } else {
diff --git a/system/memory_ldst.c.inc b/system/memory_ldst.c.inc
index 0e6f3940a9..7f32d3d9ff 100644
--- a/system/memory_ldst.c.inc
+++ b/system/memory_ldst.c.inc
@@ -34,7 +34,7 @@ static inline uint32_t glue(address_space_ldl_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, false, attrs);
-    if (l < 4 || !memory_access_is_direct(mr, false)) {
+    if (l < 4 || !memory_access_is_direct(mr, false, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         /* I/O case */
@@ -103,7 +103,7 @@ static inline uint64_t glue(address_space_ldq_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, false, attrs);
-    if (l < 8 || !memory_access_is_direct(mr, false)) {
+    if (l < 8 || !memory_access_is_direct(mr, false, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         /* I/O case */
@@ -170,7 +170,7 @@ uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, false, attrs);
-    if (!memory_access_is_direct(mr, false)) {
+    if (!memory_access_is_direct(mr, false, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         /* I/O case */
@@ -207,7 +207,7 @@ static inline uint16_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, false, attrs);
-    if (l < 2 || !memory_access_is_direct(mr, false)) {
+    if (l < 2 || !memory_access_is_direct(mr, false, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         /* I/O case */
@@ -277,7 +277,7 @@ void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (l < 4 || !memory_access_is_direct(mr, true)) {
+    if (l < 4 || !memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
 
         r = memory_region_dispatch_write(mr, addr1, val, MO_32, attrs);
@@ -314,7 +314,7 @@ static inline void glue(address_space_stl_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (l < 4 || !memory_access_is_direct(mr, true)) {
+    if (l < 4 || !memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
         r = memory_region_dispatch_write(mr, addr1, val,
                                          MO_32 | devend_memop(endian), attrs);
@@ -377,7 +377,7 @@ void glue(address_space_stb, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (!memory_access_is_direct(mr, true)) {
+    if (!memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
         r = memory_region_dispatch_write(mr, addr1, val, MO_8, attrs);
     } else {
@@ -410,7 +410,7 @@ static inline void glue(address_space_stw_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (l < 2 || !memory_access_is_direct(mr, true)) {
+    if (l < 2 || !memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
         r = memory_region_dispatch_write(mr, addr1, val,
                                          MO_16 | devend_memop(endian), attrs);
@@ -474,7 +474,7 @@ static void glue(address_space_stq_internal, SUFFIX)(ARG1_DECL,
 
     RCU_READ_LOCK();
     mr = TRANSLATE(addr, &addr1, &l, true, attrs);
-    if (l < 8 || !memory_access_is_direct(mr, true)) {
+    if (l < 8 || !memory_access_is_direct(mr, true, attrs)) {
         release_lock |= prepare_mmio_access(mr);
         r = memory_region_dispatch_write(mr, addr1, val,
                                          MO_64 | devend_memop(endian), attrs);
diff --git a/system/physmem.c b/system/physmem.c
index 7cfcc6cafa..9766c6d2e0 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -573,7 +573,7 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr addr, hwaddr *xlat,
                                     is_write, true, &as, attrs);
     mr = section.mr;
 
-    if (xen_enabled() && memory_access_is_direct(mr, is_write)) {
+    if (xen_enabled() && memory_access_is_direct(mr, is_write, attrs)) {
         hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
         *plen = MIN(page, *plen);
     }
@@ -2869,7 +2869,7 @@ static MemTxResult flatview_write_continue_step(MemTxAttrs attrs,
         return MEMTX_ACCESS_ERROR;
     }
 
-    if (!memory_access_is_direct(mr, true)) {
+    if (!memory_access_is_direct(mr, true, attrs)) {
         uint64_t val;
         MemTxResult result;
         bool release_lock = prepare_mmio_access(mr);
@@ -2965,7 +2965,7 @@ static MemTxResult flatview_read_continue_step(MemTxAttrs attrs, uint8_t *buf,
         return MEMTX_ACCESS_ERROR;
     }
 
-    if (!memory_access_is_direct(mr, false)) {
+    if (!memory_access_is_direct(mr, false, attrs)) {
         /* I/O case */
         uint64_t val;
         MemTxResult result;
@@ -3274,7 +3274,7 @@ static bool flatview_access_valid(FlatView *fv, hwaddr addr, hwaddr len,
     while (len > 0) {
         l = len;
         mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
-        if (!memory_access_is_direct(mr, is_write)) {
+        if (!memory_access_is_direct(mr, is_write, attrs)) {
             l = memory_access_size(mr, l, addr);
             if (!memory_region_access_valid(mr, xlat, l, is_write, attrs)) {
                 return false;
@@ -3354,7 +3354,7 @@ void *address_space_map(AddressSpace *as,
     fv = address_space_to_flatview(as);
     mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
 
-    if (!memory_access_is_direct(mr, is_write)) {
+    if (!memory_access_is_direct(mr, is_write, attrs)) {
         size_t used = qatomic_read(&as->bounce_buffer_size);
         for (;;) {
             hwaddr alloc = MIN(as->max_bounce_buffer_size - used, l);
@@ -3487,7 +3487,7 @@ int64_t address_space_cache_init(MemoryRegionCache *cache,
 
     mr = cache->mrs.mr;
     memory_region_ref(mr);
-    if (memory_access_is_direct(mr, is_write)) {
+    if (memory_access_is_direct(mr, is_write, MEMTXATTRS_UNSPECIFIED)) {
         /* We don't care about the memory attributes here as we're only
          * doing this if we found actual RAM, which behaves the same
          * regardless of attributes; so UNSPECIFIED is fine.
-- 
2.48.1

Re: [PATCH v3 5/7] memory: pass MemTxAttrs to memory_access_is_direct()

[Peter Xu]

On Mon, Feb 10, 2025 at 09:46:46AM +0100, David Hildenbrand wrote:
> We want to pass another flag that will be stored in MemTxAttrs. So pass
> MemTxAttrs directly.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/core/loader.c          |  2 +-
>  hw/remote/vfio-user-obj.c |  2 +-
>  include/exec/memory.h     |  5 +++--
>  system/memory_ldst.c.inc  | 18 +++++++++---------
>  system/physmem.c          | 12 ++++++------
>  5 files changed, 20 insertions(+), 19 deletions(-)

This breaks mac builds.. I'll squash:

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index aa1455b629..1554f3b801 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -137,7 +137,8 @@ static void apple_gfx_destroy_task(AppleGFXState *s, PGTask_t *task)
                                          MEMTXATTRS_UNSPECIFIED);
 
     if (!ram_region || ram_region_length < length ||
-        !memory_access_is_direct(ram_region, !read_only)) {
+        !memory_access_is_direct(ram_region, !read_only,
+                                MEMTXATTRS_UNSPECIFIED)) {
         return NULL;
     }

-- 
Peter Xu

Re: [PATCH v3 5/7] memory: pass MemTxAttrs to memory_access_is_direct()

[David Hildenbrand]

On 12.02.25 16:21, Peter Xu wrote:
> On Mon, Feb 10, 2025 at 09:46:46AM +0100, David Hildenbrand wrote:
>> We want to pass another flag that will be stored in MemTxAttrs. So pass
>> MemTxAttrs directly.
>>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   hw/core/loader.c          |  2 +-
>>   hw/remote/vfio-user-obj.c |  2 +-
>>   include/exec/memory.h     |  5 +++--
>>   system/memory_ldst.c.inc  | 18 +++++++++---------
>>   system/physmem.c          | 12 ++++++------
>>   5 files changed, 20 insertions(+), 19 deletions(-)
> 
> This breaks mac builds.. I'll squash:

Hm, maybe not part of ordinary CIs ... or recent changes.

Thanks!

> 
> diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
> index aa1455b629..1554f3b801 100644
> --- a/hw/display/apple-gfx.m
> +++ b/hw/display/apple-gfx.m
> @@ -137,7 +137,8 @@ static void apple_gfx_destroy_task(AppleGFXState *s, PGTask_t *task)
>                                            MEMTXATTRS_UNSPECIFIED);
>   
>       if (!ram_region || ram_region_length < length ||
> -        !memory_access_is_direct(ram_region, !read_only)) {
> +        !memory_access_is_direct(ram_region, !read_only,
> +                                MEMTXATTRS_UNSPECIFIED)) {
>           return NULL;
>       }
> 


-- 
Cheers,

David / dhildenb

[PATCH v3 6/7] hmp: use cpu_get_phys_page_debug() in hmp_gva2gpa()

[David Hildenbrand]

We don't need the MemTxAttrs, so let's simply use the simpler function
variant.

Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 monitor/hmp-cmds-target.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
index 0300faa8a2..0d2e9dce69 100644
--- a/monitor/hmp-cmds-target.c
+++ b/monitor/hmp-cmds-target.c
@@ -301,7 +301,6 @@ void hmp_gpa2hva(Monitor *mon, const QDict *qdict)
 void hmp_gva2gpa(Monitor *mon, const QDict *qdict)
 {
     target_ulong addr = qdict_get_int(qdict, "addr");
-    MemTxAttrs attrs;
     CPUState *cs = mon_get_cpu(mon);
     hwaddr gpa;
 
@@ -310,7 +309,7 @@ void hmp_gva2gpa(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    gpa  = cpu_get_phys_page_attrs_debug(cs, addr & TARGET_PAGE_MASK, &attrs);
+    gpa  = cpu_get_phys_page_debug(cs, addr & TARGET_PAGE_MASK);
     if (gpa == -1) {
         monitor_printf(mon, "Unmapped\n");
     } else {
-- 
2.48.1

[PATCH v3 7/7] physmem: teach cpu_memory_rw_debug() to write to more memory regions

[David Hildenbrand]

Right now, we only allow for writing to memory regions that allow direct
access using memcpy etc; all other writes are simply ignored. This
implies that debugging guests will not work as expected when writing
to MMIO device regions.

Let's extend cpu_memory_rw_debug() to write to more memory regions,
including MMIO device regions. Reshuffle the condition in
memory_access_is_direct() to make it easier to read and add a comment.

While this change implies that debug access can now also write to MMIO
devices, we now are also permit ELF image loads and similar users of
cpu_memory_rw_debug() to write to MMIO devices; currently we ignore
these writes.

Peter assumes [1] that there's probably a class of guest images, which
will start writing junk (likely zeroes) into device model registers; we
previously would silently ignore any such bogus ELF sections. Likely
these images are of questionable correctness and this can be ignored. If
ever a problem, we could make these cases use address_space_write_rom()
instead, which is left unchanged for now.

This patch is based on previous work by Stefan Zabka.

[1] https://lore.kernel.org/all/CAFEAcA_2CEJKFyjvbwmpt=on=GgMVamQ5hiiVt+zUr6AY3X=Xg@mail.gmail.com/

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/cpu-system.c    | 13 +++++++++----
 include/exec/memattrs.h |  5 ++++-
 include/exec/memory.h   |  3 ++-
 system/physmem.c        |  9 ++-------
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/core/cpu-system.c b/hw/core/cpu-system.c
index 6aae28a349..6e307c8959 100644
--- a/hw/core/cpu-system.c
+++ b/hw/core/cpu-system.c
@@ -51,13 +51,18 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
                                      MemTxAttrs *attrs)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
+    hwaddr paddr;
 
     if (cc->sysemu_ops->get_phys_page_attrs_debug) {
-        return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
+        paddr = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
+    } else {
+        /* Fallback for CPUs which don't implement the _attrs_ hook */
+        *attrs = MEMTXATTRS_UNSPECIFIED;
+        paddr = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
     }
-    /* Fallback for CPUs which don't implement the _attrs_ hook */
-    *attrs = MEMTXATTRS_UNSPECIFIED;
-    return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
+    /* Indicate that this is a debug access. */
+    attrs->debug = 1;
+    return paddr;
 }
 
 hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 060b7e7131..8db1d30464 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -44,6 +44,8 @@ typedef struct MemTxAttrs {
      * (see MEMTX_ACCESS_ERROR).
      */
     unsigned int memory:1;
+    /* Debug access that can even write to ROM. */
+    unsigned int debug:1;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
 
@@ -56,7 +58,8 @@ typedef struct MemTxAttrs {
      * Bus masters which don't specify any attributes will get this
      * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
      * distinguish "all attributes deliberately clear" from
-     * "didn't specify" if necessary.
+     * "didn't specify" if necessary. "debug" can be set alongside
+     * "unspecified".
      */
     bool unspecified;
 
diff --git a/include/exec/memory.h b/include/exec/memory.h
index b18ecf933e..78c4e0aec8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -3018,7 +3018,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write,
     if (!memory_region_supports_direct_access(mr)) {
         return false;
     }
-    if (is_write) {
+    /* Debug access can write to ROM. */
+    if (is_write && !attrs.debug) {
         return !mr->readonly && !mr->rom_device;
     }
     return true;
diff --git a/system/physmem.c b/system/physmem.c
index 9766c6d2e0..486316b651 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3680,13 +3680,8 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
         if (l > len)
             l = len;
         phys_addr += (addr & ~TARGET_PAGE_MASK);
-        if (is_write) {
-            res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
-                                          attrs, buf, l);
-        } else {
-            res = address_space_read(cpu->cpu_ases[asidx].as, phys_addr,
-                                     attrs, buf, l);
-        }
+        res = address_space_rw(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
+                               l, is_write);
         if (res != MEMTX_OK) {
             return -1;
         }
-- 
2.48.1

Re: [PATCH v3 0/7] physmem: teach cpu_memory_rw_debug() to write to more memory regions

[Peter Xu]

On Mon, Feb 10, 2025 at 09:46:41AM +0100, David Hildenbrand wrote:
> This is a follow-up to [1], implementing it by avoiding the use of
> address_space_write_rom() in cpu_memory_rw_debug() completely, and
> teaching address_space_write() about debug access instead, the can also
> write to ROM.
> 
> The goal is to let GDB via cpu_memory_rw_debug() to also properly write to
> MMIO device regions, not just RAM/ROM.
> 
> It's worth noting that other users of address_space_write_rom() are
> left unchanged. Maybe hw/core/loader.c and friends could now be converted
> to to a debug access via address_space_write() instead?
> 
> Survives a basic gitlab CI build/check.
> 
> [1] https://lore.kernel.org/all/20241220195923.314208-1-git@zabka.it/
> 
> v2 -> v3:
> * Rebased, only a minor conflict in the last patch.
> 
> v1 -> v2:
>  * Split up "physmem: disallow direct access to RAM DEVICE in
>    address_space_write_rom()" into 4 patches

queued.

-- 
Peter Xu

Re: [PATCH v3 0/7] physmem: teach cpu_memory_rw_debug() to write to more memory regions

[Stefan Zabka]

Sorry for the delayed engagement, I failed to apply the patch set from 
the mailing list and had to remember that David had published this 
change set on GitHub.

Tested-by: Stefan Zabka <git@zabka.it>

This addresses my initial use case of being able to write to a single 
MMIO device. I have not set up a scenario with an interleaving of
MMIO and RAM/ROM regions to ensure that a single large write is 
correctly handled there.

Reviewed-by: Stefan Zabka <git@zabka.it>

I don't know if this counts for anything, but I've read through the 
entire patch series, tried to make sense of it and couldn't spot any 
issues. It should be noted that I am a terrible C programmer and have 
only written basic devices so far.

On 10/02/2025 09:46, David Hildenbrand wrote:
> This is a follow-up to [1], implementing it by avoiding the use of
> address_space_write_rom() in cpu_memory_rw_debug() completely, and
> teaching address_space_write() about debug access instead, the can also
> write to ROM.
> 
> The goal is to let GDB via cpu_memory_rw_debug() to also properly write to
> MMIO device regions, not just RAM/ROM.
> 
> It's worth noting that other users of address_space_write_rom() are
> left unchanged. Maybe hw/core/loader.c and friends could now be converted
> to to a debug access via address_space_write() instead?
> 
> Survives a basic gitlab CI build/check.
> 
> [1] https://lore.kernel.org/all/20241220195923.314208-1-git@zabka.it/
> 
> v2 -> v3:
> * Rebased, only a minor conflict in the last patch.
> 
> v1 -> v2:
>   * Split up "physmem: disallow direct access to RAM DEVICE in
>     address_space_write_rom()" into 4 patches
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Cc: Jagannathan Raman <jag.raman@oracle.com>
> Cc: "Dr. David Alan Gilbert" <dave@treblig.org>
> Cc: Stefan Zabka <git@zabka.it>
> 
> David Hildenbrand (7):
>    physmem: factor out memory_region_is_ram_device() check in
>      memory_access_is_direct()
>    physmem: factor out RAM/ROMD check in memory_access_is_direct()
>    physmem: factor out direct access check into
>      memory_region_supports_direct_access()
>    physmem: disallow direct access to RAM DEVICE in
>      address_space_write_rom()
>    memory: pass MemTxAttrs to memory_access_is_direct()
>    hmp: use cpu_get_phys_page_debug() in hmp_gva2gpa()
>    physmem: teach cpu_memory_rw_debug() to write to more memory regions
> 
>   hw/core/cpu-system.c      | 13 +++++++++----
>   hw/core/loader.c          |  2 +-
>   hw/remote/vfio-user-obj.c |  2 +-
>   include/exec/memattrs.h   |  5 ++++-
>   include/exec/memory.h     | 35 +++++++++++++++++++++++++++--------
>   monitor/hmp-cmds-target.c |  3 +--
>   system/memory_ldst.c.inc  | 18 +++++++++---------
>   system/physmem.c          | 24 +++++++++---------------
>   8 files changed, 61 insertions(+), 41 deletions(-)
> 

Re: [PATCH v3 0/7] physmem: teach cpu_memory_rw_debug() to write to more memory regions

[David Hildenbrand]

On 13.02.25 00:26, Stefan Zabka wrote:
> Sorry for the delayed engagement, I failed to apply the patch set from
> the mailing list and had to remember that David had published this
> change set on GitHub.
> 
> Tested-by: Stefan Zabka <git@zabka.it>
> 
> This addresses my initial use case of being able to write to a single
> MMIO device. I have not set up a scenario with an interleaving of
> MMIO and RAM/ROM regions to ensure that a single large write is
> correctly handled there.
> 
> Reviewed-by: Stefan Zabka <git@zabka.it>
> 

Thanks a bunch!

> I don't know if this counts for anything, but I've read through the
> entire patch series, tried to make sense of it and couldn't spot any
> issues. It should be noted that I am a terrible C programmer and have
> only written basic devices so far.
Hard to believe ("terrible C programmer") :) Any review is appreciated!

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 0/7] physmem: teach cpu_memory_rw_debug() to write to more memory regions

[Peter Xu]

On Thu, Feb 13, 2025 at 12:26:42AM +0100, Stefan Zabka wrote:
> Sorry for the delayed engagement, I failed to apply the patch set from the
> mailing list and had to remember that David had published this change set on
> GitHub.
> 
> Tested-by: Stefan Zabka <git@zabka.it>
> 
> This addresses my initial use case of being able to write to a single MMIO
> device. I have not set up a scenario with an interleaving of
> MMIO and RAM/ROM regions to ensure that a single large write is correctly
> handled there.
> 
> Reviewed-by: Stefan Zabka <git@zabka.it>
> 
> I don't know if this counts for anything, but I've read through the entire
> patch series, tried to make sense of it and couldn't spot any issues. It
> should be noted that I am a terrible C programmer and have only written
> basic devices so far.

Thanks, that's always helpful. It's already in a pull, but if I'll need to
respin the pull I'll attach the tags.

-- 
Peter Xu