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 <da...@redhat.com>
>>> ---
>>> 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 :)
--
Thanks,
David / dhildenb
