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

2024-05-20 Thread Perry Hung

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é  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 
BugLink: https://bugs.launchpad.net/qemu/+bug/1625216


Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213
Signed-off-by: Perry Hung 
---
   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




[PATCH] physmem: allow debug writes to MMIO regions

2024-05-13 Thread Perry Hung
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 
---
 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 {
-- 
2.45.0