dma/i82374: avoid double creation of i82374 device

PATCH In review Eduardo Otubo 5 participants 10 messages Sep 1, 2017 – Sep 9, 2017

lore

Share & tools

lore

Eduardo Otubo

author

2017-09-01 11:03 UTC #0 lore raw

PATCH 1/1 dma/i82374: avoid double creation of i82374 device

When used with the following command line:

./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374

QEMU with machine type 40p already creates the device i82374. If specified in the command line, it will try to create it again, hence generating the error. One way to avoid this problem is to set user_creatable=false.

Signed-off-by Eduardo Otubo <[email protected]>

---
 hw/dma/i82374.c | 1 +
 1 file changed, 1 insertion(+)

hw/dma/i82374.c +1 -0

index 6c0f975df0..5275d822e0 100644
@@ -139,6 +139,7 @@ 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;
 }
 
 static const TypeInfo i82374_info = {

Signature · 1 line

2.13.5

Eduardo Habkost

2017-09-01 14:31 UTC #1 lore raw

i82374 is compiled in only on ppc and sh4, so I'm CCing the maintainers for those architectures.

Quoted 7 lines · Eduardo Otubo

When used with the following command line:
./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374
QEMU with machine type 40p already creates the device i82374. If specified in the command line, it will try to create it again, hence generating the error.

Which error?

Quoted 4 lines

One way to avoid this problem is to set
user_creatable=false.
Signed-off-by Eduardo Otubo <[email protected]>

The patch does more than just avoiding double creation: it prevents usage of "-device i82374" completely.

Maybe nobody needs it to work with -device today (would the device even work?) and it is OK to set user_creatable=false until we fix the crash. But we need to be sure of that.

hw/dma/i82374.c static void i82374_class_init(ObjectClass *klass, void *data) · line 142

Quoted 13 lines

---
 hw/dma/i82374.c | 1 +
 1 file changed, 1 insertion(+)

hw/dma/i82374.c +1 -0

index 6c0f975df0..5275d822e0 100644
@@ -139,6 +139,7 @@ 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;

A "Reason:" comment explaining why user_creatable=false is mandatory. See the comment above user_creatable declaration in qdev-core.h for reference.

I suggest the following:

/*
* Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma()
* if the device is instantiated twice.
*/

Signature · 1 line

Eduardo

Markus Armbruster

2017-09-01 15:34 UTC #2 lore raw

Quoted 45 lines · Eduardo Habkost

i82374 is compiled in only on ppc and sh4, so I'm CCing the maintainers for those architectures.
Quoted 7 lines · Eduardo Otubo
When used with the following command line:
./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374
QEMU with machine type 40p already creates the device i82374. If specified in the command line, it will try to create it again, hence generating the error.
Which error?
Quoted 4 lines
One way to avoid this problem is to set
user_creatable=false.
Signed-off-by Eduardo Otubo <[email protected]>
The patch does more than just avoiding double creation: it prevents usage of "-device i82374" completely.
Maybe nobody needs it to work with -device today (would the device even work?) and it is OK to set user_creatable=false until we fix the crash. But we need to be sure of that.
hw/dma/i82374.c static void i82374_class_init(ObjectClass *klass, void *data) · line 142
Quoted 13 lines
---
 hw/dma/i82374.c | 1 +
 1 file changed, 1 insertion(+)
hw/dma/i82374.c +1 -0
index 6c0f975df0..5275d822e0 100644
@@ -139,6 +139,7 @@ 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;
A "Reason:" comment explaining why user_creatable=false is mandatory. See the comment above user_creatable declaration in qdev-core.h for reference.

Quoted 6 lines

I suggest the following:
/*
* Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma()
* if the device is instantiated twice.
*/

We need to find out *why* it crashes. Once we know, we can likely write a better comment.

Eduardo Habkost

2017-09-01 15:44 UTC #3 lore raw

Quoted 49 lines · Markus Armbruster

Quoted 47 lines · Eduardo Habkost
i82374 is compiled in only on ppc and sh4, so I'm CCing the maintainers for those architectures.
Quoted 7 lines · Eduardo Otubo
When used with the following command line:
./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374
QEMU with machine type 40p already creates the device i82374. If specified in the command line, it will try to create it again, hence generating the error.
Which error?
Quoted 4 lines
One way to avoid this problem is to set
user_creatable=false.
Signed-off-by Eduardo Otubo <[email protected]>
The patch does more than just avoiding double creation: it prevents usage of "-device i82374" completely.
Maybe nobody needs it to work with -device today (would the device even work?) and it is OK to set user_creatable=false until we fix the crash. But we need to be sure of that.
hw/dma/i82374.c static void i82374_class_init(ObjectClass *klass, void *data) · line 142
Quoted 13 lines
---
 hw/dma/i82374.c | 1 +
 1 file changed, 1 insertion(+)
hw/dma/i82374.c +1 -0
index 6c0f975df0..5275d822e0 100644
@@ -139,6 +139,7 @@ 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;
A "Reason:" comment explaining why user_creatable=false is mandatory. See the comment above user_creatable declaration in qdev-core.h for reference.
I suggest the following:

Quoted 7 lines

Quoted 4 lines
/*
* Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma()
* if the device is instantiated twice.
*/
We need to find out *why* it crashes. Once we know, we can likely write a better comment.

It crashes because isa_bus_dma() isn't supposed to be called twice for the same bus.

Making isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of asserting would be even better than setting user_creatable=false.

Signature · 1 line

Eduardo

Eduardo Otubo

author

2017-09-07 08:38 UTC #4 lore raw

Quoted 6 lines · Eduardo Habkost

Quoted 5 lines · Markus Armbruster
Quoted 3 lines · Eduardo Habkost
i82374 is compiled in only on ppc and sh4, so I'm CCing the maintainers for those architectures.

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines · Eduardo Otubo
Quoted 7 lines
When used with the following command line:
./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374
QEMU with machine type 40p already creates the device i82374. If specified in the command line, it will try to create it again, hence generating the error.

Well pointed, forgot to describe the actual error. I may inlcude for the next version of the patch. For for correctness sake, here it is:

./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)

