Re: [PATCH 13/15] memory: allow creating MemoryRegions before accelerators

From: Igor Mammedov <imammedo@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH 13/15] memory: allow creating MemoryRegions before accelerators
Date: Mon, 7 Dec 2020 17:38:43 +0100	[thread overview]
Message-ID: <20201207173843.4cc85ead@redhat.com> (raw)
In-Reply-To: <20201202081854.4126071-14-pbonzini@redhat.com>

On Wed,  2 Dec 2020 03:18:52 -0500
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Compute the DIRTY_MEMORY_CODE bit in memory_region_get_dirty_log_mask
> instead of memory_region_init_*.  This makes it possible to allocate
> memory backend objects at any time.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  softmmu/memory.c | 12 ++++++------
>  softmmu/vl.c     |  6 +-----
>  2 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 11ca94d037..89a4723fe5 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1548,7 +1548,6 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr,
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
>      mr->ram_block = qemu_ram_alloc(size, share, mr, &err);
> -    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>      if (err) {
>          mr->size = int128_zero();
>          object_unparent(OBJECT(mr));
> @@ -1573,7 +1572,6 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
>      mr->destructor = memory_region_destructor_ram;
>      mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized,
>                                                mr, &err);
> -    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>      if (err) {
>          mr->size = int128_zero();
>          object_unparent(OBJECT(mr));
> @@ -1598,7 +1596,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>      mr->destructor = memory_region_destructor_ram;
>      mr->align = align;
>      mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err);
> -    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>      if (err) {
>          mr->size = int128_zero();
>          object_unparent(OBJECT(mr));
> @@ -1622,7 +1619,6 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>      mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
>                                             share ? RAM_SHARED : 0,
>                                             fd, &err);
> -    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>      if (err) {
>          mr->size = int128_zero();
>          object_unparent(OBJECT(mr));
> @@ -1641,7 +1637,6 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>      mr->ram = true;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
> -    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>  
>      /* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL.  */
>      assert(ptr != NULL);
> @@ -1661,7 +1656,7 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr,
>      mr->ops = &ram_device_mem_ops;
>      mr->opaque = mr;
>      mr->destructor = memory_region_destructor_ram;
> -    mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
> +
>      /* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL.  */
>      assert(ptr != NULL);
>      mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_fatal);
> @@ -1819,6 +1814,11 @@ uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr)
>                               memory_region_is_iommu(mr))) {
>          mask |= (1 << DIRTY_MEMORY_MIGRATION);
>      }
> +
> +    if (tcg_enabled() && rb) {
> +        /* TCG only cares about dirty memory logging for RAM, not IOMMU.  */
> +        mask |= (1 << DIRTY_MEMORY_CODE);
> +    }
>      return mask;
>  }

Is following also necessary?:

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 11ca94d037..f1de42e50f 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2033,7 +2033,8 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
     }
 
     memory_region_transaction_begin();
-    mr->dirty_log_mask = (mr->dirty_log_mask & ~mask) | (log * mask);
+    mr->dirty_log_mask = (memory_region_get_dirty_log_mask(mr) & ~mask) |
+                         (log * mask);
     memory_region_update_pending |= mr->enabled;
     memory_region_transaction_commit();
 }

>  
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 0f63d80472..023c16245b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1715,11 +1715,7 @@ static bool object_create_early(const char *type, QemuOpts *opts)
>          return false;
>      }
>  
> -    /* Memory allocation by backends needs to be done
> -     * after configure_accelerator() (due to the tcg_enabled()
> -     * checks at memory_region_init_*()).
> -     *
> -     * Also, allocation of large amounts of memory may delay
> +    /* Allocation of large amounts of memory may delay
>       * chardev initialization for too long, and trigger timeouts
>       * on software that waits for a monitor socket to be created
>       * (e.g. libvirt).


