From 4c7c8563191fd65d98cba05352a1fc1fbef6d817 Mon Sep 17 00:00:00 2001 From: Jonathan Cameron Date: Thu, 7 Mar 2024 15:37:07 +0000 Subject: physmem: Rename addr1 to more informative mr_addr in flatview_read/write() and similar The calls to flatview_read/write[_continue]() have parameters addr and addr1 but the names give no indication of what they are addresses of. Rename addr1 to mr_addr to reflect that it is the translated address offset within the MemoryRegion returned by flatview_translate(). Similarly rename the parameter in address_space_read/write_cached_slow() Suggested-by: Peter Xu Signed-off-by: Jonathan Cameron Reviewed-by: David Hildenbrand Link: https://lore.kernel.org/r/20240307153710.30907-2-Jonathan.Cameron@huawei.com Signed-off-by: Peter Xu --- system/physmem.c | 50 +++++++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 25 deletions(-) (limited to 'system/physmem.c') diff --git a/system/physmem.c b/system/physmem.c index 6e9ed97597..e92bed50a6 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -2685,7 +2685,7 @@ static bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, MemTxAttrs attrs, const void *ptr, - hwaddr len, hwaddr addr1, + hwaddr len, hwaddr mr_addr, hwaddr l, MemoryRegion *mr) { uint8_t *ram_ptr; @@ -2695,12 +2695,12 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, const uint8_t *buf = ptr; for (;;) { - if (!flatview_access_allowed(mr, attrs, addr1, l)) { + if (!flatview_access_allowed(mr, attrs, mr_addr, l)) { result |= MEMTX_ACCESS_ERROR; /* Keep going. */ } else if (!memory_access_is_direct(mr, true)) { release_lock |= prepare_mmio_access(mr); - l = memory_access_size(mr, l, addr1); + l = memory_access_size(mr, l, mr_addr); /* XXX: could force current_cpu to NULL to avoid potential bugs */ @@ -2715,13 +2715,13 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, (l == 8 && len >= 8)); #endif val = ldn_he_p(buf, l); - result |= memory_region_dispatch_write(mr, addr1, val, + result |= memory_region_dispatch_write(mr, mr_addr, val, size_memop(l), attrs); } else { /* RAM case */ - ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false); + ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, false); memmove(ram_ptr, buf, l); - invalidate_and_set_dirty(mr, addr1, l); + invalidate_and_set_dirty(mr, mr_addr, l); } if (release_lock) { @@ -2738,7 +2738,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, } l = len; - mr = flatview_translate(fv, addr, &addr1, &l, true, attrs); + mr = flatview_translate(fv, addr, &mr_addr, &l, true, attrs); } return result; @@ -2749,22 +2749,22 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs, const void *buf, hwaddr len) { hwaddr l; - hwaddr addr1; + hwaddr mr_addr; MemoryRegion *mr; l = len; - mr = flatview_translate(fv, addr, &addr1, &l, true, attrs); + mr = flatview_translate(fv, addr, &mr_addr, &l, true, attrs); if (!flatview_access_allowed(mr, attrs, addr, len)) { return MEMTX_ACCESS_ERROR; } return flatview_write_continue(fv, addr, attrs, buf, len, - addr1, l, mr); + mr_addr, l, mr); } /* Called within RCU critical section. */ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, MemTxAttrs attrs, void *ptr, - hwaddr len, hwaddr addr1, hwaddr l, + hwaddr len, hwaddr mr_addr, hwaddr l, MemoryRegion *mr) { uint8_t *ram_ptr; @@ -2775,14 +2775,14 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, fuzz_dma_read_cb(addr, len, mr); for (;;) { - if (!flatview_access_allowed(mr, attrs, addr1, l)) { + if (!flatview_access_allowed(mr, attrs, mr_addr, l)) { result |= MEMTX_ACCESS_ERROR; /* Keep going. */ } else if (!memory_access_is_direct(mr, false)) { /* I/O case */ release_lock |= prepare_mmio_access(mr); - l = memory_access_size(mr, l, addr1); - result |= memory_region_dispatch_read(mr, addr1, &val, + l = memory_access_size(mr, l, mr_addr); + result |= memory_region_dispatch_read(mr, mr_addr, &val, size_memop(l), attrs); /* @@ -2798,7 +2798,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, stn_he_p(buf, l, val); } else { /* RAM case */ - ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false); + ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, false); memcpy(buf, ram_ptr, l); } @@ -2816,7 +2816,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, } l = len; - mr = flatview_translate(fv, addr, &addr1, &l, false, attrs); + mr = flatview_translate(fv, addr, &mr_addr, &l, false, attrs); } return result; @@ -2827,16 +2827,16 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr, MemTxAttrs attrs, void *buf, hwaddr len) { hwaddr l; - hwaddr addr1; + hwaddr mr_addr; MemoryRegion *mr; l = len; - mr = flatview_translate(fv, addr, &addr1, &l, false, attrs); + mr = flatview_translate(fv, addr, &mr_addr, &l, false, attrs); if (!flatview_access_allowed(mr, attrs, addr, len)) { return MEMTX_ACCESS_ERROR; } return flatview_read_continue(fv, addr, attrs, buf, len, - addr1, l, mr); + mr_addr, l, mr); } MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr, @@ -3348,15 +3348,15 @@ MemTxResult address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr, void *buf, hwaddr len) { - hwaddr addr1, l; + hwaddr mr_addr, l; MemoryRegion *mr; l = len; - mr = address_space_translate_cached(cache, addr, &addr1, &l, false, + mr = address_space_translate_cached(cache, addr, &mr_addr, &l, false, MEMTXATTRS_UNSPECIFIED); return flatview_read_continue(cache->fv, addr, MEMTXATTRS_UNSPECIFIED, buf, len, - addr1, l, mr); + mr_addr, l, mr); } /* Called from RCU critical section. address_space_write_cached uses this @@ -3366,15 +3366,15 @@ MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr, const void *buf, hwaddr len) { - hwaddr addr1, l; + hwaddr mr_addr, l; MemoryRegion *mr; l = len; - mr = address_space_translate_cached(cache, addr, &addr1, &l, true, + mr = address_space_translate_cached(cache, addr, &mr_addr, &l, true, MEMTXATTRS_UNSPECIFIED); return flatview_write_continue(cache->fv, addr, MEMTXATTRS_UNSPECIFIED, buf, len, - addr1, l, mr); + mr_addr, l, mr); } #define ARG1_DECL MemoryRegionCache *cache -- cgit 1.4.1 From bcfd8ba4f5d887dc9923738131c8598e90e76bb2 Mon Sep 17 00:00:00 2001 From: Jonathan Cameron Date: Thu, 7 Mar 2024 15:37:08 +0000 Subject: physmem: Reduce local variable scope in flatview_read/write_continue() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precursor to factoring out the inner loops for reuse. Reviewed-by: Peter Xu Signed-off-by: Jonathan Cameron Reviewed-by: David Hildenbrand Reviewed-by: Philippe Mathieu-Daudé Link: https://lore.kernel.org/r/20240307153710.30907-3-Jonathan.Cameron@huawei.com Signed-off-by: Peter Xu --- system/physmem.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) (limited to 'system/physmem.c') diff --git a/system/physmem.c b/system/physmem.c index e92bed50a6..e35aa29343 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -2688,10 +2688,7 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, hwaddr len, hwaddr mr_addr, hwaddr l, MemoryRegion *mr) { - uint8_t *ram_ptr; - uint64_t val; MemTxResult result = MEMTX_OK; - bool release_lock = false; const uint8_t *buf = ptr; for (;;) { @@ -2699,7 +2696,9 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, result |= MEMTX_ACCESS_ERROR; /* Keep going. */ } else if (!memory_access_is_direct(mr, true)) { - release_lock |= prepare_mmio_access(mr); + uint64_t val; + bool release_lock = prepare_mmio_access(mr); + l = memory_access_size(mr, l, mr_addr); /* XXX: could force current_cpu to NULL to avoid potential bugs */ @@ -2717,18 +2716,21 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, val = ldn_he_p(buf, l); result |= memory_region_dispatch_write(mr, mr_addr, val, size_memop(l), attrs); + if (release_lock) { + bql_unlock(); + } + + } else { /* RAM case */ - ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, false); + + uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, + false); + memmove(ram_ptr, buf, l); invalidate_and_set_dirty(mr, mr_addr, l); } - if (release_lock) { - bql_unlock(); - release_lock = false; - } - len -= l; buf += l; addr += l; @@ -2767,10 +2769,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, hwaddr len, hwaddr mr_addr, hwaddr l, MemoryRegion *mr) { - uint8_t *ram_ptr; - uint64_t val; MemTxResult result = MEMTX_OK; - bool release_lock = false; uint8_t *buf = ptr; fuzz_dma_read_cb(addr, len, mr); @@ -2780,7 +2779,9 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, /* Keep going. */ } else if (!memory_access_is_direct(mr, false)) { /* I/O case */ - release_lock |= prepare_mmio_access(mr); + uint64_t val; + bool release_lock = prepare_mmio_access(mr); + l = memory_access_size(mr, l, mr_addr); result |= memory_region_dispatch_read(mr, mr_addr, &val, size_memop(l), attrs); @@ -2796,17 +2797,16 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, (l == 8 && len >= 8)); #endif stn_he_p(buf, l, val); + if (release_lock) { + bql_unlock(); + } } else { /* RAM case */ - ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, false); + uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, + false); memcpy(buf, ram_ptr, l); } - if (release_lock) { - bql_unlock(); - release_lock = false; - } - len -= l; buf += l; addr += l; -- cgit 1.4.1 From e7927d33cf667d2c034f7b6aa0c0d0dde22a9753 Mon Sep 17 00:00:00 2001 From: Jonathan Cameron Date: Thu, 7 Mar 2024 15:37:09 +0000 Subject: physmem: Factor out body of flatview_read/write_continue() loop This code will be reused for the address_space_cached accessors shortly. Also reduce scope of result variable now we aren't directly calling this in the loop. Signed-off-by: Jonathan Cameron Reviewed-by: David Hildenbrand Link: https://lore.kernel.org/r/20240307153710.30907-4-Jonathan.Cameron@huawei.com Signed-off-by: Peter Xu --- system/physmem.c | 169 ++++++++++++++++++++++++++++++++----------------------- 1 file changed, 99 insertions(+), 70 deletions(-) (limited to 'system/physmem.c') diff --git a/system/physmem.c b/system/physmem.c index e35aa29343..737869a3f5 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -2681,6 +2681,56 @@ static bool flatview_access_allowed(MemoryRegion *mr, MemTxAttrs attrs, return false; } +static MemTxResult flatview_write_continue_step(MemTxAttrs attrs, + const uint8_t *buf, + hwaddr len, hwaddr mr_addr, + hwaddr *l, MemoryRegion *mr) +{ + if (!flatview_access_allowed(mr, attrs, mr_addr, *l)) { + return MEMTX_ACCESS_ERROR; + } + + if (!memory_access_is_direct(mr, true)) { + uint64_t val; + MemTxResult result; + bool release_lock = prepare_mmio_access(mr); + + *l = memory_access_size(mr, *l, mr_addr); + /* + * XXX: could force current_cpu to NULL to avoid + * potential bugs + */ + + /* + * Assure Coverity (and ourselves) that we are not going to OVERRUN + * the buffer by following ldn_he_p(). + */ +#ifdef QEMU_STATIC_ANALYSIS + assert((*l == 1 && len >= 1) || + (*l == 2 && len >= 2) || + (*l == 4 && len >= 4) || + (*l == 8 && len >= 8)); +#endif + val = ldn_he_p(buf, *l); + result = memory_region_dispatch_write(mr, mr_addr, val, + size_memop(*l), attrs); + if (release_lock) { + bql_unlock(); + } + + return result; + } else { + /* RAM case */ + uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, l, + false); + + memmove(ram_ptr, buf, *l); + invalidate_and_set_dirty(mr, mr_addr, *l); + + return MEMTX_OK; + } +} + /* Called within RCU critical section. */ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, MemTxAttrs attrs, @@ -2692,44 +2742,8 @@ static MemTxResult flatview_write_continue(FlatView *fv, hwaddr addr, const uint8_t *buf = ptr; for (;;) { - if (!flatview_access_allowed(mr, attrs, mr_addr, l)) { - result |= MEMTX_ACCESS_ERROR; - /* Keep going. */ - } else if (!memory_access_is_direct(mr, true)) { - uint64_t val; - bool release_lock = prepare_mmio_access(mr); - - l = memory_access_size(mr, l, mr_addr); - /* XXX: could force current_cpu to NULL to avoid - potential bugs */ - - /* - * Assure Coverity (and ourselves) that we are not going to OVERRUN - * the buffer by following ldn_he_p(). - */ -#ifdef QEMU_STATIC_ANALYSIS - assert((l == 1 && len >= 1) || - (l == 2 && len >= 2) || - (l == 4 && len >= 4) || - (l == 8 && len >= 8)); -#endif - val = ldn_he_p(buf, l); - result |= memory_region_dispatch_write(mr, mr_addr, val, - size_memop(l), attrs); - if (release_lock) { - bql_unlock(); - } - - - } else { - /* RAM case */ - - uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, - false); - - memmove(ram_ptr, buf, l); - invalidate_and_set_dirty(mr, mr_addr, l); - } + result |= flatview_write_continue_step(attrs, buf, len, mr_addr, &l, + mr); len -= l; buf += l; @@ -2763,6 +2777,52 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs, mr_addr, l, mr); } +static MemTxResult flatview_read_continue_step(MemTxAttrs attrs, uint8_t *buf, + hwaddr len, hwaddr mr_addr, + hwaddr *l, + MemoryRegion *mr) +{ + if (!flatview_access_allowed(mr, attrs, mr_addr, *l)) { + return MEMTX_ACCESS_ERROR; + } + + if (!memory_access_is_direct(mr, false)) { + /* I/O case */ + uint64_t val; + MemTxResult result; + bool release_lock = prepare_mmio_access(mr); + + *l = memory_access_size(mr, *l, mr_addr); + result = memory_region_dispatch_read(mr, mr_addr, &val, size_memop(*l), + attrs); + + /* + * Assure Coverity (and ourselves) that we are not going to OVERRUN + * the buffer by following stn_he_p(). + */ +#ifdef QEMU_STATIC_ANALYSIS + assert((*l == 1 && len >= 1) || + (*l == 2 && len >= 2) || + (*l == 4 && len >= 4) || + (*l == 8 && len >= 8)); +#endif + stn_he_p(buf, *l, val); + + if (release_lock) { + bql_unlock(); + } + return result; + } else { + /* RAM case */ + uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, l, + false); + + memcpy(buf, ram_ptr, *l); + + return MEMTX_OK; + } +} + /* Called within RCU critical section. */ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, MemTxAttrs attrs, void *ptr, @@ -2774,38 +2834,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr, fuzz_dma_read_cb(addr, len, mr); for (;;) { - if (!flatview_access_allowed(mr, attrs, mr_addr, l)) { - result |= MEMTX_ACCESS_ERROR; - /* Keep going. */ - } else if (!memory_access_is_direct(mr, false)) { - /* I/O case */ - uint64_t val; - bool release_lock = prepare_mmio_access(mr); - - l = memory_access_size(mr, l, mr_addr); - result |= memory_region_dispatch_read(mr, mr_addr, &val, - size_memop(l), attrs); - - /* - * Assure Coverity (and ourselves) that we are not going to OVERRUN - * the buffer by following stn_he_p(). - */ -#ifdef QEMU_STATIC_ANALYSIS - assert((l == 1 && len >= 1) || - (l == 2 && len >= 2) || - (l == 4 && len >= 4) || - (l == 8 && len >= 8)); -#endif - stn_he_p(buf, l, val); - if (release_lock) { - bql_unlock(); - } - } else { - /* RAM case */ - uint8_t *ram_ptr = qemu_ram_ptr_length(mr->ram_block, mr_addr, &l, - false); - memcpy(buf, ram_ptr, l); - } + result |= flatview_read_continue_step(attrs, buf, len, mr_addr, &l, mr); len -= l; buf += l; -- cgit 1.4.1 From 47293c922cb4d70aae5b8100936ffc624d2a4076 Mon Sep 17 00:00:00 2001 From: Jonathan Cameron Date: Thu, 7 Mar 2024 15:37:10 +0000 Subject: physmem: Fix wrong address in large address_space_read/write_cached_slow() If the access is bigger than the MemoryRegion supports, flatview_read/write_continue() will attempt to update the Memory Region. but the address passed to flatview_translate() is relative to the cache, not to the FlatView. On arm/virt with interleaved CXL memory emulation and virtio-blk-pci this lead to the first part of descriptor being read from the CXL memory and the second part from PA 0x8 which happens to be a blank region of a flash chip and all ffs on this particular configuration. Note this test requires the out of tree ARM support for CXL, but the problem is more general. Avoid this by adding new address_space_read_continue_cached() and address_space_write_continue_cached() which share all the logic with the flatview versions except for the MemoryRegion lookup which is unnecessary as the MemoryRegionCache only covers one MemoryRegion. Signed-off-by: Jonathan Cameron Link: https://lore.kernel.org/r/20240307153710.30907-5-Jonathan.Cameron@huawei.com Signed-off-by: Peter Xu --- system/physmem.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 57 insertions(+), 6 deletions(-) (limited to 'system/physmem.c') diff --git a/system/physmem.c b/system/physmem.c index 737869a3f5..6cfb7a80ab 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -3370,6 +3370,59 @@ static inline MemoryRegion *address_space_translate_cached( return section.mr; } +/* Called within RCU critical section. */ +static MemTxResult address_space_write_continue_cached(MemTxAttrs attrs, + const void *ptr, + hwaddr len, + hwaddr mr_addr, + hwaddr l, + MemoryRegion *mr) +{ + MemTxResult result = MEMTX_OK; + const uint8_t *buf = ptr; + + for (;;) { + result |= flatview_write_continue_step(attrs, buf, len, mr_addr, &l, + mr); + + len -= l; + buf += l; + mr_addr += l; + + if (!len) { + break; + } + + l = len; + } + + return result; +} + +/* Called within RCU critical section. */ +static MemTxResult address_space_read_continue_cached(MemTxAttrs attrs, + void *ptr, hwaddr len, + hwaddr mr_addr, hwaddr l, + MemoryRegion *mr) +{ + MemTxResult result = MEMTX_OK; + uint8_t *buf = ptr; + + for (;;) { + result |= flatview_read_continue_step(attrs, buf, len, mr_addr, &l, mr); + len -= l; + buf += l; + mr_addr += l; + + if (!len) { + break; + } + l = len; + } + + return result; +} + /* Called from RCU critical section. address_space_read_cached uses this * out of line function when the target is an MMIO or IOMMU region. */ @@ -3383,9 +3436,8 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr, l = len; mr = address_space_translate_cached(cache, addr, &mr_addr, &l, false, MEMTXATTRS_UNSPECIFIED); - return flatview_read_continue(cache->fv, - addr, MEMTXATTRS_UNSPECIFIED, buf, len, - mr_addr, l, mr); + return address_space_read_continue_cached(MEMTXATTRS_UNSPECIFIED, + buf, len, mr_addr, l, mr); } /* Called from RCU critical section. address_space_write_cached uses this @@ -3401,9 +3453,8 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr, l = len; mr = address_space_translate_cached(cache, addr, &mr_addr, &l, true, MEMTXATTRS_UNSPECIFIED); - return flatview_write_continue(cache->fv, - addr, MEMTXATTRS_UNSPECIFIED, buf, len, - mr_addr, l, mr); + return address_space_write_continue_cached(MEMTXATTRS_UNSPECIFIED, + buf, len, mr_addr, l, mr); } #define ARG1_DECL MemoryRegionCache *cache -- cgit 1.4.1