| 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
On Mon, 20 May 2024 at 19:48, Perry Hung <perry@mosi.io> wrote: > > 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? Personally I don't think it matters very much, because the user shouldn't really be doing something silly like that in the first place. If they do, they get to deal with whatever problems result. My feeling is that the neater place to put the handling of memory regions that aren't RAM/ROM is probably in address_space_write_rom_internal(). But doing that makes me nervous, because that's in the file-loading path and I bet there are dubious guest images out there that put data in some area covered by a device and currently rely on it being ignored when the image is loaded... thanks -- 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(-)