[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges |
Date: |
Thu, 5 Jul 2012 00:26:14 +0300 |
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 <address@hidden>
> > ---
> > 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
- [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Andreas Färber, 2012/07/04
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Michael S. Tsirkin, 2012/07/04
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges,
Michael S. Tsirkin <=
- Message not available
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Paolo Bonzini, 2012/07/05
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Andreas Färber, 2012/07/05
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Paolo Bonzini, 2012/07/05
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Michael S. Tsirkin, 2012/07/05
- Message not available
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Michael S. Tsirkin, 2012/07/05
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Andreas Färber, 2012/07/05
- Re: [Qemu-ppc] [PATCH v3 14/14] pci: Tidy up PCI host bridges, Anthony Liguori, 2012/07/05