[Qemu-devel] [PATCH 00/22] Indirection Cleanup

[Qemu-devel] [PATCH 00/22] Indirection Cleanup

[Juan Quintela]

This patch series clean up "half" converted qemu drivers that had changed from:

struct FOOState

to

typedef PCIFOOState {
    PCIDevice dev;
    FOOState foo;
} PCIFOOState;

It just moves PCIDevice to be the 1st field of FOOState.

Once there, other cleanups were done:
a - pci_dev pointer from FOOState to PCIFOOState is removed, jsut use s->dev
    The field is leave only in the drivers that also emulate isa.
b- Once there, transformo
   PCIFOOState *s = (PCIFOOState *)pci_dev
   to
   PCIFOOState *s = DO_UPCAST(PCIFOOState, dev, pci_dev)
   where pci_dev is a PCI_DEVICE.
c- again, once there, remove all the casts from void * (they are not
   needed since '89)
   PCIFOOState *s = (PCIFOOState *)opaque;
   to
   PCIFOOState *s = opaque;
d- Start of vga.c cleanup.  It is not trivial, as just now
   VGAState == VGACommonState, functions need to be changed to use the right value.

ToDo:
- pcnet:  It needs a different approach, because it can be both a PCIDevice
  or a SysBus device.
- vga: It have to separate the common part for the not common part, problems now
  is that VGAState is used for both the common state and the standard vga.
  To make things more interesting, different bits are "inherited" from vga
  in different devices:
  - cirrus_vga
  - vmware_vga
  - blizzard (vga that is not pci)
  - vga common state has a pci_dev pointer, that is only needed by std vga,
    as cirrus_vga stores it (with this patches) otherplace, blizzard is not pci
    .... you get the idea
- I guess some other driver is missing, but my fast grep didn't found it.

Later, Juan.


Juan Quintela (22):
  eepro100: convert casts to DO_UPCAST()
  eepro100: cast a void * makes no sense
  eepro100: Remove unused indirection of PCIDevice
  es1370: Remove unused indirection of PCIES1370State and ES1370State
  ne2000: remove casts from void *
  ne2000: pci_dev has this very value with the right type
  ne2000: Remove unneeded double indirection of PCINE2000State
  ne2000: change pci_dev to is_pci
  pci: remove casts from void *
  rtl8139: Remove unneeded double indirection of PCIRTL8139State
  rtl8139: remove pointless cast from void *
  lsi53c895a: remove pointless cast from void *
  lsi53c895a: use DO_UPCAST to cast from PCIDevice
  lsi53c895a: rename PCIDevice field from pci_dev to dev (consistence)
  lsi53c895a: LSIState is a PCIDevice is a DeviceHost
  usb-ohci: Remove unneeded double indirection of OHCIPCIState
  cirrus_vga: Remove unneeded double indirection of PCICirrusVGAState
  cirrus_vga: remove pointless cast from void *
  cirrus_vga: change use of pci_dev for is_pci
  Introduce vga_common_reset() to be able to typcheck vga_reset()
  vga: Rename vga_state -> vga
  Everything outside of vga.c should use VGACommonState

 hw/cirrus_vga.c |   59 ++++++++++++++++++++++---------------------------
 hw/eepro100.c   |   65 ++++++++++++++++++++++--------------------------------
 hw/es1370.c     |   31 ++++++++-----------------
 hw/lsi53c895a.c |   65 ++++++++++++++++++++++++++++---------------------------
 hw/ne2000.c     |   41 ++++++++++++++--------------------
 hw/pci.c        |    8 +++---
 hw/rtl8139.c    |   42 ++++++++++++----------------------
 hw/usb-ohci.c   |   34 +++++++++++-----------------
 hw/vga.c        |   26 +++++++++++++--------
 hw/vga_int.h    |   12 ++++------
 hw/vmware_vga.c |    4 +-
 11 files changed, 170 insertions(+), 217 deletions(-)

[Qemu-devel] [PATCH 01/22] eepro100: convert casts to DO_UPCAST()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/eepro100.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index ec31a6a..0031d36 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1,4 +1,4 @@
-/*
+ /*
  * QEMU i8255x (PRO100) emulation
  *
  * Copyright (c) 2006-2007 Stefan Weil
@@ -1346,7 +1346,7 @@ typedef struct PCIEEPRO100State {
 static void pci_map(PCIDevice * pci_dev, int region_num,
                     uint32_t addr, uint32_t size, int type)
 {
-    PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
+    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);
     EEPRO100State *s = &d->eepro100;

     logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
@@ -1420,7 +1420,7 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {
 static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
                          uint32_t addr, uint32_t size, int type)
 {
-    PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
+    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);

     logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
            region_num, addr, size, type);
@@ -1720,7 +1720,7 @@ static void nic_cleanup(VLANClientState *vc)

 static int pci_nic_uninit(PCIDevice *dev)
 {
-    PCIEEPRO100State *d = (PCIEEPRO100State *) dev;
+    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, dev);
     EEPRO100State *s = &d->eepro100;

     cpu_unregister_io_memory(s->mmio_index);
@@ -1730,7 +1730,7 @@ static int pci_nic_uninit(PCIDevice *dev)

 static void nic_init(PCIDevice *pci_dev, uint32_t device)
 {
-    PCIEEPRO100State *d = (PCIEEPRO100State *)pci_dev;
+    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);
     EEPRO100State *s;

     logout("\n");
-- 
1.6.2.5

[Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/eepro100.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 0031d36..09083c2 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)

 static void nic_reset(void *opaque)
 {
-    EEPRO100State *s = (EEPRO100State *) opaque;
+    EEPRO100State *s = opaque;
     logout("%p\n", s);
     static int first;
     if (!first) {
@@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size

 static int nic_load(QEMUFile * f, void *opaque, int version_id)
 {
-    EEPRO100State *s = (EEPRO100State *) opaque;
+    EEPRO100State *s = opaque;
     int i;
     int ret;

@@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)

 static void nic_save(QEMUFile * f, void *opaque)
 {
-    EEPRO100State *s = (EEPRO100State *) opaque;
+    EEPRO100State *s = opaque;
     int i;

     if (s->pci_dev)
-- 
1.6.2.5

[Qemu-devel] [PATCH 03/22] eepro100: Remove unused indirection of PCIDevice

[Juan Quintela]

Once there, there is no way that we don't have a PCI Device at save/load time. Remove the check

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/eepro100.c |   57 +++++++++++++++++++++++----------------------------------
 1 files changed, 23 insertions(+), 34 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 09083c2..d9f18c6 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -181,6 +181,7 @@ typedef enum {
 } ru_state_t;

 typedef struct {
+    PCIDevice dev;
 #if 1
     uint8_t cmd;
     uint32_t start;
@@ -200,7 +201,6 @@ typedef struct {
     uint8_t curpag;
     uint8_t mult[8];            /* multicast mask array */
     int mmio_index;
-    PCIDevice *pci_dev;
     VLANClientState *vc;
 #endif
     uint8_t scb_stat;           /* SCB stat/ack byte */
@@ -304,7 +304,7 @@ static void disable_interrupt(EEPRO100State * s)
 {
     if (s->int_stat) {
         logout("interrupt disabled\n");
-        qemu_irq_lower(s->pci_dev->irq[0]);
+        qemu_irq_lower(s->dev.irq[0]);
         s->int_stat = 0;
     }
 }
@@ -313,7 +313,7 @@ static void enable_interrupt(EEPRO100State * s)
 {
     if (!s->int_stat) {
         logout("interrupt enabled\n");
-        qemu_irq_raise(s->pci_dev->irq[0]);
+        qemu_irq_raise(s->dev.irq[0]);
         s->int_stat = 1;
     }
 }
@@ -392,7 +392,7 @@ static void eepro100_fcp_interrupt(EEPRO100State * s)
 static void pci_reset(EEPRO100State * s)
 {
     uint32_t device = s->device;
-    uint8_t *pci_conf = s->pci_dev->config;
+    uint8_t *pci_conf = s->dev.config;

     logout("%p\n", s);

@@ -1338,16 +1338,10 @@ static void ioport_write4(void *opaque, uint32_t addr, uint32_t val)
 /***********************************************************/
 /* PCI EEPRO100 definitions */

-typedef struct PCIEEPRO100State {
-    PCIDevice dev;
-    EEPRO100State eepro100;
-} PCIEEPRO100State;
-
 static void pci_map(PCIDevice * pci_dev, int region_num,
                     uint32_t addr, uint32_t size, int type)
 {
-    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);
-    EEPRO100State *s = &d->eepro100;
+    EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);

     logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
            region_num, addr, size, type);
@@ -1420,15 +1414,15 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {
 static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
                          uint32_t addr, uint32_t size, int type)
 {
-    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);
+    EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);

     logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
            region_num, addr, size, type);

     if (region_num == 0) {
         /* Map control / status registers. */
-        cpu_register_physical_memory(addr, size, d->eepro100.mmio_index);
-        d->eepro100.region[region_num] = addr;
+        cpu_register_physical_memory(addr, size, s->mmio_index);
+        s->region[region_num] = addr;
     }
 }

@@ -1551,8 +1545,8 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)
     if (version_id > 3)
         return -EINVAL;

-    if (s->pci_dev && version_id >= 3) {
-        ret = pci_device_load(s->pci_dev, f);
+    if (version_id >= 3) {
+        ret = pci_device_load(&s->dev, f);
         if (ret < 0)
             return ret;
     }
@@ -1637,8 +1631,7 @@ static void nic_save(QEMUFile * f, void *opaque)
     EEPRO100State *s = opaque;
     int i;

-    if (s->pci_dev)
-        pci_device_save(s->pci_dev, f);
+    pci_device_save(&s->dev, f);

     qemu_put_8s(f, &s->rxcr);

@@ -1720,8 +1713,7 @@ static void nic_cleanup(VLANClientState *vc)

 static int pci_nic_uninit(PCIDevice *dev)
 {
-    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, dev);
-    EEPRO100State *s = &d->eepro100;
+    EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, dev);

     cpu_unregister_io_memory(s->mmio_index);

@@ -1730,16 +1722,13 @@ static int pci_nic_uninit(PCIDevice *dev)

 static void nic_init(PCIDevice *pci_dev, uint32_t device)
 {
-    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);
-    EEPRO100State *s;
+    EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);

     logout("\n");

-    d->dev.unregister = pci_nic_uninit;
+    s->dev.unregister = pci_nic_uninit;

-    s = &d->eepro100;
     s->device = device;
-    s->pci_dev = &d->dev;

     pci_reset(s);

@@ -1748,24 +1737,24 @@ static void nic_init(PCIDevice *pci_dev, uint32_t device)
     s->eeprom = eeprom93xx_new(EEPROM_SIZE);

     /* Handler for memory-mapped I/O */
-    d->eepro100.mmio_index =
+    s->mmio_index =
         cpu_register_io_memory(pci_mmio_read, pci_mmio_write, s);

-    pci_register_bar(&d->dev, 0, PCI_MEM_SIZE,
+    pci_register_bar(&s->dev, 0, PCI_MEM_SIZE,
                            PCI_ADDRESS_SPACE_MEM |
                            PCI_ADDRESS_SPACE_MEM_PREFETCH, pci_mmio_map);
-    pci_register_bar(&d->dev, 1, PCI_IO_SIZE, PCI_ADDRESS_SPACE_IO,
+    pci_register_bar(&s->dev, 1, PCI_IO_SIZE, PCI_ADDRESS_SPACE_IO,
                            pci_map);
-    pci_register_bar(&d->dev, 2, PCI_FLASH_SIZE, PCI_ADDRESS_SPACE_MEM,
+    pci_register_bar(&s->dev, 2, PCI_FLASH_SIZE, PCI_ADDRESS_SPACE_MEM,
                            pci_mmio_map);

-    qdev_get_macaddr(&d->dev.qdev, s->macaddr);
+    qdev_get_macaddr(&s->dev.qdev, s->macaddr);
     logout("macaddr: %s\n", nic_dump(&s->macaddr[0], 6));
     assert(s->region[1] == 0);

     nic_reset(s);

-    s->vc = qdev_get_vlan_client(&d->dev.qdev,
+    s->vc = qdev_get_vlan_client(&s->dev.qdev,
                                  nic_can_receive, nic_receive, NULL,
                                  nic_cleanup, s);

@@ -1794,15 +1783,15 @@ static void pci_i82559er_init(PCIDevice *dev)
 static PCIDeviceInfo eepro100_info[] = {
     {
         .qdev.name = "i82551",
-        .qdev.size = sizeof(PCIEEPRO100State),
+        .qdev.size = sizeof(EEPRO100State),
         .init      = pci_i82551_init,
     },{
         .qdev.name = "i82557b",
-        .qdev.size = sizeof(PCIEEPRO100State),
+        .qdev.size = sizeof(EEPRO100State),
         .init      = pci_i82557b_init,
     },{
         .qdev.name = "i82559er",
-        .qdev.size = sizeof(PCIEEPRO100State),
+        .qdev.size = sizeof(EEPRO100State),
         .init      = pci_i82559er_init,
     },{
         /* end of list */
-- 
1.6.2.5

[Qemu-devel] [PATCH 04/22] es1370: Remove unused indirection of PCIES1370State and ES1370State

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/es1370.c |   31 ++++++++++---------------------
 1 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/hw/es1370.c b/hw/es1370.c
index 5c9af0e..bdb4c7c 100644
--- a/hw/es1370.c
+++ b/hw/es1370.c
@@ -266,8 +266,7 @@ struct chan {
 };

 typedef struct ES1370State {
-    PCIDevice *pci_dev;
-
+    PCIDevice dev;
     QEMUSoundCard card;
     struct chan chan[NB_CHANNELS];
     SWVoiceOut *dac_voice[2];
@@ -280,11 +279,6 @@ typedef struct ES1370State {
     uint32_t sctl;
 } ES1370State;

-typedef struct PCIES1370State {
-    PCIDevice dev;
-    ES1370State es1370;
-} PCIES1370State;
-
 struct chan_bits {
     uint32_t ctl_en;
     uint32_t stat_int;
@@ -327,7 +321,7 @@ static void es1370_update_status (ES1370State *s, uint32_t new_status)
     else {
         s->status = new_status & ~STAT_INTR;
     }
-    qemu_set_irq (s->pci_dev->irq[0], !!level);
+    qemu_set_irq (s->dev.irq[0], !!level);
 }

 static void es1370_reset (ES1370State *s)
@@ -353,7 +347,7 @@ static void es1370_reset (ES1370State *s)
             s->dac_voice[i] = NULL;
         }
     }
-    qemu_irq_lower (s->pci_dev->irq[0]);
+    qemu_irq_lower (s->dev.irq[0]);
 }

 static void es1370_maybe_lower_irq (ES1370State *s, uint32_t sctl)
@@ -915,8 +909,7 @@ static void es1370_adc_callback (void *opaque, int avail)
 static void es1370_map (PCIDevice *pci_dev, int region_num,
                         uint32_t addr, uint32_t size, int type)
 {
-    PCIES1370State *d = (PCIES1370State *) pci_dev;
-    ES1370State *s = &d->es1370;
+    ES1370State *s = DO_UPCAST(ES1370State, dev, pci_dev);

     (void) region_num;
     (void) size;
@@ -936,7 +929,7 @@ static void es1370_save (QEMUFile *f, void *opaque)
     ES1370State *s = opaque;
     size_t i;

-    pci_device_save (s->pci_dev, f);
+    pci_device_save (&s->dev, f);
     for (i = 0; i < NB_CHANNELS; ++i) {
         struct chan *d = &s->chan[i];
         qemu_put_be32s (f, &d->shift);
@@ -962,7 +955,7 @@ static int es1370_load (QEMUFile *f, void *opaque, int version_id)
     if (version_id != 2)
         return -EINVAL;

-    ret = pci_device_load (s->pci_dev, f);
+    ret = pci_device_load (&s->dev, f);
     if (ret)
         return ret;

@@ -1007,9 +1000,8 @@ static void es1370_on_reset (void *opaque)

 static void es1370_initfn(PCIDevice *dev)
 {
-    PCIES1370State *d = DO_UPCAST(PCIES1370State, dev, dev);
-    ES1370State *s = &d->es1370;
-    uint8_t *c = d->dev.config;
+    ES1370State *s = DO_UPCAST(ES1370State, dev, dev);
+    uint8_t *c = s->dev.config;

     pci_config_set_vendor_id (c, PCI_VENDOR_ID_ENSONIQ);
     pci_config_set_device_id (c, PCI_DEVICE_ID_ENSONIQ_ES1370);
@@ -1035,10 +1027,7 @@ static void es1370_initfn(PCIDevice *dev)
     c[0x3e] = 0x0c;
     c[0x3f] = 0x80;

-    s = &d->es1370;
-    s->pci_dev = &d->dev;
-
-    pci_register_bar (&d->dev, 0, 256, PCI_ADDRESS_SPACE_IO, es1370_map);
+    pci_register_bar (&s->dev, 0, 256, PCI_ADDRESS_SPACE_IO, es1370_map);
     register_savevm ("es1370", 0, 2, es1370_save, es1370_load, s);
     qemu_register_reset (es1370_on_reset, s);

@@ -1055,7 +1044,7 @@ int es1370_init (PCIBus *bus)
 static PCIDeviceInfo es1370_info = {
     .qdev.name    = "ES1370",
     .qdev.desc    = "ENSONIQ AudioPCI ES1370",
-    .qdev.size    = sizeof (PCIES1370State),
+    .qdev.size    = sizeof (ES1370State),
     .init         = es1370_initfn,
 };

-- 
1.6.2.5

[Qemu-devel] [PATCH 05/22] ne2000: remove casts from void *

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ne2000.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ne2000.c b/hw/ne2000.c
index b9c018a..1555fc3 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -650,7 +650,7 @@ static uint32_t ne2000_reset_ioport_read(void *opaque, uint32_t addr)

 static void ne2000_save(QEMUFile* f,void* opaque)
 {
-	NE2000State* s=(NE2000State*)opaque;
+	NE2000State* s = opaque;
         uint32_t tmp;

         if (s->pci_dev)
@@ -681,7 +681,7 @@ static void ne2000_save(QEMUFile* f,void* opaque)

 static int ne2000_load(QEMUFile* f,void* opaque,int version_id)
 {
-	NE2000State* s=(NE2000State*)opaque;
+	NE2000State* s = opaque;
         int ret;
         uint32_t tmp;

-- 
1.6.2.5

[Qemu-devel] [PATCH 06/22] ne2000: pci_dev has this very value with the right type

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ne2000.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/ne2000.c b/hw/ne2000.c
index 1555fc3..070afcc 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -820,7 +820,7 @@ static void pci_ne2000_init(PCIDevice *pci_dev)
                            PCI_ADDRESS_SPACE_IO, ne2000_map);
     s = &d->ne2000;
     s->irq = d->dev.irq[0];
-    s->pci_dev = (PCIDevice *)d;
+    s->pci_dev = pci_dev;
     qdev_get_macaddr(&d->dev.qdev, s->macaddr);
     ne2000_reset(s);
     s->vc = qdev_get_vlan_client(&d->dev.qdev,
-- 
1.6.2.5

[Qemu-devel] [PATCH 07/22] ne2000: Remove unneeded double indirection of PCINE2000State

[Juan Quintela]

Once there, do the right DO_UPCAST instead of cast.
Note that we maintain pci_dev field in this case, because there exist ne2000 isa cards.  They to diferentiate them is that isa ones don't set this link.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ne2000.c |   27 ++++++++++-----------------
 1 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/hw/ne2000.c b/hw/ne2000.c
index 070afcc..7ac4fd3 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -122,6 +122,7 @@
 #define NE2000_MEM_SIZE     NE2000_PMEM_END

 typedef struct NE2000State {
+    PCIDevice dev;
     uint8_t cmd;
     uint32_t start;
     uint32_t stop;
@@ -771,16 +772,10 @@ void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)
 /***********************************************************/
 /* PCI NE2000 definitions */

-typedef struct PCINE2000State {
-    PCIDevice dev;
-    NE2000State ne2000;
-} PCINE2000State;
-
 static void ne2000_map(PCIDevice *pci_dev, int region_num,
                        uint32_t addr, uint32_t size, int type)
 {
-    PCINE2000State *d = (PCINE2000State *)pci_dev;
-    NE2000State *s = &d->ne2000;
+    NE2000State *s = DO_UPCAST(NE2000State, dev, pci_dev);

     register_ioport_write(addr, 16, 1, ne2000_ioport_write, s);
     register_ioport_read(addr, 16, 1, ne2000_ioport_read, s);
@@ -805,25 +800,23 @@ static void ne2000_cleanup(VLANClientState *vc)

 static void pci_ne2000_init(PCIDevice *pci_dev)
 {
-    PCINE2000State *d = (PCINE2000State *)pci_dev;
-    NE2000State *s;
+    NE2000State *s = DO_UPCAST(NE2000State, dev, pci_dev);
     uint8_t *pci_conf;

-    pci_conf = d->dev.config;
+    pci_conf = s->dev.config;
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REALTEK);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REALTEK_8029);
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; // header_type
     pci_conf[0x3d] = 1; // interrupt pin 0

-    pci_register_bar(&d->dev, 0, 0x100,
+    pci_register_bar(&s->dev, 0, 0x100,
                            PCI_ADDRESS_SPACE_IO, ne2000_map);
-    s = &d->ne2000;
-    s->irq = d->dev.irq[0];
-    s->pci_dev = pci_dev;
-    qdev_get_macaddr(&d->dev.qdev, s->macaddr);
+    s->irq = s->dev.irq[0];
+    s->pci_dev = &s->dev;
+    qdev_get_macaddr(&s->dev.qdev, s->macaddr);
     ne2000_reset(s);
-    s->vc = qdev_get_vlan_client(&d->dev.qdev,
+    s->vc = qdev_get_vlan_client(&s->dev.qdev,
                                  ne2000_can_receive, ne2000_receive, NULL,
                                  ne2000_cleanup, s);

@@ -834,7 +827,7 @@ static void pci_ne2000_init(PCIDevice *pci_dev)

 static PCIDeviceInfo ne2000_info = {
     .qdev.name = "ne2k_pci",
-    .qdev.size = sizeof(PCINE2000State),
+    .qdev.size = sizeof(NE2000State),
     .init      = pci_ne2000_init,
 };

-- 
1.6.2.5

[Qemu-devel] [PATCH 08/22] ne2000: change pci_dev to is_pci

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ne2000.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/ne2000.c b/hw/ne2000.c
index 7ac4fd3..33477b6 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -142,7 +142,7 @@ typedef struct NE2000State {
     uint8_t mult[8]; /* multicast mask array */
     qemu_irq irq;
     int isa_io_base;
-    PCIDevice *pci_dev;
+    int is_pci;
     VLANClientState *vc;
     uint8_t macaddr[6];
     uint8_t mem[NE2000_MEM_SIZE];
@@ -654,8 +654,8 @@ static void ne2000_save(QEMUFile* f,void* opaque)
 	NE2000State* s = opaque;
         uint32_t tmp;

-        if (s->pci_dev)
-            pci_device_save(s->pci_dev, f);
+        if (s->is_pci)
+            pci_device_save(&s->dev, f);

         qemu_put_8s(f, &s->rxcr);

@@ -689,8 +689,8 @@ static int ne2000_load(QEMUFile* f,void* opaque,int version_id)
         if (version_id > 3)
             return -EINVAL;

-        if (s->pci_dev && version_id >= 3) {
-            ret = pci_device_load(s->pci_dev, f);
+        if (s->is_pci && version_id >= 3) {
+            ret = pci_device_load(&s->dev, f);
             if (ret < 0)
                 return ret;
         }
@@ -813,7 +813,7 @@ static void pci_ne2000_init(PCIDevice *pci_dev)
     pci_register_bar(&s->dev, 0, 0x100,
                            PCI_ADDRESS_SPACE_IO, ne2000_map);
     s->irq = s->dev.irq[0];
-    s->pci_dev = &s->dev;
+    s->is_pci = 1;
     qdev_get_macaddr(&s->dev.qdev, s->macaddr);
     ne2000_reset(s);
     s->vc = qdev_get_vlan_client(&s->dev.qdev,
-- 
1.6.2.5

[Qemu-devel] [PATCH 09/22] pci: remove casts from void *

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/pci.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 27eac04..319f2ed 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -75,7 +75,7 @@ static PCIBus *first_bus;

 static void pcibus_save(QEMUFile *f, void *opaque)
 {
-    PCIBus *bus = (PCIBus *)opaque;
+    PCIBus *bus = opaque;
     int i;

     qemu_put_be32(f, bus->nirq);
@@ -85,7 +85,7 @@ static void pcibus_save(QEMUFile *f, void *opaque)

 static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)
 {
-    PCIBus *bus = (PCIBus *)opaque;
+    PCIBus *bus = opaque;
     int i, nirq;

     if (version_id != 1)
@@ -106,7 +106,7 @@ static int  pcibus_load(QEMUFile *f, void *opaque, int version_id)

 static void pci_bus_reset(void *opaque)
 {
-    PCIBus *bus = (PCIBus *)opaque;
+    PCIBus *bus = opaque;
     int i;

     for (i = 0; i < bus->nirq; i++) {
@@ -624,7 +624,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
 /* 0 <= irq_num <= 3. level must be 0 or 1 */
 static void pci_set_irq(void *opaque, int irq_num, int level)
 {
-    PCIDevice *pci_dev = (PCIDevice *)opaque;
+    PCIDevice *pci_dev = opaque;
     PCIBus *bus;
     int change;

-- 
1.6.2.5

[Qemu-devel] [PATCH 10/22] rtl8139: Remove unneeded double indirection of PCIRTL8139State

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/rtl8139.c |   38 +++++++++++++-------------------------
 1 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index fcd6d95..a79b066 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -423,6 +423,7 @@ static void RTL8139TallyCounters_load(QEMUFile* f, RTL8139TallyCounters *tally_c
 static void RTL8139TallyCounters_save(QEMUFile* f, RTL8139TallyCounters *tally_counters);

 typedef struct RTL8139State {
+    PCIDevice dev;
     uint8_t phys[8]; /* mac address */
     uint8_t mult[8]; /* multicast mask array */

@@ -463,7 +464,6 @@ typedef struct RTL8139State {
     uint16_t CpCmd;
     uint8_t  TxThresh;

-    PCIDevice *pci_dev;
     VLANClientState *vc;
     uint8_t macaddr[6];
     int rtl8139_mmio_io_addr;
@@ -692,7 +692,7 @@ static void rtl8139_update_irq(RTL8139State *s)
     DEBUG_PRINT(("RTL8139: Set IRQ to %d (%04x %04x)\n",
        isr ? 1 : 0, s->IntrStatus, s->IntrMask));

-    qemu_set_irq(s->pci_dev->irq[0], (isr != 0));
+    qemu_set_irq(s->dev.irq[0], (isr != 0));
 }

 #define POLYNOMIAL 0x04c11db6
@@ -3121,7 +3121,7 @@ static void rtl8139_save(QEMUFile* f,void* opaque)
     RTL8139State* s=(RTL8139State*)opaque;
     unsigned int i;

-    pci_device_save(s->pci_dev, f);
+    pci_device_save(&s->dev, f);

     qemu_put_buffer(f, s->phys, 6);
     qemu_put_buffer(f, s->mult, 8);
@@ -3215,7 +3215,7 @@ static int rtl8139_load(QEMUFile* f,void* opaque,int version_id)
             return -EINVAL;

     if (version_id >= 3) {
-        ret = pci_device_load(s->pci_dev, f);
+        ret = pci_device_load(&s->dev, f);
         if (ret < 0)
             return ret;
     }
@@ -3323,16 +3323,10 @@ static int rtl8139_load(QEMUFile* f,void* opaque,int version_id)
 /***********************************************************/
 /* PCI RTL8139 definitions */

-typedef struct PCIRTL8139State {
-    PCIDevice dev;
-    RTL8139State rtl8139;
-} PCIRTL8139State;
-
 static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
                        uint32_t addr, uint32_t size, int type)
 {
-    PCIRTL8139State *d = (PCIRTL8139State *)pci_dev;
-    RTL8139State *s = &d->rtl8139;
+    RTL8139State *s = DO_UPCAST(RTL8139State, dev, pci_dev);

     cpu_register_physical_memory(addr + 0, 0x100, s->rtl8139_mmio_io_addr);
 }
@@ -3340,8 +3334,7 @@ static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,
 static void rtl8139_ioport_map(PCIDevice *pci_dev, int region_num,
                        uint32_t addr, uint32_t size, int type)
 {
-    PCIRTL8139State *d = (PCIRTL8139State *)pci_dev;
-    RTL8139State *s = &d->rtl8139;
+    RTL8139State *s = DO_UPCAST(RTL8139State, dev, pci_dev);

     register_ioport_write(addr, 0x100, 1, rtl8139_ioport_writeb, s);
     register_ioport_read( addr, 0x100, 1, rtl8139_ioport_readb,  s);
@@ -3437,8 +3430,7 @@ static void rtl8139_cleanup(VLANClientState *vc)

 static int pci_rtl8139_uninit(PCIDevice *dev)
 {
-    PCIRTL8139State *d = (PCIRTL8139State *)dev;
-    RTL8139State *s = &d->rtl8139;
+    RTL8139State *s = DO_UPCAST(RTL8139State, dev, dev);

     cpu_unregister_io_memory(s->rtl8139_mmio_io_addr);

@@ -3447,13 +3439,12 @@ static int pci_rtl8139_uninit(PCIDevice *dev)

 static void pci_rtl8139_init(PCIDevice *dev)
 {
-    PCIRTL8139State *d = (PCIRTL8139State *)dev;
-    RTL8139State *s;
+    RTL8139State * s = DO_UPCAST(RTL8139State, dev, dev);
     uint8_t *pci_conf;

-    d->dev.unregister = pci_rtl8139_uninit;
+    s->dev.unregister = pci_rtl8139_uninit;

-    pci_conf = d->dev.config;
+    pci_conf = s->dev.config;
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_REALTEK);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_REALTEK_8139);
     pci_conf[0x04] = 0x05; /* command = I/O space, Bus Master */
@@ -3463,19 +3454,16 @@ static void pci_rtl8139_init(PCIDevice *dev)
     pci_conf[0x3d] = 1;    /* interrupt pin 0 */
     pci_conf[0x34] = 0xdc;

-    s = &d->rtl8139;
-
     /* I/O handler for memory-mapped I/O */
     s->rtl8139_mmio_io_addr =
     cpu_register_io_memory(rtl8139_mmio_read, rtl8139_mmio_write, s);

-    pci_register_bar(&d->dev, 0, 0x100,
+    pci_register_bar(&s->dev, 0, 0x100,
                            PCI_ADDRESS_SPACE_IO,  rtl8139_ioport_map);

-    pci_register_bar(&d->dev, 1, 0x100,
+    pci_register_bar(&s->dev, 1, 0x100,
                            PCI_ADDRESS_SPACE_MEM, rtl8139_mmio_map);

-    s->pci_dev = (PCIDevice *)d;
     qdev_get_macaddr(&dev->qdev, s->macaddr);
     qemu_register_reset(rtl8139_reset, s);
     rtl8139_reset(s);
@@ -3501,7 +3489,7 @@ static void pci_rtl8139_init(PCIDevice *dev)

 static PCIDeviceInfo rtl8139_info = {
     .qdev.name = "rtl8139",
-    .qdev.size = sizeof(PCIRTL8139State),
+    .qdev.size = sizeof(RTL8139State),
     .init      = pci_rtl8139_init,
 };

-- 
1.6.2.5

[Qemu-devel] [PATCH 11/22] rtl8139: remove pointless cast from void *

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/rtl8139.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index a79b066..830ab88 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3118,7 +3118,7 @@ static uint32_t rtl8139_mmio_readl(void *opaque, target_phys_addr_t addr)

 static void rtl8139_save(QEMUFile* f,void* opaque)
 {
-    RTL8139State* s=(RTL8139State*)opaque;
+    RTL8139State* s = opaque;
     unsigned int i;

     pci_device_save(&s->dev, f);
@@ -3206,7 +3206,7 @@ static void rtl8139_save(QEMUFile* f,void* opaque)

 static int rtl8139_load(QEMUFile* f,void* opaque,int version_id)
 {
-    RTL8139State* s=(RTL8139State*)opaque;
+    RTL8139State* s = opaque;
     unsigned int i;
     int ret;

-- 
1.6.2.5

[Qemu-devel] [PATCH 12/22] lsi53c895a: remove pointless cast from void *

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/lsi53c895a.c |   38 +++++++++++++++++++-------------------
 1 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index f749a45..f8501d4 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -635,7 +635,7 @@ static int lsi_queue_tag(LSIState *s, uint32_t tag, uint32_t arg)
 static void lsi_command_complete(void *opaque, int reason, uint32_t tag,
                                  uint32_t arg)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     int out;

     out = (s->sstat1 & PHASE_MASK) == PHASE_DO;
@@ -1724,14 +1724,14 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val)

 static void lsi_mmio_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;

     lsi_reg_writeb(s, addr & 0xff, val);
 }

 static void lsi_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;

     addr &= 0xff;
     lsi_reg_writeb(s, addr, val & 0xff);
@@ -1740,7 +1740,7 @@ static void lsi_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)

 static void lsi_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;

     addr &= 0xff;
     lsi_reg_writeb(s, addr, val & 0xff);
@@ -1751,14 +1751,14 @@ static void lsi_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)

 static uint32_t lsi_mmio_readb(void *opaque, target_phys_addr_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;

     return lsi_reg_readb(s, addr & 0xff);
 }

 static uint32_t lsi_mmio_readw(void *opaque, target_phys_addr_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     uint32_t val;

     addr &= 0xff;
@@ -1769,7 +1769,7 @@ static uint32_t lsi_mmio_readw(void *opaque, target_phys_addr_t addr)

 static uint32_t lsi_mmio_readl(void *opaque, target_phys_addr_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     uint32_t val;
     addr &= 0xff;
     val = lsi_reg_readb(s, addr);
@@ -1793,7 +1793,7 @@ static CPUWriteMemoryFunc *lsi_mmio_writefn[3] = {

 static void lsi_ram_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     uint32_t newval;
     int shift;

@@ -1807,7 +1807,7 @@ static void lsi_ram_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)

 static void lsi_ram_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     uint32_t newval;

     addr &= 0x1fff;
@@ -1823,7 +1823,7 @@ static void lsi_ram_writew(void *opaque, target_phys_addr_t addr, uint32_t val)

 static void lsi_ram_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;

     addr &= 0x1fff;
     s->script_ram[addr >> 2] = val;
@@ -1831,7 +1831,7 @@ static void lsi_ram_writel(void *opaque, target_phys_addr_t addr, uint32_t val)

 static uint32_t lsi_ram_readb(void *opaque, target_phys_addr_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     uint32_t val;

     addr &= 0x1fff;
@@ -1842,7 +1842,7 @@ static uint32_t lsi_ram_readb(void *opaque, target_phys_addr_t addr)

 static uint32_t lsi_ram_readw(void *opaque, target_phys_addr_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     uint32_t val;

     addr &= 0x1fff;
@@ -1854,7 +1854,7 @@ static uint32_t lsi_ram_readw(void *opaque, target_phys_addr_t addr)

 static uint32_t lsi_ram_readl(void *opaque, target_phys_addr_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;

     addr &= 0x1fff;
     return le32_to_cpu(s->script_ram[addr >> 2]);
@@ -1874,13 +1874,13 @@ static CPUWriteMemoryFunc *lsi_ram_writefn[3] = {

 static uint32_t lsi_io_readb(void *opaque, uint32_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     return lsi_reg_readb(s, addr & 0xff);
 }

 static uint32_t lsi_io_readw(void *opaque, uint32_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     uint32_t val;
     addr &= 0xff;
     val = lsi_reg_readb(s, addr);
@@ -1890,7 +1890,7 @@ static uint32_t lsi_io_readw(void *opaque, uint32_t addr)

 static uint32_t lsi_io_readl(void *opaque, uint32_t addr)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     uint32_t val;
     addr &= 0xff;
     val = lsi_reg_readb(s, addr);
@@ -1902,13 +1902,13 @@ static uint32_t lsi_io_readl(void *opaque, uint32_t addr)

 static void lsi_io_writeb(void *opaque, uint32_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     lsi_reg_writeb(s, addr & 0xff, val);
 }

 static void lsi_io_writew(void *opaque, uint32_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     addr &= 0xff;
     lsi_reg_writeb(s, addr, val & 0xff);
     lsi_reg_writeb(s, addr + 1, (val >> 8) & 0xff);
@@ -1916,7 +1916,7 @@ static void lsi_io_writew(void *opaque, uint32_t addr, uint32_t val)

 static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)
 {
-    LSIState *s = (LSIState *)opaque;
+    LSIState *s = opaque;
     addr &= 0xff;
     lsi_reg_writeb(s, addr, val & 0xff);
     lsi_reg_writeb(s, addr + 1, (val >> 8) & 0xff);
-- 
1.6.2.5

[Qemu-devel] [PATCH 13/22] lsi53c895a: use DO_UPCAST to cast from PCIDevice

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/lsi53c895a.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index f8501d4..7ebf452 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -1927,7 +1927,7 @@ static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)
 static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
                            uint32_t addr, uint32_t size, int type)
 {
-    LSIState *s = (LSIState *)pci_dev;
+    LSIState *s = DO_UPCAST(LSIState, pci_dev, pci_dev);

     DPRINTF("Mapping IO at %08x\n", addr);

@@ -1942,7 +1942,7 @@ static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
 static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
                             uint32_t addr, uint32_t size, int type)
 {
-    LSIState *s = (LSIState *)pci_dev;
+    LSIState *s = DO_UPCAST(LSIState, pci_dev, pci_dev);

     DPRINTF("Mapping ram at %08x\n", addr);
     s->script_ram_base = addr;
@@ -1952,7 +1952,7 @@ static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
 static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
                              uint32_t addr, uint32_t size, int type)
 {
-    LSIState *s = (LSIState *)pci_dev;
+    LSIState *s = DO_UPCAST(LSIState, pci_dev, pci_dev);

     DPRINTF("Mapping registers at %08x\n", addr);
     cpu_register_physical_memory(addr + 0, 0x400, s->mmio_io_addr);
@@ -2153,7 +2153,7 @@ static int lsi_scsi_load(QEMUFile *f, void *opaque, int version_id)

 static int lsi_scsi_uninit(PCIDevice *d)
 {
-    LSIState *s = (LSIState *) d;
+    LSIState *s = DO_UPCAST(LSIState, pci_dev, d);

     cpu_unregister_io_memory(s->mmio_io_addr);
     cpu_unregister_io_memory(s->ram_io_addr);
@@ -2165,7 +2165,7 @@ static int lsi_scsi_uninit(PCIDevice *d)

 static void lsi_scsi_init(PCIDevice *dev)
 {
-    LSIState *s = (LSIState *)dev;
+    LSIState *s = DO_UPCAST(LSIState, pci_dev, dev);
     uint8_t *pci_conf;

     pci_conf = s->pci_dev.config;
-- 
1.6.2.5

[Qemu-devel] [PATCH 14/22] lsi53c895a: rename PCIDevice field from pci_dev to dev (consistence)

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/lsi53c895a.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index 7ebf452..d154b23 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -175,7 +175,7 @@ typedef struct {
 } lsi_queue;

 typedef struct {
-    PCIDevice pci_dev;
+    PCIDevice dev;
     int mmio_io_addr;
     int ram_io_addr;
     uint32_t script_ram_base;
@@ -410,7 +410,7 @@ static void lsi_update_irq(LSIState *s)
                 level, s->dstat, s->sist1, s->sist0);
         last_level = level;
     }
-    qemu_set_irq(s->pci_dev.irq[0], level);
+    qemu_set_irq(s->dev.irq[0], level);
 }

 /* Stop SCRIPTS execution and raise a SCSI interrupt.  */
@@ -1927,7 +1927,7 @@ static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)
 static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
                            uint32_t addr, uint32_t size, int type)
 {
-    LSIState *s = DO_UPCAST(LSIState, pci_dev, pci_dev);
+    LSIState *s = DO_UPCAST(LSIState, dev, pci_dev);

     DPRINTF("Mapping IO at %08x\n", addr);

@@ -1942,7 +1942,7 @@ static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,
 static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
                             uint32_t addr, uint32_t size, int type)
 {
-    LSIState *s = DO_UPCAST(LSIState, pci_dev, pci_dev);
+    LSIState *s = DO_UPCAST(LSIState, dev, pci_dev);

     DPRINTF("Mapping ram at %08x\n", addr);
     s->script_ram_base = addr;
@@ -1952,7 +1952,7 @@ static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,
 static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,
                              uint32_t addr, uint32_t size, int type)
 {
-    LSIState *s = DO_UPCAST(LSIState, pci_dev, pci_dev);
+    LSIState *s = DO_UPCAST(LSIState, dev, pci_dev);

     DPRINTF("Mapping registers at %08x\n", addr);
     cpu_register_physical_memory(addr + 0, 0x400, s->mmio_io_addr);
@@ -1980,7 +1980,7 @@ void lsi_scsi_attach(DeviceState *host, BlockDriverState *bd, int id)
     s->scsi_dev[id] = scsi_generic_init(bd, 1, lsi_command_complete, s);
     if (s->scsi_dev[id] == NULL)
         s->scsi_dev[id] = scsi_disk_init(bd, 1, lsi_command_complete, s);
-    bd->private = &s->pci_dev;
+    bd->private = &s->dev;
 }

 static void lsi_scsi_save(QEMUFile *f, void *opaque)
@@ -1991,7 +1991,7 @@ static void lsi_scsi_save(QEMUFile *f, void *opaque)
     assert(s->current_dma_len == 0);
     assert(s->active_commands == 0);

-    pci_device_save(&s->pci_dev, f);
+    pci_device_save(&s->dev, f);

     qemu_put_sbe32s(f, &s->carry);
     qemu_put_sbe32s(f, &s->sense);
@@ -2074,7 +2074,7 @@ static int lsi_scsi_load(QEMUFile *f, void *opaque, int version_id)
         return -EINVAL;
     }

-    if ((ret = pci_device_load(&s->pci_dev, f)) < 0)
+    if ((ret = pci_device_load(&s->dev, f)) < 0)
         return ret;

     qemu_get_sbe32s(f, &s->carry);
@@ -2153,7 +2153,7 @@ static int lsi_scsi_load(QEMUFile *f, void *opaque, int version_id)

 static int lsi_scsi_uninit(PCIDevice *d)
 {
-    LSIState *s = DO_UPCAST(LSIState, pci_dev, d);
+    LSIState *s = DO_UPCAST(LSIState, dev, d);

     cpu_unregister_io_memory(s->mmio_io_addr);
     cpu_unregister_io_memory(s->ram_io_addr);
@@ -2165,10 +2165,10 @@ static int lsi_scsi_uninit(PCIDevice *d)

 static void lsi_scsi_init(PCIDevice *dev)
 {
-    LSIState *s = DO_UPCAST(LSIState, pci_dev, dev);
+    LSIState *s = DO_UPCAST(LSIState, dev, dev);
     uint8_t *pci_conf;

-    pci_conf = s->pci_dev.config;
+    pci_conf = s->dev.config;

     /* PCI Vendor ID (word) */
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_LSI_LOGIC);
@@ -2198,7 +2198,7 @@ static void lsi_scsi_init(PCIDevice *dev)
     s->queue = qemu_malloc(sizeof(lsi_queue));
     s->queue_len = 1;
     s->active_commands = 0;
-    s->pci_dev.unregister = lsi_scsi_uninit;
+    s->dev.unregister = lsi_scsi_uninit;

     lsi_soft_reset(s);

-- 
1.6.2.5

[Qemu-devel] [PATCH 15/22] lsi53c895a: LSIState is a PCIDevice is a DeviceHost

[Juan Quintela]

Go figure.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/lsi53c895a.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index d154b23..2db3245 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -1960,7 +1960,8 @@ static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,

 void lsi_scsi_attach(DeviceState *host, BlockDriverState *bd, int id)
 {
-    LSIState *s = (LSIState *)host;
+    PCIDevice *d = DO_UPCAST(PCIDevice, qdev, host);
+    LSIState *s = DO_UPCAST(LSIState, dev, d);

     if (id < 0) {
         for (id = 0; id < LSI_MAX_DEVS; id++) {
-- 
1.6.2.5

[Qemu-devel] [PATCH 16/22] usb-ohci: Remove unneeded double indirection of OHCIPCIState

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/usb-ohci.c |   34 ++++++++++++++--------------------
 1 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/hw/usb-ohci.c b/hw/usb-ohci.c
index 4c42ec0..14139b8 100644
--- a/hw/usb-ohci.c
+++ b/hw/usb-ohci.c
@@ -65,6 +65,7 @@ enum ohci_type {
 };

 typedef struct {
+    PCIDevice dev;
     qemu_irq irq;
     enum ohci_type type;
     int mem;
@@ -1698,41 +1699,34 @@ static void usb_ohci_init(OHCIState *ohci, int num_ports, int devfn,
     ohci_reset(ohci);
 }

-typedef struct {
-    PCIDevice pci_dev;
-    OHCIState state;
-} OHCIPCIState;
-
 static void ohci_mapfunc(PCIDevice *pci_dev, int i,
             uint32_t addr, uint32_t size, int type)
 {
-    OHCIPCIState *ohci = (OHCIPCIState *)pci_dev;
-    cpu_register_physical_memory(addr, size, ohci->state.mem);
+    OHCIState *ohci = DO_UPCAST(OHCIState, dev, pci_dev);
+    cpu_register_physical_memory(addr, size, ohci->mem);
 }

 void usb_ohci_init_pci(struct PCIBus *bus, int num_ports, int devfn)
 {
-    OHCIPCIState *ohci;
+    PCIDevice *pci_dev = pci_register_device(bus, "OHCI USB", sizeof(OHCIState),
+                                             devfn, NULL, NULL);
+    OHCIState *ohci = DO_UPCAST(OHCIState, dev, pci_dev);

-    ohci = (OHCIPCIState *)pci_register_device(bus, "OHCI USB", sizeof(*ohci),
-                                               devfn, NULL, NULL);
     if (ohci == NULL) {
         fprintf(stderr, "usb-ohci: Failed to register PCI device\n");
         return;
     }

-    pci_config_set_vendor_id(ohci->pci_dev.config, PCI_VENDOR_ID_APPLE);
-    pci_config_set_device_id(ohci->pci_dev.config,
-                             PCI_DEVICE_ID_APPLE_IPID_USB);
-    ohci->pci_dev.config[0x09] = 0x10; /* OHCI */
-    pci_config_set_class(ohci->pci_dev.config, PCI_CLASS_SERIAL_USB);
-    ohci->pci_dev.config[0x3d] = 0x01; /* interrupt pin 1 */
+    pci_config_set_vendor_id(ohci->dev.config, PCI_VENDOR_ID_APPLE);
+    pci_config_set_device_id(ohci->dev.config, PCI_DEVICE_ID_APPLE_IPID_USB);
+    ohci->dev.config[0x09] = 0x10; /* OHCI */
+    pci_config_set_class(ohci->dev.config, PCI_CLASS_SERIAL_USB);
+    ohci->dev.config[0x3d] = 0x01; /* interrupt pin 1 */

-    usb_ohci_init(&ohci->state, num_ports, devfn, ohci->pci_dev.irq[0],
-                  OHCI_TYPE_PCI, ohci->pci_dev.name, 0);
+    usb_ohci_init(ohci, num_ports, devfn, ohci->dev.irq[0],
+                  OHCI_TYPE_PCI, ohci->dev.name, 0);

-    pci_register_bar((struct PCIDevice *)ohci, 0, 256,
-                           PCI_ADDRESS_SPACE_MEM, ohci_mapfunc);
+    pci_register_bar(pci_dev, 0, 256, PCI_ADDRESS_SPACE_MEM, ohci_mapfunc);
 }

 void usb_ohci_init_pxa(target_phys_addr_t base, int num_ports, int devfn,
-- 
1.6.2.5

[Qemu-devel] [PATCH 17/22] cirrus_vga: Remove unneeded double indirection of PCICirrusVGAState

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/cirrus_vga.c |   30 ++++++++++++------------------
 1 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 95d822a..6f0fb91 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -238,6 +238,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s,
                               uint8_t *dst, int dst_pitch, int width, int height);

 typedef struct CirrusVGAState {
+    PCIDevice dev;
     VGACommonState vga;

     int cirrus_linear_io_addr;
@@ -282,11 +283,6 @@ typedef struct CirrusVGAState {
     int bustype;
 } CirrusVGAState;

-typedef struct PCICirrusVGAState {
-    PCIDevice dev;
-    CirrusVGAState cirrus_vga;
-} PCICirrusVGAState;
-
 static uint8_t rop_to_index[256];

 /***************************************
@@ -3125,7 +3121,7 @@ static void cirrus_reset(void *opaque)
 {
     CirrusVGAState *s = opaque;

-    vga_reset(s);
+    vga_reset(&s->vga);
     unmap_linear_vram(s);
     s->vga.sr[0x06] = 0x0f;
     if (s->device_id == CIRRUS_ID_CLGD5446) {
@@ -3263,7 +3259,7 @@ void isa_cirrus_vga_init(void)
 static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
 			       uint32_t addr, uint32_t size, int type)
 {
-    CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
+    CirrusVGAState *s = DO_UPCAST(CirrusVGAState, dev, d);

     /* XXX: add byte swapping apertures */
     cpu_register_physical_memory(addr, s->vga.vram_size,
@@ -3284,7 +3280,7 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
 static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
 				uint32_t addr, uint32_t size, int type)
 {
-    CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
+    CirrusVGAState *s = DO_UPCAST(CirrusVGAState, dev, d);

     cpu_register_physical_memory(addr, CIRRUS_PNPMMIO_SIZE,
 				 s->cirrus_mmio_io_addr);
@@ -3293,26 +3289,24 @@ static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,
 static void pci_cirrus_write_config(PCIDevice *d,
                                     uint32_t address, uint32_t val, int len)
 {
-    PCICirrusVGAState *pvs = container_of(d, PCICirrusVGAState, dev);
-    CirrusVGAState *s = &pvs->cirrus_vga;
+    CirrusVGAState *s = DO_UPCAST(CirrusVGAState, dev, d);

     pci_default_write_config(d, address, val, len);
-    if (s->vga.map_addr && pvs->dev.io_regions[0].addr == -1)
+    if (s->vga.map_addr && s->dev.io_regions[0].addr == -1)
         s->vga.map_addr = 0;
     cirrus_update_memory_access(s);
 }

 static void pci_cirrus_vga_initfn(PCIDevice *dev)
 {
-     PCICirrusVGAState *d = DO_UPCAST(PCICirrusVGAState, dev, dev);
-     CirrusVGAState *s = &d->cirrus_vga;
-     uint8_t *pci_conf = d->dev.config;
+     CirrusVGAState *s = DO_UPCAST(CirrusVGAState, dev, dev);
+     uint8_t *pci_conf = s->dev.config;
      int device_id = CIRRUS_ID_CLGD5446;

      /* setup VGA */
      vga_common_init(&s->vga, VGA_RAM_SIZE);
      cirrus_init_common(s, device_id, 1);
-     s->vga.pci_dev = (PCIDevice *)d;
+     s->vga.pci_dev = dev;
      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
                                       s->vga.screen_dump, s->vga.text_update,
                                       &s->vga);
@@ -3328,10 +3322,10 @@ static void pci_cirrus_vga_initfn(PCIDevice *dev)
      /* memory #0 LFB */
      /* memory #1 memory-mapped I/O */
      /* XXX: s->vga.vram_size must be a power of two */
-     pci_register_bar((PCIDevice *)d, 0, 0x2000000,
+     pci_register_bar(&s->dev, 0, 0x2000000,
                       PCI_ADDRESS_SPACE_MEM_PREFETCH, cirrus_pci_lfb_map);
      if (device_id == CIRRUS_ID_CLGD5446) {
-         pci_register_bar((PCIDevice *)d, 1, CIRRUS_PNPMMIO_SIZE,
+         pci_register_bar(&s->dev, 1, CIRRUS_PNPMMIO_SIZE,
                           PCI_ADDRESS_SPACE_MEM, cirrus_pci_mmio_map);
      }
      /* XXX: ROM BIOS */
@@ -3344,7 +3338,7 @@ void pci_cirrus_vga_init(PCIBus *bus)

 static PCIDeviceInfo cirrus_vga_info = {
     .qdev.name    = "Cirrus VGA",
-    .qdev.size    = sizeof(PCICirrusVGAState),
+    .qdev.size    = sizeof(CirrusVGAState),
     .init         = pci_cirrus_vga_initfn,
     .config_write = pci_cirrus_write_config,
 };
-- 
1.6.2.5

[Qemu-devel] [PATCH 18/22] cirrus_vga: remove pointless cast from void *

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/cirrus_vga.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 6f0fb91..a40f300 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2342,7 +2342,7 @@ static void cirrus_cursor_draw_line(VGAState *s1, uint8_t *d1, int scr_y)

 static uint32_t cirrus_linear_readb(void *opaque, target_phys_addr_t addr)
 {
-    CirrusVGAState *s = (CirrusVGAState *) opaque;
+    CirrusVGAState *s = opaque;
     uint32_t ret;

     addr &= s->cirrus_addr_mask;
@@ -2401,7 +2401,7 @@ static uint32_t cirrus_linear_readl(void *opaque, target_phys_addr_t addr)
 static void cirrus_linear_writeb(void *opaque, target_phys_addr_t addr,
 				 uint32_t val)
 {
-    CirrusVGAState *s = (CirrusVGAState *) opaque;
+    CirrusVGAState *s = opaque;
     unsigned mode;

     addr &= s->cirrus_addr_mask;
@@ -2529,7 +2529,7 @@ static uint32_t cirrus_linear_bitblt_readl(void *opaque, target_phys_addr_t addr
 static void cirrus_linear_bitblt_writeb(void *opaque, target_phys_addr_t addr,
 				 uint32_t val)
 {
-    CirrusVGAState *s = (CirrusVGAState *) opaque;
+    CirrusVGAState *s = opaque;

     if (s->cirrus_srcptr != s->cirrus_srcptr_end) {
 	/* bitblt */
@@ -2915,7 +2915,7 @@ static void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)

 static uint32_t cirrus_mmio_readb(void *opaque, target_phys_addr_t addr)
 {
-    CirrusVGAState *s = (CirrusVGAState *) opaque;
+    CirrusVGAState *s = opaque;

     addr &= CIRRUS_PNPMMIO_SIZE - 1;

@@ -2959,7 +2959,7 @@ static uint32_t cirrus_mmio_readl(void *opaque, target_phys_addr_t addr)
 static void cirrus_mmio_writeb(void *opaque, target_phys_addr_t addr,
 			       uint32_t val)
 {
-    CirrusVGAState *s = (CirrusVGAState *) opaque;
+    CirrusVGAState *s = opaque;

     addr &= CIRRUS_PNPMMIO_SIZE - 1;

-- 
1.6.2.5

[Qemu-devel] [PATCH 19/22] cirrus_vga: change use of pci_dev for is_pci

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/cirrus_vga.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index a40f300..93cc1ff 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -241,6 +241,7 @@ typedef struct CirrusVGAState {
     PCIDevice dev;
     VGACommonState vga;

+    int is_pci;
     int cirrus_linear_io_addr;
     int cirrus_linear_bitblt_io_addr;
     int cirrus_mmio_io_addr;
@@ -3017,8 +3018,8 @@ static void cirrus_vga_save(QEMUFile *f, void *opaque)
 {
     CirrusVGAState *s = opaque;

-    if (s->vga.pci_dev)
-        pci_device_save(s->vga.pci_dev, f);
+    if (s->is_pci)
+        pci_device_save(&s->dev, f);

     qemu_put_be32s(f, &s->vga.latch);
     qemu_put_8s(f, &s->vga.sr_index);
@@ -3063,8 +3064,8 @@ static int cirrus_vga_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id > 2)
         return -EINVAL;

-    if (s->vga.pci_dev && version_id >= 2) {
-        ret = pci_device_load(s->vga.pci_dev, f);
+    if (s->is_pci && version_id >= 2) {
+        ret = pci_device_load(&s->dev, f);
         if (ret < 0)
             return ret;
     }
@@ -3306,7 +3307,7 @@ static void pci_cirrus_vga_initfn(PCIDevice *dev)
      /* setup VGA */
      vga_common_init(&s->vga, VGA_RAM_SIZE);
      cirrus_init_common(s, device_id, 1);
-     s->vga.pci_dev = dev;
+     s->is_pci = 1;
      s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
                                       s->vga.screen_dump, s->vga.text_update,
                                       &s->vga);
-- 
1.6.2.5

[Qemu-devel] [PATCH 20/22] Introduce vga_common_reset() to be able to typcheck vga_reset()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/cirrus_vga.c |    2 +-
 hw/vga.c        |   10 +++++++---
 hw/vga_int.h    |    2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 93cc1ff..63bef22 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3122,7 +3122,7 @@ static void cirrus_reset(void *opaque)
 {
     CirrusVGAState *s = opaque;

-    vga_reset(&s->vga);
+    vga_common_reset(&s->vga);
     unmap_linear_vram(s);
     s->vga.sr[0x06] = 0x0f;
     if (s->device_id == CIRRUS_ID_CLGD5446) {
diff --git a/hw/vga.c b/hw/vga.c
index 4a0f197..0ff0216 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1871,10 +1871,8 @@ static void vga_invalidate_display(void *opaque)
     s->full_update = 1;
 }

-void vga_reset(void *opaque)
+void vga_common_reset(VGACommonState *s)
 {
-    VGAState *s = (VGAState *) opaque;
-
     s->lfb_addr = 0;
     s->lfb_end = 0;
     s->map_addr = 0;
@@ -1940,6 +1938,12 @@ void vga_reset(void *opaque)
     }
 }

+static void vga_reset(void *opaque)
+{
+    VGAState *s = (VGAState *) opaque;
+    vga_common_reset(s);
+}
+
 #define TEXTMODE_X(x)	((x) % width)
 #define TEXTMODE_Y(x)	((x) / width)
 #define VMEM2CHTYPE(v)	((v & 0xff0007ff) | \
diff --git a/hw/vga_int.h b/hw/vga_int.h
index eb2d6ea..bb28872 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -192,7 +192,7 @@ static inline int c6_to_8(int v)

 void vga_common_init(VGAState *s, int vga_ram_size);
 void vga_init(VGAState *s);
-void vga_reset(void *s);
+void vga_common_reset(VGACommonState *s);

 void vga_dirty_log_start(VGAState *s);

-- 
1.6.2.5

[Qemu-devel] [PATCH 21/22] vga: Rename vga_state -> vga

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/vga.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index 0ff0216..82f199d 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2225,7 +2225,7 @@ static int vga_load(QEMUFile *f, void *opaque, int version_id)

 typedef struct PCIVGAState {
     PCIDevice dev;
-    VGAState vga_state;
+    VGAState vga;
 } PCIVGAState;

 void vga_dirty_log_start(VGAState *s)
@@ -2243,7 +2243,7 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
                     uint32_t addr, uint32_t size, int type)
 {
     PCIVGAState *d = (PCIVGAState *)pci_dev;
-    VGAState *s = &d->vga_state;
+    VGAState *s = &d->vga;
     if (region_num == PCI_ROM_SLOT) {
         cpu_register_physical_memory(addr, s->bios_size, s->bios_offset);
     } else {
@@ -2477,7 +2477,7 @@ static void pci_vga_write_config(PCIDevice *d,
                                  uint32_t address, uint32_t val, int len)
 {
     PCIVGAState *pvs = container_of(d, PCIVGAState, dev);
-    VGAState *s = &pvs->vga_state;
+    VGAState *s = &pvs->vga;

     pci_default_write_config(d, address, val, len);
     if (s->map_addr && pvs->dev.io_regions[0].addr == -1)
@@ -2487,7 +2487,7 @@ static void pci_vga_write_config(PCIDevice *d,
 static void pci_vga_initfn(PCIDevice *dev)
 {
      PCIVGAState *d = DO_UPCAST(PCIVGAState, dev, dev);
-     VGAState *s = &d->vga_state;
+     VGAState *s = &d->vga;
      uint8_t *pci_conf = d->dev.config;

      // vga + console init
@@ -2537,8 +2537,8 @@ static PCIDeviceInfo vga_info = {
     .init         = pci_vga_initfn,
     .config_write = pci_vga_write_config,
     .qdev.props   = (Property[]) {
-        DEFINE_PROP_HEX32("bios-offset", PCIVGAState, vga_state.bios_offset, 0),
-        DEFINE_PROP_HEX32("bios-size",   PCIVGAState, vga_state.bios_size,   0),
+        DEFINE_PROP_HEX32("bios-offset", PCIVGAState, vga.bios_offset, 0),
+        DEFINE_PROP_HEX32("bios-size",   PCIVGAState, vga.bios_size,   0),
         DEFINE_PROP_END_OF_LIST(),
     }
 };
-- 
1.6.2.5

[Qemu-devel] [PATCH 22/22] Everything outside of vga.c should use VGACommonState

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/cirrus_vga.c |   10 +++++-----
 hw/vga.c        |    4 +++-
 hw/vga_int.h    |   10 ++++------
 hw/vmware_vga.c |    4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 63bef22..d9cc915 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -1075,7 +1075,7 @@ static void cirrus_write_bitblt(CirrusVGAState * s, unsigned reg_value)
  *
  ***************************************/

-static void cirrus_get_offsets(VGAState *s1,
+static void cirrus_get_offsets(VGACommonState *s1,
                                uint32_t *pline_offset,
                                uint32_t *pstart_addr,
                                uint32_t *pline_compare)
@@ -1123,7 +1123,7 @@ static uint32_t cirrus_get_bpp16_depth(CirrusVGAState * s)
     return ret;
 }

-static int cirrus_get_bpp(VGAState *s1)
+static int cirrus_get_bpp(VGACommonState *s1)
 {
     CirrusVGAState * s = container_of(s1, CirrusVGAState, vga);
     uint32_t ret = 8;
@@ -1161,7 +1161,7 @@ static int cirrus_get_bpp(VGAState *s1)
     return ret;
 }

-static void cirrus_get_resolution(VGAState *s, int *pwidth, int *pheight)
+static void cirrus_get_resolution(VGACommonState *s, int *pwidth, int *pheight)
 {
     int width, height;

@@ -2231,7 +2231,7 @@ static inline void cirrus_cursor_compute_yrange(CirrusVGAState *s)

 /* NOTE: we do not currently handle the cursor bitmap change, so we
    update the cursor only if it moves. */
-static void cirrus_cursor_invalidate(VGAState *s1)
+static void cirrus_cursor_invalidate(VGACommonState *s1)
 {
     CirrusVGAState *s = container_of(s1, CirrusVGAState, vga);
     int size;
@@ -2260,7 +2260,7 @@ static void cirrus_cursor_invalidate(VGAState *s1)
     }
 }

-static void cirrus_cursor_draw_line(VGAState *s1, uint8_t *d1, int scr_y)
+static void cirrus_cursor_draw_line(VGACommonState *s1, uint8_t *d1, int scr_y)
 {
     CirrusVGAState *s = container_of(s1, CirrusVGAState, vga);
     int w, h, bpp, x1, x2, poffset;
diff --git a/hw/vga.c b/hw/vga.c
index 82f199d..0385614 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -149,6 +149,8 @@ static uint32_t expand4[256];
 static uint16_t expand2[256];
 static uint8_t expand4to8[16];

+typedef VGACommonState VGAState;
+
 static void vga_screen_dump(void *opaque, const char *filename);

 static void vga_dumb_update_retrace_info(VGAState *s)
@@ -2254,7 +2256,7 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
     }
 }

-void vga_common_init(VGAState *s, int vga_ram_size)
+void vga_common_init(VGACommonState *s, int vga_ram_size)
 {
     int i, j, v, b;

diff --git a/hw/vga_int.h b/hw/vga_int.h
index bb28872..eb837ff 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -180,8 +180,6 @@ typedef struct VGACommonState {
     union vga_retrace retrace_info;
 } VGACommonState;

-typedef VGACommonState VGAState;
-
 static inline int c6_to_8(int v)
 {
     int b;
@@ -190,15 +188,15 @@ static inline int c6_to_8(int v)
     return (v << 2) | (b << 1) | b;
 }

-void vga_common_init(VGAState *s, int vga_ram_size);
-void vga_init(VGAState *s);
+void vga_common_init(VGACommonState *s, int vga_ram_size);
+void vga_init(VGACommonState *s);
 void vga_common_reset(VGACommonState *s);

-void vga_dirty_log_start(VGAState *s);
+void vga_dirty_log_start(VGACommonState *s);

 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);
 void vga_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val);
-void vga_invalidate_scanlines(VGAState *s, int y1, int y2);
+void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
 int ppm_save(const char *filename, struct DisplaySurface *ds);

 void vga_draw_cursor_line_8(uint8_t *d1, const uint8_t *src1,
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index 5ceebf1..0d8a72f 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1129,8 +1129,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
     vmsvga_reset(s);

 #ifdef EMBED_STDVGA
-    vga_common_init((VGAState *) s, vga_ram_size);
-    vga_init((VGAState *) s);
+    vga_common_init(&s->vga, vga_ram_size);
+    vga_init(&s->vga);
 #else
     s->vram_size = vga_ram_size;
     s->vram_offset = qemu_ram_alloc(vga_ram_size);
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 00/22] Indirection Cleanup

[Gerd Hoffmann]

On 08/24/09 13:03, Juan Quintela wrote:
> This patch series clean up "half" converted qemu drivers that had changed from:
>
> struct FOOState
>
> to
>
> typedef PCIFOOState {
>      PCIDevice dev;
>      FOOState foo;
> } PCIFOOState;
>
> It just moves PCIDevice to be the 1st field of FOOState.

Note that there are a bunch of cases where it actually makes sense to 
have PCIFOOState because foo can be connected to different busses and 
thus there is a ISAFOOState (or will be once ISA is qdevified too).

Disclaimer: did't look through the patches yet, just scanned the 
subjects.  But I've seen ne2000 + vga on the list.  For both devices ISA 
variants exist.  Likewise OHCI, there are also non-PCI variants.

> - pcnet:  It needs a different approach, because it can be both a PCIDevice
>    or a SysBus device.

That one too ;)
Why do you want to change it?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 07/22] ne2000: Remove unneeded double indirection of PCINE2000State

[Gerd Hoffmann]

>   typedef struct NE2000State {
> +    PCIDevice dev;
>       uint8_t cmd;

> -typedef struct PCINE2000State {
> -    PCIDevice dev;
> -    NE2000State ne2000;
> -} PCINE2000State;

NACK.

We'll have a ...

typedef struct ISANE2000State {
    ISADevice dev;
    NE2000State ne2000;
} ISANE2000State;

... some day.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 15/22] lsi53c895a: LSIState is a PCIDevice is a DeviceHost

[Gerd Hoffmann]

>   void lsi_scsi_attach(DeviceState *host, BlockDriverState *bd, int id)
>   {
> -    LSIState *s = (LSIState *)host;
> +    PCIDevice *d = DO_UPCAST(PCIDevice, qdev, host);
> +    LSIState *s = DO_UPCAST(LSIState, dev, d);

LSIState *s = DO_UPCAST(LSIState, dev.qdev, host);

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 16/22] usb-ohci: Remove unneeded double indirection of OHCIPCIState

[Gerd Hoffmann]

> --- a/hw/usb-ohci.c
> +++ b/hw/usb-ohci.c

NACK.  There are non-pci OHCI users.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 07/22] ne2000: Remove unneeded double indirection of PCINE2000State

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
>>   typedef struct NE2000State {
>> +    PCIDevice dev;
>>       uint8_t cmd;
>
>> -typedef struct PCINE2000State {
>> -    PCIDevice dev;
>> -    NE2000State ne2000;
>> -} PCINE2000State;
>
> NACK.
>
> We'll have a ...
>
> typedef struct ISANE2000State {
>    ISADevice dev;
>    NE2000State ne2000;
> } ISANE2000State;
>
> ... some day.

Ok, redoing it this way.
Thanks for the review.

Later, Juan.

Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense

[Stefan Weil]

Juan Quintela schrieb:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/eepro100.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 0031d36..09083c2 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>
>  static void nic_reset(void *opaque)
>  {
> -    EEPRO100State *s = (EEPRO100State *) opaque;
> +    EEPRO100State *s = opaque;
>      logout("%p\n", s);
>      static int first;
>      if (!first) {
> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
>
>  static int nic_load(QEMUFile * f, void *opaque, int version_id)
>  {
> -    EEPRO100State *s = (EEPRO100State *) opaque;
> +    EEPRO100State *s = opaque;
>      int i;
>      int ret;
>
> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)
>
>  static void nic_save(QEMUFile * f, void *opaque)
>  {
> -    EEPRO100State *s = (EEPRO100State *) opaque;
> +    EEPRO100State *s = opaque;
>      int i;
>
>      if (s->pci_dev)
>   

I wrote these type casts, and I think they make sense.
In C++ code, they are even mandatory.

I think the arguments why C++ requires this kind of
type casts apply to C code (like in Qemu) as well.

If it is possible with no or very litte efforts to write
code which is C and C++ compatible, I prefer to do so.

So please don't apply this patch.

Regards
Stefan Weil

[Qemu-devel] Re: [PATCH 00/22] Indirection Cleanup

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
> On 08/24/09 13:03, Juan Quintela wrote:
>> This patch series clean up "half" converted qemu drivers that had changed from:
>>
>> struct FOOState
>>
>> to
>>
>> typedef PCIFOOState {
>>      PCIDevice dev;
>>      FOOState foo;
>> } PCIFOOState;
>>
>> It just moves PCIDevice to be the 1st field of FOOState.
>
> Note that there are a bunch of cases where it actually makes sense to
> have PCIFOOState because foo can be connected to different busses and
> thus there is a ISAFOOState (or will be once ISA is qdevified too).
>
> Disclaimer: did't look through the patches yet, just scanned the
> subjects.  But I've seen ne2000 + vga on the list.  For both devices
> ISA variants exist.  Likewise OHCI, there are also non-PCI variants.

on the PCI/ISA difference:
* ne2000: just looked again at it, we could probably found a way to get
  that around.
* vga: this needs major refactoring.
* OHCI: I did the change, and everything compiled, didn't found the
  other uses :p

I didn't changed it because then I had to replicate the save/load
functions. (looking at it again, it appears that this ones are the only diff)

>> - pcnet:  It needs a different approach, because it can be both a PCIDevice
>>    or a SysBus device.
>
> That one too ;)
> Why do you want to change it?

PCNetState first field is

PCIDevice *pci_dev.

and that device is also for lance.  Will take a look at your approach
for diferentaiting them.

Later, Juan.

Re: [Qemu-devel] [PATCH 01/22] eepro100: convert casts to DO_UPCAST()

[Stefan Weil]

Juan Quintela schrieb:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/eepro100.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index ec31a6a..0031d36 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -1,4 +1,4 @@
> -/*
> + /*
>   * QEMU i8255x (PRO100) emulation
>   *
>   * Copyright (c) 2006-2007 Stefan Weil
>   

Please don't change formatting of multiline comments like this.

> @@ -1346,7 +1346,7 @@ typedef struct PCIEEPRO100State {
>  static void pci_map(PCIDevice * pci_dev, int region_num,
>                      uint32_t addr, uint32_t size, int type)
>  {
> -    PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
> +    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);
>      EEPRO100State *s = &d->eepro100;
>
>      logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
> @@ -1420,7 +1420,7 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {
>  static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
>                           uint32_t addr, uint32_t size, int type)
>  {
> -    PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev;
> +    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);
>
>      logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
>             region_num, addr, size, type);
> @@ -1720,7 +1720,7 @@ static void nic_cleanup(VLANClientState *vc)
>
>  static int pci_nic_uninit(PCIDevice *dev)
>  {
> -    PCIEEPRO100State *d = (PCIEEPRO100State *) dev;
> +    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, dev);
>      EEPRO100State *s = &d->eepro100;
>
>      cpu_unregister_io_memory(s->mmio_index);
> @@ -1730,7 +1730,7 @@ static int pci_nic_uninit(PCIDevice *dev)
>
>  static void nic_init(PCIDevice *pci_dev, uint32_t device)
>  {
> -    PCIEEPRO100State *d = (PCIEEPRO100State *)pci_dev;
> +    PCIEEPRO100State *d = DO_UPCAST(PCIEEPRO100State, dev, pci_dev);
>      EEPRO100State *s;
>
>      logout("\n");
>   

[Qemu-devel] Re: [PATCH 01/22] eepro100: convert casts to DO_UPCAST()

[Juan Quintela]

Stefan Weil <weil@mail.berlios.de> wrote:
> Juan Quintela schrieb:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/eepro100.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index ec31a6a..0031d36 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -1,4 +1,4 @@
>> -/*
>> + /*
>>   * QEMU i8255x (PRO100) emulation
>>   *
>>   * Copyright (c) 2006-2007 Stefan Weil
>>   
>
> Please don't change formatting of multiline comments like this.

Opp, that was a typo. Sorry.

later, Juan.

[Qemu-devel] Re: [PATCH 02/22] eepro100: cast a void * makes no sense

[Juan Quintela]

Stefan Weil <Stefan.Weil@weilnetz.de> wrote:
> Juan Quintela schrieb:
>>
>>  static void nic_reset(void *opaque)
>>  {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;
>>      logout("%p\n", s);
>>      static int first;
>>      if (!first) {

Hi

> I wrote these type casts, and I think they make sense.

In C, they made no sense. they are a nop.
In qemu, they are not used almost anywhere.

> In C++ code, they are even mandatory.

This is no C++.


> I think the arguments why C++ requires this kind of
> type casts apply to C code (like in Qemu) as well.

And they are?  I don't see what they buy us.


> If it is possible with no or very litte efforts to write
> code which is C and C++ compatible, I prefer to do so.

If it is possible to have a consistent code base, with little effort,
I prefer to do so.

> So please don't apply this patch.

I think it should be applied (qemu sense), but you do eepro100 work, and
I don't do/plan to do more eepro100 work.

I guess it is your call.

Later, Juan.

Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense

[Markus Armbruster]

Stefan Weil <Stefan.Weil@weilnetz.de> writes:

> Juan Quintela schrieb:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/eepro100.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 0031d36..09083c2 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>>
>>  static void nic_reset(void *opaque)
>>  {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;
>>      logout("%p\n", s);
>>      static int first;
>>      if (!first) {
>> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t size
>>
>>  static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>  {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;
>>      int i;
>>      int ret;
>>
>> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>
>>  static void nic_save(QEMUFile * f, void *opaque)
>>  {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;
>>      int i;
>>
>>      if (s->pci_dev)
>>   
>
> I wrote these type casts, and I think they make sense.
> In C++ code, they are even mandatory.

Yes, but this isn't C++.

> I think the arguments why C++ requires this kind of
> type casts apply to C code (like in Qemu) as well.
>
> If it is possible with no or very litte efforts to write
> code which is C and C++ compatible, I prefer to do so.

I respectfully disagree.  Casts from "void *" to "T *" are pure noise.
Getting into the habit of writing noise casts runs the risk of silencing
warnings that flag real type errors.

Re: [Qemu-devel] Re: [PATCH 02/22] eepro100: cast a void * makes no sense

[Anthony Liguori]

Juan Quintela wrote:
> I think it should be applied (qemu sense), but you do eepro100 work, and
> I don't do/plan to do more eepro100 work.
>   

I agree with Juan.

Overall consistency in the code base is extremely important.  Stefan, if 
you disagree with this style, you should submit a cleanup patch that 
converts every occurrence of this idiom.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense

[Gerd Hoffmann]

>>   static void nic_save(QEMUFile * f, void *opaque)
>>   {
>> -    EEPRO100State *s = (EEPRO100State *) opaque;
>> +    EEPRO100State *s = opaque;

> I wrote these type casts, and I think they make sense.

Why?

There is no technical reason for it.

And in that case it IMHO doesn't make sense to keep the cast as 
documentation.

> If it is possible with no or very litte efforts to write
> code which is C and C++ compatible, I prefer to do so.

Point being?  I doubt qemu will go C++ ...

cheers,
   Gerd

[Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Stefan Weil]

Markus Armbruster schrieb:
> Stefan Weil <Stefan.Weil@weilnetz.de> writes:
>
>> Juan Quintela schrieb:
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>> hw/eepro100.c | 6 +++---
>>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>> index 0031d36..09083c2 100644
>>> --- a/hw/eepro100.c
>>> +++ b/hw/eepro100.c
>>> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>>>
>>> static void nic_reset(void *opaque)
>>> {
>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>> + EEPRO100State *s = opaque;
>>> logout("%p\n", s);
>>> static int first;
>>> if (!first) {
>>> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState
>>> *vc, const uint8_t * buf, size_t size
>>>
>>> static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>> {
>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>> + EEPRO100State *s = opaque;
>>> int i;
>>> int ret;
>>>
>>> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void
>>> *opaque, int version_id)
>>>
>>> static void nic_save(QEMUFile * f, void *opaque)
>>> {
>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>> + EEPRO100State *s = opaque;
>>> int i;
>>>
>>> if (s->pci_dev)
>>>
>> I wrote these type casts, and I think they make sense.
>> In C++ code, they are even mandatory.
>
> Yes, but this isn't C++.
>
>> I think the arguments why C++ requires this kind of
>> type casts apply to C code (like in Qemu) as well.
>>
>> If it is possible with no or very litte efforts to write
>> code which is C and C++ compatible, I prefer to do so.
>
> I respectfully disagree. Casts from "void *" to "T *" are pure noise.
> Getting into the habit of writing noise casts runs the risk of silencing
> warnings that flag real type errors.

Hello

Do you only disagree with my first sentence or with both sentences?

Currently, I seem to be alone with my opinion (at least in qemu-devel).
Nevertheless, the designers of C++ thought that casts from void * to
T * were something very important. I don't know the history of their
decision. I personally think that deriving a data type T from some
bytes in memory which can contain anything is an operation which is
worth being documented by the programmer, and this is exactly what
the cast does.

My main reason why I try to write portable code is my personal
experience with code reuse and the future development of compilers.
It is quite possible that some day C compilers will require
type casts like C++ compilers do today.

And even today I want to be able to share C code from QEMU with
program code written in C++ (which makes a large part of my
business work).

Anthony, there is already a mixture of coding styles in QEMU
(also regarding type casts). This is not surprising for an
open source project with many contributors. Do we really
need additional regulations? I think the existing ones
(those in CODING_STYLE) are very good, and they are sufficient.
I'd appreciate it very much if all code in QEMU would
respect this documented coding style. Today, it does not,
and there was an agreement that we do not write commits which
only change the coding style (at least for white space).
I suggest to stick to this agreement for non white space
coding style, too.

Let me give one more C/C++ example. Today, many data structures
are declared like this: typedef struct T { ... } T;
There is nothing wrong with it, but it can be improved in
several ways:

* The declaration does not work with C++ (yes, I know that many
  programmers are not interested in C++ compatibility for QEMU).

* The declaration allows variable declarations using struct T var;
  or just T var; (which is the QEMU style). I think a declaration
  which does not enforce the correct style is less good.

* The declaration contains noise (bad argument, but many people
  like speaking of noise) :-)

Why don't we declare structures like this: typedef struct { ... } T;?
I suggest this to be the new coding style for structure declarations
because it is shorter, C++ compatible and unambiguous.

Kind regards

Stefan

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Gerd Hoffmann]

   Hi,

> Why don't we declare structures like this: typedef struct { ... } T;?
> I suggest this to be the new coding style for structure declarations
> because it is shorter, C++ compatible and unambiguous.

There are quite a few cases where this will simply not work.  They 
usually use a slightly different declaration style though:

typedef struct T T;
struct T {
   /* stuff here */
};

Reasons why this is used/needed:

(1) structs pointing to each other, like this:

typedef struct A A;
typedef struct B B;

struct A {
   B *b;
};
struct B {
   A *a;
};

(2) keep the struct private, let others pass around pointers
     to the struct in a typesafe way though:

header file:

typedef struct T T;
T* get_foo(args);
int do_something_with_foo(T*);

source file:

struct T {
   /* stuff here */
};

cheers,
   Gerd

Re: [Qemu-devel] Coding style, C++ compatible code

[Markus Armbruster]

Stefan Weil <weil@mail.berlios.de> writes:

> Markus Armbruster schrieb:
>> Stefan Weil <Stefan.Weil@weilnetz.de> writes:
>>
>>> Juan Quintela schrieb:
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>> hw/eepro100.c | 6 +++---
>>>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>>> index 0031d36..09083c2 100644
>>>> --- a/hw/eepro100.c
>>>> +++ b/hw/eepro100.c
>>>> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>>>>
>>>> static void nic_reset(void *opaque)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> logout("%p\n", s);
>>>> static int first;
>>>> if (!first) {
>>>> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState
>>>> *vc, const uint8_t * buf, size_t size
>>>>
>>>> static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> int i;
>>>> int ret;
>>>>
>>>> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void
>>>> *opaque, int version_id)
>>>>
>>>> static void nic_save(QEMUFile * f, void *opaque)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> int i;
>>>>
>>>> if (s->pci_dev)
>>>>
>>> I wrote these type casts, and I think they make sense.
>>> In C++ code, they are even mandatory.
>>
>> Yes, but this isn't C++.
>>
>>> I think the arguments why C++ requires this kind of
>>> type casts apply to C code (like in Qemu) as well.
>>>
>>> If it is possible with no or very litte efforts to write
>>> code which is C and C++ compatible, I prefer to do so.
>>
>> I respectfully disagree. Casts from "void *" to "T *" are pure noise.
>> Getting into the habit of writing noise casts runs the risk of silencing
>> warnings that flag real type errors.
>
> Hello
>
> Do you only disagree with my first sentence or with both sentences?

We can certainly discuss how to write better C, but appealing to the
authority of the C++ rationale is an argument I don't buy.

I prefer not to attempt to write C code that behaves identically when
interpreted as C++ (extern "C" seems safer and easier).

Does this answer your question?

> Currently, I seem to be alone with my opinion (at least in qemu-devel).
> Nevertheless, the designers of C++ thought that casts from void * to
> T * were something very important. I don't know the history of their
> decision. I personally think that deriving a data type T from some
> bytes in memory which can contain anything is an operation which is
> worth being documented by the programmer, and this is exactly what
> the cast does.

I don't see much value in that, sorry.

> My main reason why I try to write portable code is my personal
> experience with code reuse and the future development of compilers.
> It is quite possible that some day C compilers will require
> type casts like C++ compilers do today.

I can't read the ISO C committee's mind, but I believe such an
incompatible change is extremely unlikely, given the pains they've taken
not to break existing C code.

> And even today I want to be able to share C code from QEMU with
> program code written in C++ (which makes a large part of my
> business work).

How about extern "C"?

[...]

Re: [Qemu-devel] Coding style, C++ compatible code

[Anthony Liguori]

Markus Armbruster wrote:
> Stefan Weil <weil@mail.berlios.de> writes:
>   
> We can certainly discuss how to write better C, but appealing to the
> authority of the C++ rationale is an argument I don't buy.
>   
C++ compatibility is not a goal of QEMU AFAIK.

We make extensive use of the fact that you can use struct Foo or Foo as 
a declaration via the TAILQ_ENTRY macro.  With your proposal, that macro 
would no longer work.

Regards,

Anthony Liguori

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Måns Rullgård]

Stefan Weil <weil@mail.berlios.de> writes:

> Markus Armbruster schrieb:
>> Stefan Weil <Stefan.Weil@weilnetz.de> writes:
>>
>>> Juan Quintela schrieb:
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>> hw/eepro100.c | 6 +++---
>>>> 1 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>>>> index 0031d36..09083c2 100644
>>>> --- a/hw/eepro100.c
>>>> +++ b/hw/eepro100.c
>>>> @@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)
>>>>
>>>> static void nic_reset(void *opaque)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> logout("%p\n", s);
>>>> static int first;
>>>> if (!first) {
>>>> @@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState
>>>> *vc, const uint8_t * buf, size_t size
>>>>
>>>> static int nic_load(QEMUFile * f, void *opaque, int version_id)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> int i;
>>>> int ret;
>>>>
>>>> @@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void
>>>> *opaque, int version_id)
>>>>
>>>> static void nic_save(QEMUFile * f, void *opaque)
>>>> {
>>>> - EEPRO100State *s = (EEPRO100State *) opaque;
>>>> + EEPRO100State *s = opaque;
>>>> int i;
>>>>
>>>> if (s->pci_dev)
>>>>
>>> I wrote these type casts, and I think they make sense.
>>> In C++ code, they are even mandatory.
>>
>> Yes, but this isn't C++.
>>
>>> I think the arguments why C++ requires this kind of
>>> type casts apply to C code (like in Qemu) as well.
>>>
>>> If it is possible with no or very litte efforts to write
>>> code which is C and C++ compatible, I prefer to do so.
>>
>> I respectfully disagree. Casts from "void *" to "T *" are pure noise.
>> Getting into the habit of writing noise casts runs the risk of silencing
>> warnings that flag real type errors.
>
> Hello
>
> Do you only disagree with my first sentence or with both sentences?
>
> Currently, I seem to be alone with my opinion (at least in qemu-devel).
> Nevertheless, the designers of C++ thought that casts from void * to
> T * were something very important. I don't know the history of their
> decision.

I don't know the history of C++ either.  What I do know is that the
void* type was added to the C language precisely to *avoid* such
casts.  The original K&R C used char* as return type for malloc() etc,
and this of course required a cast.

> I personally think that deriving a data type T from some bytes in
> memory which can contain anything is an operation which is worth
> being documented by the programmer, and this is exactly what the
> cast does.

The declaration and assignment already make that perfectly clear.  The
cast is at best noise, and often hides a real error.

> My main reason why I try to write portable code is my personal
> experience with code reuse and the future development of compilers.
> It is quite possible that some day C compilers will require
> type casts like C++ compilers do today.

Any syntax or semantics can potentially change in the future.  If *I*
were in charge, I'd make frivolous casts an *error*.

> And even today I want to be able to share C code from QEMU with
> program code written in C++ (which makes a large part of my
> business work).

That's your problem.  Please don't bring it here.

> Let me give one more C/C++ example. Today, many data structures
> are declared like this: typedef struct T { ... } T;
> There is nothing wrong with it, but it can be improved in
> several ways:
>
> * The declaration does not work with C++ (yes, I know that many
>   programmers are not interested in C++ compatibility for QEMU).

Nor does it work in Fortran or COBOL.

> * The declaration allows variable declarations using struct T var;
>   or just T var; (which is the QEMU style). I think a declaration
>   which does not enforce the correct style is less good.
>
> * The declaration contains noise (bad argument, but many people
>   like speaking of noise) :-)
>
> Why don't we declare structures like this: typedef struct { ... } T;?
> I suggest this to be the new coding style for structure declarations
> because it is shorter, C++ compatible and unambiguous.

In my personal projects, I never use typedefs for structs.  Typing
those 5 characters isn't much of a burden, and it makes it
unambiguously clear that something is a struct and not something
else.  It even works in C++, should you care.

C is not C++, even though much of the syntax is confusingly similar.
Trying to shoehorn the same coding style onto both is a mistake, and
does more harm than good whichever of the languages you are writing
in.

-- 
Måns Rullgård
mans@mansr.com

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Avi Kivity]

On 08/26/2009 04:52 PM, Stefan Weil wrote:
>>> I wrote these type casts, and I think they make sense.
>>> In C++ code, they are even mandatory.
>>>        
>> Yes, but this isn't C++.
>>
>>      
>>> I think the arguments why C++ requires this kind of
>>> type casts apply to C code (like in Qemu) as well.
>>>
>>> If it is possible with no or very litte efforts to write
>>> code which is C and C++ compatible, I prefer to do so.
>>>        
>> I respectfully disagree. Casts from "void *" to "T *" are pure noise.
>> Getting into the habit of writing noise casts runs the risk of silencing
>> warnings that flag real type errors.
>>      
> Hello
>
> Do you only disagree with my first sentence or with both sentences?
>
> Currently, I seem to be alone with my opinion (at least in qemu-devel).
>    

You are not alone.

> Nevertheless, the designers of C++ thought that casts from void * to
> T * were something very important. I don't know the history of their
> decision. I personally think that deriving a data type T from some
> bytes in memory which can contain anything is an operation which is
> worth being documented by the programmer, and this is exactly what
> the cast does.
>    

Yes.

> Let me give one more C/C++ example. Today, many data structures
> are declared like this: typedef struct T { ... } T;
> There is nothing wrong with it, but it can be improved in
> several ways:
>
> * The declaration does not work with C++ (yes, I know that many
>    programmers are not interested in C++ compatibility for QEMU).
>    

It does work in C++.

> * The declaration allows variable declarations using struct T var;
>    or just T var; (which is the QEMU style). I think a declaration
>    which does not enforce the correct style is less good.
>    

It's often needed, for example to have a struct T * in T.


-- 
error compiling committee.c: too many arguments to function

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Reimar Döffinger]

On Wed, Aug 26, 2009 at 04:20:14PM +0100, Måns Rullgård wrote:
> Stefan Weil <weil@mail.berlios.de> writes:
> > I personally think that deriving a data type T from some bytes in
> > memory which can contain anything is an operation which is worth
> > being documented by the programmer, and this is exactly what the
> > cast does.
> 
> The declaration and assignment already make that perfectly clear.  The
> cast is at best noise, and often hides a real error.

There are additional points, having all those casts makes people used to
them and generally makes it much harder to spot those that are just
wrong (not that C++ does not have this issue, since basically all
conversions/uses of void* are wrong, with C they are unavoidable).
Having casts for malloc results also makes it needlessly hard to change
the type. Use
var = malloc(count * sizeof(*var))
and you only need to change the declaration, add a cast and you also
have to change all places where such allocations are done.

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Avi Kivity]

On 08/26/2009 06:58 PM, Reimar Döffinger wrote:
>
> There are additional points, having all those casts makes people used to
> them and generally makes it much harder to spot those that are just
> wrong (not that C++ does not have this issue, since basically all
> conversions/uses of void* are wrong, with C they are unavoidable).
> Having casts for malloc results also makes it needlessly hard to change
> the type. Use
> var = malloc(count * sizeof(*var))
> and you only need to change the declaration, add a cast and you also
> have to change all places where such allocations are done.
>    

Or better, NEW() and NEW_ARRAY().

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Jamie Lokier]

Gerd Hoffmann wrote:
>   Hi,
> 
> >Why don't we declare structures like this: typedef struct { ... } T;?
> >I suggest this to be the new coding style for structure declarations
> >because it is shorter, C++ compatible and unambiguous.
> 
> There are quite a few cases where this will simply not work.  They 
> usually use a slightly different declaration style though:
...
> (1) structs pointing to each other, like this:
> 
> typedef struct A A;
> typedef struct B B;

You can use "typedef struct _A A" to be C++ compatible, but it fails
to be shorter so I wouldn't recommend it ;-)

-- Jamie

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Jamie Lokier]

Avi Kivity wrote:
> On 08/26/2009 04:52 PM, Stefan Weil wrote:
> >Nevertheless, the designers of C++ thought that casts from void * to
> >T * were something very important. I don't know the history of their
> >decision. I personally think that deriving a data type T from some
> >bytes in memory which can contain anything is an operation which is
> >worth being documented by the programmer, and this is exactly what
> >the cast does.
> 
> Yes.

Not really.  Adding the cast can hide bugs.

If you omit the cast, a C compiler will at least warn, and maybe
error, if the original pointer is not "void *", or is "const void *"
and you are removing the constness.

With the cast, the C compiler will silently let you compile buggy code.

In C++, results vary.  You are supposed to use a base class pointer,
and if you need a cast, you are supposed to use "static_cast<>"
anyway.

In C++, it's an error without the cast, but it can also be an error
with the cast:

    g++ -Wall test.cc -c -Werror=old-style-cast
    test.cc: In function ‘S* foo(void*)’:
    test.cc:5: error: use of old-style cast

    struct S *foo(void *ptr)
    {
        return (struct S *)ptr;
    }

Of course there are dirty tricks to let you omit the "void *" pointer
altogether.  Let's not go there :-)

-- Jamie

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[malc]

On Wed, 26 Aug 2009, Jamie Lokier wrote:

> Gerd Hoffmann wrote:
> >   Hi,
> > 
> > >Why don't we declare structures like this: typedef struct { ... } T;?
> > >I suggest this to be the new coding style for structure declarations
> > >because it is shorter, C++ compatible and unambiguous.
> > 
> > There are quite a few cases where this will simply not work.  They 
> > usually use a slightly different declaration style though:
> ..
> > (1) structs pointing to each other, like this:
> > 
> > typedef struct A A;
> > typedef struct B B;
> 
> You can use "typedef struct _A A" to be C++ compatible, but it fails
> to be shorter so I wouldn't recommend it ;-)

This is neither C nor C++ compatible, in fact it breaks both.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Jamie Lokier]

malc wrote:
> > > (1) structs pointing to each other, like this:
> > > 
> > > typedef struct A A;
> > > typedef struct B B;
> > 
> > You can use "typedef struct _A A" to be C++ compatible, but it fails
> > to be shorter so I wouldn't recommend it ;-)
> 
> This is neither C nor C++ compatible, in fact it breaks both.

You'll have to explain that statement.

It compiles fine for me in C and C++:

   typedef struct _A A;
   typedef struct _B B;

   struct _A { struct _B *b; };
   struct _B { struct _A *a; };

   A *foo(B *arg);

Using this works equally well:

   struct _A { B *b; };
   struct _B { A *a; };

-- Jamie

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[malc]

On Wed, 26 Aug 2009, Jamie Lokier wrote:

> malc wrote:
> > > > (1) structs pointing to each other, like this:
> > > > 
> > > > typedef struct A A;
> > > > typedef struct B B;
> > > 
> > > You can use "typedef struct _A A" to be C++ compatible, but it fails
> > > to be shorter so I wouldn't recommend it ;-)
> > 
> > This is neither C nor C++ compatible, in fact it breaks both.
> 
> You'll have to explain that statement.

ISO/IEC 9899:1999 7.1.3#1
and 17.4.3.1.2 of n2315 (Draft of c++0x)

mandate that those names are reserved.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Jamie Lokier]

malc wrote:
> On Wed, 26 Aug 2009, Jamie Lokier wrote:
> > malc wrote:
> > > > > (1) structs pointing to each other, like this:
> > > > > 
> > > > > typedef struct A A;
> > > > > typedef struct B B;
> > > > 
> > > > You can use "typedef struct _A A" to be C++ compatible, but it fails
> > > > to be shorter so I wouldn't recommend it ;-)
> > > 
> > > This is neither C nor C++ compatible, in fact it breaks both.
> > 
> > You'll have to explain that statement.
> 
> ISO/IEC 9899:1999 7.1.3#1
> and 17.4.3.1.2 of n2315 (Draft of c++0x)
> 
> mandate that those names are reserved.

The _A is just an example; just substitute an identifier you're happy
with.  Use lower case, or put the underscore at the end, or something else.

It's actively unhelpful to say "that is not C/C++ compatible" without
explaining that it's for an tangential reason which is easily avoided.

-- Jamie

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Måns Rullgård]

Kent Harris <ksh@vine.com> writes:

> Stefan,
>
> I'm with you 100 percent.  I already complained to this group a couple
> of months ago about the void* casting issue (and also using "private"
> as a structure element name as that barfs C++ as well -- there are
> others.)
>
> For my situation, these problems within C-language source files are
> not an issue but when they occur inside a QEMU header file that I must
> include from my C++ device models, my compilations fail.
> Consequently, I have a whole series of patch files to the QEMU source
> that I have to apply to correct these (trivial) coding errors.  Each
> time a new QEMU release comes out, I have to recreate the patch
> files.  Frustrating to say the least.

#define private cxx_private
#include <qemu.h>
#undef  private

Job done.

-- 
Måns Rullgård
mans@mansr.com

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Måns Rullgård]

Kent Harris <ksh@vine.com> writes:

> Stefan,
>
> I'm with you 100 percent.  I already complained to this group a couple
> of months ago about the void* casting issue (and also using "private"
> as a structure element name as that barfs C++ as well -- there are
> others.)
>
> For my situation, these problems within C-language source files are
> not an issue but when they occur inside a QEMU header file that I must
> include from my C++ device models, my compilations fail.
> Consequently, I have a whole series of patch files to the QEMU source
> that I have to apply to correct these (trivial) coding errors.  Each
> time a new QEMU release comes out, I have to recreate the patch
> files.  Frustrating to say the least.

I have a great idea.  For maximum portability, lets all start writing
code like this: http://ideology.com.au/polyglot/polyglot.txt That way,
it will work in almost any language, and everybody will be happy.

-- 
Måns Rullgård
mans@mansr.com

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[malc]

On Thu, 27 Aug 2009, M?ns Rullg?rd wrote:

> Kent Harris <ksh@vine.com> writes:
> 
> > Stefan,
> >
> > I'm with you 100 percent.  I already complained to this group a couple
> > of months ago about the void* casting issue (and also using "private"
> > as a structure element name as that barfs C++ as well -- there are
> > others.)
> >
> > For my situation, these problems within C-language source files are
> > not an issue but when they occur inside a QEMU header file that I must
> > include from my C++ device models, my compilations fail.
> > Consequently, I have a whole series of patch files to the QEMU source
> > that I have to apply to correct these (trivial) coding errors.  Each
> > time a new QEMU release comes out, I have to recreate the patch
> > files.  Frustrating to say the least.
> 
> I have a great idea.  For maximum portability, lets all start writing
> code like this: http://ideology.com.au/polyglot/polyglot.txt That way,
> it will work in almost any language, and everybody will be happy.

Reminds me of http://fly.srk.fer.hr/ioccc/1986/applin.c

-- 
mailto:av1474@comtv.ru

Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)

[Stuart Brady]

On Wed, Aug 26, 2009 at 07:08:55PM +0300, Avi Kivity wrote:
> Or better, NEW() and NEW_ARRAY().

ISTR this being discussed before, but there was some disagreement
regarding whether it is preferable to have:

   QEMU_NEW(ptr);

or:

   ptr = QEMU_NEW(type);

If the type is incorrect, the latter form would still at least yield a
warning (and now therefore a build failure).  It seems slightly more 
readable to me, so that's the form that I would have preferred...

Is there any reason that this wouldn't be accepted, or should I start
submitting patches?

Cheers,
-- 
Stuart Brady