From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmXH2-0007tm-I9 for qemu-devel@nongnu.org; Wed, 04 Jul 2012 17:38:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SmXGy-00070L-TA for qemu-devel@nongnu.org; Wed, 04 Jul 2012 17:38:12 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:64383) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmXGy-0006zh-Ls for qemu-devel@nongnu.org; Wed, 04 Jul 2012 17:38:08 -0400 Received: by obbta14 with SMTP id ta14so12445443obb.4 for ; Wed, 04 Jul 2012 14:38:06 -0700 (PDT) Message-ID: <4FF4B7BB.8060301@codemonkey.ws> Date: Wed, 04 Jul 2012 16:38:03 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1341422373-13614-1-git-send-email-afaerber@suse.de> <1341422373-13614-15-git-send-email-afaerber@suse.de> <20120704211717.GC27653@redhat.com> <20120704212614.GA27711@redhat.com> In-Reply-To: <20120704212614.GA27711@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: jbaron@redhat.com, Alexander Graf , qemu-devel@nongnu.org, =?ISO-8859-1?Q?Andreas_F=E4rber?= , New World , pbonzini@redhat.com, =?ISO-8859-1?Q?Andreas_F=E4rber?= On 07/04/2012 04:26 PM, Michael S. Tsirkin wrote: > 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 >>> --- >>> 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. I'm not a huge fan of using the cast operation like this. I'd much prefer: PCIHostState *pci_host = PCI_HOST_BRIDGE(s); pci_host->bus = b; But using the macro is absolutely the right thing to do. Macro casts never fail. If there is a user error, then it will cause an abort(). Using a macro has a lot of advantages as demonstrated by this patch. It makes refactoring a hell of a lot easier. If you look at my early QOM patches, it involved a lot of "change complex touching of fields" with wrapping functions/macros to have better encapsulation. Having to touch a bunch of files just to rename 'host' to 'parent_obj' is ugly. Regards, Anthony Liguori > >>> 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