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

[Marcel Apfelbaum]

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

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)