| Message ID | 20240513233305.2975295-1-perry@mosi.io |
|---|---|
| State | New |
| Headers | show |
| Series | physmem: allow debug writes to MMIO regions | expand |
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.
On Wed, 15 May 2024 at 13:49, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > 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 { The other option is to make address_space_write_rom_internal() also write to devices... > 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. You can definitely do some silly things if we remove this restriction. On the other hand if you're using gdb as a debugger on real (bare metal) hardware does anything stop you doing that? -- PMM
Philippe, Peter, Thank you for the comments. I am not even sure what the semantics of putting a breakpoint or watchpoint on device regions are supposed to be. I would imagine it is architecture-specific as to whether this is even allowed. It appears for example, that armv8-a allows watchpoints to be set on any type of memory. armv7-a prohibits watchpoints on Device or Strongly-ordered memory that might be accessed by instructions multiple times (e.g LDM and LDC instructions). What is the current behavior for QEMU and what should breakpoints/watchpoints do when placed on IO memory? -perry On 5/20/24 10:22 AM, Peter Maydell wrote: > On Wed, 15 May 2024 at 13:49, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> 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 { > The other option is to make address_space_write_rom_internal() > also write to devices... > >> 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. > You can definitely do some silly things if we remove this > restriction. > > On the other hand if you're using gdb as a debugger on real > (bare metal) hardware does anything stop you doing that? > > -- PMM
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 {
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. 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(-)