| Message ID | 20180511131953.12905-17-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | MemoryDevice: use multi stage hotplug handlers | expand |
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 <david@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 > + > + /* initialize device memory address space */ > + if (machine->ram_size < machine->maxram_size) { > + ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size; > + > + if (QEMU_ALIGN_UP(machine->maxram_size, TARGET_PAGE_SIZE) != > + machine->maxram_size) { > + error_report("maximum memory size must be aligned to multiple of " > + "%d bytes", TARGET_PAGE_SIZE); > + exit(EXIT_FAILURE); > + } > + > + machine->device_memory->base = machine->ram_size; > + memory_region_init(&machine->device_memory->mr, OBJECT(ms), > + "device-memory", device_mem_size); > + memory_region_add_subregion(sysmem, machine->device_memory->base, > + &machine->device_memory->mr); > + > + } > + > /* Initialize storage key device */ > s390_skeys_init(); > /* Initialize storage attributes device */ > @@ -304,7 +328,7 @@ static void ccw_init(MachineState *machine) > DeviceState *dev; > > s390_sclp_init(); > - s390_memory_init(machine->ram_size); > + s390_memory_init(machine); > > /* init CPUs (incl. CPU model) early so s390_has_feature() works */ > s390_init_cpus(machine); > -- > 2.14.3 > >
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 <david@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 I don't see any reason to not use g_new0() instead of g_malloc0(sizeof(...)), as it's more readable. 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.
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 <david@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 :)
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 <david@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 :) 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 Opsfelder Araujo <muriloo@linux.ibm.com> writes: > 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 <david@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? Yes, there is: we prefer g_new(T, n) over g_malloc(sizeof(T) * n) even when n==1. Commit b45c03f585e explains: g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors. 'One' doesn't apply when n==1. 'Two' does. We're okay with things like T *v = g_malloc(sizeof(*v)). Yes, 'two' applies here as well, but screwups are relatively unlikely. >> >> 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. Quite possible to specify the wrong size in other ways, and the type checker can't save you then (that's 'two'), although Coverity might. >> Easy to rename types. Renaming a type is exactly as easy as renaming a variable or any other identifer: you have to update all occurences. >> 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'm sympathetic of this argument in general, but not here. g_malloc() already differs from malloc(), and for good reasons. Moreover, we use g_new() all over the place; there's simply no way to avoid understanding it. >> I am a simple man, I prefer functions with one parameter if only one >> parameter is needed :) The second parameter is moderately ugly in the non-array case. But then GLib is full of ugly. We'll live. >> > 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. No. Use g_new() with types, g_malloc() / g_malloc_n() with other expressions. When both are practical, consider preferring g_new() for extra type checking. Also consider readability. > I understand and agree that using g_malloc() should not be a blocker for > a patch, as Eduardo stated. The assignment that triggered this sub-thread machine->device_memory = g_malloc0(sizeof(*machine->device_memory)); is a matter of taste. I'd prefer machine->device_memory = g_new0(DeviceMemoryState, 1); myself, because I find it easier to read. Giving the type checker the actual type to work with is a nice bonus. > 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. We tend to update HACKING when we find ourselves debating the same things over and over. Perhaps this is such a case.
On 15.05.2018 07:58, Markus Armbruster wrote: > Murilo Opsfelder Araujo <muriloo@linux.ibm.com> writes: > >> 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 <david@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? > > Yes, there is: we prefer g_new(T, n) over g_malloc(sizeof(T) * n) even > when n==1. Commit b45c03f585e explains: > > g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, > for two reasons. One, it catches multiplication overflowing size_t. > Two, it returns T * rather than void *, which lets the compiler catch > more type errors. > > 'One' doesn't apply when n==1. 'Two' does. > > We're okay with things like T *v = g_malloc(sizeof(*v)). Yes, 'two' > applies here as well, but screwups are relatively unlikely. > >>>>> 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. > > Quite possible to specify the wrong size in other ways, and the type > checker can't save you then (that's 'two'), although Coverity might. Good point about the type checker! > >>> Easy to rename types. > > Renaming a type is exactly as easy as renaming a variable or any other > identifer: you have to update all occurences. > And that means touching more lines. >> 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. > > We tend to update HACKING when we find ourselves debating the same > things over and over. Perhaps this is such a case. > I don't want to get too involved in this discussion. (I have other problems to solve :) ) If we make this a rule, I want somebody to convert all applicable cases to the desired format. (we won't be able to convert all cases, e.g. structs with variable sized member arrays.) Thanks!
On Tue, May 15, 2018 at 09:57:43AM +0200, David Hildenbrand wrote: > On 15.05.2018 07:58, Markus Armbruster wrote: > > Murilo Opsfelder Araujo <muriloo@linux.ibm.com> writes: > > > >> 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 <david@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? > > > > Yes, there is: we prefer g_new(T, n) over g_malloc(sizeof(T) * n) even > > when n==1. Commit b45c03f585e explains: > > > > g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, > > for two reasons. One, it catches multiplication overflowing size_t. > > Two, it returns T * rather than void *, which lets the compiler catch > > more type errors. > > > > 'One' doesn't apply when n==1. 'Two' does. > > > > We're okay with things like T *v = g_malloc(sizeof(*v)). Yes, 'two' > > applies here as well, but screwups are relatively unlikely. > > > >>>>> 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. > > > > Quite possible to specify the wrong size in other ways, and the type > > checker can't save you then (that's 'two'), although Coverity might. > > Good point about the type checker! > > > > >>> Easy to rename types. > > > > Renaming a type is exactly as easy as renaming a variable or any other > > identifer: you have to update all occurences. > > > > And that means touching more lines. > > >> 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. > > > > We tend to update HACKING when we find ourselves debating the same > > things over and over. Perhaps this is such a case. > > > > I don't want to get too involved in this discussion. (I have other > problems to solve :) ) > > If we make this a rule, I want somebody to convert all applicable cases > to the desired format. (we won't be able to convert all cases, e.g. > structs with variable sized member arrays.) I'll try to create a semantic patch to cover most of the cases. For now, I only sent an update to the HACKING file: http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg03362.html
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)); + + /* initialize device memory address space */ + if (machine->ram_size < machine->maxram_size) { + ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size; + + if (QEMU_ALIGN_UP(machine->maxram_size, TARGET_PAGE_SIZE) != + machine->maxram_size) { + error_report("maximum memory size must be aligned to multiple of " + "%d bytes", TARGET_PAGE_SIZE); + exit(EXIT_FAILURE); + } + + machine->device_memory->base = machine->ram_size; + memory_region_init(&machine->device_memory->mr, OBJECT(ms), + "device-memory", device_mem_size); + memory_region_add_subregion(sysmem, machine->device_memory->base, + &machine->device_memory->mr); + + } + /* Initialize storage key device */ s390_skeys_init(); /* Initialize storage attributes device */ @@ -304,7 +328,7 @@ static void ccw_init(MachineState *machine) DeviceState *dev; s390_sclp_init(); - s390_memory_init(machine->ram_size); + s390_memory_init(machine); /* init CPUs (incl. CPU model) early so s390_has_feature() works */ s390_init_cpus(machine);
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 <david@redhat.com> --- hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)