Re: [PATCH v5 05/14] softmmu/memory: Pass ram_flags to memory_region_init_ram_shared_nomigrate()

[David Hildenbrand]

On 20.04.21 12:40, Philippe Mathieu-Daudé wrote:
> On 4/20/21 12:27 PM, David Hildenbrand wrote:
> > On 20.04.21 12:20, Philippe Mathieu-Daudé wrote:
> > > Hi David,
> > >
> > > On 4/13/21 11:14 AM, David Hildenbrand wrote:
> > > > Let's forward ram_flags instead, renaming
> > > > memory_region_init_ram_shared_nomigrate() into
> > > > memory_region_init_ram_flags_nomigrate(). Forward flags to
> > > > qemu_ram_alloc() and qemu_ram_alloc_internal().
> > > >
> > > > Reviewed-by: Peter Xu <peterx@redhat.com>
> > > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > > ---
> > > >    backends/hostmem-ram.c                        |  6 +++--
> > > >    hw/m68k/next-cube.c                           |  4 ++--
> > > >    include/exec/memory.h                         | 24 +++++++++----------
> > > >    include/exec/ram_addr.h                       |  2 +-
> > > >    .../memory-region-housekeeping.cocci          |  8 +++----
> > > >    softmmu/memory.c                              | 20 ++++++++--------
> > >
> > > OK up to here, but the qemu_ram_alloc_internal() changes
> > > in softmmu/physmem.c belong to a different patch (except
> > > the line adding "new_block->flags = ram_flags").
> > > Do you mind splitting it?
> >
> > Can you elaborate? Temporarily passing both "ram_flags" and "bool
> > resizeable, bool share" to qemu_ram_alloc_internal()?
> >
> > I don't see a big benefit in doing that besides even more effective
> > changes in two individual patches. But maybe if you elaborate, i can see
> > what you would like to see :)
>
> In this patch I see
>
> 1/ change a parameter and propagate it
> 2/ adapt assertions
>
> I'd rather review the assertions modified / cleaned in another patch,
> simply because it required me 2 different mental efforts to review the
> first part and the second part. But maybe it is not possible, so I'll
> review the 2nd part here.

Well, previously you could pass in "bool resizeable, bool share", now 
you can pass in ram_flags with RAM_SHARED, RAM_RESIZEABLE. So that 
assertion part actually looked fairly straight forward to me.

> > diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> > index cc59f05593..fdcd38ba61 100644
> > --- a/softmmu/physmem.c
> > +++ b/softmmu/physmem.c
> > @@ -2108,12 +2108,14 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t
> size, ram_addr_t max_size,
> >                                     void (*resized)(const char*,
> >                                                     uint64_t length,
> >                                                     void *host),
> > -                                  void *host, bool resizeable, bool
> share,
> > +                                  void *host, uint32_t ram_flags,
> >                                     MemoryRegion *mr, Error **errp)
> >   {
> >       RAMBlock *new_block;
> >       Error *local_err = NULL;
>
> Maybe also:
>
>         assert(!host || (ram_flags & RAM_PREALLOC));

It should be even stricter I think

assert(!host ^ (ram_flags & RAM_PREALLOC));

I'd move that change definitely to a separate patch.

Thanks!

--
Thanks,

David / dhildenb