[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 05/14] softmmu/memory: Pass ram_flags to memory_region_ini

From: David Hildenbrand
Subject: Re: [PATCH v5 05/14] softmmu/memory: Pass ram_flags to memory_region_init_ram_shared_nomigrate()
Date: Tue, 20 Apr 2021 13:10:16 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

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



David / dhildenb

reply via email to

[Prev in Thread] Current Thread [Next in Thread]