Finish cleaning up qemu_init

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

Posted by Paolo Bonzini 5 years, 2 months ago

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;
 }
 
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).
-- 
2.26.2

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

Posted by Igor Mammedov 5 years, 2 months ago

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.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> 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;
>  }
>  
> 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).

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

Posted by Igor Mammedov 5 years, 2 months ago

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).

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

Posted by Paolo Bonzini 5 years, 2 months ago

On 07/12/20 17:38, Igor Mammedov wrote:
> 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();
>   }

No, 1 << DIRTY_MEMORY_CODE moves altogether from mr->dirty_log_mask to 
being only in the return value of memory_region_get_dirty_log_mask.

Paolo

>>   
>> 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).
>