Re: [PATCH] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[David Hildenbrand]

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