On Thu, Jul 05, 2012 at 12:17:17AM +0300, Michael S. Tsirkin wrote: > On Wed, Jul 04, 2012 at 07:19:33PM +0200, Andreas Färber wrote: > > Uglify the parent field to enforce QOM-style access via casts. > > Don't just typedef PCIHostState, either use it directly or embed it. > > > > Signed-off-by: Andreas Färber <afaer...@suse.de> > > --- > > hw/alpha_typhoon.c | 4 ++-- > > hw/dec_pci.c | 2 +- > > hw/grackle_pci.c | 2 +- > > hw/gt64xxx.c | 26 ++++++++++++++++---------- > > hw/piix_pci.c | 6 ++++-- > > hw/ppc4xx_pci.c | 8 +++++--- > > hw/ppce500_pci.c | 2 +- > > hw/prep_pci.c | 8 +++++--- > > hw/spapr_pci.c | 12 +++++++----- > > hw/spapr_pci.h | 2 +- > > hw/unin_pci.c | 14 +++++++------- > > 11 files changed, 50 insertions(+), 36 deletions(-) > > > > diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c > > index 58025a3..955d628 100644 > > --- a/hw/alpha_typhoon.c > > +++ b/hw/alpha_typhoon.c > > @@ -46,7 +46,7 @@ typedef struct TyphoonPchip { > > OBJECT_CHECK(TyphoonState, (obj), TYPE_TYPHOON_PCI_HOST_BRIDGE) > > > > typedef struct TyphoonState { > > - PCIHostState host; > > + PCIHostState parent_obj; > > > > TyphoonCchip cchip; > > TyphoonPchip pchip; > > @@ -770,7 +770,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus > > **isa_bus, > > b = pci_register_bus(dev, "pci", > > typhoon_set_irq, sys_map_irq, s, > > &s->pchip.reg_mem, addr_space_io, 0, 64); > > - s->host.bus = b; > > + PCI_HOST_BRIDGE(s)->bus = b; > > > > /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB. */ > > memory_region_init_io(&s->pchip.reg_iack, &alpha_pci_iack_ops, b, > > Sorry I don't understand. > why are we making code ugly apparently intentionally?
Just to clarify: replacing upcasts which are always safe with downcasts which can fail is what I consider especially ugly. > > diff --git a/hw/dec_pci.c b/hw/dec_pci.c > > index de16361..c30ade3 100644 > > --- a/hw/dec_pci.c > > +++ b/hw/dec_pci.c > > @@ -43,7 +43,7 @@ > > #define DEC_21154(obj) OBJECT_CHECK(DECState, (obj), TYPE_DEC_21154) > > > > typedef struct DECState { > > - PCIHostState host_state; > > + PCIHostState parent_obj; > > } DECState; > > > > static int dec_map_irq(PCIDevice *pci_dev, int irq_num) > > diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c > > index 066f6e1..67da307 100644 > > --- a/hw/grackle_pci.c > > +++ b/hw/grackle_pci.c > > @@ -41,7 +41,7 @@ > > OBJECT_CHECK(GrackleState, (obj), TYPE_GRACKLE_PCI_HOST_BRIDGE) > > > > typedef struct GrackleState { > > - PCIHostState host_state; > > + PCIHostState parent_obj; > > > > MemoryRegion pci_mmio; > > MemoryRegion pci_hole; > > diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c > > index 857758e..e95e664 100644 > > --- a/hw/gt64xxx.c > > +++ b/hw/gt64xxx.c > > @@ -235,7 +235,7 @@ > > OBJECT_CHECK(GT64120State, (obj), TYPE_GT64120_PCI_HOST_BRIDGE) > > > > typedef struct GT64120State { > > - PCIHostState pci; > > + PCIHostState parent_obj; > > > > uint32_t regs[GT_REGS]; > > PCI_MAPPING_ENTRY(PCI0IO); > > @@ -315,6 +315,7 @@ static void gt64120_writel (void *opaque, > > target_phys_addr_t addr, > > uint64_t val, unsigned size) > > { > > GT64120State *s = opaque; > > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > > uint32_t saddr; > > > > if (!(s->regs[GT_CPU] & 0x00001000)) > > @@ -535,13 +536,15 @@ static void gt64120_writel (void *opaque, > > target_phys_addr_t addr, > > /* not implemented */ > > break; > > case GT_PCI0_CFGADDR: > > - s->pci.config_reg = val & 0x80fffffc; > > + phb->config_reg = val & 0x80fffffc; > > break; > > case GT_PCI0_CFGDATA: > > - if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci.config_reg & > > 0x00fff800)) > > + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) > > { > > val = bswap32(val); > > - if (s->pci.config_reg & (1u << 31)) > > - pci_data_write(s->pci.bus, s->pci.config_reg, val, 4); > > + } > > + if (phb->config_reg & (1u << 31)) { > > + pci_data_write(phb->bus, phb->config_reg, val, 4); > > + } > > break; > > > > /* Interrupts */ > > @@ -594,6 +597,7 @@ static uint64_t gt64120_readl (void *opaque, > > target_phys_addr_t addr, unsigned size) > > { > > GT64120State *s = opaque; > > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > > uint32_t val; > > uint32_t saddr; > > > > @@ -775,15 +779,17 @@ static uint64_t gt64120_readl (void *opaque, > > > > /* PCI Internal */ > > case GT_PCI0_CFGADDR: > > - val = s->pci.config_reg; > > + val = phb->config_reg; > > break; > > case GT_PCI0_CFGDATA: > > - if (!(s->pci.config_reg & (1 << 31))) > > + if (!(phb->config_reg & (1 << 31))) { > > val = 0xffffffff; > > - else > > - val = pci_data_read(s->pci.bus, s->pci.config_reg, 4); > > - if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci.config_reg & > > 0x00fff800)) > > + } else { > > + val = pci_data_read(phb->bus, phb->config_reg, 4); > > + } > > + if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) > > { > > val = bswap32(val); > > + } > > break; > > > > case GT_PCI0_CMD: > > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > > index 0523d81..9522a27 100644 > > --- a/hw/piix_pci.c > > +++ b/hw/piix_pci.c > > @@ -36,7 +36,9 @@ > > * http://download.intel.com/design/chipsets/datashts/29054901.pdf > > */ > > > > -typedef PCIHostState I440FXState; > > +typedef struct I440FXState { > > + PCIHostState parent_obj; > > +} I440FXState; > > > > #define PIIX_NUM_PIC_IRQS 16 /* i8259 * 2 */ > > #define PIIX_NUM_PIRQS 4ULL /* PIRQ[A-D] */ > > @@ -273,7 +275,7 @@ static PCIBus *i440fx_common_init(const char > > *device_name, > > dev = qdev_create(NULL, "i440FX-pcihost"); > > s = PCI_HOST_BRIDGE(dev); > > s->address_space = address_space_mem; > > - b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space, > > + b = pci_bus_new(dev, NULL, pci_address_space, > > address_space_io, 0); > > s->bus = b; > > object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), > > NULL); > > diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c > > index 5583321..a14fd42 100644 > > --- a/hw/ppc4xx_pci.c > > +++ b/hw/ppc4xx_pci.c > > @@ -52,7 +52,7 @@ struct PCITargetMap { > > #define PPC4xx_PCI_NR_PTMS 2 > > > > struct PPC4xxPCIState { > > - PCIHostState pci_state; > > + PCIHostState parent_obj; > > > > struct PCIMasterMap pmm[PPC4xx_PCI_NR_PMMS]; > > struct PCITargetMap ptm[PPC4xx_PCI_NR_PTMS]; > > @@ -96,16 +96,18 @@ static uint64_t pci4xx_cfgaddr_read(void *opaque, > > target_phys_addr_t addr, > > unsigned size) > > { > > PPC4xxPCIState *ppc4xx_pci = opaque; > > + PCIHostState *phb = PCI_HOST_BRIDGE(ppc4xx_pci); > > > > - return ppc4xx_pci->pci_state.config_reg; > > + return phb->config_reg; > > } > > > > static void pci4xx_cfgaddr_write(void *opaque, target_phys_addr_t addr, > > uint64_t value, unsigned size) > > { > > PPC4xxPCIState *ppc4xx_pci = opaque; > > + PCIHostState *phb = PCI_HOST_BRIDGE(ppc4xx_pci); > > > > - ppc4xx_pci->pci_state.config_reg = value & ~0x3; > > + phb->config_reg = value & ~0x3; > > } > > > > static const MemoryRegionOps pci4xx_cfgaddr_ops = { > > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c > > index 3333967..92b1dc0 100644 > > --- a/hw/ppce500_pci.c > > +++ b/hw/ppce500_pci.c > > @@ -78,7 +78,7 @@ struct pci_inbound { > > OBJECT_CHECK(PPCE500PCIState, (obj), TYPE_PPC_E500_PCI_HOST_BRIDGE) > > > > struct PPCE500PCIState { > > - PCIHostState pci_state; > > + PCIHostState parent_obj; > > > > struct pci_outbound pob[PPCE500_PCI_NR_POBS]; > > struct pci_inbound pib[PPCE500_PCI_NR_PIBS]; > > diff --git a/hw/prep_pci.c b/hw/prep_pci.c > > index 35cb9b2..cc44e61 100644 > > --- a/hw/prep_pci.c > > +++ b/hw/prep_pci.c > > @@ -34,7 +34,7 @@ > > OBJECT_CHECK(PREPPCIState, (obj), TYPE_RAVEN_PCI_HOST_BRIDGE) > > > > typedef struct PRePPCIState { > > - PCIHostState host_state; > > + PCIHostState parent_obj; > > > > MemoryRegion intack; > > qemu_irq irq[4]; > > @@ -60,14 +60,16 @@ static void ppc_pci_io_write(void *opaque, > > target_phys_addr_t addr, > > uint64_t val, unsigned int size) > > { > > PREPPCIState *s = opaque; > > - pci_data_write(s->host_state.bus, PPC_PCIIO_config(addr), val, size); > > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > > + pci_data_write(phb->bus, PPC_PCIIO_config(addr), val, size); > > } > > > > static uint64_t ppc_pci_io_read(void *opaque, target_phys_addr_t addr, > > unsigned int size) > > { > > PREPPCIState *s = opaque; > > - return pci_data_read(s->host_state.bus, PPC_PCIIO_config(addr), size); > > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > > + return pci_data_read(phb->bus, PPC_PCIIO_config(addr), size); > > } > > > > static const MemoryRegionOps PPC_PCIIO_ops = { > > diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c > > index 7d84801..9231e0e 100644 > > --- a/hw/spapr_pci.c > > +++ b/hw/spapr_pci.c > > @@ -36,16 +36,18 @@ static PCIDevice *find_dev(sPAPREnvironment *spapr, > > uint64_t buid, uint32_t config_addr) > > { > > int devfn = (config_addr >> 8) & 0xFF; > > - sPAPRPHBState *phb; > > + sPAPRPHBState *sphb; > > > > - QLIST_FOREACH(phb, &spapr->phbs, list) { > > + QLIST_FOREACH(sphb, &spapr->phbs, list) { > > + PCIHostState *phb; > > BusChild *kid; > > > > - if (phb->buid != buid) { > > + if (sphb->buid != buid) { > > continue; > > } > > > > - QTAILQ_FOREACH(kid, &phb->host_state.bus->qbus.children, sibling) { > > + phb = PCI_HOST_BRIDGE(sphb); > > + QTAILQ_FOREACH(kid, &BUS(phb->bus)->children, sibling) { > > PCIDevice *dev = (PCIDevice *)kid->child; > > if (dev->devfn == devfn) { > > return dev; > > @@ -319,7 +321,7 @@ static int spapr_phb_init(SysBusDevice *s) > > pci_spapr_set_irq, pci_spapr_map_irq, phb, > > &phb->memspace, &phb->iospace, > > PCI_DEVFN(0, 0), PCI_NUM_PINS); > > - phb->host_state.bus = bus; > > + PCI_HOST_BRIDGE(phb)->bus = bus; > > > > liobn = SPAPR_PCI_BASE_LIOBN | (pci_find_domain(bus) << 16); > > phb->dma = spapr_tce_new_dma_context(liobn, 0x40000000); > > diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h > > index 06e2742..6840814 100644 > > --- a/hw/spapr_pci.h > > +++ b/hw/spapr_pci.h > > @@ -33,7 +33,7 @@ > > OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE) > > > > typedef struct sPAPRPHBState { > > - PCIHostState host_state; > > + PCIHostState parent_obj; > > > > uint64_t buid; > > char *busname; > > diff --git a/hw/unin_pci.c b/hw/unin_pci.c > > index 0db7c1f..d3aaa5a 100644 > > --- a/hw/unin_pci.c > > +++ b/hw/unin_pci.c > > @@ -53,7 +53,7 @@ static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, > > 0x1e }; > > OBJECT_CHECK(UNINState, (obj), TYPE_U3_AGP_HOST_BRIDGE) > > > > typedef struct UNINState { > > - PCIHostState host_state; > > + PCIHostState parent_obj; > > > > MemoryRegion pci_mmio; > > MemoryRegion pci_hole; > > @@ -114,22 +114,22 @@ static uint32_t unin_get_config_reg(uint32_t reg, > > uint32_t addr) > > static void unin_data_write(void *opaque, target_phys_addr_t addr, > > uint64_t val, unsigned len) > > { > > - UNINState *s = opaque; > > + PCIHostState *phb = PCI_HOST_BRIDGE(opaque); > > UNIN_DPRINTF("write addr %" TARGET_FMT_plx " len %d val %"PRIx64"\n", > > addr, len, val); > > - pci_data_write(s->host_state.bus, > > - unin_get_config_reg(s->host_state.config_reg, addr), > > + pci_data_write(phb->bus, > > + unin_get_config_reg(phb->config_reg, addr), > > val, len); > > } > > > > static uint64_t unin_data_read(void *opaque, target_phys_addr_t addr, > > unsigned len) > > { > > - UNINState *s = opaque; > > + PCIHostState *phb = PCI_HOST_BRIDGE(opaque); > > uint32_t val; > > > > - val = pci_data_read(s->host_state.bus, > > - unin_get_config_reg(s->host_state.config_reg, > > addr), > > + val = pci_data_read(phb->bus, > > + unin_get_config_reg(phb->config_reg, addr), > > len); > > UNIN_DPRINTF("read addr %" TARGET_FMT_plx " len %d val %x\n", > > addr, len, val); > > -- > > 1.7.7