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

[Paolo Bonzini]

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