[PATCH] physmem: allow debug writes to MMIO regions

[PATCH] physmem: allow debug writes to MMIO regions

[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 <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 {
-- 
2.45.0

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

[Philippe Mathieu-Daudé]

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.

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

[Peter Maydell]

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

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

[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é <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

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

[Peter Maydell]

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