system/physmem.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
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.1On 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
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;
}
--
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
© 2016 - 2024 Red Hat, Inc.