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 <pet...@redhat.com> >>> Signed-off-by: David Hildenbrand <da...@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. > 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)); > + assert((ram_flags & ~(RAM_SHARED | RAM_RESIZEABLE)) == 0); > + > size = HOST_PAGE_ALIGN(size); > max_size = HOST_PAGE_ALIGN(max_size); > new_block = g_malloc0(sizeof(*new_block)); > @@ -2125,15 +2127,10 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size, > new_block->fd = -1; > new_block->page_size = qemu_real_host_page_size; > new_block->host = host; > + new_block->flags = ram_flags; > if (host) { > new_block->flags |= RAM_PREALLOC; > } We could also remove this statement ... > - if (share) { > - new_block->flags |= RAM_SHARED; > - } > - if (resizeable) { > - new_block->flags |= RAM_RESIZEABLE; > - } > ram_block_add(new_block, &local_err); > if (local_err) { > g_free(new_block); > @@ -2146,15 +2143,14 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size, > RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host, > MemoryRegion *mr, Error **errp) > { > - return qemu_ram_alloc_internal(size, size, NULL, host, false, > - false, mr, errp); ... by passing RAM_PREALLOC here. > + return qemu_ram_alloc_internal(size, size, NULL, host, 0, mr, errp); > } > > -RAMBlock *qemu_ram_alloc(ram_addr_t size, bool share, > +RAMBlock *qemu_ram_alloc(ram_addr_t size, uint32_t ram_flags, > MemoryRegion *mr, Error **errp) > { > - return qemu_ram_alloc_internal(size, size, NULL, NULL, false, > - share, mr, errp); > + assert((ram_flags & ~RAM_SHARED) == 0); > + return qemu_ram_alloc_internal(size, size, NULL, NULL, ram_flags, mr, errp); > } > > RAMBlock *qemu_ram_alloc_resizeable(ram_addr_t size, ram_addr_t maxsz, > @@ -2163,8 +2159,8 @@ RAMBlock *qemu_ram_alloc_resizeable(ram_addr_t size, ram_addr_t maxsz, > void *host), > MemoryRegion *mr, Error **errp) > { > - return qemu_ram_alloc_internal(size, maxsz, resized, NULL, true, > - false, mr, errp); > + return qemu_ram_alloc_internal(size, maxsz, resized, NULL, > + RAM_RESIZEABLE, mr, errp); > } > > static void reclaim_ramblock(RAMBlock *block) >