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

[David Hildenbrand]

On 20.12.24 20:49, Stefan Zabka wrote:
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213
> Signed-off-by: Stefan Zabka <git@zabka.it>
> ---
> Addressed initial review by David Hildenbrand
> The other change made more sense to me, so I'd like to write a test
> to verify that an AddressSpace like
> 0x00..0x0F MMIO Device A
> 0x10..0x1F ROM
> 0x20..0x2F MMIO Device B
> > and a debug write from 0x00-0x2F still writes to MMIO Device B
> and that there isn't an early exit in address_space_rw
> when it encounters a ROM region.

Good point, I suspect that will be problematic.

Maybe address_space_write() should be taught to be able "force write" to 
ROM instead, so it can just handle everything as part of a single loop. 
But that's a bit more churn ...

Building another loop around address_space_write_rom+address_space_write 
looks a bit suboptimal, but maybe it really is the low-hanging fruit here.

> How would I go about doing that?
> ---
>   system/physmem.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index dc1db3a384..623f41ae06 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3573,12 +3573,13 @@ 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 && is_write) {
> +            /* Fallback since it might be a ROM region*/
> +            /* TODO verify that this works as expected*/
> +            res = address_space_write_rom(cpu->cpu_ases[asidx].as,
> +                                          phys_addr, attrs, buf, l);
>           }
>           if (res != MEMTX_OK) {
>               return -1;


--
Cheers,

David / dhildenb