[Qemu-devel] [PATCH 0/2] prep/i82378: simplify and enhance i82378 chipset implementation

[Qemu-devel] [PATCH 0/2] prep/i82378: simplify and enhance i82378 chipset implementation

[Hervé Poussineau]

Hi,

First patch simply move the choice of isa_mem_base from i82378 to PReP PCI chipset,
where it belongs.
Second patch rewrites most of the i82378 implementation, now that old_portio
memory access method has been removed. It also updates implementation to current
QEMU standards.

A lot of work will be needed for PReP Raven PCI chipset, including creating
correct regions for PCI memory region, PCI I/O region, custom PCI address space...
However, that's not the goal of this small patchset.

Hervé Poussineau (2):
  prep_pci: set isa_mem_base in the PCI host bridge
  i82378: cleanup implementation

 hw/isa/i82378.c    |  223 +++++++++++-----------------------------------------
 hw/pci-host/prep.c |    2 +
 2 files changed, 50 insertions(+), 175 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/2] prep_pci: set isa_mem_base in the PCI host bridge

[Hervé Poussineau]

Currently, it is done by i82378 device, which shouldn't care of it.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/isa/i82378.c    |    3 ---
 hw/pci-host/prep.c |    2 ++
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index b25ed04..de71d81 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -45,7 +45,6 @@ typedef struct I82378State {
 typedef struct PCIi82378State {
     PCIDevice pci_dev;
     uint32_t isa_io_base;
-    uint32_t isa_mem_base;
     I82378State state;
 } PCIi82378State;
 
@@ -234,7 +233,6 @@ static int pci_i82378_init(PCIDevice *dev)
     pci_set_long(dev->wmask + PCI_BASE_ADDRESS_0, 0);
     pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, pci->isa_io_base);
 
-    isa_mem_base = pci->isa_mem_base;
     isa_bus_new(&dev->qdev, pci_address_space_io(dev));
 
     i82378_init(&dev->qdev, s);
@@ -244,7 +242,6 @@ static int pci_i82378_init(PCIDevice *dev)
 
 static Property i82378_properties[] = {
     DEFINE_PROP_HEX32("iobase", PCIi82378State, isa_io_base, 0x80000000),
-    DEFINE_PROP_HEX32("membase", PCIi82378State, isa_mem_base, 0xc0000000),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
index b41d564..d6bcc63 100644
--- a/hw/pci-host/prep.c
+++ b/hw/pci-host/prep.c
@@ -119,6 +119,8 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
     MemoryRegion *address_space_mem = get_system_memory();
     int i;
 
+    isa_mem_base = 0xc0000000;
+
     for (i = 0; i < 4; i++) {
         sysbus_init_irq(dev, &s->irq[i]);
     }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/2] prep_pci: set isa_mem_base in the PCI host bridge

[Andreas Färber]

