Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213
Signed-off-by: Stefan Zabka <git@zabka.it>
---
 system/physmem.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index dc1db3a384..53cdccefcb 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3556,6 +3556,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
     hwaddr phys_addr;
     vaddr l, page;
     uint8_t *buf = ptr;
+    bool is_memcpy_access;
 
     cpu_synchronize_state(cpu);
     while (len > 0) {
@@ -3574,11 +3575,24 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
             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);
+            /* if ram/rom region we access the memory
+              via memcpy instead of via the cpu */
+            hwaddr mr_len, addr1;
+            AddressSpace *as = cpu->cpu_ases[asidx].as;
+            MemoryRegion *mr = address_space_translate(as, phys_addr, &addr1, &mr_len, is_write, attrs);
+
+            is_memcpy_access = memory_region_is_ram(mr) || memory_region_is_romd(mr);
+            if(!is_memcpy_access) {
+                l = memory_access_size(mr, l, addr1);
+            }
+        } else {
+            is_memcpy_access = false;
+        }
+
+        if (is_write && is_memcpy_access) {
+            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.47.1

On 18.12.24 20:31, Stefan Zabka wrote:
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213
> Signed-off-by: Stefan Zabka <git@zabka.it>
> ---
>   system/physmem.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index dc1db3a384..53cdccefcb 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3556,6 +3556,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>       hwaddr phys_addr;
>       vaddr l, page;
>       uint8_t *buf = ptr;
> +    bool is_memcpy_access;
>   
>       cpu_synchronize_state(cpu);
>       while (len > 0) {
> @@ -3574,11 +3575,24 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>               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);
> +            /* if ram/rom region we access the memory
> +              via memcpy instead of via the cpu */

Coding style says: "Multiline comment blocks should have a row of stars 
on the left, and the initial /* and terminating */ both on their own lines:"

> +            hwaddr mr_len, addr1;
> +            AddressSpace *as = cpu->cpu_ases[asidx].as;
> +            MemoryRegion *mr = address_space_translate(as, phys_addr, &addr1, &mr_len, is_write, attrs);
> +
> +            is_memcpy_access = memory_region_is_ram(mr) || memory_region_is_romd(mr);
> +            if(!is_memcpy_access) {
> +                l = memory_access_size(mr, l, addr1);
> +            }
> +        } else {
> +            is_memcpy_access = false;
> +        }

Is this really required? The additional address_space_translate() is 
nasty, and flatview_write_continue_step() will perform a memmove 
directly to the RAMBlock if memory_access_is_direct() allows for it.


Would it be possible to always use address_space_rw()?

If there is a reason we need the address_space_write_rom(), could we 
simply fallback to it if address_space_rw(true) failed? That might make 
it all simpler.

> +
> +        if (is_write && is_memcpy_access) {
> +            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;


-- 
Cheers,

David / dhildenb

Sorry, I forgot to add the following comments:
1. This is my first time contributing via e-mail. Please lmk if I got 
anything wrong.
2. This is a minimally adapted version of the change proposed by 
andreas-rasmusson in the linked bug. Do I need to credit them in the patch?