Quoted 34 lines

Quoted 34 lines
Quoted 34 lines
Which error?
Quoted 4 lines
One way to avoid this problem is to set
user_creatable=false.
Signed-off-by Eduardo Otubo <[email protected]>
The patch does more than just avoiding double creation: it prevents usage of "-device i82374" completely.
Maybe nobody needs it to work with -device today (would the device even work?) and it is OK to set user_creatable=false until we fix the crash. But we need to be sure of that.
hw/dma/i82374.c static void i82374_class_init(ObjectClass *klass, void *data) · line 142
Quoted 13 lines
---
 hw/dma/i82374.c | 1 +
 1 file changed, 1 insertion(+)
hw/dma/i82374.c +1 -0
index 6c0f975df0..5275d822e0 100644
@@ -139,6 +139,7 @@ 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;
A "Reason:" comment explaining why user_creatable=false is mandatory. See the comment above user_creatable declaration in qdev-core.h for reference.

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
I suggest the following:
/*
* Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma()
* if the device is instantiated twice.
*/

I agree with the comment above. If there's nothing left to fix/add I'll just send a v2 for this shortly.

Quoted 14 lines

Quoted 3 lines
We need to find out *why* it crashes. Once we know, we can likely write a better comment.
It crashes because isa_bus_dma() isn't supposed to be called twice for the same bus.
Making isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of asserting would be even better than setting user_creatable=false.
Signature · 1 line
Eduardo

Signature · 2 lines

Eduardo Otubo
Senior Software Engineer @ RedHat

Eduardo Habkost

2017-09-09 21:52 UTC #5 lore raw

Quoted 4 lines · Eduardo Otubo

Quoted 3 lines · Eduardo Habkost
Quoted 2 lines · Markus Armbruster
Quoted 1 line · Eduardo Habkost
On Fri, Sep 01, 2017 at 01:03:32PM +0200, Eduardo Otubo wrote:

