Re: [qemu-s390x] [Qemu-ppc] [Qemu-devel] [PATCH v2 16/17] s390x: initialize memory region for memory devices

[Murilo Opsfelder Araujo]

On Sat, May 12, 2018 at 09:53:54AM +0200, David Hildenbrand wrote:
> On 11.05.2018 20:43, Eduardo Habkost wrote:
> > On Fri, May 11, 2018 at 03:34:05PM -0300, Murilo Opsfelder Araujo wrote:
> >> On Fri, May 11, 2018 at 03:19:52PM +0200, David Hildenbrand wrote:
> >>> While s390x has no real interface for communicating devices mapped into
> >>> the physical address space of the guest, paravirtualized devices can
> >>> easily expose the applicable address range themselves.
> >>>
> >>> So let's use the difference between maxram_size and ram_size as the size
> >>> for our hotplug memory area (just as on other architectures).
> >>>
> >>> Signed-off-by: David Hildenbrand <address@hidden>
> >>> ---
> >>>  hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++--
> >>>  1 file changed, 26 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >>> index ee0a2b124f..09b755282b 100644
> >>> --- a/hw/s390x/s390-virtio-ccw.c
> >>> +++ b/hw/s390x/s390-virtio-ccw.c
> >>> @@ -157,9 +157,11 @@ static void virtio_ccw_register_hcalls(void)
> >>>  #define KVM_MEM_MAX_NR_PAGES ((1ULL << 31) - 1)
> >>>  #define SEG_MSK (~0xfffffULL)
> >>>  #define KVM_SLOT_MAX_BYTES ((KVM_MEM_MAX_NR_PAGES * TARGET_PAGE_SIZE) & 
> >>> SEG_MSK)
> >>> -static void s390_memory_init(ram_addr_t mem_size)
> >>> +static void s390_memory_init(MachineState *machine)
> >>>  {
> >>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> >>>      MemoryRegion *sysmem = get_system_memory();
> >>> +    ram_addr_t mem_size = machine->ram_size;
> >>>      ram_addr_t chunk, offset = 0;
> >>>      unsigned int number = 0;
> >>>      gchar *name;
> >>> @@ -181,6 +183,28 @@ static void s390_memory_init(ram_addr_t mem_size)
> >>>      }
> >>>      g_free(name);
> >>>  
> >>> +    /* always allocate the device memory information */
> >>> +    machine->device_memory = g_malloc0(sizeof(*machine->device_memory));
> >>
> >> Is there any QEMU guideline/preference/recommendation in using g_new0
> >> vs. g_malloc0?
> >>
> >> I recall Paolo suggesting g_new0 instead of g_malloc0 in another patch:
> >>
> >>   http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg02372.html
> > 
> 
> This patch comes unmodified from my same queue, therefore the code looks
> identical :)
> 
> > I don't see any reason to not use g_new0() instead of
> > g_malloc0(sizeof(...)), as it's more readable.
> 
> I clearly favor g_malloc over g_new (except for arrays) for two simple
> reasons
> 
> 1. No need to specify the type. Impossible to specify the wrong type.
> Easy to rename types.
> 
> 2. Every C developer should be able to understand what g_malloc() does.
> This is not true for g_new. Especially as it might look strange for C++
> developers (new vs. new[] - why don't we have g_new() vs. g_new_array())
> 
> I am a simple man, I prefer functions with one parameter if only one
> parameter is needed :)
> 
> > 
> > But I don't think it's a problem that should block the patch from
> > being merged.  We have hundreds of g_malloc*(sizeof(...)) calls
> > in the tree.
> 
> I assume there are a lot of hard feelings about this. I will continue
> using g_malloc() for scalars until the last user is removed from the
> QEMU source code. Or there is a coding style statement about it (haven't
> found one) ... or people start to curse me when I send patches :)

Having g_malloc() for scalars and g_new() for arrays makes sense.

I understand and agree that using g_malloc() should not be a blocker for
a patch, as Eduardo stated.

Looking at the history, there are quite a few patches replacing
g_malloc*() by g_new*() because "is safer against overflow" (see commit
071d4054770205ddb8a58a9e2735069d8fe52af1 as an example):

    git log --oneline --grep=g_new

Perhaps we just need to update "3. Low level memory management" section
in HACKING file describing the situations where g_new() is preferred vs.
g_malloc() and vice-versa; and use the agreed criteria to ack/nack
patches.

-- 
Murilo