[PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[Stefan Zabka]

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213
Signed-off-by: Stefan Zabka <git@zabka.it>
---
Addressed initial review by David Hildenbrand
The other change made more sense to me, so I'd like to write a test
to verify that an AddressSpace like
0x00..0x0F MMIO Device A
0x10..0x1F ROM
0x20..0x2F MMIO Device B

and a debug write from 0x00-0x2F still writes to MMIO Device B
and that there isn't an early exit in address_space_rw
when it encounters a ROM region.

How would I go about doing that?
---
 system/physmem.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index dc1db3a384..623f41ae06 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3573,12 +3573,13 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
         if (l > len)
             l = len;
         phys_addr += (addr & ~TARGET_PAGE_MASK);
-        if (is_write) {
-            res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
-                                          attrs, buf, l);
-        } else {
-            res = address_space_read(cpu->cpu_ases[asidx].as, phys_addr,
-                                     attrs, buf, l);
+        res = address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
+                                   attrs, buf, l, is_write);
+        if (res != MEMTX_OK && is_write) {
+            /* Fallback since it might be a ROM region*/
+            /* TODO verify that this works as expected*/
+            res = address_space_write_rom(cpu->cpu_ases[asidx].as,
+                                          phys_addr, attrs, buf, l);
         }
         if (res != MEMTX_OK) {
             return -1;
-- 
2.47.1

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[David Hildenbrand]

On 20.12.24 20:49, Stefan Zabka wrote:
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213
> Signed-off-by: Stefan Zabka <git@zabka.it>
> ---
> Addressed initial review by David Hildenbrand
> The other change made more sense to me, so I'd like to write a test
> to verify that an AddressSpace like
> 0x00..0x0F MMIO Device A
> 0x10..0x1F ROM
> 0x20..0x2F MMIO Device B
 > > and a debug write from 0x00-0x2F still writes to MMIO Device B
> and that there isn't an early exit in address_space_rw
> when it encounters a ROM region.

Good point, I suspect that will be problematic.

Maybe address_space_write() should be taught to be able "force write" to 
ROM instead, so it can just handle everything as part of a single loop. 
But that's a bit more churn ...

Building another loop around address_space_write_rom+address_space_write 
looks a bit suboptimal, but maybe it really is the low-hanging fruit here.

> 
> How would I go about doing that?
> ---
>   system/physmem.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index dc1db3a384..623f41ae06 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3573,12 +3573,13 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>           if (l > len)
>               l = len;
>           phys_addr += (addr & ~TARGET_PAGE_MASK);
> -        if (is_write) {
> -            res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
> -                                          attrs, buf, l);
> -        } else {
> -            res = address_space_read(cpu->cpu_ases[asidx].as, phys_addr,
> -                                     attrs, buf, l);
> +        res = address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> +                                   attrs, buf, l, is_write);
> +        if (res != MEMTX_OK && is_write) {
> +            /* Fallback since it might be a ROM region*/
> +            /* TODO verify that this works as expected*/
> +            res = address_space_write_rom(cpu->cpu_ases[asidx].as,
> +                                          phys_addr, attrs, buf, l);
>           }
>           if (res != MEMTX_OK) {
>               return -1;


-- 
Cheers,

David / dhildenb

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[vringar]

On 20/12/2024 21:59, David Hildenbrand wrote:
> Good point, I suspect that will be problematic.

Looking at flatview_write_continue I see no early exit, so maybe it 
would still try to get through everything and work as we are hoping,
but that's why I'd like to write a test for it.
I'm just not sure if it should be a unit test or a QTest and
what examples I could look to copy.

> Maybe address_space_write() should be taught to be able "force write" to 
> ROM instead, so it can just handle everything as part of a single loop. 
> But that's a bit more churn ...

Tbh I'm not sure whether I understand the intricacies of this code well 
enough to write such a patch without significant guidance.

> Building another loop around address_space_write_rom+address_space_write 
> looks a bit suboptimal, but maybe it really is the low-hanging fruit here.

I don't understand what you mean by this
Do you mean keeping the current patch or going back to v1 or a different 
third approach?

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[David Hildenbrand]

On 20.12.24 23:22, vringar wrote:
> On 20/12/2024 21:59, David Hildenbrand wrote:
>> Good point, I suspect that will be problematic.
> 
> Looking at flatview_write_continue I see no early exit, so maybe it
> would still try to get through everything and work as we are hoping,
> but that's why I'd like to write a test for it.

I thought flatview_write()->flatview_access_allowed() would reject it, but I think that does not apply here.

address_space_write_rom() can write to memory_region_ram() and memory_region_is_romd(), independent of read-only protection.

> I'm just not sure if it should be a unit test or a QTest and
> what examples I could look to copy.
> 
>> Maybe address_space_write() should be taught to be able "force write" to
>> ROM instead, so it can just handle everything as part of a single loop.
>> But that's a bit more churn ...
> 
> Tbh I'm not sure whether I understand the intricacies of this code well
> enough to write such a patch without significant guidance.

In flatview_write_continue_step(), we call memory_access_is_direct(). That would
have to be taught to behave just like address_space_write_rom(): allow direct
access if memory_region_ram() || memory_region_is_romd().

Maybe something like the following could work (some more memory_access_is_direct()
callers have to be fixed to make it compile):


diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index e27c18f3dc..89a5e75f6f 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -50,6 +50,8 @@ typedef struct MemTxAttrs {
       * (see MEMTX_ACCESS_ERROR).
       */
      unsigned int memory:1;
+    /* Debug access that can even write to ROM. */
+    unsigned int debug:1;
      /* Requester ID (for MSI for example) */
      unsigned int requester_id:16;
  
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9458e2801d..78d014aa59 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2986,9 +2986,13 @@ MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
  int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr);
  bool prepare_mmio_access(MemoryRegion *mr);
  
-static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
+static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write,
+                                           bool is_debug)
  {
      if (is_write) {
+        if (is_debug) {
+            return memory_region_is_ram(mr) || memory_region_is_romd(mr);
+        }
          return memory_region_is_ram(mr) && !mr->readonly &&
                 !mr->rom_device && !memory_region_is_ram_device(mr);
      } else {
@@ -3027,7 +3031,7 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
              fv = address_space_to_flatview(as);
              l = len;
              mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
-            if (len == l && memory_access_is_direct(mr, false)) {
+            if (len == l && memory_access_is_direct(mr, false, false)) {
                  ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
                  memcpy(buf, ptr, len);
              } else {
diff --git a/system/physmem.c b/system/physmem.c
index dc1db3a384..deb56395b7 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -571,7 +571,7 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr addr, hwaddr *xlat,
                                      is_write, true, &as, attrs);
      mr = section.mr;
  
-    if (xen_enabled() && memory_access_is_direct(mr, is_write)) {
+    if (xen_enabled() && memory_access_is_direct(mr, is_write, attrs.debug)) {
          hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
          *plen = MIN(page, *plen);
      }
@@ -2761,7 +2761,7 @@ static MemTxResult flatview_write_continue_step(MemTxAttrs attrs,
          return MEMTX_ACCESS_ERROR;
      }
  
-    if (!memory_access_is_direct(mr, true)) {
+    if (!memory_access_is_direct(mr, true, attrs.debug)) {
          uint64_t val;
          MemTxResult result;
          bool release_lock = prepare_mmio_access(mr);
@@ -2857,7 +2857,7 @@ static MemTxResult flatview_read_continue_step(MemTxAttrs attrs, uint8_t *buf,
          return MEMTX_ACCESS_ERROR;
      }
  
-    if (!memory_access_is_direct(mr, false)) {
+    if (!memory_access_is_direct(mr, false, attrs.debug)) {
          /* I/O case */
          uint64_t val;
          MemTxResult result;
@@ -3011,7 +3011,7 @@ enum write_rom_type {
      FLUSH_CACHE,
  };
  
-static inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
+static /inline MemTxResult address_space_write_rom_internal(AddressSpace *as,
                                                             hwaddr addr,
                                                             MemTxAttrs attrs,
                                                             const void *ptr,
@@ -3167,7 +3167,7 @@ static bool flatview_access_valid(FlatView *fv, hwaddr addr, hwaddr len,
      while (len > 0) {
          l = len;
          mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
-        if (!memory_access_is_direct(mr, is_write)) {
+        if (!memory_access_is_direct(mr, is_write, attrs.debug)) {
              l = memory_access_size(mr, l, addr);
              if (!memory_region_access_valid(mr, xlat, l, is_write, attrs)) {
                  return false;
@@ -3247,7 +3247,7 @@ void *address_space_map(AddressSpace *as,
      fv = address_space_to_flatview(as);
      mr = flatview_translate(fv, addr, &xlat, &l, is_write, attrs);
  
-    if (!memory_access_is_direct(mr, is_write)) {
+    if (!memory_access_is_direct(mr, is_write, attrs.debug)) {
          size_t used = qatomic_read(&as->bounce_buffer_size);
          for (;;) {
              hwaddr alloc = MIN(as->max_bounce_buffer_size - used, l);
@@ -3380,7 +3380,7 @@ int64_t address_space_cache_init(MemoryRegionCache *cache,
  
      mr = cache->mrs.mr;
      memory_region_ref(mr);
-    if (memory_access_is_direct(mr, is_write)) {
+    if (memory_access_is_direct(mr, is_write, false)) {
          /* We don't care about the memory attributes here as we're only
           * doing this if we found actual RAM, which behaves the same
           * regardless of attributes; so UNSPECIFIED is fine.
@@ -3565,6 +3565,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
  
          page = addr & TARGET_PAGE_MASK;
          phys_addr = cpu_get_phys_page_attrs_debug(cpu, page, &attrs);
+        attrs.debug = 1;
          asidx = cpu_asidx_from_attrs(cpu, attrs);
          /* if no physical page mapped, return an error */
          if (phys_addr == -1)
@@ -3573,13 +3574,8 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
          if (l > len)
              l = len;
          phys_addr += (addr & ~TARGET_PAGE_MASK);
-        if (is_write) {
-            res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
-                                          attrs, buf, l);
-        } else {
-            res = address_space_read(cpu->cpu_ases[asidx].as, phys_addr,
-                                     attrs, buf, l);
-        }
+        res = address_space_rw(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
+                               l);
          if (res != MEMTX_OK) {
              return -1;
          }
-- 
2.47.1


Completely uncompiled/untested, just to share what I have in mind.


> 
>> Building another loop around address_space_write_rom+address_space_write
>> looks a bit suboptimal, but maybe it really is the low-hanging fruit here.
> 
> I don't understand what you mean by this
> Do you mean keeping the current patch or going back to v1 or a different
> third approach?

Yes, but I would actually prefer something that doesn't require that.
Let's wait for opinions from others first.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[Stefan Zabka]

On 21/12/2024 15:55, David Hildenbrand wrote:
 > Let's wait for opinions from others first.

<https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-your-patch-seems-to-have-been-ignored> 
states that two weeks is a reasonable amount of time for follow-up.

Should I also ping the original patch? I thought pinging the thread
would be more appropriate, as it contains relevant information.

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[David Hildenbrand]

On 08.01.25 19:35, Stefan Zabka wrote:
> On 21/12/2024 15:55, David Hildenbrand wrote:
>   > Let's wait for opinions from others first.
> 
> <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-your-patch-seems-to-have-been-ignored>
> states that two weeks is a reasonable amount of time for follow-up.
> 
> Should I also ping the original patch? I thought pinging the thread
> would be more appropriate, as it contains relevant information.
> 

I just pushed a compiling version of the attrs.debug approach to:

	https://github.com/davidhildenbrand/qemu/tree/debug_access

With two preparation patches, the relevant patch is:


 From 2e85cb1724385e4b8640364415832c030e5c5e6d Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Wed, 8 Jan 2025 20:58:00 +0100
Subject: [PATCH] physmem: allow cpu_memory_rw_debug to write to MMIO devices

Signed-off-by: David Hildenbrand <david@redhat.com>
---
  hw/core/cpu-sysemu.c    | 13 +++++++++----
  include/exec/memattrs.h |  5 ++++-
  include/exec/memory.h   |  2 ++
  system/physmem.c        |  9 ++-------
  4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/hw/core/cpu-sysemu.c b/hw/core/cpu-sysemu.c
index 2a9a2a4eb5..0aa0a569e4 100644
--- a/hw/core/cpu-sysemu.c
+++ b/hw/core/cpu-sysemu.c
@@ -51,13 +51,18 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
                                       MemTxAttrs *attrs)
  {
      CPUClass *cc = CPU_GET_CLASS(cpu);
+    hwaddr paddr;
  
      if (cc->sysemu_ops->get_phys_page_attrs_debug) {
-        return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
+        paddr = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
+    } else {
+        /* Fallback for CPUs which don't implement the _attrs_ hook */
+        *attrs = MEMTXATTRS_UNSPECIFIED;
+        paddr = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
      }
-    /* Fallback for CPUs which don't implement the _attrs_ hook */
-    *attrs = MEMTXATTRS_UNSPECIFIED;
-    return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
+    /* Indicate that this is a debug access. */
+    attrs->debug = 1;
+    return paddr;
  }
  
  hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index e27c18f3dc..14e0edaa58 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -26,7 +26,8 @@ typedef struct MemTxAttrs {
      /* Bus masters which don't specify any attributes will get this
       * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
       * distinguish "all attributes deliberately clear" from
-     * "didn't specify" if necessary.
+     * "didn't specify" if necessary. "debug" can be set alongside
+     * "unspecified".
       */
      unsigned int unspecified:1;
      /*
@@ -50,6 +51,8 @@ typedef struct MemTxAttrs {
       * (see MEMTX_ACCESS_ERROR).
       */
      unsigned int memory:1;
+    /* Debug access that can even write to ROM. */
+    unsigned int debug:1;
      /* Requester ID (for MSI for example) */
      unsigned int requester_id:16;
  
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3dcadcf3a2..7458082455 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2990,6 +2990,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write,
                                             MemTxAttrs attrs)
  {
      if (is_write) {
+        if (attrs.debug)
+            return memory_region_is_ram(mr) || memory_region_is_romd(mr);
          return memory_region_is_ram(mr) && !mr->readonly &&
                 !mr->rom_device && !memory_region_is_ram_device(mr);
      } else {
diff --git a/system/physmem.c b/system/physmem.c
index 3c2a0b0289..03d3618039 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3573,13 +3573,8 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
          if (l > len)
              l = len;
          phys_addr += (addr & ~TARGET_PAGE_MASK);
-        if (is_write) {
-            res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
-                                          attrs, buf, l);
-        } else {
-            res = address_space_read(cpu->cpu_ases[asidx].as, phys_addr,
-                                     attrs, buf, l);
-        }
+        res = address_space_rw(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
+                               l, is_write);
          if (res != MEMTX_OK) {
              return -1;
          }
-- 
2.47.1


You could could give that a churn if that solves your issue as well.

Let me CC more people that might have an opinion on how it should be done.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[Peter Maydell]

On Wed, 8 Jan 2025 at 20:10, David Hildenbrand <david@redhat.com> wrote:
>
> On 08.01.25 19:35, Stefan Zabka wrote:
> > On 21/12/2024 15:55, David Hildenbrand wrote:
> >   > Let's wait for opinions from others first.
> >
> > <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-your-patch-seems-to-have-been-ignored>
> > states that two weeks is a reasonable amount of time for follow-up.
> >
> > Should I also ping the original patch? I thought pinging the thread
> > would be more appropriate, as it contains relevant information.
> >
>
> I just pushed a compiling version of the attrs.debug approach to:
>
>         https://github.com/davidhildenbrand/qemu/tree/debug_access

I think this approach (having a 'debug' attribute in the MemTxAttrs
seems reasonable. I do note that if we allow this kind of access
to write to MMIO devices then we are also permitting ELF (and other)
image loads to write to MMIO devices where currently we ignore those.
That means there's probably a class of guest images (of dubious
correctness) which will start writing junk (likely zeroes) into
device model registers; we previously would silently ignore any
such bogus ELF sections.

Q: should we suggest behaviour for device models if they see a
'debug = 1' transaction, e.g. "don't update your internal state
for a debug read if you have clear-on-read or similar kinds of
register fields" ?

> With two preparation patches, the relevant patch is:
>
>
>  From 2e85cb1724385e4b8640364415832c030e5c5e6d Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Wed, 8 Jan 2025 20:58:00 +0100
> Subject: [PATCH] physmem: allow cpu_memory_rw_debug to write to MMIO devices
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/core/cpu-sysemu.c    | 13 +++++++++----
>   include/exec/memattrs.h |  5 ++++-
>   include/exec/memory.h   |  2 ++
>   system/physmem.c        |  9 ++-------
>   4 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/hw/core/cpu-sysemu.c b/hw/core/cpu-sysemu.c
> index 2a9a2a4eb5..0aa0a569e4 100644
> --- a/hw/core/cpu-sysemu.c
> +++ b/hw/core/cpu-sysemu.c
> @@ -51,13 +51,18 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
>                                        MemTxAttrs *attrs)
>   {
>       CPUClass *cc = CPU_GET_CLASS(cpu);
> +    hwaddr paddr;
>
>       if (cc->sysemu_ops->get_phys_page_attrs_debug) {
> -        return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
> +        paddr = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
> +    } else {
> +        /* Fallback for CPUs which don't implement the _attrs_ hook */
> +        *attrs = MEMTXATTRS_UNSPECIFIED;
> +        paddr = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
>       }
> -    /* Fallback for CPUs which don't implement the _attrs_ hook */
> -    *attrs = MEMTXATTRS_UNSPECIFIED;
> -    return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
> +    /* Indicate that this is a debug access. */
> +    attrs->debug = 1;
> +    return paddr;
>   }
>
>   hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index e27c18f3dc..14e0edaa58 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -26,7 +26,8 @@ typedef struct MemTxAttrs {
>       /* Bus masters which don't specify any attributes will get this
>        * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
>        * distinguish "all attributes deliberately clear" from
> -     * "didn't specify" if necessary.
> +     * "didn't specify" if necessary. "debug" can be set alongside
> +     * "unspecified".
>        */

This feels like one of those changes which is "*probably* OK,
but breaks a thing that was previously an invariant" ones.
But trying to get all targets to support reporting real
attributes is probably too long a side-quest (we can't really
do it in cpu_get_phys_page_attrs_debug() because I don't think
we have a pre-existing way to ask the target "are you in usermode
now"; the rest of the fields we could validly leave 0).

>       unsigned int unspecified:1;
>       /*
> @@ -50,6 +51,8 @@ typedef struct MemTxAttrs {
>        * (see MEMTX_ACCESS_ERROR).
>        */
>       unsigned int memory:1;
> +    /* Debug access that can even write to ROM. */
> +    unsigned int debug:1;
>       /* Requester ID (for MSI for example) */
>       unsigned int requester_id:16;
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 3dcadcf3a2..7458082455 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2990,6 +2990,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write,
>                                              MemTxAttrs attrs)
>   {
>       if (is_write) {
> +        if (attrs.debug)
> +            return memory_region_is_ram(mr) || memory_region_is_romd(mr);

It's a bit weird that the condition for "debug access can write"
is not the same as the one for "debug access can read"...

>           return memory_region_is_ram(mr) && !mr->readonly &&
>                  !mr->rom_device && !memory_region_is_ram_device(mr);
>       } else {

-- PMM

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[David Hildenbrand]

On 10.01.25 16:44, Peter Maydell wrote:
> On Wed, 8 Jan 2025 at 20:10, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 08.01.25 19:35, Stefan Zabka wrote:
>>> On 21/12/2024 15:55, David Hildenbrand wrote:
>>>    > Let's wait for opinions from others first.
>>>
>>> <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-your-patch-seems-to-have-been-ignored>
>>> states that two weeks is a reasonable amount of time for follow-up.
>>>
>>> Should I also ping the original patch? I thought pinging the thread
>>> would be more appropriate, as it contains relevant information.
>>>
>>
>> I just pushed a compiling version of the attrs.debug approach to:
>>
>>          https://github.com/davidhildenbrand/qemu/tree/debug_access
> 
> I think this approach (having a 'debug' attribute in the MemTxAttrs
> seems reasonable. I do note that if we allow this kind of access
> to write to MMIO devices then we are also permitting ELF (and other)
> image loads to write to MMIO devices where currently we ignore those.

Right. The "debug" access semantics will change for all users.

> That means there's probably a class of guest images (of dubious
> correctness) which will start writing junk (likely zeroes) into
> device model registers; we previously would silently ignore any
> such bogus ELF sections.

Do you have any specific instances in mind that we could try?

> 
> Q: should we suggest behaviour for device models if they see a
> 'debug = 1' transaction, e.g. "don't update your internal state
> for a debug read if you have clear-on-read or similar kinds of
> register fields" ?

Not an expert on that, but I assume you mean that we start making use of 
this debug flag also to modify *read* behavior, not only *write* 
behavior. That would make sense to me.

> 
>> With two preparation patches, the relevant patch is:
>>
>>
>>   From 2e85cb1724385e4b8640364415832c030e5c5e6d Mon Sep 17 00:00:00 2001
>> From: David Hildenbrand <david@redhat.com>
>> Date: Wed, 8 Jan 2025 20:58:00 +0100
>> Subject: [PATCH] physmem: allow cpu_memory_rw_debug to write to MMIO devices
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>    hw/core/cpu-sysemu.c    | 13 +++++++++----
>>    include/exec/memattrs.h |  5 ++++-
>>    include/exec/memory.h   |  2 ++
>>    system/physmem.c        |  9 ++-------
>>    4 files changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/core/cpu-sysemu.c b/hw/core/cpu-sysemu.c
>> index 2a9a2a4eb5..0aa0a569e4 100644
>> --- a/hw/core/cpu-sysemu.c
>> +++ b/hw/core/cpu-sysemu.c
>> @@ -51,13 +51,18 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
>>                                         MemTxAttrs *attrs)
>>    {
>>        CPUClass *cc = CPU_GET_CLASS(cpu);
>> +    hwaddr paddr;
>>
>>        if (cc->sysemu_ops->get_phys_page_attrs_debug) {
>> -        return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
>> +        paddr = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
>> +    } else {
>> +        /* Fallback for CPUs which don't implement the _attrs_ hook */
>> +        *attrs = MEMTXATTRS_UNSPECIFIED;
>> +        paddr = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
>>        }
>> -    /* Fallback for CPUs which don't implement the _attrs_ hook */
>> -    *attrs = MEMTXATTRS_UNSPECIFIED;
>> -    return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
>> +    /* Indicate that this is a debug access. */
>> +    attrs->debug = 1;
>> +    return paddr;
>>    }
>>
>>    hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index e27c18f3dc..14e0edaa58 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -26,7 +26,8 @@ typedef struct MemTxAttrs {
>>        /* Bus masters which don't specify any attributes will get this
>>         * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
>>         * distinguish "all attributes deliberately clear" from
>> -     * "didn't specify" if necessary.
>> +     * "didn't specify" if necessary. "debug" can be set alongside
>> +     * "unspecified".
>>         */
> 
> This feels like one of those changes which is "*probably* OK,
> but breaks a thing that was previously an invariant" ones.
> But trying to get all targets to support reporting real
> attributes is probably too long a side-quest (we can't really
> do it in cpu_get_phys_page_attrs_debug() because I don't think
> we have a pre-existing way to ask the target "are you in usermode
> now"; the rest of the fields we could validly leave 0).

Yes, and having this as part of MemTxAttrs looked cleaner than passing 
yet another flag (bool debug) around.

> 
>>        unsigned int unspecified:1;
>>        /*
>> @@ -50,6 +51,8 @@ typedef struct MemTxAttrs {
>>         * (see MEMTX_ACCESS_ERROR).
>>         */
>>        unsigned int memory:1;
>> +    /* Debug access that can even write to ROM. */
>> +    unsigned int debug:1;
>>        /* Requester ID (for MSI for example) */
>>        unsigned int requester_id:16;
>>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 3dcadcf3a2..7458082455 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -2990,6 +2990,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write,
>>                                               MemTxAttrs attrs)
>>    {
>>        if (is_write) {
>> +        if (attrs.debug)
>> +            return memory_region_is_ram(mr) || memory_region_is_romd(mr);
> 
> It's a bit weird that the condition for "debug access can write"
> is not the same as the one for "debug access can read"...

True, for now I copied what the write_rom function would have done.

Now I wonder why we don't allow read-access to 
memory_region_is_ram_device. Does anybody know why?

Either that one is wrong, or the write_rom function should have also 
ignored it.

Likely we should do here

if (attrs.debug) {
	return memory_region_is_ram(mr) || memory_region_is_romd(mr);
} else if (is_write) {
	...
}


Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 8 Jan 2025 at 20:10, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 08.01.25 19:35, Stefan Zabka wrote:
>> > On 21/12/2024 15:55, David Hildenbrand wrote:
>> >   > Let's wait for opinions from others first.
>> >
>> > <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-your-patch-seems-to-have-been-ignored>
>> > states that two weeks is a reasonable amount of time for follow-up.
>> >
>> > Should I also ping the original patch? I thought pinging the thread
>> > would be more appropriate, as it contains relevant information.
>> >
>>
>> I just pushed a compiling version of the attrs.debug approach to:
>>
>>         https://github.com/davidhildenbrand/qemu/tree/debug_access
>
> I think this approach (having a 'debug' attribute in the MemTxAttrs
> seems reasonable. I do note that if we allow this kind of access
> to write to MMIO devices then we are also permitting ELF (and other)
> image loads to write to MMIO devices where currently we ignore those.
> That means there's probably a class of guest images (of dubious
> correctness) which will start writing junk (likely zeroes) into
> device model registers; we previously would silently ignore any
> such bogus ELF sections.
>
> Q: should we suggest behaviour for device models if they see a
> 'debug = 1' transaction, e.g. "don't update your internal state
> for a debug read if you have clear-on-read or similar kinds of
> register fields" ?

What do we do for device models that want to know which CPU things are
coming from, as per:

  https://gitlab.com/qemu-project/qemu/-/issues/124

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[Peter Maydell]

On Fri, 10 Jan 2025 at 16:55, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Wed, 8 Jan 2025 at 20:10, David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 08.01.25 19:35, Stefan Zabka wrote:
> >> > On 21/12/2024 15:55, David Hildenbrand wrote:
> >> >   > Let's wait for opinions from others first.
> >> >
> >> > <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-your-patch-seems-to-have-been-ignored>
> >> > states that two weeks is a reasonable amount of time for follow-up.
> >> >
> >> > Should I also ping the original patch? I thought pinging the thread
> >> > would be more appropriate, as it contains relevant information.
> >> >
> >>
> >> I just pushed a compiling version of the attrs.debug approach to:
> >>
> >>         https://github.com/davidhildenbrand/qemu/tree/debug_access
> >
> > I think this approach (having a 'debug' attribute in the MemTxAttrs
> > seems reasonable. I do note that if we allow this kind of access
> > to write to MMIO devices then we are also permitting ELF (and other)
> > image loads to write to MMIO devices where currently we ignore those.
> > That means there's probably a class of guest images (of dubious
> > correctness) which will start writing junk (likely zeroes) into
> > device model registers; we previously would silently ignore any
> > such bogus ELF sections.
> >
> > Q: should we suggest behaviour for device models if they see a
> > 'debug = 1' transaction, e.g. "don't update your internal state
> > for a debug read if you have clear-on-read or similar kinds of
> > register fields" ?
>
> What do we do for device models that want to know which CPU things are
> coming from, as per:
>
>   https://gitlab.com/qemu-project/qemu/-/issues/124

I think that's an orthogonal issue. We already have a problem that
you can see on reads where because we don't encode the CPU ID
into the txattrs devices have to work around this by using
the current_cpu global, which isn't set for gdbstub accesses
(and so if the device doesn't have handling for 'current_cpu == NULL'
then it might crash).

But this patch only changes the handling of writes, which puts them
in the same situation as read accesses. If the write falls over it's
probably also the case that they were already falling over on reads.

thanks
-- PMM

Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[Peter Maydell]

On Wed, 8 Jan 2025 at 18:36, Stefan Zabka <git@zabka.it> wrote:
>
> On 21/12/2024 15:55, David Hildenbrand wrote:
>  > Let's wait for opinions from others first.
>
> <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-your-patch-seems-to-have-been-ignored>
> states that two weeks is a reasonable amount of time for follow-up.
>
> Should I also ping the original patch? I thought pinging the thread
> would be more appropriate, as it contains relevant information.

Two weeks is fine, but in this case the two weeks are across the
Christmas holidays, so it's likely that people might not have
got to things yet...

-- PMM