[...]

Quoted 16 lines

Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
hw/dma/i82374.c static void i82374_class_init(ObjectClass *klass, void *data) · line 142
Quoted 9 lines
hw/dma/i82374.c +1 -0
index 6c0f975df0..5275d822e0 100644
@@ -139,6 +139,7 @@ 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;
A "Reason:" comment explaining why user_creatable=false is mandatory. See the comment above user_creatable declaration in qdev-core.h for reference.
I suggest the following:

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
/*
* Reason: i82374_realize() crashes (assertion failure inside isa_bus_dma()
* if the device is instantiated twice.
*/
I agree with the comment above. If there's nothing left to fix/add I'll just send a v2 for this shortly.

I suggest amending it with the additional info below, explaining that isa_bus_dma() isn't supposed to be called twice, and that we need to make isa_bus_dma()/DMA_init()/i82374_realize() return an appropriate error if the device is instantiated twice.

Quoted 19 lines

Quoted 15 lines
Quoted 4 lines
We need to find out *why* it crashes. Once we know, we can likely write a better comment.
It crashes because isa_bus_dma() isn't supposed to be called twice for the same bus.
Making isa_bus_dma()/DMA_init()/i82374_realize() return an error instead of asserting would be even better than setting user_creatable=false.
Signature · 1 line
Eduardo
Signature · 2 lines
Eduardo Otubo
Senior Software Engineer @ RedHat

Signature · 1 line

Eduardo

Aurelien Jarno

2017-09-02 09:15 UTC #6 lore raw

In reply to Eduardo Habkost

Quoted 2 lines · Eduardo Habkost

i82374 is compiled in only on ppc and sh4, so I'm CCing the maintainers for those architectures.

The i82374 device is not useful nor usable on SH4. It has just been added in commit 85d3846a39 to be able to run the tests.

Aurelien

Signature · 2 lines

Aurelien Jarno                          GPG: 4096R/1DDD8C9B
[email protected]                 http://www.aurel32.net

Eduardo Otubo

author

2017-09-07 08:39 UTC #7 lore raw

Quoted 9 lines · Aurelien Jarno

Quoted 2 lines · Eduardo Habkost
i82374 is compiled in only on ppc and sh4, so I'm CCing the maintainers for those architectures.
The i82374 device is not useful nor usable on SH4. It has just been added in commit 85d3846a39 to be able to run the tests.
Aurelien

Any word from the ppc guys?

Signature · 2 lines

Eduardo Otubo
Senior Software Engineer @ RedHat

Paolo Bonzini

2017-09-07 16:05 UTC #8 lore raw

In reply to Aurelien Jarno

Il 02 set 2017 11:17 AM, "Aurelien Jarno" <[email protected]> ha scritto:

Quoted 2 lines · Eduardo Habkost

i82374 is compiled in only on ppc and sh4, so I'm CCing the maintainers for those architectures.

The i82374 device is not useful nor usable on SH4. It has just been added in commit 85d3846a39 to be able to run the tests.

But that means that the patch is wrong and probably was not tested with "make check".

Eduardo's proposal is the right one.

Paolo

Aurelien

--
Aurelien Jarno GPG: 4096R/1DDD8C9B
[email protected] http://www.aurel32.net

Eduardo Otubo

author

2017-09-08 07:57 UTC #9 lore raw

Quoted 6 lines · Paolo Bonzini

Il 02 set 2017 11:17 AM, "Aurelien Jarno" <[email protected]> ha scritto:
Quoted 2 lines · Eduardo Habkost
i82374 is compiled in only on ppc and sh4, so I'm CCing the maintainers for those architectures.

Quoted 9 lines

The i82374 device is not useful nor usable on SH4. It has just been added in commit 85d3846a39 to be able to run the tests.
But that means that the patch is wrong and probably was not tested with "make check".
Eduardo's proposal is the right one.

Great, I'll just add the missing details and resend the v2. Thanks for the review.

Regards,

Signature · 2 lines

Eduardo Otubo
Senior Software Engineer @ RedHat