| Message ID | 20241220195923.314208-1-git@zabka.it (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices | expand |
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;
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?
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; }
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;
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(-)