Am 23.07.2013 23:16, schrieb Hervé Poussineau:
> Currently, it is done by i82378 device, which shouldn't care of it.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/isa/i82378.c    |    3 ---
>  hw/pci-host/prep.c |    2 ++
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
> index b25ed04..de71d81 100644
> --- a/hw/isa/i82378.c
> +++ b/hw/isa/i82378.c
> @@ -45,7 +45,6 @@ typedef struct I82378State {
>  typedef struct PCIi82378State {
>      PCIDevice pci_dev;
>      uint32_t isa_io_base;
> -    uint32_t isa_mem_base;
>      I82378State state;
>  } PCIi82378State;
>  
> @@ -234,7 +233,6 @@ static int pci_i82378_init(PCIDevice *dev)
>      pci_set_long(dev->wmask + PCI_BASE_ADDRESS_0, 0);
>      pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, pci->isa_io_base);
>  
> -    isa_mem_base = pci->isa_mem_base;
>      isa_bus_new(&dev->qdev, pci_address_space_io(dev));
>  
>      i82378_init(&dev->qdev, s);
> @@ -244,7 +242,6 @@ static int pci_i82378_init(PCIDevice *dev)
>  
>  static Property i82378_properties[] = {
>      DEFINE_PROP_HEX32("iobase", PCIi82378State, isa_io_base, 0x80000000),
> -    DEFINE_PROP_HEX32("membase", PCIi82378State, isa_mem_base, 0xc0000000),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index b41d564..d6bcc63 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -119,6 +119,8 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>      MemoryRegion *address_space_mem = get_system_memory();
>      int i;
>  
> +    isa_mem_base = 0xc0000000;
> +
>      for (i = 0; i < 4; i++) {
>          sysbus_init_irq(dev, &s->irq[i]);
>      }

Patch is okay with me, but I wonder what we still need the global
isa_mem_base for? The only users seem to be VGA, adding offsets to it.

Regards,
Andreas

Re: [Qemu-devel] [PATCH 1/2] prep_pci: set isa_mem_base in the PCI host bridge

[Hervé Poussineau]

Andreas Färber a écrit :
> Am 23.07.2013 23:16, schrieb Hervé Poussineau:
>> Currently, it is done by i82378 device, which shouldn't care of it.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>  hw/isa/i82378.c    |    3 ---
>>  hw/pci-host/prep.c |    2 ++
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
>> index b25ed04..de71d81 100644
>> --- a/hw/isa/i82378.c
>> +++ b/hw/isa/i82378.c
>> @@ -45,7 +45,6 @@ typedef struct I82378State {
>>  typedef struct PCIi82378State {
>>      PCIDevice pci_dev;
>>      uint32_t isa_io_base;
>> -    uint32_t isa_mem_base;
>>      I82378State state;
>>  } PCIi82378State;
>>  
>> @@ -234,7 +233,6 @@ static int pci_i82378_init(PCIDevice *dev)
>>      pci_set_long(dev->wmask + PCI_BASE_ADDRESS_0, 0);
>>      pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, pci->isa_io_base);
>>  
>> -    isa_mem_base = pci->isa_mem_base;
>>      isa_bus_new(&dev->qdev, pci_address_space_io(dev));
>>  
>>      i82378_init(&dev->qdev, s);
>> @@ -244,7 +242,6 @@ static int pci_i82378_init(PCIDevice *dev)
>>  
>>  static Property i82378_properties[] = {
>>      DEFINE_PROP_HEX32("iobase", PCIi82378State, isa_io_base, 0x80000000),
>> -    DEFINE_PROP_HEX32("membase", PCIi82378State, isa_mem_base, 0xc0000000),
>>      DEFINE_PROP_END_OF_LIST()
>>  };
>>  
>> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
>> index b41d564..d6bcc63 100644
>> --- a/hw/pci-host/prep.c
>> +++ b/hw/pci-host/prep.c
>> @@ -119,6 +119,8 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>>      MemoryRegion *address_space_mem = get_system_memory();
>>      int i;
>>  
>> +    isa_mem_base = 0xc0000000;
>> +
>>      for (i = 0; i < 4; i++) {
>>          sysbus_init_irq(dev, &s->irq[i]);
>>      }
> 
> Patch is okay with me, but I wonder what we still need the global
> isa_mem_base for? The only users seem to be VGA, adding offsets to it.

Indeed, isa_mem_base should be removed, and PCI bus regions (memory and 
I/O) should be adapted in PCI host.
However, while I have a patch pending, it is not ready yet, so I prefer 
to postpone it past QEMU 1.6.

Hervé

[Qemu-devel] [PATCH 2/2] i82378: cleanup implementation

[Hervé Poussineau]

- i82378 only exists on PCI bus ; do not split implementation in 2 structures
- remove BARs, which are not specified in datasheet
- replace custom isa_mmio implementation by PCI bus IO region usage
- use QOM casts when required

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/isa/i82378.c |  220 ++++++++++++-------------------------------------------
 1 file changed, 48 insertions(+), 172 deletions(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index de71d81..f2045de 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -22,134 +22,27 @@
 #include "hw/timer/i8254.h"
 #include "hw/audio/pcspk.h"
 
-//#define DEBUG_I82378
-
-#ifdef DEBUG_I82378
-#define DPRINTF(fmt, ...) \
-do { fprintf(stderr, "i82378: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) \
-do {} while (0)
-#endif
-
-#define BADF(fmt, ...) \
-do { fprintf(stderr, "i82378 ERROR: " fmt , ## __VA_ARGS__); } while (0)
+#define TYPE_I82378 "i82378"
+#define I82378(obj) \
+    OBJECT_CHECK(I82378State, (obj), TYPE_I82378)
 
 typedef struct I82378State {
+    PCIDevice parent_obj;
     qemu_irq out[2];
     qemu_irq *i8259;
     MemoryRegion io;
-    MemoryRegion mem;
 } I82378State;
 
-typedef struct PCIi82378State {
-    PCIDevice pci_dev;
-    uint32_t isa_io_base;
-    I82378State state;
-} PCIi82378State;
-
-static const VMStateDescription vmstate_pci_i82378 = {
-    .name = "pci-i82378",
+static const VMStateDescription vmstate_i82378 = {
+    .name = "i82378",
     .version_id = 0,
     .minimum_version_id = 0,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(pci_dev, PCIi82378State),
+        VMSTATE_PCI_DEVICE(parent_obj, I82378State),
         VMSTATE_END_OF_LIST()
     },
 };
 
-static void i82378_io_write(void *opaque, hwaddr addr,
-                            uint64_t value, unsigned int size)
-{
-    switch (size) {
-    case 1:
-        DPRINTF("%s: " TARGET_FMT_plx "=%02" PRIx64 "\n", __func__,
-                addr, value);
-        cpu_outb(addr, value);
-        break;
-    case 2:
-        DPRINTF("%s: " TARGET_FMT_plx "=%04" PRIx64 "\n", __func__,
-                addr, value);
-        cpu_outw(addr, value);
-        break;
-    case 4:
-        DPRINTF("%s: " TARGET_FMT_plx "=%08" PRIx64 "\n", __func__,
-                addr, value);
-        cpu_outl(addr, value);
-        break;
-    default:
-        abort();
-    }
-}
-
-static uint64_t i82378_io_read(void *opaque, hwaddr addr,
-                               unsigned int size)
-{
-    DPRINTF("%s: " TARGET_FMT_plx "\n", __func__, addr);
-    switch (size) {
-    case 1:
-        return cpu_inb(addr);
-    case 2:
-        return cpu_inw(addr);
-    case 4:
-        return cpu_inl(addr);
-    default:
-        abort();
-    }
-}
-
-static const MemoryRegionOps i82378_io_ops = {
-    .read = i82378_io_read,
-    .write = i82378_io_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-static void i82378_mem_write(void *opaque, hwaddr addr,
-                             uint64_t value, unsigned int size)
-{
-    switch (size) {
-    case 1:
-        DPRINTF("%s: " TARGET_FMT_plx "=%02" PRIx64 "\n", __func__,
-                addr, value);
-        cpu_outb(addr, value);
-        break;
-    case 2:
-        DPRINTF("%s: " TARGET_FMT_plx "=%04" PRIx64 "\n", __func__,
-                addr, value);
-        cpu_outw(addr, value);
-        break;
-    case 4:
-        DPRINTF("%s: " TARGET_FMT_plx "=%08" PRIx64 "\n", __func__,
-                addr, value);
-        cpu_outl(addr, value);
-        break;
-    default:
-        abort();
-    }
-}
-
-static uint64_t i82378_mem_read(void *opaque, hwaddr addr,
-                                unsigned int size)
-{
-    DPRINTF("%s: " TARGET_FMT_plx "\n", __func__, addr);
-    switch (size) {
-    case 1:
-        return cpu_inb(addr);
-    case 2:
-        return cpu_inw(addr);
-    case 4:
-        return cpu_inl(addr);
-    default:
-        abort();
-    }
-}
-
-static const MemoryRegionOps i82378_mem_ops = {
-    .read = i82378_mem_read,
-    .write = i82378_mem_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 static void i82378_request_out0_irq(void *opaque, int irq, int level)
 {
     I82378State *s = opaque;
@@ -159,19 +52,36 @@ static void i82378_request_out0_irq(void *opaque, int irq, int level)
 static void i82378_request_pic_irq(void *opaque, int irq, int level)
 {
     DeviceState *dev = opaque;
-    PCIDevice *pci = DO_UPCAST(PCIDevice, qdev, dev);
-    PCIi82378State *s = DO_UPCAST(PCIi82378State, pci_dev, pci);
-
-    qemu_set_irq(s->state.i8259[irq], level);
+    qemu_set_irq(I82378(dev)->i8259[irq], level);
 }
 
-static void i82378_init(DeviceState *dev, I82378State *s)
+static void i82378_realize(DeviceState *dev, Error **errp)
 {
-    ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
-    ISADevice *pit;
+    PCIDevice *pci = PCI_DEVICE(dev);
+    I82378State *s = I82378(dev);
+    DeviceClass *dc;
+    uint8_t *pci_conf;
+    ISABus *isabus;
     ISADevice *isa;
     qemu_irq *out0_irq;
 
+    dc = DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev))));
+    assert(dc);
+    dc->realize(dev, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    pci_conf = pci->config;
+    pci_set_word(pci_conf + PCI_COMMAND,
+                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
+    pci_set_word(pci_conf + PCI_STATUS,
+                 PCI_STATUS_DEVSEL_MEDIUM);
+
+    pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */
+
+    isabus = isa_bus_new(dev, pci_address_space_io(pci));
+
     /* This device has:
        2 82C59 (irq)
        1 82C54 (pit)
@@ -182,9 +92,6 @@ static void i82378_init(DeviceState *dev, I82378State *s)
        All devices accept byte access only, except timer
      */
 
-    qdev_init_gpio_out(dev, s->out, 2);
-    qdev_init_gpio_in(dev, i82378_request_pic_irq, 16);
-
     /* Workaround the fact that i8259 is not qdev'ified... */
     out0_irq = qemu_allocate_irqs(i82378_request_out0_irq, s, 1);
 
@@ -193,10 +100,10 @@ static void i82378_init(DeviceState *dev, I82378State *s)
     isa_bus_irqs(isabus, s->i8259);
 
     /* 1 82C54 (pit) */
-    pit = pit_init(isabus, 0x40, 0, NULL);
+    isa = pit_init(isabus, 0x40, 0, NULL);
 
     /* speaker */
-    pcspk_init(isabus, pit);
+    pcspk_init(isabus, isa);
 
     /* 2 82C37 (dma) */
     isa = isa_create_simple(isabus, "i82374");
@@ -206,71 +113,40 @@ static void i82378_init(DeviceState *dev, I82378State *s)
     isa_create_simple(isabus, "mc146818rtc");
 }
 
-static int pci_i82378_init(PCIDevice *dev)
+static void i82378_instance_init(Object *obj)
 {
-    PCIi82378State *pci = DO_UPCAST(PCIi82378State, pci_dev, dev);
-    I82378State *s = &pci->state;
-    uint8_t *pci_conf;
-
-    pci_conf = dev->config;
-    pci_set_word(pci_conf + PCI_COMMAND,
-                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
-    pci_set_word(pci_conf + PCI_STATUS,
-                 PCI_STATUS_DEVSEL_MEDIUM);
-
-    pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin 0 */
-
-    memory_region_init_io(&s->io, OBJECT(pci), &i82378_io_ops, s,
-                          "i82378-io", 0x00010000);
-    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->io);
-
-    memory_region_init_io(&s->mem, OBJECT(pci), &i82378_mem_ops, s,
-                          "i82378-mem", 0x01000000);
-    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
-
-    /* Make I/O address read only */
-    pci_set_word(dev->wmask + PCI_COMMAND, PCI_COMMAND_SPECIAL);
-    pci_set_long(dev->wmask + PCI_BASE_ADDRESS_0, 0);
-    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, pci->isa_io_base);
+    DeviceState *dev = DEVICE(obj);
+    I82378State *s = I82378(obj);
 
-    isa_bus_new(&dev->qdev, pci_address_space_io(dev));
-
-    i82378_init(&dev->qdev, s);
-
-    return 0;
+    qdev_init_gpio_out(dev, s->out, 2);
+    qdev_init_gpio_in(dev, i82378_request_pic_irq, 16);
 }
 
-static Property i82378_properties[] = {
-    DEFINE_PROP_HEX32("iobase", PCIi82378State, isa_io_base, 0x80000000),
-    DEFINE_PROP_END_OF_LIST()
-};
-
-static void pci_i82378_class_init(ObjectClass *klass, void *data)
+static void i82378_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    k->init = pci_i82378_init;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82378;
     k->revision = 0x03;
     k->class_id = PCI_CLASS_BRIDGE_ISA;
-    k->subsystem_vendor_id = 0x0;
-    k->subsystem_id = 0x0;
-    dc->vmsd = &vmstate_pci_i82378;
-    dc->props = i82378_properties;
+    dc->realize = i82378_realize;
+    dc->vmsd = &vmstate_i82378;
+    dc->no_user = 1;
 }
 
-static const TypeInfo pci_i82378_info = {
-    .name = "i82378",
+static const TypeInfo i82378_info = {
+    .name = TYPE_I82378,
     .parent = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PCIi82378State),
-    .class_init = pci_i82378_class_init,
+    .instance_size = sizeof(I82378State),
+    .instance_init = i82378_instance_init,
+    .class_init = i82378_class_init,
 };
 
 static void i82378_register_types(void)
 {
-    type_register_static(&pci_i82378_info);
+    type_register_static(&i82378_info);
 }
 
 type_init(i82378_register_types)
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/2] i82378: cleanup implementation

[Andreas Färber]

Hi,

Am 23.07.2013 23:16, schrieb Hervé Poussineau:
> - i82378 only exists on PCI bus ; do not split implementation in 2 structures
> - remove BARs, which are not specified in datasheet
> - replace custom isa_mmio implementation by PCI bus IO region usage
> - use QOM casts when required
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

Thanks for adopting some of the latest patterns without being asked to!

I've queued this with some style changes, but apart from testing issues
as CC'ed, I have a major question/concern in the bottom...

> ---
>  hw/isa/i82378.c |  220 ++++++++++++-------------------------------------------
>  1 file changed, 48 insertions(+), 172 deletions(-)
> 
> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
> index de71d81..f2045de 100644
> --- a/hw/isa/i82378.c
> +++ b/hw/isa/i82378.c
> @@ -22,134 +22,27 @@
>  #include "hw/timer/i8254.h"
>  #include "hw/audio/pcspk.h"
>  
> -//#define DEBUG_I82378
> -
> -#ifdef DEBUG_I82378
> -#define DPRINTF(fmt, ...) \
> -do { fprintf(stderr, "i82378: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define DPRINTF(fmt, ...) \
> -do {} while (0)
> -#endif
> -
> -#define BADF(fmt, ...) \
> -do { fprintf(stderr, "i82378 ERROR: " fmt , ## __VA_ARGS__); } while (0)
> +#define TYPE_I82378 "i82378"
> +#define I82378(obj) \
> +    OBJECT_CHECK(I82378State, (obj), TYPE_I82378)
>  
>  typedef struct I82378State {
> +    PCIDevice parent_obj;
>      qemu_irq out[2];
>      qemu_irq *i8259;
>      MemoryRegion io;
> -    MemoryRegion mem;
>  } I82378State;
>  
> -typedef struct PCIi82378State {
> -    PCIDevice pci_dev;
> -    uint32_t isa_io_base;
> -    I82378State state;
> -} PCIi82378State;
> -
> -static const VMStateDescription vmstate_pci_i82378 = {
> -    .name = "pci-i82378",
> +static const VMStateDescription vmstate_i82378 = {
> +    .name = "i82378",
>      .version_id = 0,
>      .minimum_version_id = 0,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCI_DEVICE(pci_dev, PCIi82378State),
> +        VMSTATE_PCI_DEVICE(parent_obj, I82378State),
>          VMSTATE_END_OF_LIST()
>      },
>  };
>  
> -static void i82378_io_write(void *opaque, hwaddr addr,
> -                            uint64_t value, unsigned int size)
> -{
> -    switch (size) {
> -    case 1:
> -        DPRINTF("%s: " TARGET_FMT_plx "=%02" PRIx64 "\n", __func__,
> -                addr, value);
> -        cpu_outb(addr, value);
> -        break;
> -    case 2:
> -        DPRINTF("%s: " TARGET_FMT_plx "=%04" PRIx64 "\n", __func__,
> -                addr, value);
> -        cpu_outw(addr, value);
> -        break;
> -    case 4:
> -        DPRINTF("%s: " TARGET_FMT_plx "=%08" PRIx64 "\n", __func__,
> -                addr, value);
> -        cpu_outl(addr, value);
> -        break;
> -    default:
> -        abort();
> -    }
> -}
> -
> -static uint64_t i82378_io_read(void *opaque, hwaddr addr,
> -                               unsigned int size)
> -{
> -    DPRINTF("%s: " TARGET_FMT_plx "\n", __func__, addr);
> -    switch (size) {
> -    case 1:
> -        return cpu_inb(addr);
> -    case 2:
> -        return cpu_inw(addr);
> -    case 4:
> -        return cpu_inl(addr);
> -    default:
> -        abort();
> -    }
> -}
> -
> -static const MemoryRegionOps i82378_io_ops = {
> -    .read = i82378_io_read,
> -    .write = i82378_io_write,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
> -static void i82378_mem_write(void *opaque, hwaddr addr,
> -                             uint64_t value, unsigned int size)
> -{
> -    switch (size) {
> -    case 1:
> -        DPRINTF("%s: " TARGET_FMT_plx "=%02" PRIx64 "\n", __func__,
> -                addr, value);
> -        cpu_outb(addr, value);
> -        break;
> -    case 2:
> -        DPRINTF("%s: " TARGET_FMT_plx "=%04" PRIx64 "\n", __func__,
> -                addr, value);
> -        cpu_outw(addr, value);
> -        break;
> -    case 4:
> -        DPRINTF("%s: " TARGET_FMT_plx "=%08" PRIx64 "\n", __func__,
> -                addr, value);
> -        cpu_outl(addr, value);
> -        break;
> -    default:
> -        abort();
> -    }
> -}
> -
> -static uint64_t i82378_mem_read(void *opaque, hwaddr addr,
> -                                unsigned int size)
> -{
> -    DPRINTF("%s: " TARGET_FMT_plx "\n", __func__, addr);
> -    switch (size) {
> -    case 1:
> -        return cpu_inb(addr);
> -    case 2:
> -        return cpu_inw(addr);
> -    case 4:
> -        return cpu_inl(addr);
> -    default:
> -        abort();
> -    }
> -}
> -
> -static const MemoryRegionOps i82378_mem_ops = {
> -    .read = i82378_mem_read,
> -    .write = i82378_mem_write,
> -    .endianness = DEVICE_LITTLE_ENDIAN,
> -};
> -
>  static void i82378_request_out0_irq(void *opaque, int irq, int level)
>  {
>      I82378State *s = opaque;
> @@ -159,19 +52,36 @@ static void i82378_request_out0_irq(void *opaque, int irq, int level)
>  static void i82378_request_pic_irq(void *opaque, int irq, int level)
>  {
>      DeviceState *dev = opaque;
> -    PCIDevice *pci = DO_UPCAST(PCIDevice, qdev, dev);
> -    PCIi82378State *s = DO_UPCAST(PCIi82378State, pci_dev, pci);
> -
> -    qemu_set_irq(s->state.i8259[irq], level);
> +    qemu_set_irq(I82378(dev)->i8259[irq], level);

Changed that back to a variable s - FOO(bar)->baz is undesired.

>  }
>  
> -static void i82378_init(DeviceState *dev, I82378State *s)
> +static void i82378_realize(DeviceState *dev, Error **errp)
>  {
> -    ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> -    ISADevice *pit;
> +    PCIDevice *pci = PCI_DEVICE(dev);
> +    I82378State *s = I82378(dev);
> +    DeviceClass *dc;
> +    uint8_t *pci_conf;
> +    ISABus *isabus;
>      ISADevice *isa;
>      qemu_irq *out0_irq;
>  
> +    dc = DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev))));

This is going into uncharted territories. ;) I consider it wrong to use
object_get_class() - we should use object_class_by_name() to allow for
derived types and I'll put it into a macro that I'll try to align with
Peter C.'s and my QOM work.

> +    assert(dc);

This shouldn't be necessary?

> +    dc->realize(dev, errp);
> +    if (error_is_set(errp)) {
> +        return;

This doesn't do what you want. You need a local err variable to return
here, errp might be NULL => !error_is_set(errp).

> +    }
> +
> +    pci_conf = pci->config;
> +    pci_set_word(pci_conf + PCI_COMMAND,
> +                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> +    pci_set_word(pci_conf + PCI_STATUS,
> +                 PCI_STATUS_DEVSEL_MEDIUM);
> +
> +    pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */
> +
> +    isabus = isa_bus_new(dev, pci_address_space_io(pci));
> +
>      /* This device has:
>         2 82C59 (irq)
>         1 82C54 (pit)
> @@ -182,9 +92,6 @@ static void i82378_init(DeviceState *dev, I82378State *s)
>         All devices accept byte access only, except timer
>       */
>  
> -    qdev_init_gpio_out(dev, s->out, 2);
> -    qdev_init_gpio_in(dev, i82378_request_pic_irq, 16);
> -
>      /* Workaround the fact that i8259 is not qdev'ified... */
>      out0_irq = qemu_allocate_irqs(i82378_request_out0_irq, s, 1);
>  
> @@ -193,10 +100,10 @@ static void i82378_init(DeviceState *dev, I82378State *s)
>      isa_bus_irqs(isabus, s->i8259);
>  
>      /* 1 82C54 (pit) */
> -    pit = pit_init(isabus, 0x40, 0, NULL);
> +    isa = pit_init(isabus, 0x40, 0, NULL);
>  
>      /* speaker */
> -    pcspk_init(isabus, pit);
> +    pcspk_init(isabus, isa);
>  
>      /* 2 82C37 (dma) */
>      isa = isa_create_simple(isabus, "i82374");
> @@ -206,71 +113,40 @@ static void i82378_init(DeviceState *dev, I82378State *s)
>      isa_create_simple(isabus, "mc146818rtc");
>  }
>  
> -static int pci_i82378_init(PCIDevice *dev)
> +static void i82378_instance_init(Object *obj)
>  {
> -    PCIi82378State *pci = DO_UPCAST(PCIi82378State, pci_dev, dev);
> -    I82378State *s = &pci->state;
> -    uint8_t *pci_conf;
> -
> -    pci_conf = dev->config;
> -    pci_set_word(pci_conf + PCI_COMMAND,
> -                 PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
> -    pci_set_word(pci_conf + PCI_STATUS,
> -                 PCI_STATUS_DEVSEL_MEDIUM);
> -
> -    pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin 0 */
> -
> -    memory_region_init_io(&s->io, OBJECT(pci), &i82378_io_ops, s,
> -                          "i82378-io", 0x00010000);
> -    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->io);
> -
> -    memory_region_init_io(&s->mem, OBJECT(pci), &i82378_mem_ops, s,
> -                          "i82378-mem", 0x01000000);
> -    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
> -
> -    /* Make I/O address read only */
> -    pci_set_word(dev->wmask + PCI_COMMAND, PCI_COMMAND_SPECIAL);
> -    pci_set_long(dev->wmask + PCI_BASE_ADDRESS_0, 0);
> -    pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, pci->isa_io_base);
> +    DeviceState *dev = DEVICE(obj);
> +    I82378State *s = I82378(obj);
>  
> -    isa_bus_new(&dev->qdev, pci_address_space_io(dev));
> -
> -    i82378_init(&dev->qdev, s);
> -
> -    return 0;
> +    qdev_init_gpio_out(dev, s->out, 2);
> +    qdev_init_gpio_in(dev, i82378_request_pic_irq, 16);
>  }
>  
> -static Property i82378_properties[] = {
> -    DEFINE_PROP_HEX32("iobase", PCIi82378State, isa_io_base, 0x80000000),
> -    DEFINE_PROP_END_OF_LIST()
> -};
> -
> -static void pci_i82378_class_init(ObjectClass *klass, void *data)
> +static void i82378_class_init(ObjectClass *klass, void *data)
>  {
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> -    k->init = pci_i82378_init;
>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>      k->device_id = PCI_DEVICE_ID_INTEL_82378;
>      k->revision = 0x03;
>      k->class_id = PCI_CLASS_BRIDGE_ISA;
> -    k->subsystem_vendor_id = 0x0;
> -    k->subsystem_id = 0x0;
> -    dc->vmsd = &vmstate_pci_i82378;
> -    dc->props = i82378_properties;
> +    dc->realize = i82378_realize;
> +    dc->vmsd = &vmstate_i82378;

(FWIW dc->categories has been merged here.)

> +    dc->no_user = 1;

Why do you do this? For one, according to Anthony it should no longer be
used, and for another, Paolo's endianness-test (make check) is using
-device i82378 for various other ppc and sh4 machines IIUC. make check
still succeeds for ppc with this patch though, so that might be due tot
-device ignoring DeviceClass::no_user?

Hoping to get this in shape for -rc1.

Regards,
Andreas

>  }
>  
> -static const TypeInfo pci_i82378_info = {
> -    .name = "i82378",
> +static const TypeInfo i82378_info = {
> +    .name = TYPE_I82378,
>      .parent = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(PCIi82378State),
> -    .class_init = pci_i82378_class_init,
> +    .instance_size = sizeof(I82378State),
> +    .instance_init = i82378_instance_init,
> +    .class_init = i82378_class_init,
>  };
>  
>  static void i82378_register_types(void)
>  {
> -    type_register_static(&pci_i82378_info);
> +    type_register_static(&i82378_info);
>  }
>  
>  type_init(i82378_register_types)
> 

Re: [Qemu-devel] [PATCH 2/2] i82378: cleanup implementation

[Hervé Poussineau]

Andreas Färber a écrit :
> Hi,
> 
> Am 23.07.2013 23:16, schrieb Hervé Poussineau:
>> - i82378 only exists on PCI bus ; do not split implementation in 2 structures
>> - remove BARs, which are not specified in datasheet
>> - replace custom isa_mmio implementation by PCI bus IO region usage
>> - use QOM casts when required
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> 
> Thanks for adopting some of the latest patterns without being asked to!
> 
> I've queued this with some style changes, but apart from testing issues
> as CC'ed, I have a major question/concern in the bottom...
> 
>> ---
>>  hw/isa/i82378.c |  220 ++++++++++++-------------------------------------------
>>  1 file changed, 48 insertions(+), 172 deletions(-)
>>
>> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
>> index de71d81..f2045de 100644
>> --- a/hw/isa/i82378.c
>> +++ b/hw/isa/i82378.c

[...]

>> @@ -159,19 +52,36 @@ static void i82378_request_out0_irq(void *opaque, int irq, int level)
>>  static void i82378_request_pic_irq(void *opaque, int irq, int level)
>>  {
>>      DeviceState *dev = opaque;
>> -    PCIDevice *pci = DO_UPCAST(PCIDevice, qdev, dev);
>> -    PCIi82378State *s = DO_UPCAST(PCIi82378State, pci_dev, pci);
>> -
>> -    qemu_set_irq(s->state.i8259[irq], level);
>> +    qemu_set_irq(I82378(dev)->i8259[irq], level);
> 
> Changed that back to a variable s - FOO(bar)->baz is undesired.

OK

> 
>>  }
>>  
>> -static void i82378_init(DeviceState *dev, I82378State *s)
>> +static void i82378_realize(DeviceState *dev, Error **errp)
>>  {
>> -    ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>> -    ISADevice *pit;
>> +    PCIDevice *pci = PCI_DEVICE(dev);
>> +    I82378State *s = I82378(dev);
>> +    DeviceClass *dc;
>> +    uint8_t *pci_conf;
>> +    ISABus *isabus;
>>      ISADevice *isa;
>>      qemu_irq *out0_irq;
>>  
>> +    dc = DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev))));
> 
> This is going into uncharted territories. ;) I consider it wrong to use
> object_get_class() - we should use object_class_by_name() to allow for
> derived types and I'll put it into a macro that I'll try to align with
> Peter C.'s and my QOM work.

OK

>> +    assert(dc);
> 
> This shouldn't be necessary?

OK. It can be removed.

> 
>> +    dc->realize(dev, errp);
>> +    if (error_is_set(errp)) {
>> +        return;
> 
> This doesn't do what you want. You need a local err variable to return
> here, errp might be NULL => !error_is_set(errp).

OK

[...]

>>  
>> -    k->init = pci_i82378_init;
>>      k->vendor_id = PCI_VENDOR_ID_INTEL;
>>      k->device_id = PCI_DEVICE_ID_INTEL_82378;
>>      k->revision = 0x03;
>>      k->class_id = PCI_CLASS_BRIDGE_ISA;
>> -    k->subsystem_vendor_id = 0x0;
>> -    k->subsystem_id = 0x0;
>> -    dc->vmsd = &vmstate_pci_i82378;
>> -    dc->props = i82378_properties;
>> +    dc->realize = i82378_realize;
>> +    dc->vmsd = &vmstate_i82378;
> 
> (FWIW dc->categories has been merged here.)

Yes, but it has been merged after I sent this patch...

> 
>> +    dc->no_user = 1;
> 
> Why do you do this? For one, according to Anthony it should no longer be
> used, and for another, Paolo's endianness-test (make check) is using
> -device i82378 for various other ppc and sh4 machines IIUC. make check
> still succeeds for ppc with this patch though, so that might be due tot
> -device ignoring DeviceClass::no_user?

I probably copied it from another chipset device, maybe i440fx.
I don't really mind removing it.

Yes, I double-checked that make check still works for all architectures.

> Hoping to get this in shape for -rc1.

Sure. Should I send a v2, as it seems you already queued it?

Hervé

Re: [Qemu-devel] [PATCH 2/2] i82378: cleanup implementation

[Andreas Färber]

Am 30.07.2013 22:06, schrieb Hervé Poussineau:
> Andreas Färber a écrit :
>> Am 23.07.2013 23:16, schrieb Hervé Poussineau:
>>>  }
>>>  
>>> -static void i82378_init(DeviceState *dev, I82378State *s)
>>> +static void i82378_realize(DeviceState *dev, Error **errp)
>>>  {
>>> -    ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
>>> -    ISADevice *pit;
>>> +    PCIDevice *pci = PCI_DEVICE(dev);
>>> +    I82378State *s = I82378(dev);
>>> +    DeviceClass *dc;
>>> +    uint8_t *pci_conf;
>>> +    ISABus *isabus;
>>>      ISADevice *isa;
>>>      qemu_irq *out0_irq;
>>>  
>>> +    dc =
>>> DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev))));
>>
>> This is going into uncharted territories. ;) I consider it wrong to use
>> object_get_class() - we should use object_class_by_name() to allow for
>> derived types and I'll put it into a macro that I'll try to align with
>> Peter C.'s and my QOM work.
> 
> OK

While this gave me an inspiration for my virtio refactoring (it is
possible to convert device by device by calling the parent's
DeviceClass::realize as done here, as long as the *Class::init is called
conditionally through the DeviceClass::init implementation), I am
concerned about a single device deviating from initialization order here
(hw/pci/pci.c:pci_qdev_init() does things after calling PCIDevice::init,
namely ROM handling and bus hotplug) and will revert this to an
old-style qdev initfn for 1.6 bugfix.

[...]
>>> +    dc->no_user = 1;
>>
>> Why do you do this? For one, according to Anthony it should no longer be
>> used, and for another, Paolo's endianness-test (make check) is using
>> -device i82378 for various other ppc and sh4 machines IIUC. make check
>> still succeeds for ppc with this patch though, so that might be due to
>> -device ignoring DeviceClass::no_user?
> 
> I probably copied it from another chipset device, maybe i440fx.
> I don't really mind removing it.

Good.

> Yes, I double-checked that make check still works for all architectures.
> 
>> Hoping to get this in shape for -rc1.
> 
> Sure. Should I send a v2, as it seems you already queued it?

Since -rc1 is due tomorrow, I'll put together a pull myself tonight.

Andreas

Re: [Qemu-devel] [PATCH 2/2] i82378: cleanup implementation

[Andreas Färber]

Am 23.07.2013 23:16, schrieb Hervé Poussineau:
> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
> index de71d81..f2045de 100644
> --- a/hw/isa/i82378.c
> +++ b/hw/isa/i82378.c
[...]
> -static const VMStateDescription vmstate_pci_i82378 = {
> -    .name = "pci-i82378",
> +static const VMStateDescription vmstate_i82378 = {
> +    .name = "i82378",
>      .version_id = 0,
>      .minimum_version_id = 0,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCI_DEVICE(pci_dev, PCIi82378State),
> +        VMSTATE_PCI_DEVICE(parent_obj, I82378State),
>          VMSTATE_END_OF_LIST()
>      },
>  };
[snip]

Renaming the section would break backwards compatibility, we'll need to
live with that name.

Andreas

Re: [Qemu-devel] [PATCH 2/2] i82378: cleanup implementation

[Hervé Poussineau]

Andreas Färber a écrit :
> Am 23.07.2013 23:16, schrieb Hervé Poussineau:
>> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
>> index de71d81..f2045de 100644
>> --- a/hw/isa/i82378.c
>> +++ b/hw/isa/i82378.c
> [...]
>> -static const VMStateDescription vmstate_pci_i82378 = {
>> -    .name = "pci-i82378",
>> +static const VMStateDescription vmstate_i82378 = {
>> +    .name = "i82378",
>>      .version_id = 0,
>>      .minimum_version_id = 0,
>>      .fields = (VMStateField[]) {
>> -        VMSTATE_PCI_DEVICE(pci_dev, PCIi82378State),
>> +        VMSTATE_PCI_DEVICE(parent_obj, I82378State),
>>          VMSTATE_END_OF_LIST()
>>      },
>>  };
> [snip]
> 
> Renaming the section would break backwards compatibility, we'll need to
> live with that name.

Not a problem.
Or, you can update version_id and minimum_version_id to 1, and ignore 
backward compatibility problems, as i82378 is only used in PPC machine 
by default.
Do as you want.

Hervé