Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges

[Anthony Liguori <anthony@codemonkey.ws>]

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<afaerber@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.

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