Re: [PATCH] physmem: allow debug writes to MMIO regions

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Perry,

On 14/5/24 01:33, Perry Hung wrote:
> Writes from GDB to memory-mapped IO regions are currently silently
> dropped. cpu_memory_rw_debug() calls address_space_write_rom(), which
> calls address_space_write_rom_internal(), which ignores all non-ram/rom
> regions.
> 
> Add a check for MMIO regions and direct those to address_space_rw()
> instead.
> 

Reported-by: Andreas Rasmusson <andreas.rasmusson@gmail.com>
BugLink: https://bugs.launchpad.net/qemu/+bug/1625216

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213
> Signed-off-by: Perry Hung <perry@mosi.io>
> ---
>   system/physmem.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index 342b7a8fd4..013cdd2ab1 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3508,7 +3508,10 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>           if (l > len)
>               l = len;
>           phys_addr += (addr & ~TARGET_PAGE_MASK);
> -        if (is_write) {
> +        if (cpu_physical_memory_is_io(phys_addr)) {
> +            res = address_space_rw(cpu->cpu_ases[asidx].as, phys_addr, attrs,
> +                                   buf, l, is_write);
> +        } else if (is_write) {
>               res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
>                                             attrs, buf, l);
>           } else {

I wonder if we shouldn't be safer with a preliminary patch
adding a 'can_do_io' boolean argument to cpu_memory_rw_debug()
(updating the call sites), then this patch would become:

     if (can_do_io && cpu_physical_memory_is_io(phys_addr)) {

One of my worries for example is if someone accidently insert
a breakpoint at a I/O address, the device might change its
state and return MEMTX_OK which is confusing.

Regards,

Phil.