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