* Yury Kotov (yury-ko...@yandex-team.ru) wrote: > This patch isn't intended to merge. Just to reproduce a problem. > > The test for x-ignore-shread capability fails on aarch64 + tcg: > Memory content inconsistency at 44c00000 first_byte = 2 last_byte = 1 current > = d1 hit_edge = 1 > Memory content inconsistency at 44c01000 first_byte = 2 last_byte = 1 current > = 77 hit_edge = 1 > > I expected that QEMU doesn't write to guest RAM until VM starts, but it > happens in this test. By this patch I found what causes this problem. > > Backtrace: > 0 0x00007fb9e40affea in __memcpy_avx_unaligned () at > ../sysdeps/x86_64/multiarch/memcpy-avx-unaligned.S:118 > 1 0x000055f4a296dd84 in address_space_write_rom_internal () at exec.c:3458 > 2 0x000055f4a296de3a in address_space_write_rom () at exec.c:3479 > 3 0x000055f4a2d519ff in rom_reset () at hw/core/loader.c:1101 > 4 0x000055f4a2d475ec in qemu_devices_reset () at hw/core/reset.c:69 > 5 0x000055f4a2c90a28 in qemu_system_reset () at vl.c:1675 > 6 0x000055f4a2c9851d in main () at vl.c:4552 > > To fix this particular we can skip these writes for the first system_reset > if -incoming is set. But I'm not sure how to fix this problem in general. > May be to introduce a contract which forbids to write to system_ram in such > case? > > What do you think?
I thought that ROMs would either: a) Be mapped shared from a file but then read-only and unwritten or b) Be written to during boot - but this wouldn't be main memory, so wouldn't be affected by your shared flag. So which case is happening here? How are the ROMs being mapped - is this actually a write to main memory or the RAMBlock backing just the ROM? Dave > Signed-off-by: Yury Kotov <yury-ko...@yandex-team.ru> > --- > backends/hostmem-file.c | 2 +- > exec.c | 15 +++++++++++++-- > include/exec/cpu-common.h | 2 ++ > include/exec/memory.h | 3 +++ > include/qemu/mmap-alloc.h | 2 +- > migration/ram.c | 2 ++ > util/mmap-alloc.c | 6 ++++-- > util/oslib-posix.c | 2 +- > vl.c | 8 ++++++++ > 9 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c > index ce54788048..146fa2bc70 100644 > --- a/backends/hostmem-file.c > +++ b/backends/hostmem-file.c > @@ -61,7 +61,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error > **errp) > memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), > name, > backend->size, fb->align, > - (backend->share ? RAM_SHARED : 0) | > + (backend->share ? (RAM_SHARED | > RAM_READONLY) : 0) | > (fb->is_pmem ? RAM_PMEM : 0), > fb->mem_path, errp); > g_free(name); > diff --git a/exec.c b/exec.c > index 1d4f3784d6..cd86e9b837 100644 > --- a/exec.c > +++ b/exec.c > @@ -1863,7 +1863,7 @@ static void *file_ram_alloc(RAMBlock *block, > } > > area = qemu_ram_mmap(fd, memory, block->mr->align, > - block->flags & RAM_SHARED); > + block->flags & RAM_SHARED, block->flags & > RAM_READONLY); > if (area == MAP_FAILED) { > error_setg_errno(errp, errno, > "unable to map backing store for guest RAM"); > @@ -2259,7 +2259,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, > MemoryRegion *mr, > int64_t file_size; > > /* Just support these ram flags by now. */ > - assert((ram_flags & ~(RAM_SHARED | RAM_PMEM)) == 0); > + assert((ram_flags & ~(RAM_SHARED | RAM_PMEM | RAM_READONLY)) == 0); > > if (xen_enabled()) { > error_setg(errp, "-mem-path not supported with Xen"); > @@ -2485,6 +2485,17 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length) > } > } > } > + > +void qemu_ram_unprotect_all(void) > +{ > + RAMBlock *block; > + rcu_read_lock(); > + RAMBLOCK_FOREACH(block) { > + int ret = mprotect(block->host, block->max_length, PROT_READ | > PROT_WRITE); > + assert(ret == 0); > + } > + rcu_read_unlock(); > +} > #endif /* !_WIN32 */ > > /* Return a host pointer to ram allocated with qemu_ram_alloc. > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h > index cef8b88a2a..2ad875be06 100644 > --- a/include/exec/cpu-common.h > +++ b/include/exec/cpu-common.h > @@ -63,6 +63,8 @@ typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, > uint32_t value); > typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr); > > void qemu_ram_remap(ram_addr_t addr, ram_addr_t length); > +void qemu_ram_unprotect_all(void); > + > /* This should not be used by devices. */ > ram_addr_t qemu_ram_addr_from_host(void *ptr); > RAMBlock *qemu_ram_block_by_name(const char *name); > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 1625913f84..b1cb5a48d7 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -126,6 +126,9 @@ typedef struct IOMMUNotifier IOMMUNotifier; > /* RAM is a persistent kind memory */ > #define RAM_PMEM (1 << 5) > > +/* RAM is readonly*/ > +#define RAM_READONLY (1 << 6) > + > static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, > IOMMUNotifierFlag flags, > hwaddr start, hwaddr end, > diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h > index ef04f0ed5b..f704d9b7e3 100644 > --- a/include/qemu/mmap-alloc.h > +++ b/include/qemu/mmap-alloc.h > @@ -7,7 +7,7 @@ size_t qemu_fd_getpagesize(int fd); > > size_t qemu_mempath_getpagesize(const char *mem_path); > > -void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared); > +void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared, bool > readonly); > > void qemu_ram_munmap(int fd, void *ptr, size_t size); > > diff --git a/migration/ram.c b/migration/ram.c > index 35bd6213e9..252fc80e6c 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -4211,6 +4211,8 @@ static int ram_load(QEMUFile *f, void *opaque, int > version_id) > */ > rcu_read_lock(); > > + qemu_ram_unprotect_all(); > + > if (postcopy_running) { > ret = ram_load_postcopy(f); > } > diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c > index 8565885420..7dc194d909 100644 > --- a/util/mmap-alloc.c > +++ b/util/mmap-alloc.c > @@ -75,9 +75,10 @@ size_t qemu_mempath_getpagesize(const char *mem_path) > return getpagesize(); > } > > -void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared) > +void *qemu_ram_mmap(int fd, size_t size, size_t align, bool shared, bool > readonly) > { > int flags; > + int prots; > int guardfd; > size_t offset; > size_t pagesize; > @@ -128,9 +129,10 @@ void *qemu_ram_mmap(int fd, size_t size, size_t align, > bool shared) > flags = MAP_FIXED; > flags |= fd == -1 ? MAP_ANONYMOUS : 0; > flags |= shared ? MAP_SHARED : MAP_PRIVATE; > + prots = PROT_READ | (readonly ? 0 : PROT_WRITE); > offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr; > > - ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE, flags, fd, > 0); > + ptr = mmap(guardptr + offset, size, prots, flags, fd, 0); > > if (ptr == MAP_FAILED) { > munmap(guardptr, total); > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 37c5854b9c..c57466f8d9 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -203,7 +203,7 @@ void *qemu_memalign(size_t alignment, size_t size) > void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared) > { > size_t align = QEMU_VMALLOC_ALIGN; > - void *ptr = qemu_ram_mmap(-1, size, align, shared); > + void *ptr = qemu_ram_mmap(-1, size, align, shared, false); > > if (ptr == MAP_FAILED) { > return NULL; > diff --git a/vl.c b/vl.c > index 4c5cc0d8ad..2cc675ad21 100644 > --- a/vl.c > +++ b/vl.c > @@ -2950,6 +2950,12 @@ static void register_global_properties(MachineState > *ms) > user_register_global_props(); > } > > +static void unprotect_ram(void *opaque) > +{ > + error_report(__func__); > + qemu_ram_unprotect_all(); > +} > + > int main(int argc, char **argv, char **envp) > { > int i; > @@ -4537,6 +4543,8 @@ int main(int argc, char **argv, char **envp) > > replay_start(); > > + qemu_register_reset(unprotect_ram, NULL); > + > /* This checkpoint is required by replay to separate prior clock > reading from the other reads, because timer polling functions query > clock values from the log. */ > -- > 2.21.0 > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK