From 52012209e1802e67aa186459e3e965f669e553df Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Mon, 10 Feb 2025 09:46:42 +0100 Subject: physmem: factor out memory_region_is_ram_device() check in memory_access_is_direct() As documented in commit 4a2e242bbb306 ("memory: Don't use memcpy for ram_device regions"), we disallow direct access to RAM DEVICE regions. Let's make this clearer to prepare for further changes. Note that romd regions will never be RAM DEVICE at the same time. Reviewed-by: Peter Xu Signed-off-by: David Hildenbrand Link: https://lore.kernel.org/r/20250210084648.33798-2-david@redhat.com Signed-off-by: Peter Xu --- include/exec/memory.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) (limited to 'include/exec/memory.h') diff --git a/include/exec/memory.h b/include/exec/memory.h index 9f73b59867..5cd7574c60 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -2997,12 +2997,19 @@ bool prepare_mmio_access(MemoryRegion *mr); static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) { + /* + * RAM DEVICE regions can be accessed directly using memcpy, but it might + * be MMIO and access using mempy can be wrong (e.g., using instructions not + * intended for MMIO access). So we treat this as IO. + */ + if (memory_region_is_ram_device(mr)) { + return false; + } if (is_write) { return memory_region_is_ram(mr) && !mr->readonly && - !mr->rom_device && !memory_region_is_ram_device(mr); + !mr->rom_device; } else { - return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) || - memory_region_is_romd(mr); + return memory_region_is_ram(mr) || memory_region_is_romd(mr); } } -- cgit 1.4.1 From e76d7b6b8cd564d4d5ea6e7c7daea541e100caa4 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Mon, 10 Feb 2025 09:46:43 +0100 Subject: physmem: factor out RAM/ROMD check in memory_access_is_direct() Let's factor more of the generic "is this directly accessible" check, independent of the "write" condition out. Note that the "!mr->rom_device" check in the write case essentially disallows the memory_region_is_romd() condition again. Further note that RAM DEVICE regions are also RAM regions, so we can check for RAM+ROMD first. This is a preparation for further changes. Reviewed-by: Peter Xu Signed-off-by: David Hildenbrand Link: https://lore.kernel.org/r/20250210084648.33798-3-david@redhat.com Signed-off-by: Peter Xu --- include/exec/memory.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'include/exec/memory.h') diff --git a/include/exec/memory.h b/include/exec/memory.h index 5cd7574c60..cb35c38402 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -2997,6 +2997,10 @@ bool prepare_mmio_access(MemoryRegion *mr); static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) { + /* ROM DEVICE regions only allow direct access if in ROMD mode. */ + if (!memory_region_is_ram(mr) && !memory_region_is_romd(mr)) { + return false; + } /* * RAM DEVICE regions can be accessed directly using memcpy, but it might * be MMIO and access using mempy can be wrong (e.g., using instructions not @@ -3006,11 +3010,9 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) return false; } if (is_write) { - return memory_region_is_ram(mr) && !mr->readonly && - !mr->rom_device; - } else { - return memory_region_is_ram(mr) || memory_region_is_romd(mr); + return !mr->readonly && !mr->rom_device; } + return true; } /** -- cgit 1.4.1 From 7fd970a7d35af543992bf85e77b75de6b8125eb1 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Mon, 10 Feb 2025 09:46:44 +0100 Subject: physmem: factor out direct access check into memory_region_supports_direct_access() Let's factor the complete "directly accessible" check independent of the "write" condition out so we can reuse it next. We can now split up the checks RAM and ROMD check, so we really only check for RAM DEVICE in case of RAM -- ROM DEVICE is neither RAM not RAM DEVICE. Reviewed-by: Peter Xu Signed-off-by: David Hildenbrand Link: https://lore.kernel.org/r/20250210084648.33798-4-david@redhat.com Signed-off-by: Peter Xu --- include/exec/memory.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) (limited to 'include/exec/memory.h') diff --git a/include/exec/memory.h b/include/exec/memory.h index cb35c38402..4e2cf95ab6 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -2995,10 +2995,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_region_supports_direct_access(MemoryRegion *mr) { /* ROM DEVICE regions only allow direct access if in ROMD mode. */ - if (!memory_region_is_ram(mr) && !memory_region_is_romd(mr)) { + if (memory_region_is_romd(mr)) { + return true; + } + if (!memory_region_is_ram(mr)) { return false; } /* @@ -3006,7 +3009,12 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) * be MMIO and access using mempy can be wrong (e.g., using instructions not * intended for MMIO access). So we treat this as IO. */ - if (memory_region_is_ram_device(mr)) { + return !memory_region_is_ram_device(mr); +} + +static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write) +{ + if (!memory_region_supports_direct_access(mr)) { return false; } if (is_write) { -- cgit 1.4.1 From d732b5a4ac3e8222e9527654f067bb766fdaecb6 Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Mon, 10 Feb 2025 09:46:46 +0100 Subject: memory: pass MemTxAttrs to memory_access_is_direct() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We want to pass another flag that will be stored in MemTxAttrs. So pass MemTxAttrs directly. Reviewed-by: Peter Xu Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: David Hildenbrand Link: https://lore.kernel.org/r/20250210084648.33798-6-david@redhat.com [peterx: Fix MacOS builds] Signed-off-by: Peter Xu --- hw/core/loader.c | 2 +- hw/display/apple-gfx.m | 3 ++- hw/remote/vfio-user-obj.c | 2 +- include/exec/memory.h | 5 +++-- system/memory_ldst.c.inc | 18 +++++++++--------- system/physmem.c | 12 ++++++------ 6 files changed, 22 insertions(+), 20 deletions(-) (limited to 'include/exec/memory.h') diff --git a/hw/core/loader.c b/hw/core/loader.c index fd25c5e01b..332b879a0b 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -144,7 +144,7 @@ ssize_t load_image_mr(const char *filename, MemoryRegion *mr) { ssize_t size; - if (!memory_access_is_direct(mr, false)) { + if (!memory_access_is_direct(mr, false, MEMTXATTRS_UNSPECIFIED)) { /* Can only load an image into RAM or ROM */ return -1; } diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m index aa1455b629..1554f3b801 100644 --- a/hw/display/apple-gfx.m +++ b/hw/display/apple-gfx.m @@ -137,7 +137,8 @@ void *apple_gfx_host_ptr_for_gpa_range(uint64_t guest_physical, MEMTXATTRS_UNSPECIFIED); if (!ram_region || ram_region_length < length || - !memory_access_is_direct(ram_region, !read_only)) { + !memory_access_is_direct(ram_region, !read_only, + MEMTXATTRS_UNSPECIFIED)) { return NULL; } diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c index 9e5ff6d87a..6e51a92856 100644 --- a/hw/remote/vfio-user-obj.c +++ b/hw/remote/vfio-user-obj.c @@ -358,7 +358,7 @@ static int vfu_object_mr_rw(MemoryRegion *mr, uint8_t *buf, hwaddr offset, int access_size; uint64_t val; - if (memory_access_is_direct(mr, is_write)) { + if (memory_access_is_direct(mr, is_write, MEMTXATTRS_UNSPECIFIED)) { /** * Some devices expose a PCI expansion ROM, which could be buffer * based as compared to other regions which are primarily based on diff --git a/include/exec/memory.h b/include/exec/memory.h index 4e2cf95ab6..b18ecf933e 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -3012,7 +3012,8 @@ static inline bool memory_region_supports_direct_access(MemoryRegion *mr) return !memory_region_is_ram_device(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, + MemTxAttrs attrs) { if (!memory_region_supports_direct_access(mr)) { return false; @@ -3053,7 +3054,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, attrs)) { ptr = qemu_map_ram_ptr(mr->ram_block, addr1); memcpy(buf, ptr, len); } else { diff --git a/system/memory_ldst.c.inc b/system/memory_ldst.c.inc index 0e6f3940a9..7f32d3d9ff 100644 --- a/system/memory_ldst.c.inc +++ b/system/memory_ldst.c.inc @@ -34,7 +34,7 @@ static inline uint32_t glue(address_space_ldl_internal, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, false, attrs); - if (l < 4 || !memory_access_is_direct(mr, false)) { + if (l < 4 || !memory_access_is_direct(mr, false, attrs)) { release_lock |= prepare_mmio_access(mr); /* I/O case */ @@ -103,7 +103,7 @@ static inline uint64_t glue(address_space_ldq_internal, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, false, attrs); - if (l < 8 || !memory_access_is_direct(mr, false)) { + if (l < 8 || !memory_access_is_direct(mr, false, attrs)) { release_lock |= prepare_mmio_access(mr); /* I/O case */ @@ -170,7 +170,7 @@ uint8_t glue(address_space_ldub, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, false, attrs); - if (!memory_access_is_direct(mr, false)) { + if (!memory_access_is_direct(mr, false, attrs)) { release_lock |= prepare_mmio_access(mr); /* I/O case */ @@ -207,7 +207,7 @@ static inline uint16_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, false, attrs); - if (l < 2 || !memory_access_is_direct(mr, false)) { + if (l < 2 || !memory_access_is_direct(mr, false, attrs)) { release_lock |= prepare_mmio_access(mr); /* I/O case */ @@ -277,7 +277,7 @@ void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, true, attrs); - if (l < 4 || !memory_access_is_direct(mr, true)) { + if (l < 4 || !memory_access_is_direct(mr, true, attrs)) { release_lock |= prepare_mmio_access(mr); r = memory_region_dispatch_write(mr, addr1, val, MO_32, attrs); @@ -314,7 +314,7 @@ static inline void glue(address_space_stl_internal, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, true, attrs); - if (l < 4 || !memory_access_is_direct(mr, true)) { + if (l < 4 || !memory_access_is_direct(mr, true, attrs)) { release_lock |= prepare_mmio_access(mr); r = memory_region_dispatch_write(mr, addr1, val, MO_32 | devend_memop(endian), attrs); @@ -377,7 +377,7 @@ void glue(address_space_stb, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, true, attrs); - if (!memory_access_is_direct(mr, true)) { + if (!memory_access_is_direct(mr, true, attrs)) { release_lock |= prepare_mmio_access(mr); r = memory_region_dispatch_write(mr, addr1, val, MO_8, attrs); } else { @@ -410,7 +410,7 @@ static inline void glue(address_space_stw_internal, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, true, attrs); - if (l < 2 || !memory_access_is_direct(mr, true)) { + if (l < 2 || !memory_access_is_direct(mr, true, attrs)) { release_lock |= prepare_mmio_access(mr); r = memory_region_dispatch_write(mr, addr1, val, MO_16 | devend_memop(endian), attrs); @@ -474,7 +474,7 @@ static void glue(address_space_stq_internal, SUFFIX)(ARG1_DECL, RCU_READ_LOCK(); mr = TRANSLATE(addr, &addr1, &l, true, attrs); - if (l < 8 || !memory_access_is_direct(mr, true)) { + if (l < 8 || !memory_access_is_direct(mr, true, attrs)) { release_lock |= prepare_mmio_access(mr); r = memory_region_dispatch_write(mr, addr1, val, MO_64 | devend_memop(endian), attrs); diff --git a/system/physmem.c b/system/physmem.c index cff15ca1df..8745c10c9d 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -573,7 +573,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)) { hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr; *plen = MIN(page, *plen); } @@ -2869,7 +2869,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)) { uint64_t val; MemTxResult result; bool release_lock = prepare_mmio_access(mr); @@ -2965,7 +2965,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)) { /* I/O case */ uint64_t val; MemTxResult result; @@ -3274,7 +3274,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)) { l = memory_access_size(mr, l, addr); if (!memory_region_access_valid(mr, xlat, l, is_write, attrs)) { return false; @@ -3354,7 +3354,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)) { size_t used = qatomic_read(&as->bounce_buffer_size); for (;;) { hwaddr alloc = MIN(as->max_bounce_buffer_size - used, l); @@ -3487,7 +3487,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, MEMTXATTRS_UNSPECIFIED)) { /* 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. -- cgit 1.4.1 From 1cceedd7726556052d3d3bcf08a07b7762f8aa7c Mon Sep 17 00:00:00 2001 From: David Hildenbrand Date: Mon, 10 Feb 2025 09:46:48 +0100 Subject: physmem: teach cpu_memory_rw_debug() to write to more memory regions Right now, we only allow for writing to memory regions that allow direct access using memcpy etc; all other writes are simply ignored. This implies that debugging guests will not work as expected when writing to MMIO device regions. Let's extend cpu_memory_rw_debug() to write to more memory regions, including MMIO device regions. Reshuffle the condition in memory_access_is_direct() to make it easier to read and add a comment. While this change implies that debug access can now also write to MMIO devices, we now are also permit ELF image loads and similar users of cpu_memory_rw_debug() to write to MMIO devices; currently we ignore these writes. Peter assumes [1] that there's probably a class of guest images, which will start writing junk (likely zeroes) into device model registers; we previously would silently ignore any such bogus ELF sections. Likely these images are of questionable correctness and this can be ignored. If ever a problem, we could make these cases use address_space_write_rom() instead, which is left unchanged for now. This patch is based on previous work by Stefan Zabka. [1] https://lore.kernel.org/all/CAFEAcA_2CEJKFyjvbwmpt=on=GgMVamQ5hiiVt+zUr6AY3X=Xg@mail.gmail.com/ Resolves: https://gitlab.com/qemu-project/qemu/-/issues/213 Reviewed-by: Peter Xu Signed-off-by: David Hildenbrand Link: https://lore.kernel.org/r/20250210084648.33798-8-david@redhat.com Signed-off-by: Peter Xu --- hw/core/cpu-system.c | 13 +++++++++---- include/exec/memattrs.h | 5 ++++- include/exec/memory.h | 3 ++- system/physmem.c | 9 ++------- 4 files changed, 17 insertions(+), 13 deletions(-) (limited to 'include/exec/memory.h') diff --git a/hw/core/cpu-system.c b/hw/core/cpu-system.c index 6aae28a349..6e307c8959 100644 --- a/hw/core/cpu-system.c +++ b/hw/core/cpu-system.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 060b7e7131..8db1d30464 100644 --- a/include/exec/memattrs.h +++ b/include/exec/memattrs.h @@ -44,6 +44,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; @@ -56,7 +58,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". */ bool unspecified; diff --git a/include/exec/memory.h b/include/exec/memory.h index b18ecf933e..78c4e0aec8 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -3018,7 +3018,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write, if (!memory_region_supports_direct_access(mr)) { return false; } - if (is_write) { + /* Debug access can write to ROM. */ + if (is_write && !attrs.debug) { return !mr->readonly && !mr->rom_device; } return true; diff --git a/system/physmem.c b/system/physmem.c index 8745c10c9d..d3efdf13d3 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -3680,13 +3680,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; } -- cgit 1.4.1