| Message ID | a808c0865b720e22ca2929ec3d362d4610fbad51.1502708830.git.mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
On 14 August 2017 at 12:07, Michael Tokarev <mjt@tls.msk.ru> wrote: > From: Thomas Huth <thuth@redhat.com> > > QEMU currently abort()s if the user tries to specify the mmio_interface > device without parameters: > > x86_64-softmmu/qemu-system-x86_64 -nographic -device mmio_interface > qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv: > Assertion `*errp == ((void *)0)' failed. > Aborted (core dumped) > > This happens because the realize function is trying to set the errp > twice in this case. After setting an error, the realize function > should immediately return instead. It seems like it should be an error to permit this to be created from the command line at all -- the device is intended only as an internal implementation detail of the memory system, and it has a PROP_PTR property which can't be sensibly set from the command line. This patch is a correct fix for an immediate problem, but we should disable using this via -device somehow. thanks -- PMM
On 14.08.2017 13:45, Peter Maydell wrote: > On 14 August 2017 at 12:07, Michael Tokarev <mjt@tls.msk.ru> wrote: >> From: Thomas Huth <thuth@redhat.com> >> >> QEMU currently abort()s if the user tries to specify the mmio_interface >> device without parameters: >> >> x86_64-softmmu/qemu-system-x86_64 -nographic -device mmio_interface >> qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv: >> Assertion `*errp == ((void *)0)' failed. >> Aborted (core dumped) >> >> This happens because the realize function is trying to set the errp >> twice in this case. After setting an error, the realize function >> should immediately return instead. > > It seems like it should be an error to permit this to be > created from the command line at all That's also what thought first ... but the commit message of commit 7cc2298c46a6afa4f4ff7e5cd262809c782d701b says that it "can be hotplugged/hotunplugged" ? ... that's confusing ... Frederic, I think some more comments in the header of the file would be really useful here. Thomas
On 14 August 2017 at 12:52, Thomas Huth <thuth@redhat.com> wrote: > On 14.08.2017 13:45, Peter Maydell wrote: >> It seems like it should be an error to permit this to be >> created from the command line at all > That's also what thought first ... but the commit message of commit > 7cc2298c46a6afa4f4ff7e5cd262809c782d701b says that it "can be > hotplugged/hotunplugged" ? ... that's confusing ... It means that memory.c creates and deletes instances of the device on demand, not that it's hotplugged in the "controlled by the user simulating PCI or other hotplug" sense. thanks -- PMM
On Mon, 14 Aug 2017 12:45:16 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 14 August 2017 at 12:07, Michael Tokarev <mjt@tls.msk.ru> wrote: > > From: Thomas Huth <thuth@redhat.com> > > > > QEMU currently abort()s if the user tries to specify the mmio_interface > > device without parameters: > > > > x86_64-softmmu/qemu-system-x86_64 -nographic -device mmio_interface > > qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv: > > Assertion `*errp == ((void *)0)' failed. > > Aborted (core dumped) > > > > This happens because the realize function is trying to set the errp > > twice in this case. After setting an error, the realize function > > should immediately return instead. > > It seems like it should be an error to permit this to be > created from the command line at all -- the device is intended > only as an internal implementation detail of the memory system, > and it has a PROP_PTR property which can't be sensibly set > from the command line. > > This patch is a correct fix for an immediate problem, but we should disable > using this via -device somehow. Setting DeviceClass::user_creatable to false should prevent creating device with device_add interface. See: qdev_get_device_class() > > thanks > -- PMM >
On 08/14/2017 01:55 PM, Peter Maydell wrote: > On 14 August 2017 at 12:52, Thomas Huth <thuth@redhat.com> wrote: >> On 14.08.2017 13:45, Peter Maydell wrote: >>> It seems like it should be an error to permit this to be >>> created from the command line at all > >> That's also what thought first ... but the commit message of commit >> 7cc2298c46a6afa4f4ff7e5cd262809c782d701b says that it "can be >> hotplugged/hotunplugged" ? ... that's confusing ... > > It means that memory.c creates and deletes instances of the > device on demand, not that it's hotplugged in the "controlled by > the user simulating PCI or other hotplug" sense. > > thanks > -- PMM > Sorry, missed that I changed my email address recently. I'll add some docs and fix the things we mentioned for 2.11. For the context: https://www.mail-archive.com/qemu-devel@nongnu.org/msg446748.html Thanks, Fred
diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c index 6f004d2bab..da154e5c95 100644 --- a/hw/misc/mmio_interface.c +++ b/hw/misc/mmio_interface.c @@ -63,10 +63,12 @@ static void mmio_interface_realize(DeviceState *dev, Error **errp) if (!s->host_ptr) { error_setg(errp, "host_ptr property must be set"); + return; } if (!s->subregion) { error_setg(errp, "subregion property must be set"); + return; } memory_region_init_ram_ptr(&s->ram_mem, OBJECT(s), "ram",