19.03.2019, 12:39, "Dr. David Alan Gilbert" <dgilb...@redhat.com>:
> * 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?
It writes to the main memory. Just what I wanted to catch. For separated
RAMBlocks it works fine because separated blocks aren't shared and as a result
these aren't readonly.
P.S. For x86 qtests with this patch are passed.
Regards,
Yury
>
> 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
