Re: [Qemu-devel] [PATCH 3/5] pci: move initialization of pci's conf_addr and conf_data to common place

[Hu Tao]

On Tue, Nov 04, 2014 at 04:21:41PM +0200, Marcel Apfelbaum wrote:
> On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
> > So that standard pci host device can share them.
> > 
> > Signed-off-by: Hu Tao <address@hidden>
> > ---
> >  hw/pci-host/piix.c | 20 --------------------
> >  hw/pci-host/q35.c  |  7 -------
> >  hw/pci/pci_host.c  | 32 ++++++++++++++++++++++++++++++++
> >  3 files changed, 32 insertions(+), 27 deletions(-)
> > 
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index eb92bde..683465c 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -256,14 +256,8 @@ static void i440fx_pcihost_get_pci_hole64_end(Object 
> > *obj, Visitor *v,
> >  
> >  static void i440fx_pcihost_initfn(Object *obj)
> >  {
> > -    PCIHostState *s = PCI_HOST_BRIDGE(obj);
> >      I440FXState *d = I440FX_PCI_HOST_BRIDGE(obj);
> >  
> > -    memory_region_init_io(&s->conf_mem, obj, &pci_host_conf_le_ops, s,
> > -                          "pci-conf-idx", 4);
> > -    memory_region_init_io(&s->data_mem, obj, &pci_host_data_le_ops, s,
> > -                          "pci-conf-data", 4);
> > -
> >      object_property_add(obj, PCI_HOST_PROP_PCI_HOLE_START, "int",
> >                          i440fx_pcihost_get_pci_hole_start,
> >                          NULL, NULL, NULL, NULL);
> > @@ -283,18 +277,6 @@ static void i440fx_pcihost_initfn(Object *obj)
> >      d->pci_info.w32.end = IO_APIC_DEFAULT_ADDRESS;
> >  }
> >  
> > -static void i440fx_pcihost_realize(DeviceState *dev, Error **errp)
> > -{
> > -    PCIHostState *s = PCI_HOST_BRIDGE(dev);
> > -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > -
> > -    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
> > -    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
> > -
> > -    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
> > -    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
> > -}
> > -
> >  static int i440fx_initfn(PCIDevice *dev)
> >  {
> >      PCII440FXState *d = I440FX_PCI_DEVICE(dev);
> > @@ -755,8 +737,6 @@ static void i440fx_pcihost_class_init(ObjectClass 
> > *klass, void *data)
> >      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> >  
> >      hc->root_bus_path = i440fx_pcihost_root_bus_path;
> > -    dc->realize = i440fx_pcihost_realize;
> > -    dc->fw_name = "pci";
> >      dc->props = i440fx_props;
> >  }
> >  
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 9e66835..81eddd7 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -138,18 +138,11 @@ static void q35_host_class_init(ObjectClass *klass, 
> > void *data)
> >      dc->realize = q35_host_realize;
> >      dc->props = mch_props;
> >      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> > -    dc->fw_name = "pci";
> >  }
> >  
> >  static void q35_host_initfn(Object *obj)
> >  {
> >      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> > -    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> > -
> > -    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
> > -                          "pci-conf-idx", 4);
> > -    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
> > -                          "pci-conf-data", 4);
> >  
> >      object_initialize(&s->mch, sizeof(s->mch), TYPE_MCH_PCI_DEVICE);
> >      object_property_add_child(OBJECT(s), "mch", OBJECT(&s->mch), NULL);
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index f2a69ea..406c747 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -176,12 +176,44 @@ const MemoryRegionOps pci_host_data_be_ops = {
> >      .endianness = DEVICE_BIG_ENDIAN,
> >  };
> >  
> > +static void pci_host_initfn(Object *obj)
> > +{
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> > +
> > +    memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops, phb,
> > +                          "pci-conf-idx", 4);
> > +    memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops, phb,
> > +                          "pci-conf-data", 4);
> > +}
> > +
> > +static void pci_host_realize(DeviceState *dev, Error **errp)
> > +{
> > +    PCIHostState *s = PCI_HOST_BRIDGE(dev);
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > +
> > +    sysbus_add_io(sbd, PC_PCI_CONFIG_ADDR, &s->conf_mem);
> > +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_ADDR, 4);
> > +
> > +    sysbus_add_io(sbd, PC_PCI_CONFIG_DATA, &s->data_mem);
> > +    sysbus_init_ioports(sbd, PC_PCI_CONFIG_DATA, 4);
> Hi,
> 
> If I got this right, now we have this pci_host_realize run
> for each object of classes deriving from TYPE_PCI_HOST_BRIDGE.
> Please correct me if I am wrong.

You're absolutely right unless the subclass overides the realize
function.

> The machines PC and Q35 share the same code, however
> we have other host bridges deriving that I think they behave different:
> 
> hw/mips/gt64xxx_pci.c:1207:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-bridge/dec.c:150:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/apb.c:839:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/bonito.c:832:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/grackle.c:156:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/ppce500.c:440:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/prep.c:392:    .parent = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/uninorth.c:456:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/uninorth.c:470:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/uninorth.c:484:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/uninorth.c:498:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci-host/versatile.c:508:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/pci/pci_host.c:179:    .name = TYPE_PCI_HOST_BRIDGE,
> hw/ppc/ppc4xx_pci.c:405:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/ppc/spapr_pci.c:801:    .parent        = TYPE_PCI_HOST_BRIDGE,
> hw/sh4/sh_pci.c:193:    .parent        = TYPE_PCI_HOST_BRIDGE,
> 
> If I am right, putting this so "high" in the hierarchy
> may not be the right solution.

It is wrong indeed for those devices having different settings. So
please just ignore this patch.

Thanks for review!

> 
> Thanks,
> Marcel
> 
> > +}
> > +
> > +static void pci_host_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = pci_host_realize;
> > +    dc->fw_name = "pci";
> > +}
> > +
> >  static const TypeInfo pci_host_type_info = {
> >      .name = TYPE_PCI_HOST_BRIDGE,
> >      .parent = TYPE_SYS_BUS_DEVICE,
> >      .abstract = true,
> >      .class_size = sizeof(PCIHostBridgeClass),
> > +    .class_init = pci_host_class_init,
> >      .instance_size = sizeof(PCIHostState),
> > +    .instance_init = pci_host_initfn,
> >  };
> >  
> >  static void pci_host_register_types(void)
> 
> 
>