[Qemu-devel] [PATCHv3] dma/i82374: avoid double creation of i82374 device

* [Qemu-devel] [PATCHv3] dma/i82374: avoid double creation of i82374 device
@ 2017-11-24 13:46 Eduardo Otubo
  2017-11-24 13:56 ` no-reply
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Eduardo Otubo @ 2017-11-24 13:46 UTC (permalink / raw)
  To: qemu-devel; +Cc: mjt, agraf, ehabkost, armbru, pbonzini

v3:
 * Removed all unecessary local_err
 * Change return of isa_bus_dma() and DMA_init() from void to int8_t,
   returning -EBUSY on error and 0 on success
 * Added qdev_cleanup_nofail() in case isa_bus_dma() returns error. The
   cleanup looks safe, but please review if I didn't miss any detail

v2:
 * Removed user_creatable=false and replaced by error handling using
   Error **errp and error_propagate();

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 created 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/core/qdev.c         | 16 ++++++++++++++++
 hw/dma/i82374.c        | 13 +++++++------
 hw/dma/i8257.c         | 38 ++++++++++++++++++++++----------------
 hw/isa/isa-bus.c       |  8 ++++++--
 include/hw/isa/isa.h   |  4 ++--
 include/hw/qdev-core.h |  1 +
 6 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 606ab53c42..0ef03ee461 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -344,6 +344,22 @@ void qdev_init_nofail(DeviceState *dev)
     object_unref(OBJECT(dev));
 }
 
+void qdev_cleanup_nofail(DeviceState *dev)
+{
+    Error *err = NULL;
+
+    assert(dev->realized);
+
+    object_ref(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), false, "realized", &err);
+    if (err) {
+        error_reportf_err(err, "Clean up of device %s failed: ",
+                          object_get_typename(OBJECT(dev)));
+        exit(1);
+    }
+    object_unref(OBJECT(dev));
+}
+
 void qdev_machine_creation_done(void)
 {
     /*
diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index 6c0f975df0..9b792890b2 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/isa/isa.h"
+#include "qapi/error.h"
 
 #define TYPE_I82374 "i82374"
 #define I82374(obj) OBJECT_CHECK(I82374State, (obj), TYPE_I82374)
@@ -118,13 +119,13 @@ static void i82374_realize(DeviceState *dev, Error **errp)
 {
     I82374State *s = I82374(dev);
 
-    portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s,
-                     "i82374");
-    portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj),
+    if (DMA_init(isa_bus_from_device(ISA_DEVICE(dev)), 1, errp) == 0) {
+        portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s,
+                    "i82374");
+        portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj),
                     s->iobase);
-
-    DMA_init(isa_bus_from_device(ISA_DEVICE(dev)), 1);
-    memset(s->commands, 0, sizeof(s->commands));
+        memset(s->commands, 0, sizeof(s->commands));
+    }
 }
 
 static Property i82374_properties[] = {
diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
index bd23e893bf..7488e3dd12 100644
--- a/hw/dma/i8257.c
+++ b/hw/dma/i8257.c
@@ -622,26 +622,32 @@ static void i8257_register_types(void)
 
 type_init(i8257_register_types)
 
-void DMA_init(ISABus *bus, int high_page_enable)
+int8_t DMA_init(ISABus *bus, int high_page_enable, Error **errp)
 {
     ISADevice *isa1, *isa2;
-    DeviceState *d;
+    DeviceState *d1, *d2;
 
     isa1 = isa_create(bus, TYPE_I8257);
-    d = DEVICE(isa1);
-    qdev_prop_set_int32(d, "base", 0x00);
-    qdev_prop_set_int32(d, "page-base", 0x80);
-    qdev_prop_set_int32(d, "pageh-base", high_page_enable ? 0x480 : -1);
-    qdev_prop_set_int32(d, "dshift", 0);
-    qdev_init_nofail(d);
+    d1 = DEVICE(isa1);
+    qdev_prop_set_int32(d1, "base", 0x00);
+    qdev_prop_set_int32(d1, "page-base", 0x80);
+    qdev_prop_set_int32(d1, "pageh-base", high_page_enable ? 0x480 : -1);
+    qdev_prop_set_int32(d1, "dshift", 0);
+    qdev_init_nofail(d1);
 
     isa2 = isa_create(bus, TYPE_I8257);
-    d = DEVICE(isa2);
-    qdev_prop_set_int32(d, "base", 0xc0);
-    qdev_prop_set_int32(d, "page-base", 0x88);
-    qdev_prop_set_int32(d, "pageh-base", high_page_enable ? 0x488 : -1);
-    qdev_prop_set_int32(d, "dshift", 1);
-    qdev_init_nofail(d);
-
-    isa_bus_dma(bus, ISADMA(isa1), ISADMA(isa2));
+    d2 = DEVICE(isa2);
+    qdev_prop_set_int32(d2, "base", 0xc0);
+    qdev_prop_set_int32(d2, "page-base", 0x88);
+    qdev_prop_set_int32(d2, "pageh-base", high_page_enable ? 0x488 : -1);
+    qdev_prop_set_int32(d2, "dshift", 1);
+    qdev_init_nofail(d2);
+
+    if (isa_bus_dma(bus, ISADMA(isa1), ISADMA(isa2), errp) < 0) {
+        qdev_cleanup_nofail(d1);
+        qdev_cleanup_nofail(d2);
+        return -EBUSY;
+    }
+
+    return 0;
 }
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 348e0eab9d..4781dddaf9 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -104,12 +104,16 @@ void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, int isairq)
     qdev_connect_gpio_out(DEVICE(isadev), gpioirq, irq);
 }
 
-void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16)
+int8_t isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16, Error **errp)
 {
     assert(bus && dma8 && dma16);
-    assert(!bus->dma[0] && !bus->dma[1]);
+    if (bus->dma[0] || bus->dma[1]) {
+        error_setg(errp, "DMA already initialized on ISA bus");
+        return -EBUSY;
+    }
     bus->dma[0] = dma8;
     bus->dma[1] = dma16;
+    return 0;
 }
 
 IsaDma *isa_get_dma(ISABus *bus, int nchan)
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 95593408ef..2c7f260deb 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -103,7 +103,7 @@ void isa_bus_irqs(ISABus *bus, qemu_irq *irqs);
 qemu_irq isa_get_irq(ISADevice *dev, int isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
 void isa_connect_gpio_out(ISADevice *isadev, int gpioirq, int isairq);
-void isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16);
+int8_t isa_bus_dma(ISABus *bus, IsaDma *dma8, IsaDma *dma16, Error **errp);
 IsaDma *isa_get_dma(ISABus *bus, int nchan);
 MemoryRegion *isa_address_space(ISADevice *dev);
 MemoryRegion *isa_address_space_io(ISADevice *dev);
@@ -152,5 +152,5 @@ static inline ISABus *isa_bus_from_device(ISADevice *d)
 }
 
 /* i8257.c */
-void DMA_init(ISABus *bus, int high_page_enable);
+int8_t DMA_init(ISABus *bus, int high_page_enable, Error **errp);
 #endif
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 089146197f..77d3b43427 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -283,6 +283,7 @@ typedef struct GlobalProperty {
 DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_try_create(BusState *bus, const char *name);
 void qdev_init_nofail(DeviceState *dev);
+void qdev_cleanup_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 8+ messages in thread