| Message ID | 1558079119-320634-2-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | numa: deprecate '-numa node, mem' and default memory distribution | expand |
Igor Mammedov <imammedo@redhat.com> writes: > QEMU will crash when device-memory-region-size property is read if ms->device_memory > wasn't initialized yet (ex: property being inspected during preconfig time). Reproduced: $ qemu-system-x86_64 -nodefaults -S -display none -preconfig -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 4}, "package": "v4.0.0-828-ga7b21f6762"}, "capabilities": ["oob"]}} {"execute": "qmp_capabilities"} {"return": {}} {"execute": "qom-get", "arguments": {"path": "/machine", "property": "device-memory-region-size"}} Segmentation fault (core dumped) First time I started looking at this series, I went "I'll need a reproducer to fully understand what's up, and I don't feel like finding one now; next series, please". Second time, I had to spend a few minutes on the reproducer. Wasn't hard, since you provided a clue. Still: make review easy, include a reproducer whenever you can. > Instead of crashing return 0 if ms->device_memory hasn't been initialized. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/pc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index d98b737..de91e90 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -2461,7 +2461,11 @@ pc_machine_get_device_memory_region_size(Object *obj, Visitor *v, > Error **errp) > { > MachineState *ms = MACHINE(obj); > - int64_t value = memory_region_size(&ms->device_memory->mr); > + int64_t value = 0; > + > + if (ms->device_memory) { > + memory_region_size(&ms->device_memory->mr); > + } > > visit_type_int(v, name, &value, errp); > } This makes qom-get return 0 for the size of memory that doesn't exist, yet. A possible alternative would be setting an error. Opinions?
On Mon, 27 May 2019 18:36:25 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Igor Mammedov <imammedo@redhat.com> writes: > > > QEMU will crash when device-memory-region-size property is read if ms->device_memory > > wasn't initialized yet (ex: property being inspected during preconfig time). > > Reproduced: > > $ qemu-system-x86_64 -nodefaults -S -display none -preconfig -qmp stdio > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 4}, "package": "v4.0.0-828-ga7b21f6762"}, "capabilities": ["oob"]}} > {"execute": "qmp_capabilities"} > {"return": {}} > {"execute": "qom-get", "arguments": {"path": "/machine", "property": "device-memory-region-size"}} > Segmentation fault (core dumped) > > First time I started looking at this series, I went "I'll need a > reproducer to fully understand what's up, and I don't feel like finding > one now; next series, please". Second time, I had to spend a few > minutes on the reproducer. Wasn't hard, since you provided a clue. > Still: make review easy, include a reproducer whenever you can. sure > > > Instead of crashing return 0 if ms->device_memory hasn't been initialized. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/pc.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index d98b737..de91e90 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -2461,7 +2461,11 @@ pc_machine_get_device_memory_region_size(Object *obj, Visitor *v, > > Error **errp) > > { > > MachineState *ms = MACHINE(obj); > > - int64_t value = memory_region_size(&ms->device_memory->mr); > > + int64_t value = 0; > > + > > + if (ms->device_memory) { > > + memory_region_size(&ms->device_memory->mr); > > + } > > > > visit_type_int(v, name, &value, errp); > > } > > This makes qom-get return 0 for the size of memory that doesn't exist, > yet. > > A possible alternative would be setting an error. > > Opinions? We don't have a notion of property not set in QOM, so a code that will receive a text based error will have to parse it (horrible idea) to avoid generation of related ACPI parts. In case of not enabled memory hotplug, PC_MACHINE_DEVMEM_REGION_SIZE == 0 is valid value and it's what's expected by other code.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d98b737..de91e90 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -2461,7 +2461,11 @@ pc_machine_get_device_memory_region_size(Object *obj, Visitor *v, Error **errp) { MachineState *ms = MACHINE(obj); - int64_t value = memory_region_size(&ms->device_memory->mr); + int64_t value = 0; + + if (ms->device_memory) { + memory_region_size(&ms->device_memory->mr); + } visit_type_int(v, name, &value, errp); }
QEMU will crash when device-memory-region-size property is read if ms->device_memory wasn't initialized yet (ex: property being inspected during preconfig time). Instead of crashing return 0 if ms->device_memory hasn't been initialized. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/pc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)