| Message ID | 20170915090643.6043-1-otubo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | dma/i82374: avoid double creation of i82374 device | expand |
(oups, forgot the v2 on Subject) On Fri, Sep 15, 2017 at 11:06:43AM +0200, Eduardo Otubo wrote: > QEMU fails when used with the following command line: > > ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 > qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. > Aborted (core dumped) > > The 40p machine type already creates the device i82374. If specified in the > command line, it will try to create it again, hence generating the error. The > function isa_bus_dma() isn't supposed to be called twice for the same bus. One > way to avoid this problem is to set user_creatable=false. > > A possible fix in a near future would be making > isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of asserting > as well. > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > --- > hw/dma/i82374.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c > index 6c0f975df0..e76dea8dc7 100644 > --- a/hw/dma/i82374.c > +++ b/hw/dma/i82374.c > @@ -139,6 +139,11 @@ static void i82374_class_init(ObjectClass *klass, void *data) > dc->realize = i82374_realize; > dc->vmsd = &vmstate_i82374; > dc->props = i82374_properties; > + dc->user_creatable = false; > + /* > + * Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma() > + * if the device is instantiated twice. > + */ > } > > static const TypeInfo i82374_info = { > -- > 2.13.5 > >
On 15/09/2017 11:06, Eduardo Otubo wrote: > QEMU fails when used with the following command line: > > ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 > qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. > Aborted (core dumped) > > The 40p machine type already creates the device i82374. If specified in the > command line, it will try to create it again, hence generating the error. The > function isa_bus_dma() isn't supposed to be called twice for the same bus. One > way to avoid this problem is to set user_creatable=false. > > A possible fix in a near future would be making > isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of asserting > as well. > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > --- > hw/dma/i82374.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c > index 6c0f975df0..e76dea8dc7 100644 > --- a/hw/dma/i82374.c > +++ b/hw/dma/i82374.c > @@ -139,6 +139,11 @@ static void i82374_class_init(ObjectClass *klass, void *data) > dc->realize = i82374_realize; > dc->vmsd = &vmstate_i82374; > dc->props = i82374_properties; > + dc->user_creatable = false; > + /* > + * Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma() > + * if the device is instantiated twice. > + */ > } > > static const TypeInfo i82374_info = { > This breaks "make check", doesn't it? v2 should be the one that returns an error instead of asserting. Paolo
On Fri, Sep 15, 2017 at 12:18:11PM +0200, Paolo Bonzini wrote: > On 15/09/2017 11:06, Eduardo Otubo wrote: > > QEMU fails when used with the following command line: > > > > ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 > > qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. > > Aborted (core dumped) > > > > The 40p machine type already creates the device i82374. If specified in the > > command line, it will try to create it again, hence generating the error. The > > function isa_bus_dma() isn't supposed to be called twice for the same bus. One > > way to avoid this problem is to set user_creatable=false. > > > > A possible fix in a near future would be making > > isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of asserting > > as well. > > > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > > --- > > hw/dma/i82374.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c > > index 6c0f975df0..e76dea8dc7 100644 > > --- a/hw/dma/i82374.c > > +++ b/hw/dma/i82374.c > > @@ -139,6 +139,11 @@ static void i82374_class_init(ObjectClass *klass, void *data) > > dc->realize = i82374_realize; > > dc->vmsd = &vmstate_i82374; > > dc->props = i82374_properties; > > + dc->user_creatable = false; > > + /* > > + * Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma() > > + * if the device is instantiated twice. > > + */ > > } > > > > static const TypeInfo i82374_info = { > > > > This breaks "make check", doesn't it? > > v2 should be the one that returns an error instead of asserting. I guess I have misunderstood, then. I'll work on a patch to propagate the error then. Thanks,
On Fri, Sep 15, 2017 at 12:18:11PM +0200, Paolo Bonzini wrote: > On 15/09/2017 11:06, Eduardo Otubo wrote: > > QEMU fails when used with the following command line: > > > > ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 > > qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. > > Aborted (core dumped) > > > > The 40p machine type already creates the device i82374. If specified in the > > command line, it will try to create it again, hence generating the error. The > > function isa_bus_dma() isn't supposed to be called twice for the same bus. One > > way to avoid this problem is to set user_creatable=false. > > > > A possible fix in a near future would be making > > isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of asserting > > as well. > > > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > > --- > > hw/dma/i82374.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c > > index 6c0f975df0..e76dea8dc7 100644 > > --- a/hw/dma/i82374.c > > +++ b/hw/dma/i82374.c > > @@ -139,6 +139,11 @@ static void i82374_class_init(ObjectClass *klass, void *data) > > dc->realize = i82374_realize; > > dc->vmsd = &vmstate_i82374; > > dc->props = i82374_properties; > > + dc->user_creatable = false; > > + /* > > + * Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma() > > + * if the device is instantiated twice. > > + */ > > } > > > > static const TypeInfo i82374_info = { > > > > This breaks "make check", doesn't it? Why would it? I don't see any test code using -device i82374. (endianness-test uses -device i82378). > > v2 should be the one that returns an error instead of asserting. I agree that returning an error is better.
----- Original Message ----- > From: "Eduardo Habkost" <ehabkost@redhat.com> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: "Eduardo Otubo" <otubo@redhat.com>, qemu-devel@nongnu.org, qemu-trivial@nongnu.org, "Michael Tokarev" > <mjt@tls.msk.ru>, "Markus Armbruster" <armbru@redhat.com>, "Alexander Graf" <agraf@suse.de> > Sent: Saturday, September 16, 2017 12:21:13 AM > Subject: Re: [PATCH] dma/i82374: avoid double creation of i82374 device > > On Fri, Sep 15, 2017 at 12:18:11PM +0200, Paolo Bonzini wrote: > > On 15/09/2017 11:06, Eduardo Otubo wrote: > > > QEMU fails when used with the following command line: > > > > > > ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device > > > i82374 > > > qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion > > > `!bus->dma[0] && !bus->dma[1]' failed. > > > Aborted (core dumped) > > > > > > The 40p machine type already creates the device i82374. If specified in > > > the > > > command line, it will try to create it again, hence generating the error. > > > The > > > function isa_bus_dma() isn't supposed to be called twice for the same > > > bus. One > > > way to avoid this problem is to set user_creatable=false. > > > > > > A possible fix in a near future would be making > > > isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of > > > asserting > > > as well. > > > > > > Signed-off-by: Eduardo Otubo <otubo@redhat.com> > > > --- > > > hw/dma/i82374.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c > > > index 6c0f975df0..e76dea8dc7 100644 > > > --- a/hw/dma/i82374.c > > > +++ b/hw/dma/i82374.c > > > @@ -139,6 +139,11 @@ static void i82374_class_init(ObjectClass *klass, > > > void *data) > > > dc->realize = i82374_realize; > > > dc->vmsd = &vmstate_i82374; > > > dc->props = i82374_properties; > > > + dc->user_creatable = false; > > > + /* > > > + * Reason: i82374_realize() crashes (assertion failure inside > > > isa_bus_dma() > > > + * if the device is instantiated twice. > > > + */ > > > } > > > > > > static const TypeInfo i82374_info = { > > > > > > > This breaks "make check", doesn't it? > > Why would it? I don't see any test code using -device i82374. > (endianness-test uses -device i82378). You're right, both Aurelien and I were confused. If you want to accept this patch it would be fine then, even if giving an error may be preferrable. Paolo
15.09.2017 12:06, Eduardo Otubo wrote: > QEMU fails when used with the following command line: > > ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 > qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. > Aborted (core dumped) > > The 40p machine type already creates the device i82374. If specified in the > command line, it will try to create it again, hence generating the error. The > function isa_bus_dma() isn't supposed to be called twice for the same bus. One > way to avoid this problem is to set user_creatable=false. Applied to -trivial, thanks! /mjt
On 24/09/2017 23:02, Michael Tokarev wrote: > 15.09.2017 12:06, Eduardo Otubo wrote: >> QEMU fails when used with the following command line: >> >> ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 >> qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. >> Aborted (core dumped) >> >> The 40p machine type already creates the device i82374. If specified in the >> command line, it will try to create it again, hence generating the error. The >> function isa_bus_dma() isn't supposed to be called twice for the same bus. One >> way to avoid this problem is to set user_creatable=false. > > Applied to -trivial, thanks! Eduardo, weren't you going to send a version that propagates Error* correctly instead? Paolo
On Mon, Sep 25, 2017 at 11:11:37AM +0200, Paolo Bonzini wrote: > On 24/09/2017 23:02, Michael Tokarev wrote: > > 15.09.2017 12:06, Eduardo Otubo wrote: > >> QEMU fails when used with the following command line: > >> > >> ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 > >> qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. > >> Aborted (core dumped) > >> > >> The 40p machine type already creates the device i82374. If specified in the > >> command line, it will try to create it again, hence generating the error. The > >> function isa_bus_dma() isn't supposed to be called twice for the same bus. One > >> way to avoid this problem is to set user_creatable=false. > > > > Applied to -trivial, thanks! > > Eduardo, weren't you going to send a version that propagates Error* > correctly instead? Yes, that's correct. I can revert this patch with the error propagation patch as well, if you guys don't mind.
On 25/09/2017 11:26, Eduardo Otubo wrote: > On Mon, Sep 25, 2017 at 11:11:37AM +0200, Paolo Bonzini wrote: >> On 24/09/2017 23:02, Michael Tokarev wrote: >>> 15.09.2017 12:06, Eduardo Otubo wrote: >>>> QEMU fails when used with the following command line: >>>> >>>> ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 >>>> qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. >>>> Aborted (core dumped) >>>> >>>> The 40p machine type already creates the device i82374. If specified in the >>>> command line, it will try to create it again, hence generating the error. The >>>> function isa_bus_dma() isn't supposed to be called twice for the same bus. One >>>> way to avoid this problem is to set user_creatable=false. >>> >>> Applied to -trivial, thanks! >> >> Eduardo, weren't you going to send a version that propagates Error* >> correctly instead? > > Yes, that's correct. I can revert this patch with the error > propagation patch as well, if you guys don't mind. Sure, that's fine too. Paolo
25.09.2017 12:26, Eduardo Otubo wrote: > On Mon, Sep 25, 2017 at 11:11:37AM +0200, Paolo Bonzini wrote: >> On 24/09/2017 23:02, Michael Tokarev wrote: >>> 15.09.2017 12:06, Eduardo Otubo wrote: >>>> QEMU fails when used with the following command line: >>>> >>>> ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 >>>> qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. >>>> Aborted (core dumped) >>>> >>>> The 40p machine type already creates the device i82374. If specified in the >>>> command line, it will try to create it again, hence generating the error. The >>>> function isa_bus_dma() isn't supposed to be called twice for the same bus. One >>>> way to avoid this problem is to set user_creatable=false. >>> >>> Applied to -trivial, thanks! >> >> Eduardo, weren't you going to send a version that propagates Error* >> correctly instead? > > Yes, that's correct. I can revert this patch with the error > propagation patch as well, if you guys don't mind. Hmm. After reading the original discussion I concluded this patch is okay. I can remove it right now before the series has been applied, together with another tiny change. Thanks, /mjt
On 25/09/2017 12:54, Michael Tokarev wrote: >> Yes, that's correct. I can revert this patch with the error >> propagation patch as well, if you guys don't mind. > Hmm. After reading the original discussion I concluded this patch > is okay. I can remove it right now before the series has been > applied, together with another tiny change. No problem, it's okay for now as a trivial change. Paolo
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c index 6c0f975df0..e76dea8dc7 100644 --- a/hw/dma/i82374.c +++ b/hw/dma/i82374.c @@ -139,6 +139,11 @@ static void i82374_class_init(ObjectClass *klass, void *data) dc->realize = i82374_realize; dc->vmsd = &vmstate_i82374; dc->props = i82374_properties; + dc->user_creatable = false; + /* + * Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma() + * if the device is instantiated twice. + */ } static const TypeInfo i82374_info = {
QEMU fails when used with the following command line: ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374 qemu-system-ppc64: hw/isa/isa-bus.c:110: isa_bus_dma: Assertion `!bus->dma[0] && !bus->dma[1]' failed. Aborted (core dumped) The 40p machine type already creates the device i82374. If specified in the command line, it will try to create it again, hence generating the error. The function isa_bus_dma() isn't supposed to be called twice for the same bus. One way to avoid this problem is to set user_creatable=false. A possible fix in a near future would be making isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of asserting as well. Signed-off-by: Eduardo Otubo <otubo@redhat.com> --- hw/dma/i82374.c | 5 +++++ 1 file changed, 5 insertions(+)