* [Qemu-devel] [PATCH v3 01/14] pci: Make host bridge TypeInfos const
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 21:20 ` Michael S. Tsirkin
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 02/14] alpha_typhoon: QOM'ify Typhoon PCI host bridge Andreas Färber
` (12 subsequent siblings)
13 siblings, 1 reply; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: mst, jbaron, Alexander Graf, Andreas Färber,
open list:New World, anthony, pbonzini, Andreas Färber
Also give the sPAPR host bridge type registration functions a unique
name.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/alpha_typhoon.c | 2 +-
hw/bonito.c | 4 ++--
hw/dec_pci.c | 6 +++---
hw/grackle_pci.c | 4 ++--
hw/gt64xxx.c | 4 ++--
hw/piix_pci.c | 8 ++++----
hw/ppc4xx_pci.c | 4 ++--
hw/ppce500_pci.c | 4 ++--
hw/prep_pci.c | 4 ++--
hw/spapr_pci.c | 7 ++++---
hw/unin_pci.c | 16 ++++++++--------
11 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
index 872e112..cc63737 100644
--- a/hw/alpha_typhoon.c
+++ b/hw/alpha_typhoon.c
@@ -817,7 +817,7 @@ static void typhoon_pcihost_class_init(ObjectClass *klass, void *data)
dc->no_user = 1;
}
-static TypeInfo typhoon_pcihost_info = {
+static const TypeInfo typhoon_pcihost_info = {
.name = "typhoon-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(TyphoonState),
diff --git a/hw/bonito.c b/hw/bonito.c
index 77786f8..b990875 100644
--- a/hw/bonito.c
+++ b/hw/bonito.c
@@ -781,7 +781,7 @@ static void bonito_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_bonito;
}
-static TypeInfo bonito_info = {
+static const TypeInfo bonito_info = {
.name = "Bonito",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIBonitoState),
@@ -797,7 +797,7 @@ static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
dc->no_user = 1;
}
-static TypeInfo bonito_pcihost_info = {
+static const TypeInfo bonito_pcihost_info = {
.name = "Bonito-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(BonitoState),
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index 37337bf..5194a9f 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -66,7 +66,7 @@ static void dec_21154_pci_bridge_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_pci_device;
}
-static TypeInfo dec_21154_pci_bridge_info = {
+static const TypeInfo dec_21154_pci_bridge_info = {
.name = "dec-21154-p2p-bridge",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIBridge),
@@ -119,7 +119,7 @@ static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data)
k->is_bridge = 1;
}
-static TypeInfo dec_21154_pci_host_info = {
+static const TypeInfo dec_21154_pci_host_info = {
.name = "dec-21154",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -133,7 +133,7 @@ static void pci_dec_21154_device_class_init(ObjectClass *klass, void *data)
sdc->init = pci_dec_21154_device_init;
}
-static TypeInfo pci_dec_21154_device_info = {
+static const TypeInfo pci_dec_21154_device_info = {
.name = "dec-21154-sysbus",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(DECState),
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index 81ff3a3..35667ad 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -134,7 +134,7 @@ static void grackle_pci_class_init(ObjectClass *klass, void *data)
dc->no_user = 1;
}
-static TypeInfo grackle_pci_info = {
+static const TypeInfo grackle_pci_info = {
.name = "grackle",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -150,7 +150,7 @@ static void pci_grackle_class_init(ObjectClass *klass, void *data)
dc->no_user = 1;
}
-static TypeInfo grackle_pci_host_info = {
+static const TypeInfo grackle_pci_host_info = {
.name = "grackle-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(GrackleState),
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index a2d0e5a..04831bb 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -1147,7 +1147,7 @@ static void gt64120_pci_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_HOST;
}
-static TypeInfo gt64120_pci_info = {
+static const TypeInfo gt64120_pci_info = {
.name = "gt64120_pci",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -1161,7 +1161,7 @@ static void gt64120_class_init(ObjectClass *klass, void *data)
sdc->init = gt64120_init;
}
-static TypeInfo gt64120_info = {
+static const TypeInfo gt64120_info = {
.name = "gt64120",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(GT64120State),
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 09e84f5..5db4026 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -517,7 +517,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_ISA;
}
-static TypeInfo piix3_info = {
+static const TypeInfo piix3_info = {
.name = "PIIX3",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PIIX3State),
@@ -540,7 +540,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_ISA;
};
-static TypeInfo piix3_xen_info = {
+static const TypeInfo piix3_xen_info = {
.name = "PIIX3-xen",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PIIX3State),
@@ -564,7 +564,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_i440fx;
}
-static TypeInfo i440fx_info = {
+static const TypeInfo i440fx_info = {
.name = "i440FX",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCII440FXState),
@@ -581,7 +581,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
dc->no_user = 1;
}
-static TypeInfo i440fx_pcihost_info = {
+static const TypeInfo i440fx_pcihost_info = {
.name = "i440FX-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(I440FXState),
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 203c3cd..104ed98 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -377,7 +377,7 @@ static void ppc4xx_host_bridge_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_OTHER;
}
-static TypeInfo ppc4xx_host_bridge_info = {
+static const TypeInfo ppc4xx_host_bridge_info = {
.name = "ppc4xx-host-bridge",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -393,7 +393,7 @@ static void ppc4xx_pcihost_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_ppc4xx_pci;
}
-static TypeInfo ppc4xx_pcihost_info = {
+static const TypeInfo ppc4xx_pcihost_info = {
.name = "ppc4xx-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(PPC4xxPCIState),
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 0f60b24..99748b3 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -350,7 +350,7 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
dc->desc = "Host bridge";
}
-static TypeInfo e500_host_bridge_info = {
+static const TypeInfo e500_host_bridge_info = {
.name = "e500-host-bridge",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -366,7 +366,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_ppce500_pci;
}
-static TypeInfo e500_pcihost_info = {
+static const TypeInfo e500_pcihost_info = {
.name = "e500-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(PPCE500PCIState),
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 38dbff4..a8cdc21 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -166,7 +166,7 @@ static void raven_class_init(ObjectClass *klass, void *data)
dc->no_user = 1;
}
-static TypeInfo raven_info = {
+static const TypeInfo raven_info = {
.name = "raven",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(RavenPCIState),
@@ -183,7 +183,7 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data)
dc->no_user = 1;
}
-static TypeInfo raven_pcihost_info = {
+static const TypeInfo raven_pcihost_info = {
.name = "raven-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(PREPPCIState),
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 47ba5ff..0e0830f 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -368,7 +368,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
spapr_rtas_register("ibm,write-pci-config", rtas_ibm_write_pci_config);
}
-static TypeInfo spapr_phb_info = {
+static const TypeInfo spapr_phb_info = {
.name = "spapr-pci-host-bridge",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(sPAPRPHBState),
@@ -490,8 +490,9 @@ int spapr_populate_pci_devices(sPAPRPHBState *phb,
return 0;
}
-static void register_types(void)
+static void spapr_pci_register_types(void)
{
type_register_static(&spapr_phb_info);
}
-type_init(register_types)
+
+type_init(spapr_pci_register_types)
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 409bcd4..2b309df 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -350,7 +350,7 @@ static void unin_main_pci_host_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_HOST;
}
-static TypeInfo unin_main_pci_host_info = {
+static const TypeInfo unin_main_pci_host_info = {
.name = "uni-north-pci",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -368,7 +368,7 @@ static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_HOST;
}
-static TypeInfo u3_agp_pci_host_info = {
+static const TypeInfo u3_agp_pci_host_info = {
.name = "u3-agp",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -386,7 +386,7 @@ static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_HOST;
}
-static TypeInfo unin_agp_pci_host_info = {
+static const TypeInfo unin_agp_pci_host_info = {
.name = "uni-north-agp",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -404,7 +404,7 @@ static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data)
k->class_id = PCI_CLASS_BRIDGE_HOST;
}
-static TypeInfo unin_internal_pci_host_info = {
+static const TypeInfo unin_internal_pci_host_info = {
.name = "uni-north-internal-pci",
.parent = TYPE_PCI_DEVICE,
.instance_size = sizeof(PCIDevice),
@@ -418,7 +418,7 @@ static void pci_unin_main_class_init(ObjectClass *klass, void *data)
sbc->init = pci_unin_main_init_device;
}
-static TypeInfo pci_unin_main_info = {
+static const TypeInfo pci_unin_main_info = {
.name = "uni-north-pci-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(UNINState),
@@ -432,7 +432,7 @@ static void pci_u3_agp_class_init(ObjectClass *klass, void *data)
sbc->init = pci_u3_agp_init_device;
}
-static TypeInfo pci_u3_agp_info = {
+static const TypeInfo pci_u3_agp_info = {
.name = "u3-agp-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(UNINState),
@@ -446,7 +446,7 @@ static void pci_unin_agp_class_init(ObjectClass *klass, void *data)
sbc->init = pci_unin_agp_init_device;
}
-static TypeInfo pci_unin_agp_info = {
+static const TypeInfo pci_unin_agp_info = {
.name = "uni-north-agp-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(UNINState),
@@ -460,7 +460,7 @@ static void pci_unin_internal_class_init(ObjectClass *klass, void *data)
sbc->init = pci_unin_internal_init_device;
}
-static TypeInfo pci_unin_internal_info = {
+static const TypeInfo pci_unin_internal_info = {
.name = "uni-north-internal-pci-pcihost",
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(UNINState),
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 01/14] pci: Make host bridge TypeInfos const
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 01/14] pci: Make host bridge TypeInfos const Andreas Färber
@ 2012-07-04 21:20 ` Michael S. Tsirkin
2012-07-04 22:51 ` Andreas Färber
0 siblings, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-04 21:20 UTC (permalink / raw)
To: Andreas Färber
Cc: jbaron, qemu-devel, Alexander Graf, Andreas Färber,
open list:New World, anthony, pbonzini
On Wed, Jul 04, 2012 at 07:19:20PM +0200, Andreas Färber wrote:
> Also give the sPAPR host bridge type registration functions a unique
> name.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
I'd like to note that this is a bad practice.
Yoy write one thing in the 1 line summary
and then in the full commit log you
write about other unrelated stuff.
Make it a separate patch.
> ---
> hw/alpha_typhoon.c | 2 +-
> hw/bonito.c | 4 ++--
> hw/dec_pci.c | 6 +++---
> hw/grackle_pci.c | 4 ++--
> hw/gt64xxx.c | 4 ++--
> hw/piix_pci.c | 8 ++++----
> hw/ppc4xx_pci.c | 4 ++--
> hw/ppce500_pci.c | 4 ++--
> hw/prep_pci.c | 4 ++--
> hw/spapr_pci.c | 7 ++++---
> hw/unin_pci.c | 16 ++++++++--------
> 11 files changed, 32 insertions(+), 31 deletions(-)
>
> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
> index 872e112..cc63737 100644
> --- a/hw/alpha_typhoon.c
> +++ b/hw/alpha_typhoon.c
> @@ -817,7 +817,7 @@ static void typhoon_pcihost_class_init(ObjectClass *klass, void *data)
> dc->no_user = 1;
> }
>
> -static TypeInfo typhoon_pcihost_info = {
> +static const TypeInfo typhoon_pcihost_info = {
> .name = "typhoon-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(TyphoonState),
> diff --git a/hw/bonito.c b/hw/bonito.c
> index 77786f8..b990875 100644
> --- a/hw/bonito.c
> +++ b/hw/bonito.c
> @@ -781,7 +781,7 @@ static void bonito_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_bonito;
> }
>
> -static TypeInfo bonito_info = {
> +static const TypeInfo bonito_info = {
> .name = "Bonito",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIBonitoState),
> @@ -797,7 +797,7 @@ static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
> dc->no_user = 1;
> }
>
> -static TypeInfo bonito_pcihost_info = {
> +static const TypeInfo bonito_pcihost_info = {
> .name = "Bonito-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(BonitoState),
> diff --git a/hw/dec_pci.c b/hw/dec_pci.c
> index 37337bf..5194a9f 100644
> --- a/hw/dec_pci.c
> +++ b/hw/dec_pci.c
> @@ -66,7 +66,7 @@ static void dec_21154_pci_bridge_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_pci_device;
> }
>
> -static TypeInfo dec_21154_pci_bridge_info = {
> +static const TypeInfo dec_21154_pci_bridge_info = {
> .name = "dec-21154-p2p-bridge",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIBridge),
> @@ -119,7 +119,7 @@ static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data)
> k->is_bridge = 1;
> }
>
> -static TypeInfo dec_21154_pci_host_info = {
> +static const TypeInfo dec_21154_pci_host_info = {
> .name = "dec-21154",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -133,7 +133,7 @@ static void pci_dec_21154_device_class_init(ObjectClass *klass, void *data)
> sdc->init = pci_dec_21154_device_init;
> }
>
> -static TypeInfo pci_dec_21154_device_info = {
> +static const TypeInfo pci_dec_21154_device_info = {
> .name = "dec-21154-sysbus",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(DECState),
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index 81ff3a3..35667ad 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -134,7 +134,7 @@ static void grackle_pci_class_init(ObjectClass *klass, void *data)
> dc->no_user = 1;
> }
>
> -static TypeInfo grackle_pci_info = {
> +static const TypeInfo grackle_pci_info = {
> .name = "grackle",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -150,7 +150,7 @@ static void pci_grackle_class_init(ObjectClass *klass, void *data)
> dc->no_user = 1;
> }
>
> -static TypeInfo grackle_pci_host_info = {
> +static const TypeInfo grackle_pci_host_info = {
> .name = "grackle-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(GrackleState),
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index a2d0e5a..04831bb 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -1147,7 +1147,7 @@ static void gt64120_pci_class_init(ObjectClass *klass, void *data)
> k->class_id = PCI_CLASS_BRIDGE_HOST;
> }
>
> -static TypeInfo gt64120_pci_info = {
> +static const TypeInfo gt64120_pci_info = {
> .name = "gt64120_pci",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -1161,7 +1161,7 @@ static void gt64120_class_init(ObjectClass *klass, void *data)
> sdc->init = gt64120_init;
> }
>
> -static TypeInfo gt64120_info = {
> +static const TypeInfo gt64120_info = {
> .name = "gt64120",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(GT64120State),
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 09e84f5..5db4026 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -517,7 +517,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
> k->class_id = PCI_CLASS_BRIDGE_ISA;
> }
>
> -static TypeInfo piix3_info = {
> +static const TypeInfo piix3_info = {
> .name = "PIIX3",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PIIX3State),
> @@ -540,7 +540,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
> k->class_id = PCI_CLASS_BRIDGE_ISA;
> };
>
> -static TypeInfo piix3_xen_info = {
> +static const TypeInfo piix3_xen_info = {
> .name = "PIIX3-xen",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PIIX3State),
> @@ -564,7 +564,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_i440fx;
> }
>
> -static TypeInfo i440fx_info = {
> +static const TypeInfo i440fx_info = {
> .name = "i440FX",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCII440FXState),
> @@ -581,7 +581,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
> dc->no_user = 1;
> }
>
> -static TypeInfo i440fx_pcihost_info = {
> +static const TypeInfo i440fx_pcihost_info = {
> .name = "i440FX-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(I440FXState),
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 203c3cd..104ed98 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -377,7 +377,7 @@ static void ppc4xx_host_bridge_class_init(ObjectClass *klass, void *data)
> k->class_id = PCI_CLASS_BRIDGE_OTHER;
> }
>
> -static TypeInfo ppc4xx_host_bridge_info = {
> +static const TypeInfo ppc4xx_host_bridge_info = {
> .name = "ppc4xx-host-bridge",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -393,7 +393,7 @@ static void ppc4xx_pcihost_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_ppc4xx_pci;
> }
>
> -static TypeInfo ppc4xx_pcihost_info = {
> +static const TypeInfo ppc4xx_pcihost_info = {
> .name = "ppc4xx-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(PPC4xxPCIState),
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 0f60b24..99748b3 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -350,7 +350,7 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data)
> dc->desc = "Host bridge";
> }
>
> -static TypeInfo e500_host_bridge_info = {
> +static const TypeInfo e500_host_bridge_info = {
> .name = "e500-host-bridge",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -366,7 +366,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_ppce500_pci;
> }
>
> -static TypeInfo e500_pcihost_info = {
> +static const TypeInfo e500_pcihost_info = {
> .name = "e500-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(PPCE500PCIState),
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 38dbff4..a8cdc21 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -166,7 +166,7 @@ static void raven_class_init(ObjectClass *klass, void *data)
> dc->no_user = 1;
> }
>
> -static TypeInfo raven_info = {
> +static const TypeInfo raven_info = {
> .name = "raven",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(RavenPCIState),
> @@ -183,7 +183,7 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data)
> dc->no_user = 1;
> }
>
> -static TypeInfo raven_pcihost_info = {
> +static const TypeInfo raven_pcihost_info = {
> .name = "raven-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(PREPPCIState),
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 47ba5ff..0e0830f 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -368,7 +368,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
> spapr_rtas_register("ibm,write-pci-config", rtas_ibm_write_pci_config);
> }
>
> -static TypeInfo spapr_phb_info = {
> +static const TypeInfo spapr_phb_info = {
> .name = "spapr-pci-host-bridge",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(sPAPRPHBState),
> @@ -490,8 +490,9 @@ int spapr_populate_pci_devices(sPAPRPHBState *phb,
> return 0;
> }
>
> -static void register_types(void)
> +static void spapr_pci_register_types(void)
> {
> type_register_static(&spapr_phb_info);
> }
> -type_init(register_types)
> +
> +type_init(spapr_pci_register_types)
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 409bcd4..2b309df 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -350,7 +350,7 @@ static void unin_main_pci_host_class_init(ObjectClass *klass, void *data)
> k->class_id = PCI_CLASS_BRIDGE_HOST;
> }
>
> -static TypeInfo unin_main_pci_host_info = {
> +static const TypeInfo unin_main_pci_host_info = {
> .name = "uni-north-pci",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -368,7 +368,7 @@ static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data)
> k->class_id = PCI_CLASS_BRIDGE_HOST;
> }
>
> -static TypeInfo u3_agp_pci_host_info = {
> +static const TypeInfo u3_agp_pci_host_info = {
> .name = "u3-agp",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -386,7 +386,7 @@ static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data)
> k->class_id = PCI_CLASS_BRIDGE_HOST;
> }
>
> -static TypeInfo unin_agp_pci_host_info = {
> +static const TypeInfo unin_agp_pci_host_info = {
> .name = "uni-north-agp",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -404,7 +404,7 @@ static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data)
> k->class_id = PCI_CLASS_BRIDGE_HOST;
> }
>
> -static TypeInfo unin_internal_pci_host_info = {
> +static const TypeInfo unin_internal_pci_host_info = {
> .name = "uni-north-internal-pci",
> .parent = TYPE_PCI_DEVICE,
> .instance_size = sizeof(PCIDevice),
> @@ -418,7 +418,7 @@ static void pci_unin_main_class_init(ObjectClass *klass, void *data)
> sbc->init = pci_unin_main_init_device;
> }
>
> -static TypeInfo pci_unin_main_info = {
> +static const TypeInfo pci_unin_main_info = {
> .name = "uni-north-pci-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(UNINState),
> @@ -432,7 +432,7 @@ static void pci_u3_agp_class_init(ObjectClass *klass, void *data)
> sbc->init = pci_u3_agp_init_device;
> }
>
> -static TypeInfo pci_u3_agp_info = {
> +static const TypeInfo pci_u3_agp_info = {
> .name = "u3-agp-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(UNINState),
> @@ -446,7 +446,7 @@ static void pci_unin_agp_class_init(ObjectClass *klass, void *data)
> sbc->init = pci_unin_agp_init_device;
> }
>
> -static TypeInfo pci_unin_agp_info = {
> +static const TypeInfo pci_unin_agp_info = {
> .name = "uni-north-agp-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(UNINState),
> @@ -460,7 +460,7 @@ static void pci_unin_internal_class_init(ObjectClass *klass, void *data)
> sbc->init = pci_unin_internal_init_device;
> }
>
> -static TypeInfo pci_unin_internal_info = {
> +static const TypeInfo pci_unin_internal_info = {
> .name = "uni-north-internal-pci-pcihost",
> .parent = TYPE_SYS_BUS_DEVICE,
> .instance_size = sizeof(UNINState),
> --
> 1.7.7
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 01/14] pci: Make host bridge TypeInfos const
2012-07-04 21:20 ` Michael S. Tsirkin
@ 2012-07-04 22:51 ` Andreas Färber
2012-07-05 15:25 ` Michael S. Tsirkin
0 siblings, 1 reply; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 22:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jbaron, qemu-devel, Alexander Graf, qemu-ppc, anthony, pbonzini
Am 04.07.2012 23:20, schrieb Michael S. Tsirkin:
> On Wed, Jul 04, 2012 at 07:19:20PM +0200, Andreas Färber wrote:
>> Also give the sPAPR host bridge type registration functions a unique
>> name.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>
> I'd like to note that this is a bad practice.
> Yoy write one thing in the 1 line summary
> and then in the full commit log you
> write about other unrelated stuff.
(It seemed related by the proximity to the TypeInfo fwiw.)
> Make it a separate patch.
Do you request that for Coding Style fixes, too? See QOM'ify prep_pci
for an example that removed the only remaining violations in the file
while touching code above and below.
Background of why v2 already did multiple things at once (without anyone
complaining) was that Anthony complained that my CPU refactorings were
too long and not doing enough in one patch.
I'm already up from 2 to 14 here and don't mind splitting things up
further again but I don't think there's sufficient reason that warrants
having a patch that just renames the function for aesthetics. Would you
be okay with moving it into the corresponding QOM'ify sPAPR patch or the
final "Tidy up" patch instead?
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 01/14] pci: Make host bridge TypeInfos const
2012-07-04 22:51 ` Andreas Färber
@ 2012-07-05 15:25 ` Michael S. Tsirkin
0 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-05 15:25 UTC (permalink / raw)
To: Andreas Färber
Cc: jbaron, qemu-devel, Alexander Graf, qemu-ppc, anthony, pbonzini
On Thu, Jul 05, 2012 at 12:51:12AM +0200, Andreas Färber wrote:
> Am 04.07.2012 23:20, schrieb Michael S. Tsirkin:
> > On Wed, Jul 04, 2012 at 07:19:20PM +0200, Andreas Färber wrote:
> >> Also give the sPAPR host bridge type registration functions a unique
> >> name.
> >>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >
> > I'd like to note that this is a bad practice.
> > Yoy write one thing in the 1 line summary
> > and then in the full commit log you
> > write about other unrelated stuff.
>
> (It seemed related by the proximity to the TypeInfo fwiw.)
>
> > Make it a separate patch.
>
> Do you request that for Coding Style fixes, too? See QOM'ify prep_pci
> for an example that removed the only remaining violations in the file
> while touching code above and below.
>
> Background of why v2 already did multiple things at once (without anyone
> complaining) was that Anthony complained that my CPU refactorings were
> too long and not doing enough in one patch.
>
> I'm already up from 2 to 14 here and don't mind splitting things up
> further again but I don't think there's sufficient reason that warrants
> having a patch that just renames the function for aesthetics. Would you
> be okay with moving it into the corresponding QOM'ify sPAPR patch or the
> final "Tidy up" patch instead?
>
> Andreas
You can even leave it as is, but subject should then say
'misc cleanups in XXXX.c' ot something to that effect.
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v3 02/14] alpha_typhoon: QOM'ify Typhoon PCI host bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 01/14] pci: Make host bridge TypeInfos const Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 03/14] bonito: QOM'ify Bonito " Andreas Färber
` (11 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jbaron, Andreas Färber, anthony, mst
Introduce type constant and cast macro. Don't access DeviceState
indirectly through parent fields.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/alpha_typhoon.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
index cc63737..06e84a1 100644
--- a/hw/alpha_typhoon.c
+++ b/hw/alpha_typhoon.c
@@ -15,6 +15,8 @@
#include "exec-memory.h"
+#define TYPE_TYPHOON_PCI_HOST_BRIDGE "typhoon-pcihost"
+
typedef struct TyphoonCchip {
MemoryRegion region;
uint64_t misc;
@@ -40,8 +42,12 @@ typedef struct TyphoonPchip {
TyphoonWindow win[4];
} TyphoonPchip;
+#define TYPHOON_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(TyphoonState, (obj), TYPE_TYPHOON_PCI_HOST_BRIDGE)
+
typedef struct TyphoonState {
PCIHostState host;
+
TyphoonCchip cchip;
TyphoonPchip pchip;
MemoryRegion dchip_region;
@@ -700,16 +706,14 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
MemoryRegion *addr_space = get_system_memory();
MemoryRegion *addr_space_io = get_system_io();
DeviceState *dev;
- PCIHostState *p;
TyphoonState *s;
PCIBus *b;
int i;
- dev = qdev_create(NULL, "typhoon-pcihost");
+ dev = qdev_create(NULL, TYPE_TYPHOON_PCI_HOST_BRIDGE);
qdev_init_nofail(dev);
- p = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
- s = container_of(p, TyphoonState, host);
+ s = TYPHOON_PCI_HOST_BRIDGE(dev);
/* Remember the CPUs so that we can deliver interrupts to them. */
for (i = 0; i < 4; i++) {
@@ -763,7 +767,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
memory_region_add_subregion(addr_space, 0x801fc000000ULL,
&s->pchip.reg_io);
- b = pci_register_bus(&s->host.busdev.qdev, "pci",
+ 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;
@@ -818,7 +822,7 @@ static void typhoon_pcihost_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo typhoon_pcihost_info = {
- .name = "typhoon-pcihost",
+ .name = TYPE_TYPHOON_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(TyphoonState),
.class_init = typhoon_pcihost_class_init,
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 03/14] bonito: QOM'ify Bonito PCI host bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 01/14] pci: Make host bridge TypeInfos const Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 02/14] alpha_typhoon: QOM'ify Typhoon PCI host bridge Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 04/14] dec_pci: QOM'ify DEC 21154 PCI-PCI bridge Andreas Färber
` (10 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jbaron, Andreas Färber, anthony, mst
Introduce type constant. Avoid accessing DeviceState or SysBusDevice
indirectly through PCIHostState field.
Drop global state by passing BonitoState as opaque and adding the IRQs
and a pointer to PCIBonitoState to its state.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/bonito.c | 146 ++++++++++++++++++++++++++++++++++++-----------------------
1 files changed, 90 insertions(+), 56 deletions(-)
diff --git a/hw/bonito.c b/hw/bonito.c
index b990875..062c701 100644
--- a/hw/bonito.c
+++ b/hw/bonito.c
@@ -180,11 +180,14 @@
#define PCI_ADDR(busno,devno,funno,regno) \
((((busno)<<16)&0xff0000) + (((devno)<<11)&0xf800) + (((funno)<<8)&0x700) + (regno))
-typedef PCIHostState BonitoState;
+#define TYPE_BONITO_PCI_HOST_BRIDGE "Bonito-pcihost"
+
+typedef struct BonitoState BonitoState;
typedef struct PCIBonitoState
{
PCIDevice dev;
+
BonitoState *pcihost;
uint32_t regs[BONITO_REGS];
@@ -218,7 +221,16 @@ typedef struct PCIBonitoState
} PCIBonitoState;
-PCIBonitoState * bonito_state;
+#define BONITO_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(BonitoState, (obj), TYPE_BONITO_PCI_HOST_BRIDGE)
+
+struct BonitoState {
+ PCIHostState parent_obj;
+
+ qemu_irq *pic;
+
+ PCIBonitoState *pci_dev;
+};
static void bonito_writel(void *opaque, target_phys_addr_t addr,
uint64_t val, unsigned size)
@@ -314,9 +326,10 @@ static void bonito_pciconf_writel(void *opaque, target_phys_addr_t addr,
uint64_t val, unsigned size)
{
PCIBonitoState *s = opaque;
+ PCIDevice *d = PCI_DEVICE(s);
DPRINTF("bonito_pciconf_writel "TARGET_FMT_plx" val %x\n", addr, val);
- s->dev.config_write(&s->dev, addr, val, 4);
+ d->config_write(d, addr, val, 4);
}
static uint64_t bonito_pciconf_readl(void *opaque, target_phys_addr_t addr,
@@ -324,9 +337,10 @@ static uint64_t bonito_pciconf_readl(void *opaque, target_phys_addr_t addr,
{
PCIBonitoState *s = opaque;
+ PCIDevice *d = PCI_DEVICE(s);
DPRINTF("bonito_pciconf_readl "TARGET_FMT_plx"\n", addr);
- return s->dev.config_read(&s->dev, addr, 4);
+ return d->config_read(d, addr, 4);
}
/* north bridge PCI configure space. 0x1fe0 0000 - 0x1fe0 00ff */
@@ -402,6 +416,7 @@ static const MemoryRegionOps bonito_cop_ops = {
static uint32_t bonito_sbridge_pciaddr(void *opaque, target_phys_addr_t addr)
{
PCIBonitoState *s = opaque;
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
uint32_t cfgaddr;
uint32_t idsel;
uint32_t devno;
@@ -423,13 +438,13 @@ static uint32_t bonito_sbridge_pciaddr(void *opaque, target_phys_addr_t addr)
regno = (cfgaddr & BONITO_PCICONF_REG_MASK) >> BONITO_PCICONF_REG_OFFSET;
if (idsel == 0) {
- fprintf(stderr, "error in bonito pci config address" TARGET_FMT_plx
+ fprintf(stderr, "error in bonito pci config address " TARGET_FMT_plx
",pcimap_cfg=%x\n", addr, s->regs[BONITO_PCIMAP_CFG]);
exit(1);
}
- pciaddr = PCI_ADDR(pci_bus_num(s->pcihost->bus), devno, funno, regno);
+ pciaddr = PCI_ADDR(pci_bus_num(phb->bus), devno, funno, regno);
DPRINTF("cfgaddr %x pciaddr %x busno %x devno %d funno %d regno %d\n",
- cfgaddr, pciaddr, pci_bus_num(s->pcihost->bus), devno, funno, regno);
+ cfgaddr, pciaddr, pci_bus_num(phb->bus), devno, funno, regno);
return pciaddr;
}
@@ -438,6 +453,8 @@ static void bonito_spciconf_writeb(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
PCIBonitoState *s = opaque;
+ PCIDevice *d = PCI_DEVICE(s);
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
uint32_t pciaddr;
uint16_t status;
@@ -449,24 +466,26 @@ static void bonito_spciconf_writeb(void *opaque, target_phys_addr_t addr,
}
/* set the pci address in s->config_reg */
- s->pcihost->config_reg = (pciaddr) | (1u << 31);
- pci_data_write(s->pcihost->bus, s->pcihost->config_reg, val & 0xff, 1);
+ phb->config_reg = (pciaddr) | (1u << 31);
+ pci_data_write(phb->bus, phb->config_reg, val & 0xff, 1);
/* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
- status = pci_get_word(s->dev.config + PCI_STATUS);
+ status = pci_get_word(d->config + PCI_STATUS);
status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
- pci_set_word(s->dev.config + PCI_STATUS, status);
+ pci_set_word(d->config + PCI_STATUS, status);
}
static void bonito_spciconf_writew(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
PCIBonitoState *s = opaque;
+ PCIDevice *d = PCI_DEVICE(s);
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
uint32_t pciaddr;
uint16_t status;
DPRINTF("bonito_spciconf_writew "TARGET_FMT_plx" val %x\n", addr, val);
- assert((addr&0x1)==0);
+ assert((addr & 0x1) == 0);
pciaddr = bonito_sbridge_pciaddr(s, addr);
@@ -475,24 +494,26 @@ static void bonito_spciconf_writew(void *opaque, target_phys_addr_t addr,
}
/* set the pci address in s->config_reg */
- s->pcihost->config_reg = (pciaddr) | (1u << 31);
- pci_data_write(s->pcihost->bus, s->pcihost->config_reg, val, 2);
+ phb->config_reg = (pciaddr) | (1u << 31);
+ pci_data_write(phb->bus, phb->config_reg, val, 2);
/* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
- status = pci_get_word(s->dev.config + PCI_STATUS);
+ status = pci_get_word(d->config + PCI_STATUS);
status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
- pci_set_word(s->dev.config + PCI_STATUS, status);
+ pci_set_word(d->config + PCI_STATUS, status);
}
static void bonito_spciconf_writel(void *opaque, target_phys_addr_t addr,
uint32_t val)
{
PCIBonitoState *s = opaque;
+ PCIDevice *d = PCI_DEVICE(s);
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
uint32_t pciaddr;
uint16_t status;
DPRINTF("bonito_spciconf_writel "TARGET_FMT_plx" val %x\n", addr, val);
- assert((addr&0x3)==0);
+ assert((addr & 0x3) == 0);
pciaddr = bonito_sbridge_pciaddr(s, addr);
@@ -501,18 +522,20 @@ static void bonito_spciconf_writel(void *opaque, target_phys_addr_t addr,
}
/* set the pci address in s->config_reg */
- s->pcihost->config_reg = (pciaddr) | (1u << 31);
- pci_data_write(s->pcihost->bus, s->pcihost->config_reg, val, 4);
+ phb->config_reg = (pciaddr) | (1u << 31);
+ pci_data_write(phb->bus, phb->config_reg, val, 4);
/* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
- status = pci_get_word(s->dev.config + PCI_STATUS);
+ status = pci_get_word(d->config + PCI_STATUS);
status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
- pci_set_word(s->dev.config + PCI_STATUS, status);
+ pci_set_word(d->config + PCI_STATUS, status);
}
static uint32_t bonito_spciconf_readb(void *opaque, target_phys_addr_t addr)
{
PCIBonitoState *s = opaque;
+ PCIDevice *d = PCI_DEVICE(s);
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
uint32_t pciaddr;
uint16_t status;
@@ -524,24 +547,26 @@ static uint32_t bonito_spciconf_readb(void *opaque, target_phys_addr_t addr)
}
/* set the pci address in s->config_reg */
- s->pcihost->config_reg = (pciaddr) | (1u << 31);
+ phb->config_reg = (pciaddr) | (1u << 31);
/* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
- status = pci_get_word(s->dev.config + PCI_STATUS);
+ status = pci_get_word(d->config + PCI_STATUS);
status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
- pci_set_word(s->dev.config + PCI_STATUS, status);
+ pci_set_word(d->config + PCI_STATUS, status);
- return pci_data_read(s->pcihost->bus, s->pcihost->config_reg, 1);
+ return pci_data_read(phb->bus, phb->config_reg, 1);
}
static uint32_t bonito_spciconf_readw(void *opaque, target_phys_addr_t addr)
{
PCIBonitoState *s = opaque;
+ PCIDevice *d = PCI_DEVICE(s);
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
uint32_t pciaddr;
uint16_t status;
DPRINTF("bonito_spciconf_readw "TARGET_FMT_plx"\n", addr);
- assert((addr&0x1)==0);
+ assert((addr & 0x1) == 0);
pciaddr = bonito_sbridge_pciaddr(s, addr);
@@ -550,24 +575,26 @@ static uint32_t bonito_spciconf_readw(void *opaque, target_phys_addr_t addr)
}
/* set the pci address in s->config_reg */
- s->pcihost->config_reg = (pciaddr) | (1u << 31);
+ phb->config_reg = (pciaddr) | (1u << 31);
/* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
- status = pci_get_word(s->dev.config + PCI_STATUS);
+ status = pci_get_word(d->config + PCI_STATUS);
status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
- pci_set_word(s->dev.config + PCI_STATUS, status);
+ pci_set_word(d->config + PCI_STATUS, status);
- return pci_data_read(s->pcihost->bus, s->pcihost->config_reg, 2);
+ return pci_data_read(phb->bus, phb->config_reg, 2);
}
static uint32_t bonito_spciconf_readl(void *opaque, target_phys_addr_t addr)
{
PCIBonitoState *s = opaque;
+ PCIDevice *d = PCI_DEVICE(s);
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
uint32_t pciaddr;
uint16_t status;
DPRINTF("bonito_spciconf_readl "TARGET_FMT_plx"\n", addr);
- assert((addr&0x3) == 0);
+ assert((addr & 0x3) == 0);
pciaddr = bonito_sbridge_pciaddr(s, addr);
@@ -576,14 +603,14 @@ static uint32_t bonito_spciconf_readl(void *opaque, target_phys_addr_t addr)
}
/* set the pci address in s->config_reg */
- s->pcihost->config_reg = (pciaddr) | (1u << 31);
+ phb->config_reg = (pciaddr) | (1u << 31);
/* clear PCI_STATUS_REC_MASTER_ABORT and PCI_STATUS_REC_TARGET_ABORT */
- status = pci_get_word(s->dev.config + PCI_STATUS);
+ status = pci_get_word(d->config + PCI_STATUS);
status &= ~(PCI_STATUS_REC_MASTER_ABORT | PCI_STATUS_REC_TARGET_ABORT);
- pci_set_word(s->dev.config + PCI_STATUS, status);
+ pci_set_word(d->config + PCI_STATUS, status);
- return pci_data_read(s->pcihost->bus, s->pcihost->config_reg, 4);
+ return pci_data_read(phb->bus, phb->config_reg, 4);
}
/* south bridge PCI configure space. 0x1fe8 0000 - 0x1fef ffff */
@@ -607,13 +634,15 @@ static const MemoryRegionOps bonito_spciconf_ops = {
static void pci_bonito_set_irq(void *opaque, int irq_num, int level)
{
- qemu_irq *pic = opaque;
+ BonitoState *s = opaque;
+ qemu_irq *pic = s->pic;
+ PCIBonitoState *bonito_state = s->pci_dev;
int internal_irq = irq_num - BONITO_IRQ_BASE;
- if (bonito_state->regs[BONITO_INTEDGE] & (1<<internal_irq)) {
+ if (bonito_state->regs[BONITO_INTEDGE] & (1 << internal_irq)) {
qemu_irq_pulse(*pic);
} else { /* level triggered */
- if (bonito_state->regs[BONITO_INTPOL] & (1<<internal_irq)) {
+ if (bonito_state->regs[BONITO_INTPOL] & (1 << internal_irq)) {
qemu_irq_raise(*pic);
} else {
qemu_irq_lower(*pic);
@@ -673,13 +702,21 @@ static const VMStateDescription vmstate_bonito = {
static int bonito_pcihost_initfn(SysBusDevice *dev)
{
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, dev);
+
+ phb->bus = pci_register_bus(DEVICE(dev), "pci",
+ pci_bonito_set_irq, pci_bonito_map_irq, dev,
+ get_system_memory(), get_system_io(),
+ 0x28, 32);
+
return 0;
}
static int bonito_initfn(PCIDevice *dev)
{
PCIBonitoState *s = DO_UPCAST(PCIBonitoState, dev, dev);
- SysBusDevice *sysbus = &s->pcihost->busdev;
+ SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
+ PCIHostState *phb = FROM_SYSBUS(PCIHostState, sysbus);
/* Bonito North Bridge, built on FPGA, VENDOR_ID/DEVICE_ID are "undefined" */
pci_config_set_prog_interface(dev->config, 0x00);
@@ -691,15 +728,15 @@ static int bonito_initfn(PCIDevice *dev)
sysbus_mmio_map(sysbus, 0, BONITO_INTERNAL_REG_BASE);
/* set the north bridge pci configure mapping */
- memory_region_init_io(&s->pcihost->conf_mem, &bonito_pciconf_ops, s,
+ memory_region_init_io(&phb->conf_mem, &bonito_pciconf_ops, s,
"north-bridge-pci-config", BONITO_PCICONFIG_SIZE);
- sysbus_init_mmio(sysbus, &s->pcihost->conf_mem);
+ sysbus_init_mmio(sysbus, &phb->conf_mem);
sysbus_mmio_map(sysbus, 1, BONITO_PCICONFIG_BASE);
/* set the south bridge pci configure mapping */
- memory_region_init_io(&s->pcihost->data_mem, &bonito_spciconf_ops, s,
+ memory_region_init_io(&phb->data_mem, &bonito_spciconf_ops, s,
"south-bridge-pci-config", BONITO_SPCICONFIG_SIZE);
- sysbus_init_mmio(sysbus, &s->pcihost->data_mem);
+ sysbus_init_mmio(sysbus, &phb->data_mem);
sysbus_mmio_map(sysbus, 2, BONITO_SPCICONFIG_BASE);
memory_region_init_io(&s->iomem_ldma, &bonito_ldma_ops, s,
@@ -742,28 +779,25 @@ static int bonito_initfn(PCIDevice *dev)
PCIBus *bonito_init(qemu_irq *pic)
{
DeviceState *dev;
- PCIBus *b;
BonitoState *pcihost;
+ PCIHostState *phb;
PCIBonitoState *s;
PCIDevice *d;
- dev = qdev_create(NULL, "Bonito-pcihost");
- pcihost = FROM_SYSBUS(BonitoState, sysbus_from_qdev(dev));
- b = pci_register_bus(&pcihost->busdev.qdev, "pci", pci_bonito_set_irq,
- pci_bonito_map_irq, pic, get_system_memory(),
- get_system_io(),
- 0x28, 32);
- pcihost->bus = b;
+ dev = qdev_create(NULL, TYPE_BONITO_PCI_HOST_BRIDGE);
+ phb = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
+ pcihost = BONITO_PCI_HOST_BRIDGE(dev);
+ pcihost->pic = pic;
qdev_init_nofail(dev);
/* set the pcihost pointer before bonito_initfn is called */
- d = pci_create(b, PCI_DEVFN(0, 0), "Bonito");
+ d = pci_create(phb->bus, PCI_DEVFN(0, 0), "Bonito");
s = DO_UPCAST(PCIBonitoState, dev, d);
s->pcihost = pcihost;
- bonito_state = s;
- qdev_init_nofail(&d->qdev);
+ pcihost->pci_dev = s;
+ qdev_init_nofail(DEVICE(d));
- return b;
+ return phb->bus;
}
static void bonito_class_init(ObjectClass *klass, void *data)
@@ -798,7 +832,7 @@ static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo bonito_pcihost_info = {
- .name = "Bonito-pcihost",
+ .name = TYPE_BONITO_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(BonitoState),
.class_init = bonito_pcihost_class_init,
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 04/14] dec_pci: QOM'ify DEC 21154 PCI-PCI bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (2 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 03/14] bonito: QOM'ify Bonito " Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 05/14] grackle_pci: QOM'ify Grackle PCI host bridge Andreas Färber
` (9 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: mst, jbaron, Alexander Graf, open list:New World, anthony,
pbonzini, Andreas Färber
Introduce type constant. Introduce cast macro and drop dummy busdev
field used with FROM_SYSBUS() that would've broken SYS_BUS_DEVICE().
Avoid accessing parent fields directly.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/dec_pci.c | 21 +++++++++++----------
hw/dec_pci.h | 2 ++
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index 5194a9f..19aed1b 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -40,8 +40,9 @@
#define DEC_DPRINTF(fmt, ...)
#endif
+#define DEC_21154(obj) OBJECT_CHECK(DECState, (obj), TYPE_DEC_21154)
+
typedef struct DECState {
- SysBusDevice busdev;
PCIHostState host_state;
} DECState;
@@ -88,16 +89,16 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn)
static int pci_dec_21154_device_init(SysBusDevice *dev)
{
- DECState *s;
+ PCIHostState *phb;
- s = FROM_SYSBUS(DECState, dev);
+ phb = FROM_SYSBUS(PCIHostState, dev);
- memory_region_init_io(&s->host_state.conf_mem, &pci_host_conf_le_ops,
- &s->host_state, "pci-conf-idx", 0x1000);
- memory_region_init_io(&s->host_state.data_mem, &pci_host_data_le_ops,
- &s->host_state, "pci-data-idx", 0x1000);
- sysbus_init_mmio(dev, &s->host_state.conf_mem);
- sysbus_init_mmio(dev, &s->host_state.data_mem);
+ memory_region_init_io(&phb->conf_mem, &pci_host_conf_le_ops,
+ dev, "pci-conf-idx", 0x1000);
+ memory_region_init_io(&phb->data_mem, &pci_host_data_le_ops,
+ dev, "pci-data-idx", 0x1000);
+ sysbus_init_mmio(dev, &phb->conf_mem);
+ sysbus_init_mmio(dev, &phb->data_mem);
return 0;
}
@@ -134,7 +135,7 @@ static void pci_dec_21154_device_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo pci_dec_21154_device_info = {
- .name = "dec-21154-sysbus",
+ .name = TYPE_DEC_21154,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(DECState),
.class_init = pci_dec_21154_device_class_init,
diff --git a/hw/dec_pci.h b/hw/dec_pci.h
index 79264ba..17dc0c2 100644
--- a/hw/dec_pci.h
+++ b/hw/dec_pci.h
@@ -3,6 +3,8 @@
#include "qemu-common.h"
+#define TYPE_DEC_21154 "dec-21154-sysbus"
+
PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn);
#endif
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 05/14] grackle_pci: QOM'ify Grackle PCI host bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (3 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 04/14] dec_pci: QOM'ify DEC 21154 PCI-PCI bridge Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 06/14] gt64xxx: QOM'ify GT64120 " Andreas Färber
` (8 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: mst, jbaron, Alexander Graf, open list:Old World, anthony,
pbonzini, Andreas Färber
Introduce type constant. Introduce cast macro to drop dummy busdev field
used with FROM_SYSBUS() that would've broken SYS_BUS_DEVICE().
Avoid accessing parent fields directly.
Drop no-op reset function.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/grackle_pci.c | 57 ++++++++++++++++++++++++++---------------------------
hw/ppc_mac.h | 1 +
2 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index 35667ad..d814270 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -23,10 +23,9 @@
* THE SOFTWARE.
*/
-#include "sysbus.h"
+#include "pci_host.h"
#include "ppc_mac.h"
#include "pci.h"
-#include "pci_host.h"
/* debug Grackle */
//#define DEBUG_GRACKLE
@@ -38,9 +37,12 @@
#define GRACKLE_DPRINTF(fmt, ...)
#endif
+#define GRACKLE_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(GrackleState, (obj), TYPE_GRACKLE_PCI_HOST_BRIDGE)
+
typedef struct GrackleState {
- SysBusDevice busdev;
PCIHostState host_state;
+
MemoryRegion pci_mmio;
MemoryRegion pci_hole;
} GrackleState;
@@ -59,22 +61,20 @@ static void pci_grackle_set_irq(void *opaque, int irq_num, int level)
qemu_set_irq(pic[irq_num + 0x15], level);
}
-static void pci_grackle_reset(void *opaque)
-{
-}
-
PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io)
{
DeviceState *dev;
SysBusDevice *s;
+ PCIHostState *phb;
GrackleState *d;
- dev = qdev_create(NULL, "grackle-pcihost");
+ dev = qdev_create(NULL, TYPE_GRACKLE_PCI_HOST_BRIDGE);
qdev_init_nofail(dev);
- s = sysbus_from_qdev(dev);
- d = FROM_SYSBUS(GrackleState, s);
+ s = SYS_BUS_DEVICE(dev);
+ phb = FROM_SYSBUS(PCIHostState, s);
+ d = GRACKLE_PCI_HOST_BRIDGE(dev);
memory_region_init(&d->pci_mmio, "pci-mmio", 0x100000000ULL);
memory_region_init_alias(&d->pci_hole, "pci-hole", &d->pci_mmio,
@@ -82,36 +82,35 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
memory_region_add_subregion(address_space_mem, 0x80000000ULL,
&d->pci_hole);
- d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci",
- pci_grackle_set_irq,
- pci_grackle_map_irq,
- pic,
- &d->pci_mmio,
- address_space_io,
- 0, 4);
+ phb->bus = pci_register_bus(dev, "pci",
+ pci_grackle_set_irq,
+ pci_grackle_map_irq,
+ pic,
+ &d->pci_mmio,
+ address_space_io,
+ 0, 4);
- pci_create_simple(d->host_state.bus, 0, "grackle");
+ pci_create_simple(phb->bus, 0, "grackle");
sysbus_mmio_map(s, 0, base);
sysbus_mmio_map(s, 1, base + 0x00200000);
- return d->host_state.bus;
+ return phb->bus;
}
static int pci_grackle_init_device(SysBusDevice *dev)
{
- GrackleState *s;
+ PCIHostState *phb;
- s = FROM_SYSBUS(GrackleState, dev);
+ phb = FROM_SYSBUS(PCIHostState, dev);
- memory_region_init_io(&s->host_state.conf_mem, &pci_host_conf_le_ops,
- &s->host_state, "pci-conf-idx", 0x1000);
- memory_region_init_io(&s->host_state.data_mem, &pci_host_data_le_ops,
- &s->host_state, "pci-data-idx", 0x1000);
- sysbus_init_mmio(dev, &s->host_state.conf_mem);
- sysbus_init_mmio(dev, &s->host_state.data_mem);
+ memory_region_init_io(&phb->conf_mem, &pci_host_conf_le_ops,
+ dev, "pci-conf-idx", 0x1000);
+ memory_region_init_io(&phb->data_mem, &pci_host_data_le_ops,
+ dev, "pci-data-idx", 0x1000);
+ sysbus_init_mmio(dev, &phb->conf_mem);
+ sysbus_init_mmio(dev, &phb->data_mem);
- qemu_register_reset(pci_grackle_reset, &s->host_state);
return 0;
}
@@ -151,7 +150,7 @@ static void pci_grackle_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo grackle_pci_host_info = {
- .name = "grackle-pcihost",
+ .name = TYPE_GRACKLE_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(GrackleState),
.class_init = pci_grackle_class_init,
diff --git a/hw/ppc_mac.h b/hw/ppc_mac.h
index af75e45..7d08418 100644
--- a/hw/ppc_mac.h
+++ b/hw/ppc_mac.h
@@ -55,6 +55,7 @@ qemu_irq *heathrow_pic_init(MemoryRegion **pmem,
int nb_cpus, qemu_irq **irqs);
/* Grackle PCI */
+#define TYPE_GRACKLE_PCI_HOST_BRIDGE "grackle-pcihost"
PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
MemoryRegion *address_space_mem,
MemoryRegion *address_space_io);
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 06/14] gt64xxx: QOM'ify GT64120 PCI host bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (4 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 05/14] grackle_pci: QOM'ify Grackle PCI host bridge Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 07/14] ppc4xx_pci: QOM'ify ppc4xx " Andreas Färber
` (7 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jbaron, Andreas Färber, anthony, mst
Introduce type constant. Introduce cast macro to drop dummy busdev field
used with FROM_SYSBUS() macro that would've broken SYS_BUS_DEVICE().
Avoid accessing DeviceState indirectly through PCIHostState.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/gt64xxx.c | 35 ++++++++++++++++++++---------------
1 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index 04831bb..0677299 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -229,9 +229,14 @@
target_phys_addr_t regname ##_length; \
MemoryRegion regname ##_mem
+#define TYPE_GT64120_PCI_HOST_BRIDGE "gt64120"
+
+#define GT64120_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(GT64120State, (obj), TYPE_GT64120_PCI_HOST_BRIDGE)
+
typedef struct GT64120State {
- SysBusDevice busdev;
PCIHostState pci;
+
uint32_t regs[GT_REGS];
PCI_MAPPING_ENTRY(PCI0IO);
PCI_MAPPING_ENTRY(ISD);
@@ -1083,31 +1088,31 @@ static void gt64120_reset(void *opaque)
PCIBus *gt64120_register(qemu_irq *pic)
{
- SysBusDevice *s;
GT64120State *d;
+ PCIHostState *phb;
DeviceState *dev;
- dev = qdev_create(NULL, "gt64120");
+ dev = qdev_create(NULL, TYPE_GT64120_PCI_HOST_BRIDGE);
qdev_init_nofail(dev);
- s = sysbus_from_qdev(dev);
- d = FROM_SYSBUS(GT64120State, s);
- d->pci.bus = pci_register_bus(&d->busdev.qdev, "pci",
- gt64120_pci_set_irq, gt64120_pci_map_irq,
- pic,
- get_system_memory(),
- get_system_io(),
- PCI_DEVFN(18, 0), 4);
+ d = GT64120_PCI_HOST_BRIDGE(dev);
+ phb = &d->pci;
+ phb->bus = pci_register_bus(dev, "pci",
+ gt64120_pci_set_irq, gt64120_pci_map_irq,
+ pic,
+ get_system_memory(),
+ get_system_io(),
+ PCI_DEVFN(18, 0), 4);
memory_region_init_io(&d->ISD_mem, &isd_mem_ops, d, "isd-mem", 0x1000);
- pci_create_simple(d->pci.bus, PCI_DEVFN(0, 0), "gt64120_pci");
- return d->pci.bus;
+ pci_create_simple(phb->bus, PCI_DEVFN(0, 0), "gt64120_pci");
+ return phb->bus;
}
static int gt64120_init(SysBusDevice *dev)
{
GT64120State *s;
- s = FROM_SYSBUS(GT64120State, dev);
+ s = GT64120_PCI_HOST_BRIDGE(dev);
/* FIXME: This value is computed from registers during reset, but some
devices (e.g. VGA card) need to know it when they are registered.
@@ -1162,7 +1167,7 @@ static void gt64120_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo gt64120_info = {
- .name = "gt64120",
+ .name = TYPE_GT64120_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(GT64120State),
.class_init = gt64120_class_init,
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 07/14] ppc4xx_pci: QOM'ify ppc4xx PCI host bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (5 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 06/14] gt64xxx: QOM'ify GT64120 " Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 08/14] ppce500_pci: QOM'ify e500 " Andreas Färber
` (6 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jbaron, Andreas Färber, anthony, mst
Introduce type constant and cast macro. Avoid accessing its parent field
directly.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/ppc440_bamboo.c | 3 ++-
hw/ppc4xx.h | 2 ++
hw/ppc4xx_pci.c | 13 ++++++++-----
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/hw/ppc440_bamboo.c b/hw/ppc440_bamboo.c
index 0dd4dab..c198071 100644
--- a/hw/ppc440_bamboo.c
+++ b/hw/ppc440_bamboo.c
@@ -216,7 +216,8 @@ static void bamboo_init(ram_addr_t ram_size,
ram_bases, ram_sizes, 1);
/* PCI */
- dev = sysbus_create_varargs("ppc4xx-pcihost", PPC440EP_PCI_CONFIG,
+ dev = sysbus_create_varargs(TYPE_PPC4xx_PCI_HOST_BRIDGE,
+ PPC440EP_PCI_CONFIG,
pic[pci_irq_nrs[0]], pic[pci_irq_nrs[1]],
pic[pci_irq_nrs[2]], pic[pci_irq_nrs[3]],
NULL);
diff --git a/hw/ppc4xx.h b/hw/ppc4xx.h
index b511020..5cd78b6 100644
--- a/hw/ppc4xx.h
+++ b/hw/ppc4xx.h
@@ -53,6 +53,8 @@ void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq, int nbanks,
target_phys_addr_t *ram_sizes,
int do_init);
+#define TYPE_PPC4xx_PCI_HOST_BRIDGE "ppc4xx-pcihost"
+
PCIBus *ppc4xx_pci_init(CPUPPCState *env, qemu_irq pci_irqs[4],
target_phys_addr_t config_space,
target_phys_addr_t int_ack,
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 104ed98..599a75a 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -45,6 +45,9 @@ struct PCITargetMap {
uint32_t la;
};
+#define PPC4xx_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(PPC4xxPCIState, (obj), TYPE_PPC4xx_PCI_HOST_BRIDGE)
+
#define PPC4xx_PCI_NR_PMMS 3
#define PPC4xx_PCI_NR_PTMS 2
@@ -335,17 +338,17 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
PCIBus *b;
int i;
- h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
- s = DO_UPCAST(PPC4xxPCIState, pci_state, h);
+ h = FROM_SYSBUS(PCIHostState, dev);
+ s = PPC4xx_PCI_HOST_BRIDGE(dev);
for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
sysbus_init_irq(dev, &s->irq[i]);
}
- b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, ppc4xx_pci_set_irq,
+ b = pci_register_bus(DEVICE(dev), NULL, ppc4xx_pci_set_irq,
ppc4xx_pci_map_irq, s->irq, get_system_memory(),
get_system_io(), 0, 4);
- s->pci_state.bus = b;
+ h->bus = b;
pci_create_simple(b, 0, "ppc4xx-host-bridge");
@@ -394,7 +397,7 @@ static void ppc4xx_pcihost_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo ppc4xx_pcihost_info = {
- .name = "ppc4xx-pcihost",
+ .name = TYPE_PPC4xx_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(PPC4xxPCIState),
.class_init = ppc4xx_pcihost_class_init,
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 08/14] ppce500_pci: QOM'ify e500 PCI host bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (6 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 07/14] ppc4xx_pci: QOM'ify ppc4xx " Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 09/14] prep_pci: QOM'ify Raven " Andreas Färber
` (5 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jbaron, Andreas Färber, anthony, mst
Introduce type constant and cast macro. Avoid accessing parent fields
directly.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/ppce500_pci.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 99748b3..e4f065a 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -72,8 +72,14 @@ struct pci_inbound {
uint32_t piwar;
};
+#define TYPE_PPC_E500_PCI_HOST_BRIDGE "e500-pcihost"
+
+#define PPC_E500_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(PPCE500PCIState, (obj), TYPE_PPC_E500_PCI_HOST_BRIDGE)
+
struct PPCE500PCIState {
PCIHostState pci_state;
+
struct pci_outbound pob[PPCE500_PCI_NR_POBS];
struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
uint32_t gasket_time;
@@ -310,17 +316,17 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
MemoryRegion *address_space_mem = get_system_memory();
MemoryRegion *address_space_io = get_system_io();
- h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
- s = DO_UPCAST(PPCE500PCIState, pci_state, h);
+ h = FROM_SYSBUS(PCIHostState, dev);
+ s = PPC_E500_PCI_HOST_BRIDGE(dev);
for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
sysbus_init_irq(dev, &s->irq[i]);
}
- b = pci_register_bus(&s->pci_state.busdev.qdev, NULL, mpc85xx_pci_set_irq,
+ b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
mpc85xx_pci_map_irq, s->irq, address_space_mem,
address_space_io, PCI_DEVFN(0x11, 0), 4);
- s->pci_state.bus = b;
+ h->bus = b;
pci_create_simple(b, 0, "e500-host-bridge");
@@ -367,7 +373,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo e500_pcihost_info = {
- .name = "e500-pcihost",
+ .name = TYPE_PPC_E500_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(PPCE500PCIState),
.class_init = e500_pcihost_class_init,
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 09/14] prep_pci: QOM'ify Raven PCI host bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (7 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 08/14] ppce500_pci: QOM'ify e500 " Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 10/14] spapr_pci: QOM'ify sPAPR " Andreas Färber
` (4 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: mst, jbaron, Andreas Färber, open list:PReP, anthony,
pbonzini, Andreas Färber
Introduce type constant and cast macro. Avoid accessing parent fields
directly.
Also add missing space and braces.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/prep_pci.c | 17 ++++++++++++-----
1 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index a8cdc21..69c19df 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -28,8 +28,14 @@
#include "pc.h"
#include "exec-memory.h"
+#define TYPE_RAVEN_PCI_HOST_BRIDGE "raven-pcihost"
+
+#define RAVEN_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(PREPPCIState, (obj), TYPE_RAVEN_PCI_HOST_BRIDGE)
+
typedef struct PRePPCIState {
PCIHostState host_state;
+
MemoryRegion intack;
qemu_irq irq[4];
} PREPPCIState;
@@ -42,9 +48,10 @@ static inline uint32_t PPC_PCIIO_config(target_phys_addr_t addr)
{
int i;
- for(i = 0; i < 11; i++) {
- if ((addr & (1 << (11 + i))) != 0)
+ for (i = 0; i < 11; i++) {
+ if ((addr & (1 << (11 + i))) != 0) {
break;
+ }
}
return (addr & 0x7ff) | (i << 11);
}
@@ -97,7 +104,7 @@ static void prep_set_irq(void *opaque, int irq_num, int level)
static int raven_pcihost_init(SysBusDevice *dev)
{
PCIHostState *h = FROM_SYSBUS(PCIHostState, dev);
- PREPPCIState *s = DO_UPCAST(PREPPCIState, host_state, h);
+ PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
MemoryRegion *address_space_mem = get_system_memory();
MemoryRegion *address_space_io = get_system_io();
PCIBus *bus;
@@ -107,7 +114,7 @@ static int raven_pcihost_init(SysBusDevice *dev)
sysbus_init_irq(dev, &s->irq[i]);
}
- bus = pci_register_bus(&h->busdev.qdev, NULL,
+ bus = pci_register_bus(DEVICE(dev), NULL,
prep_set_irq, prep_map_irq, s->irq,
address_space_mem, address_space_io, 0, 4);
h->bus = bus;
@@ -184,7 +191,7 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo raven_pcihost_info = {
- .name = "raven-pcihost",
+ .name = TYPE_RAVEN_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(PREPPCIState),
.class_init = raven_pcihost_class_init,
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 10/14] spapr_pci: QOM'ify sPAPR PCI host bridge
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (8 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 09/14] prep_pci: QOM'ify Raven " Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 11/14] unin_pci: QOM'ify UniNorth PCI host bridges Andreas Färber
` (3 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, jbaron, Andreas Färber, anthony, mst
Introduce type constant. Introduce cast macro to drop bogus busdev field
that would've broken SYS_BUS_DEVICE().
Signed-off-by: Andreas Färber <afaerber@suse.de>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
---
hw/spapr_pci.c | 8 ++++----
hw/spapr_pci.h | 6 +++++-
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 0e0830f..dd2a72d 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -276,7 +276,7 @@ static DMAContext *spapr_pci_dma_context_fn(PCIBus *bus, void *opaque,
static int spapr_phb_init(SysBusDevice *s)
{
- sPAPRPHBState *phb = FROM_SYSBUS(sPAPRPHBState, s);
+ sPAPRPHBState *phb = SPAPR_PCI_HOST_BRIDGE(s);
char *namebuf;
int i;
PCIBus *bus;
@@ -314,7 +314,7 @@ static int spapr_phb_init(SysBusDevice *s)
memory_region_add_subregion(get_system_memory(), phb->io_win_addr,
&phb->iowindow);
- bus = pci_register_bus(&phb->busdev.qdev,
+ bus = pci_register_bus(DEVICE(s),
phb->busname ? phb->busname : phb->dtbusname,
pci_spapr_set_irq, pci_spapr_map_irq, phb,
&phb->memspace, &phb->iospace,
@@ -369,7 +369,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo spapr_phb_info = {
- .name = "spapr-pci-host-bridge",
+ .name = TYPE_SPAPR_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(sPAPRPHBState),
.class_init = spapr_phb_class_init,
@@ -382,7 +382,7 @@ void spapr_create_phb(sPAPREnvironment *spapr,
{
DeviceState *dev;
- dev = qdev_create(NULL, spapr_phb_info.name);
+ dev = qdev_create(NULL, TYPE_SPAPR_PCI_HOST_BRIDGE);
if (busname) {
qdev_prop_set_string(dev, "busname", g_strdup(busname));
diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
index d9e46e2..06e2742 100644
--- a/hw/spapr_pci.h
+++ b/hw/spapr_pci.h
@@ -27,8 +27,12 @@
#include "hw/pci_host.h"
#include "hw/xics.h"
+#define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
+
+#define SPAPR_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
+
typedef struct sPAPRPHBState {
- SysBusDevice busdev;
PCIHostState host_state;
uint64_t buid;
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 11/14] unin_pci: QOM'ify UniNorth PCI host bridges
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (9 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 10/14] spapr_pci: QOM'ify sPAPR " Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 12/14] pci_host: Turn into SysBus-derived QOM type Andreas Färber
` (2 subsequent siblings)
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: mst, jbaron, Alexander Graf, open list:New World, anthony,
pbonzini, Andreas Färber
Introduce type constants and cast macros.
Avoid accessing parent fields directly.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/unin_pci.c | 133 ++++++++++++++++++++++++++++++---------------------------
1 files changed, 70 insertions(+), 63 deletions(-)
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 2b309df..1fc8920 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -38,8 +38,23 @@
static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
+#define TYPE_UNI_NORTH_PCI_HOST_BRIDGE "uni-north-pci-pcihost"
+#define TYPE_UNI_NORTH_AGP_HOST_BRIDGE "uni-north-agp-pcihost"
+#define TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE "uni-north-internal-pci-pcihost"
+#define TYPE_U3_AGP_HOST_BRIDGE "u3-agp-pcihost"
+
+#define UNI_NORTH_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(UNINState, (obj), TYPE_UNI_NORTH_PCI_HOST_BRIDGE)
+#define UNI_NORTH_AGP_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(UNINState, (obj), TYPE_UNI_NORTH_AGP_HOST_BRIDGE)
+#define UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(UNINState, (obj), TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE)
+#define U3_AGP_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(UNINState, (obj), TYPE_U3_AGP_HOST_BRIDGE)
+
typedef struct UNINState {
PCIHostState host_state;
+
MemoryRegion pci_mmio;
MemoryRegion pci_hole;
} UNINState;
@@ -130,19 +145,17 @@ static const MemoryRegionOps unin_data_ops = {
static int pci_unin_main_init_device(SysBusDevice *dev)
{
PCIHostState *h;
- UNINState *s;
/* Use values found on a real PowerMac */
/* Uninorth main bus */
h = FROM_SYSBUS(PCIHostState, dev);
- s = DO_UPCAST(UNINState, host_state, h);
- memory_region_init_io(&s->host_state.conf_mem, &pci_host_conf_le_ops,
- &s->host_state, "pci-conf-idx", 0x1000);
- memory_region_init_io(&s->host_state.data_mem, &unin_data_ops, s,
+ memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops,
+ dev, "pci-conf-idx", 0x1000);
+ memory_region_init_io(&h->data_mem, &unin_data_ops, dev,
"pci-conf-data", 0x1000);
- sysbus_init_mmio(dev, &s->host_state.conf_mem);
- sysbus_init_mmio(dev, &s->host_state.data_mem);
+ sysbus_init_mmio(dev, &h->conf_mem);
+ sysbus_init_mmio(dev, &h->data_mem);
return 0;
}
@@ -151,18 +164,16 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
static int pci_u3_agp_init_device(SysBusDevice *dev)
{
PCIHostState *h;
- UNINState *s;
/* Uninorth U3 AGP bus */
h = FROM_SYSBUS(PCIHostState, dev);
- s = DO_UPCAST(UNINState, host_state, h);
- memory_region_init_io(&s->host_state.conf_mem, &pci_host_conf_le_ops,
- &s->host_state, "pci-conf-idx", 0x1000);
- memory_region_init_io(&s->host_state.data_mem, &unin_data_ops, s,
+ memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops,
+ dev, "pci-conf-idx", 0x1000);
+ memory_region_init_io(&h->data_mem, &unin_data_ops, dev,
"pci-conf-data", 0x1000);
- sysbus_init_mmio(dev, &s->host_state.conf_mem);
- sysbus_init_mmio(dev, &s->host_state.data_mem);
+ sysbus_init_mmio(dev, &h->conf_mem);
+ sysbus_init_mmio(dev, &h->data_mem);
return 0;
}
@@ -170,36 +181,32 @@ static int pci_u3_agp_init_device(SysBusDevice *dev)
static int pci_unin_agp_init_device(SysBusDevice *dev)
{
PCIHostState *h;
- UNINState *s;
/* Uninorth AGP bus */
h = FROM_SYSBUS(PCIHostState, dev);
- s = DO_UPCAST(UNINState, host_state, h);
- memory_region_init_io(&s->host_state.conf_mem, &pci_host_conf_le_ops,
- &s->host_state, "pci-conf-idx", 0x1000);
- memory_region_init_io(&s->host_state.data_mem, &pci_host_data_le_ops,
- &s->host_state, "pci-conf-data", 0x1000);
- sysbus_init_mmio(dev, &s->host_state.conf_mem);
- sysbus_init_mmio(dev, &s->host_state.data_mem);
+ memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops,
+ dev, "pci-conf-idx", 0x1000);
+ memory_region_init_io(&h->data_mem, &pci_host_data_le_ops,
+ dev, "pci-conf-data", 0x1000);
+ sysbus_init_mmio(dev, &h->conf_mem);
+ sysbus_init_mmio(dev, &h->data_mem);
return 0;
}
static int pci_unin_internal_init_device(SysBusDevice *dev)
{
PCIHostState *h;
- UNINState *s;
/* Uninorth internal bus */
h = FROM_SYSBUS(PCIHostState, dev);
- s = DO_UPCAST(UNINState, host_state, h);
- memory_region_init_io(&s->host_state.conf_mem, &pci_host_conf_le_ops,
- &s->host_state, "pci-conf-idx", 0x1000);
- memory_region_init_io(&s->host_state.data_mem, &pci_host_data_le_ops,
- &s->host_state, "pci-conf-data", 0x1000);
- sysbus_init_mmio(dev, &s->host_state.conf_mem);
- sysbus_init_mmio(dev, &s->host_state.data_mem);
+ memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops,
+ dev, "pci-conf-idx", 0x1000);
+ memory_region_init_io(&h->data_mem, &pci_host_data_le_ops,
+ dev, "pci-conf-data", 0x1000);
+ sysbus_init_mmio(dev, &h->conf_mem);
+ sysbus_init_mmio(dev, &h->data_mem);
return 0;
}
@@ -214,26 +221,26 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
/* Use values found on a real PowerMac */
/* Uninorth main bus */
- dev = qdev_create(NULL, "uni-north-pci-pcihost");
+ dev = qdev_create(NULL, TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
qdev_init_nofail(dev);
- s = sysbus_from_qdev(dev);
+ s = SYS_BUS_DEVICE(dev);
h = FROM_SYSBUS(PCIHostState, s);
- d = DO_UPCAST(UNINState, host_state, h);
+ d = UNI_NORTH_PCI_HOST_BRIDGE(dev);
memory_region_init(&d->pci_mmio, "pci-mmio", 0x100000000ULL);
memory_region_init_alias(&d->pci_hole, "pci-hole", &d->pci_mmio,
0x80000000ULL, 0x70000000ULL);
memory_region_add_subregion(address_space_mem, 0x80000000ULL,
&d->pci_hole);
- d->host_state.bus = pci_register_bus(dev, "pci",
- pci_unin_set_irq, pci_unin_map_irq,
- pic,
- &d->pci_mmio,
- address_space_io,
- PCI_DEVFN(11, 0), 4);
+ h->bus = pci_register_bus(dev, "pci",
+ pci_unin_set_irq, pci_unin_map_irq,
+ pic,
+ &d->pci_mmio,
+ address_space_io,
+ PCI_DEVFN(11, 0), 4);
#if 0
- pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north");
+ pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north");
#endif
sysbus_mmio_map(s, 0, 0xf2800000);
@@ -242,30 +249,30 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
/* DEC 21154 bridge */
#if 0
/* XXX: not activated as PPC BIOS doesn't handle multiple buses properly */
- pci_create_simple(d->host_state.bus, PCI_DEVFN(12, 0), "dec-21154");
+ pci_create_simple(h->bus, PCI_DEVFN(12, 0), "dec-21154");
#endif
/* Uninorth AGP bus */
- pci_create_simple(d->host_state.bus, PCI_DEVFN(11, 0), "uni-north-agp");
- dev = qdev_create(NULL, "uni-north-agp-pcihost");
+ pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-agp");
+ dev = qdev_create(NULL, TYPE_UNI_NORTH_AGP_HOST_BRIDGE);
qdev_init_nofail(dev);
- s = sysbus_from_qdev(dev);
+ s = SYS_BUS_DEVICE(dev);
sysbus_mmio_map(s, 0, 0xf0800000);
sysbus_mmio_map(s, 1, 0xf0c00000);
/* Uninorth internal bus */
#if 0
/* XXX: not needed for now */
- pci_create_simple(d->host_state.bus, PCI_DEVFN(14, 0),
+ pci_create_simple(h->bus, PCI_DEVFN(14, 0),
"uni-north-internal-pci");
- dev = qdev_create(NULL, "uni-north-internal-pci-pcihost");
+ dev = qdev_create(NULL, TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE);
qdev_init_nofail(dev);
- s = sysbus_from_qdev(dev);
+ s = SYS_BUS_DEVICE(dev);
sysbus_mmio_map(s, 0, 0xf4800000);
sysbus_mmio_map(s, 1, 0xf4c00000);
#endif
- return d->host_state.bus;
+ return h->bus;
}
PCIBus *pci_pmac_u3_init(qemu_irq *pic,
@@ -279,11 +286,11 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
/* Uninorth AGP bus */
- dev = qdev_create(NULL, "u3-agp-pcihost");
+ dev = qdev_create(NULL, TYPE_U3_AGP_HOST_BRIDGE);
qdev_init_nofail(dev);
- s = sysbus_from_qdev(dev);
+ s = SYS_BUS_DEVICE(dev);
h = FROM_SYSBUS(PCIHostState, s);
- d = DO_UPCAST(UNINState, host_state, h);
+ d = U3_AGP_HOST_BRIDGE(dev);
memory_region_init(&d->pci_mmio, "pci-mmio", 0x100000000ULL);
memory_region_init_alias(&d->pci_hole, "pci-hole", &d->pci_mmio,
@@ -291,19 +298,19 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
memory_region_add_subregion(address_space_mem, 0x80000000ULL,
&d->pci_hole);
- d->host_state.bus = pci_register_bus(dev, "pci",
- pci_unin_set_irq, pci_unin_map_irq,
- pic,
- &d->pci_mmio,
- address_space_io,
- PCI_DEVFN(11, 0), 4);
+ h->bus = pci_register_bus(dev, "pci",
+ pci_unin_set_irq, pci_unin_map_irq,
+ pic,
+ &d->pci_mmio,
+ address_space_io,
+ PCI_DEVFN(11, 0), 4);
sysbus_mmio_map(s, 0, 0xf0800000);
sysbus_mmio_map(s, 1, 0xf0c00000);
- pci_create_simple(d->host_state.bus, 11 << 3, "u3-agp");
+ pci_create_simple(h->bus, 11 << 3, "u3-agp");
- return d->host_state.bus;
+ return h->bus;
}
static int unin_main_pci_host_init(PCIDevice *d)
@@ -419,7 +426,7 @@ static void pci_unin_main_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo pci_unin_main_info = {
- .name = "uni-north-pci-pcihost",
+ .name = TYPE_UNI_NORTH_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(UNINState),
.class_init = pci_unin_main_class_init,
@@ -433,7 +440,7 @@ static void pci_u3_agp_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo pci_u3_agp_info = {
- .name = "u3-agp-pcihost",
+ .name = TYPE_U3_AGP_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(UNINState),
.class_init = pci_u3_agp_class_init,
@@ -447,7 +454,7 @@ static void pci_unin_agp_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo pci_unin_agp_info = {
- .name = "uni-north-agp-pcihost",
+ .name = TYPE_UNI_NORTH_AGP_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(UNINState),
.class_init = pci_unin_agp_class_init,
@@ -461,7 +468,7 @@ static void pci_unin_internal_class_init(ObjectClass *klass, void *data)
}
static const TypeInfo pci_unin_internal_info = {
- .name = "uni-north-internal-pci-pcihost",
+ .name = TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(UNINState),
.class_init = pci_unin_internal_class_init,
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 12/14] pci_host: Turn into SysBus-derived QOM type
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (10 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 11/14] unin_pci: QOM'ify UniNorth PCI host bridges Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 13/14] pci: Derive PCI host bridges from TYPE_PCI_HOST_BRIDGE Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges Andreas Färber
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Wanpeng Li, mst, jbaron, Andreas Färber,
anthony, pbonzini, Andreas Färber
From: Andreas Färber <andreas.faerber@web.de>
The preceding commits fixed misuses of FROM_SYSBUS() that led people to
add a bogus busdev field. For qdev the field order was less relevant but
for QOM the PCIHostState field (including the SysBusDevice actually
initialized with a value) must be placed first within the state struct.
To facilitate accessing the PCIHostState fields, derive all PCI host
bridges from TYPE_PCI_HOST_BRIDGE rather than TYPE_SYS_BUS_DEVICE.
We can now access PCIHostState QOM-style, with PCI_HOST_BRIDGE() macro.
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Wanpeng Li <liwp@linux.vnet.ibm.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/pci_host.c | 12 ++++++++++++
hw/pci_host.h | 5 +++++
2 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/hw/pci_host.c b/hw/pci_host.c
index 8041778..3950e94 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -165,4 +165,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
.endianness = DEVICE_BIG_ENDIAN,
};
+static const TypeInfo pci_host_type_info = {
+ .name = TYPE_PCI_HOST_BRIDGE,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .abstract = true,
+ .instance_size = sizeof(PCIHostState),
+};
+
+static void pci_host_register_types(void)
+{
+ type_register_static(&pci_host_type_info);
+}
+type_init(pci_host_register_types)
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 359e38f..4b9c300 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -30,8 +30,13 @@
#include "sysbus.h"
+#define TYPE_PCI_HOST_BRIDGE "pci-host-bridge"
+#define PCI_HOST_BRIDGE(obj) \
+ OBJECT_CHECK(PCIHostState, (obj), TYPE_PCI_HOST_BRIDGE)
+
struct PCIHostState {
SysBusDevice busdev;
+
MemoryRegion conf_mem;
MemoryRegion data_mem;
MemoryRegion mmcfg;
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 13/14] pci: Derive PCI host bridges from TYPE_PCI_HOST_BRIDGE
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (11 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 12/14] pci_host: Turn into SysBus-derived QOM type Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges Andreas Färber
13 siblings, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: mst, jbaron, Alexander Graf, Andreas Färber,
open list:New World, anthony, pbonzini, Andreas Färber
Some typedef'ed their state to PCIHostState. Use a proper struct,
and use PCIHostState and PCI_HOST_BRIDGE() where appropriate.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
hw/alpha_typhoon.c | 2 +-
hw/bonito.c | 22 +++++++++++-----------
hw/dec_pci.c | 4 ++--
hw/grackle_pci.c | 6 +++---
hw/gt64xxx.c | 4 ++--
hw/piix_pci.c | 8 ++++----
hw/ppc4xx_pci.c | 4 ++--
hw/ppc_prep.c | 4 +---
hw/ppce500_pci.c | 4 ++--
hw/prep_pci.c | 4 ++--
hw/spapr_pci.c | 2 +-
hw/unin_pci.c | 20 ++++++++++----------
12 files changed, 41 insertions(+), 43 deletions(-)
diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
index 06e84a1..58025a3 100644
--- a/hw/alpha_typhoon.c
+++ b/hw/alpha_typhoon.c
@@ -823,7 +823,7 @@ static void typhoon_pcihost_class_init(ObjectClass *klass, void *data)
static const TypeInfo typhoon_pcihost_info = {
.name = TYPE_TYPHOON_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(TyphoonState),
.class_init = typhoon_pcihost_class_init,
};
diff --git a/hw/bonito.c b/hw/bonito.c
index 062c701..6084ac4 100644
--- a/hw/bonito.c
+++ b/hw/bonito.c
@@ -416,7 +416,7 @@ static const MemoryRegionOps bonito_cop_ops = {
static uint32_t bonito_sbridge_pciaddr(void *opaque, target_phys_addr_t addr)
{
PCIBonitoState *s = opaque;
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
uint32_t cfgaddr;
uint32_t idsel;
uint32_t devno;
@@ -454,7 +454,7 @@ static void bonito_spciconf_writeb(void *opaque, target_phys_addr_t addr,
{
PCIBonitoState *s = opaque;
PCIDevice *d = PCI_DEVICE(s);
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
uint32_t pciaddr;
uint16_t status;
@@ -480,7 +480,7 @@ static void bonito_spciconf_writew(void *opaque, target_phys_addr_t addr,
{
PCIBonitoState *s = opaque;
PCIDevice *d = PCI_DEVICE(s);
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
uint32_t pciaddr;
uint16_t status;
@@ -508,7 +508,7 @@ static void bonito_spciconf_writel(void *opaque, target_phys_addr_t addr,
{
PCIBonitoState *s = opaque;
PCIDevice *d = PCI_DEVICE(s);
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
uint32_t pciaddr;
uint16_t status;
@@ -535,7 +535,7 @@ static uint32_t bonito_spciconf_readb(void *opaque, target_phys_addr_t addr)
{
PCIBonitoState *s = opaque;
PCIDevice *d = PCI_DEVICE(s);
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
uint32_t pciaddr;
uint16_t status;
@@ -561,7 +561,7 @@ static uint32_t bonito_spciconf_readw(void *opaque, target_phys_addr_t addr)
{
PCIBonitoState *s = opaque;
PCIDevice *d = PCI_DEVICE(s);
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
uint32_t pciaddr;
uint16_t status;
@@ -589,7 +589,7 @@ static uint32_t bonito_spciconf_readl(void *opaque, target_phys_addr_t addr)
{
PCIBonitoState *s = opaque;
PCIDevice *d = PCI_DEVICE(s);
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
uint32_t pciaddr;
uint16_t status;
@@ -702,7 +702,7 @@ static const VMStateDescription vmstate_bonito = {
static int bonito_pcihost_initfn(SysBusDevice *dev)
{
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, dev);
+ PCIHostState *phb = PCI_HOST_BRIDGE(dev);
phb->bus = pci_register_bus(DEVICE(dev), "pci",
pci_bonito_set_irq, pci_bonito_map_irq, dev,
@@ -716,7 +716,7 @@ static int bonito_initfn(PCIDevice *dev)
{
PCIBonitoState *s = DO_UPCAST(PCIBonitoState, dev, dev);
SysBusDevice *sysbus = SYS_BUS_DEVICE(s->pcihost);
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, sysbus);
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
/* Bonito North Bridge, built on FPGA, VENDOR_ID/DEVICE_ID are "undefined" */
pci_config_set_prog_interface(dev->config, 0x00);
@@ -785,7 +785,7 @@ PCIBus *bonito_init(qemu_irq *pic)
PCIDevice *d;
dev = qdev_create(NULL, TYPE_BONITO_PCI_HOST_BRIDGE);
- phb = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
+ phb = PCI_HOST_BRIDGE(dev);
pcihost = BONITO_PCI_HOST_BRIDGE(dev);
pcihost->pic = pic;
qdev_init_nofail(dev);
@@ -833,7 +833,7 @@ static void bonito_pcihost_class_init(ObjectClass *klass, void *data)
static const TypeInfo bonito_pcihost_info = {
.name = TYPE_BONITO_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(BonitoState),
.class_init = bonito_pcihost_class_init,
};
diff --git a/hw/dec_pci.c b/hw/dec_pci.c
index 19aed1b..de16361 100644
--- a/hw/dec_pci.c
+++ b/hw/dec_pci.c
@@ -91,7 +91,7 @@ static int pci_dec_21154_device_init(SysBusDevice *dev)
{
PCIHostState *phb;
- phb = FROM_SYSBUS(PCIHostState, dev);
+ phb = PCI_HOST_BRIDGE(dev);
memory_region_init_io(&phb->conf_mem, &pci_host_conf_le_ops,
dev, "pci-conf-idx", 0x1000);
@@ -136,7 +136,7 @@ static void pci_dec_21154_device_class_init(ObjectClass *klass, void *data)
static const TypeInfo pci_dec_21154_device_info = {
.name = TYPE_DEC_21154,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(DECState),
.class_init = pci_dec_21154_device_class_init,
};
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index d814270..066f6e1 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -73,7 +73,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic,
dev = qdev_create(NULL, TYPE_GRACKLE_PCI_HOST_BRIDGE);
qdev_init_nofail(dev);
s = SYS_BUS_DEVICE(dev);
- phb = FROM_SYSBUS(PCIHostState, s);
+ phb = PCI_HOST_BRIDGE(dev);
d = GRACKLE_PCI_HOST_BRIDGE(dev);
memory_region_init(&d->pci_mmio, "pci-mmio", 0x100000000ULL);
@@ -102,7 +102,7 @@ static int pci_grackle_init_device(SysBusDevice *dev)
{
PCIHostState *phb;
- phb = FROM_SYSBUS(PCIHostState, dev);
+ phb = PCI_HOST_BRIDGE(dev);
memory_region_init_io(&phb->conf_mem, &pci_host_conf_le_ops,
dev, "pci-conf-idx", 0x1000);
@@ -151,7 +151,7 @@ static void pci_grackle_class_init(ObjectClass *klass, void *data)
static const TypeInfo grackle_pci_host_info = {
.name = TYPE_GRACKLE_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(GrackleState),
.class_init = pci_grackle_class_init,
};
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index 0677299..857758e 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -1095,7 +1095,7 @@ PCIBus *gt64120_register(qemu_irq *pic)
dev = qdev_create(NULL, TYPE_GT64120_PCI_HOST_BRIDGE);
qdev_init_nofail(dev);
d = GT64120_PCI_HOST_BRIDGE(dev);
- phb = &d->pci;
+ phb = PCI_HOST_BRIDGE(dev);
phb->bus = pci_register_bus(dev, "pci",
gt64120_pci_set_irq, gt64120_pci_map_irq,
pic,
@@ -1168,7 +1168,7 @@ static void gt64120_class_init(ObjectClass *klass, void *data)
static const TypeInfo gt64120_info = {
.name = TYPE_GT64120_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(GT64120State),
.class_init = gt64120_class_init,
};
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 5db4026..0523d81 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -224,7 +224,7 @@ static const VMStateDescription vmstate_i440fx = {
static int i440fx_pcihost_initfn(SysBusDevice *dev)
{
- I440FXState *s = FROM_SYSBUS(I440FXState, dev);
+ PCIHostState *s = PCI_HOST_BRIDGE(dev);
memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
"pci-conf-idx", 4);
@@ -266,12 +266,12 @@ static PCIBus *i440fx_common_init(const char *device_name,
DeviceState *dev;
PCIBus *b;
PCIDevice *d;
- I440FXState *s;
+ PCIHostState *s;
PIIX3State *piix3;
PCII440FXState *f;
dev = qdev_create(NULL, "i440FX-pcihost");
- s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
+ s = PCI_HOST_BRIDGE(dev);
s->address_space = address_space_mem;
b = pci_bus_new(&s->busdev.qdev, NULL, pci_address_space,
address_space_io, 0);
@@ -583,7 +583,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
static const TypeInfo i440fx_pcihost_info = {
.name = "i440FX-pcihost",
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(I440FXState),
.class_init = i440fx_pcihost_class_init,
};
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 599a75a..5583321 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -338,7 +338,7 @@ static int ppc4xx_pcihost_initfn(SysBusDevice *dev)
PCIBus *b;
int i;
- h = FROM_SYSBUS(PCIHostState, dev);
+ h = PCI_HOST_BRIDGE(dev);
s = PPC4xx_PCI_HOST_BRIDGE(dev);
for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
@@ -398,7 +398,7 @@ static void ppc4xx_pcihost_class_init(ObjectClass *klass, void *data)
static const TypeInfo ppc4xx_pcihost_info = {
.name = TYPE_PPC4xx_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(PPC4xxPCIState),
.class_init = ppc4xx_pcihost_class_init,
};
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index be2b268..1544430 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -470,7 +470,6 @@ static void ppc_prep_init (ram_addr_t ram_size,
uint32_t kernel_base, initrd_base;
long kernel_size, initrd_size;
DeviceState *dev;
- SysBusDevice *sys;
PCIHostState *pcihost;
PCIBus *pci_bus;
PCIDevice *pci;
@@ -583,8 +582,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
}
dev = qdev_create(NULL, "raven-pcihost");
- sys = sysbus_from_qdev(dev);
- pcihost = DO_UPCAST(PCIHostState, busdev, sys);
+ pcihost = PCI_HOST_BRIDGE(dev);
pcihost->address_space = get_system_memory();
object_property_add_child(qdev_get_machine(), "raven", OBJECT(dev), NULL);
qdev_init_nofail(dev);
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index e4f065a..3333967 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -316,7 +316,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
MemoryRegion *address_space_mem = get_system_memory();
MemoryRegion *address_space_io = get_system_io();
- h = FROM_SYSBUS(PCIHostState, dev);
+ h = PCI_HOST_BRIDGE(dev);
s = PPC_E500_PCI_HOST_BRIDGE(dev);
for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
@@ -374,7 +374,7 @@ static void e500_pcihost_class_init(ObjectClass *klass, void *data)
static const TypeInfo e500_pcihost_info = {
.name = TYPE_PPC_E500_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(PPCE500PCIState),
.class_init = e500_pcihost_class_init,
};
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 69c19df..35cb9b2 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -103,7 +103,7 @@ static void prep_set_irq(void *opaque, int irq_num, int level)
static int raven_pcihost_init(SysBusDevice *dev)
{
- PCIHostState *h = FROM_SYSBUS(PCIHostState, dev);
+ PCIHostState *h = PCI_HOST_BRIDGE(dev);
PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
MemoryRegion *address_space_mem = get_system_memory();
MemoryRegion *address_space_io = get_system_io();
@@ -192,7 +192,7 @@ static void raven_pcihost_class_init(ObjectClass *klass, void *data)
static const TypeInfo raven_pcihost_info = {
.name = TYPE_RAVEN_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(PREPPCIState),
.class_init = raven_pcihost_class_init,
};
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index dd2a72d..7d84801 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -370,7 +370,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
static const TypeInfo spapr_phb_info = {
.name = TYPE_SPAPR_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(sPAPRPHBState),
.class_init = spapr_phb_class_init,
};
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 1fc8920..0db7c1f 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -148,7 +148,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
/* Use values found on a real PowerMac */
/* Uninorth main bus */
- h = FROM_SYSBUS(PCIHostState, dev);
+ h = PCI_HOST_BRIDGE(dev);
memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops,
dev, "pci-conf-idx", 0x1000);
@@ -166,7 +166,7 @@ static int pci_u3_agp_init_device(SysBusDevice *dev)
PCIHostState *h;
/* Uninorth U3 AGP bus */
- h = FROM_SYSBUS(PCIHostState, dev);
+ h = PCI_HOST_BRIDGE(dev);
memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops,
dev, "pci-conf-idx", 0x1000);
@@ -183,7 +183,7 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
PCIHostState *h;
/* Uninorth AGP bus */
- h = FROM_SYSBUS(PCIHostState, dev);
+ h = PCI_HOST_BRIDGE(dev);
memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops,
dev, "pci-conf-idx", 0x1000);
@@ -199,7 +199,7 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
PCIHostState *h;
/* Uninorth internal bus */
- h = FROM_SYSBUS(PCIHostState, dev);
+ h = PCI_HOST_BRIDGE(dev);
memory_region_init_io(&h->conf_mem, &pci_host_conf_le_ops,
dev, "pci-conf-idx", 0x1000);
@@ -224,7 +224,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic,
dev = qdev_create(NULL, TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
qdev_init_nofail(dev);
s = SYS_BUS_DEVICE(dev);
- h = FROM_SYSBUS(PCIHostState, s);
+ h = PCI_HOST_BRIDGE(s);
d = UNI_NORTH_PCI_HOST_BRIDGE(dev);
memory_region_init(&d->pci_mmio, "pci-mmio", 0x100000000ULL);
memory_region_init_alias(&d->pci_hole, "pci-hole", &d->pci_mmio,
@@ -289,7 +289,7 @@ PCIBus *pci_pmac_u3_init(qemu_irq *pic,
dev = qdev_create(NULL, TYPE_U3_AGP_HOST_BRIDGE);
qdev_init_nofail(dev);
s = SYS_BUS_DEVICE(dev);
- h = FROM_SYSBUS(PCIHostState, s);
+ h = PCI_HOST_BRIDGE(dev);
d = U3_AGP_HOST_BRIDGE(dev);
memory_region_init(&d->pci_mmio, "pci-mmio", 0x100000000ULL);
@@ -427,7 +427,7 @@ static void pci_unin_main_class_init(ObjectClass *klass, void *data)
static const TypeInfo pci_unin_main_info = {
.name = TYPE_UNI_NORTH_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(UNINState),
.class_init = pci_unin_main_class_init,
};
@@ -441,7 +441,7 @@ static void pci_u3_agp_class_init(ObjectClass *klass, void *data)
static const TypeInfo pci_u3_agp_info = {
.name = TYPE_U3_AGP_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(UNINState),
.class_init = pci_u3_agp_class_init,
};
@@ -455,7 +455,7 @@ static void pci_unin_agp_class_init(ObjectClass *klass, void *data)
static const TypeInfo pci_unin_agp_info = {
.name = TYPE_UNI_NORTH_AGP_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(UNINState),
.class_init = pci_unin_agp_class_init,
};
@@ -469,7 +469,7 @@ static void pci_unin_internal_class_init(ObjectClass *klass, void *data)
static const TypeInfo pci_unin_internal_info = {
.name = TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE,
- .parent = TYPE_SYS_BUS_DEVICE,
+ .parent = TYPE_PCI_HOST_BRIDGE,
.instance_size = sizeof(UNINState),
.class_init = pci_unin_internal_class_init,
};
--
1.7.7
^ permalink raw reply related [flat|nested] 33+ messages in thread* [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-04 17:19 [Qemu-devel] [PATCH v3 00/14] pci_host: Convert to QOM Andreas Färber
` (12 preceding siblings ...)
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 13/14] pci: Derive PCI host bridges from TYPE_PCI_HOST_BRIDGE Andreas Färber
@ 2012-07-04 17:19 ` Andreas Färber
2012-07-04 21:17 ` Michael S. Tsirkin
13 siblings, 1 reply; 33+ messages in thread
From: Andreas Färber @ 2012-07-04 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: mst, jbaron, Alexander Graf, Andreas Färber,
open list:New World, anthony, pbonzini, Andreas Färber
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,
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
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-04 17:19 ` [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges Andreas Färber
@ 2012-07-04 21:17 ` Michael S. Tsirkin
2012-07-04 21:26 ` Michael S. Tsirkin
0 siblings, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-04 21:17 UTC (permalink / raw)
To: Andreas Färber
Cc: jbaron, qemu-devel, Alexander Graf, Andreas Färber,
open list:New World, anthony, pbonzini
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?
> 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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-04 21:17 ` Michael S. Tsirkin
@ 2012-07-04 21:26 ` Michael S. Tsirkin
2012-07-04 21:38 ` Anthony Liguori
[not found] ` <4FF4C4EC.2090404@suse.de>
0 siblings, 2 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-04 21:26 UTC (permalink / raw)
To: Andreas Färber
Cc: jbaron, qemu-devel, Alexander Graf, Andreas Färber,
open list:New World, anthony, pbonzini
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.
> > 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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-04 21:26 ` Michael S. Tsirkin
@ 2012-07-04 21:38 ` Anthony Liguori
2012-07-05 8:59 ` Michael S. Tsirkin
[not found] ` <4FF4C4EC.2090404@suse.de>
1 sibling, 1 reply; 33+ messages in thread
From: Anthony Liguori @ 2012-07-04 21:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jbaron, Alexander Graf, qemu-devel, Andreas Färber,
New World, pbonzini, Andreas Färber
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-04 21:38 ` Anthony Liguori
@ 2012-07-05 8:59 ` Michael S. Tsirkin
2012-07-05 9:51 ` Paolo Bonzini
0 siblings, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-05 8:59 UTC (permalink / raw)
To: Anthony Liguori
Cc: jbaron, Alexander Graf, qemu-devel, Andreas Färber,
New World, pbonzini, Andreas Färber
On Wed, Jul 04, 2012 at 04:38:03PM -0500, Anthony Liguori wrote:
> 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().
Field accesses are better. If there is a user error, code does not
compile. They are also self-documenting to some level: you look
at a struct you see all its fields. How do you know which casts
will succeed on a given type? There's no easy way.
> 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
It still seems wrong to wrap all field accesses in macros just in case
we need to rename them later. We can pay that cost when the need arises.
--
MST
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 8:59 ` Michael S. Tsirkin
@ 2012-07-05 9:51 ` Paolo Bonzini
0 siblings, 0 replies; 33+ messages in thread
From: Paolo Bonzini @ 2012-07-05 9:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jbaron, Alexander Graf, qemu-devel, Andreas Färber,
New World, Anthony Liguori, Andreas Färber
Il 05/07/2012 10:59, Michael S. Tsirkin ha scritto:
> Field accesses are better. If there is a user error, code does not
> compile. They are also self-documenting to some level: you look
> at a struct you see all its fields. How do you know which casts
> will succeed on a given type? There's no easy way.
>
I agree.
Paolo
^ permalink raw reply [flat|nested] 33+ messages in thread
[parent not found: <4FF4C4EC.2090404@suse.de>]
* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
[not found] ` <4FF4C4EC.2090404@suse.de>
@ 2012-07-05 9:53 ` Paolo Bonzini
2012-07-05 10:15 ` Andreas Färber
2012-07-05 13:34 ` Michael S. Tsirkin
1 sibling, 1 reply; 33+ messages in thread
From: Paolo Bonzini @ 2012-07-05 9:53 UTC (permalink / raw)
To: Andreas Färber
Cc: Michael S. Tsirkin, jbaron, Alexander Graf, qemu-devel,
Andreas Färber,
"list@suse.de"@zmta06.collab.prod.int.phx2.redhat.com:New
World, anthony, open
Il 05/07/2012 00:34, Andreas Färber ha scritto:
>> > Just to clarify: replacing upcasts which are always safe
>> > with downcasts which can fail is what I consider especially ugly.
> As per Anthony the parent field in the QOM instance structs is not
> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
> that it doesn't even show up in gtk-doc documentation. If it is unused,
> its name becomes irrelevant and could even be "reserved" if we so
> wanted. Renaming it to whatever proves that all old references are gone.
I disagree with removing static checks whenever possible.
> Background is that qdev and QOM work differently with regards to
> inheritance: as mentioned in the preceding patch, for qdev the parent
> was (had to be) identified by name and could be anywhere in the struct;
Not entirely true, being at the beginning of the struct is already
enforced by using DO_UPCAST (which is admittedly a strange name for a
downcast macro) instead of container_of.
> for QOM the parent is a subset of the struct from the start and it's
> supposed to be accessed through the struct type that provides the
> fields, the usual way to get such a pointer is through
> OBJECT_CHECK()-derived cast macros.
Paolo
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 9:53 ` Paolo Bonzini
@ 2012-07-05 10:15 ` Andreas Färber
2012-07-05 10:36 ` Paolo Bonzini
2012-07-05 13:21 ` Michael S. Tsirkin
0 siblings, 2 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-05 10:15 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Michael S. Tsirkin, jbaron, Alexander Graf, qemu-devel,
Andreas Färber,
"list@suse.de"@zmta06.collab.prod.int.phx2.redhat.com:New
World, anthony, open
Am 05.07.2012 11:53, schrieb Paolo Bonzini:
> Il 05/07/2012 00:34, Andreas Färber ha scritto:
>>>> Just to clarify: replacing upcasts which are always safe
>>>> with downcasts which can fail is what I consider especially ugly.
>> As per Anthony the parent field in the QOM instance structs is not
>> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
>> that it doesn't even show up in gtk-doc documentation. If it is unused,
>> its name becomes irrelevant and could even be "reserved" if we so
>> wanted. Renaming it to whatever proves that all old references are gone.
>
> I disagree with removing static checks whenever possible.
>
>> Background is that qdev and QOM work differently with regards to
>> inheritance: as mentioned in the preceding patch, for qdev the parent
>> was (had to be) identified by name and could be anywhere in the struct;
>
> Not entirely true, being at the beginning of the struct is already
> enforced by using DO_UPCAST (which is admittedly a strange name for a
> downcast macro) instead of container_of.
If you look at the patchset you will find that it was not properly enforced!
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 10:15 ` Andreas Färber
@ 2012-07-05 10:36 ` Paolo Bonzini
2012-07-05 13:21 ` Michael S. Tsirkin
1 sibling, 0 replies; 33+ messages in thread
From: Paolo Bonzini @ 2012-07-05 10:36 UTC (permalink / raw)
To: Andreas Färber
Cc: Michael S. Tsirkin, jbaron, Alexander Graf, qemu-devel,
Andreas Färber,
"list@suse.de"@zmta06.collab.prod.int.phx2.redhat.com:New
World, anthony, open
> > Not entirely true, being at the beginning of the struct is already
> > enforced by using DO_UPCAST (which is admittedly a strange name for
> > a downcast macro) instead of container_of.
>
> If you look at the patchset you will find that it was not properly
> enforced!
Well, what *is* enforced? :)
Paolo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 10:15 ` Andreas Färber
2012-07-05 10:36 ` Paolo Bonzini
@ 2012-07-05 13:21 ` Michael S. Tsirkin
1 sibling, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-05 13:21 UTC (permalink / raw)
To: Andreas Färber
Cc: jbaron, qemu-devel, Alexander Graf, Andreas Färber,
"list@suse.de"@zmta06.collab.prod.int.phx2.redhat.com:New
World, anthony, Paolo Bonzini, open
On Thu, Jul 05, 2012 at 12:15:04PM +0200, Andreas Färber wrote:
> Am 05.07.2012 11:53, schrieb Paolo Bonzini:
> > Il 05/07/2012 00:34, Andreas Färber ha scritto:
> >>>> Just to clarify: replacing upcasts which are always safe
> >>>> with downcasts which can fail is what I consider especially ugly.
> >> As per Anthony the parent field in the QOM instance structs is not
> >> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
> >> that it doesn't even show up in gtk-doc documentation. If it is unused,
> >> its name becomes irrelevant and could even be "reserved" if we so
> >> wanted. Renaming it to whatever proves that all old references are gone.
> >
> > I disagree with removing static checks whenever possible.
> >
> >> Background is that qdev and QOM work differently with regards to
> >> inheritance: as mentioned in the preceding patch, for qdev the parent
> >> was (had to be) identified by name and could be anywhere in the struct;
> >
> > Not entirely true, being at the beginning of the struct is already
> > enforced by using DO_UPCAST (which is admittedly a strange name for a
> > downcast macro) instead of container_of.
>
> If you look at the patchset you will find that it was not properly enforced!
>
> Andreas
Yes DO_UPCAST is IMO evil and should go - everyone can eiter use dynamic
QOM types with lookup by name; or stop passing nonsafe void * pointers
around and use container_of consistently.
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
[not found] ` <4FF4C4EC.2090404@suse.de>
2012-07-05 9:53 ` Paolo Bonzini
@ 2012-07-05 13:34 ` Michael S. Tsirkin
2012-07-05 13:42 ` Andreas Färber
2012-07-05 13:54 ` Anthony Liguori
1 sibling, 2 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-05 13:34 UTC (permalink / raw)
To: Andreas Färber
Cc: jbaron, qemu-devel, Alexander Graf, Andreas Färber,
New World, anthony, pbonzini, open
On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
> > 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.
>
> As per Anthony the parent field in the QOM instance structs is not
> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
> that it doesn't even show up in gtk-doc documentation.
PCIHostState is not private here. And if it were, it won't be
of any use because compiler does not read gtk-doc documentation
and neither do developers.
> If it is unused,
> its name becomes irrelevant and could even be "reserved" if we so
> wanted. Renaming it to whatever proves that all old references are gone.
Adding arbitrary rules like that only seems to make code future proof.
People will not remember and will use the field anyway,
then when you try to change it there will be work to be done.
So let's do the work when we really need it.
> Background is that qdev and QOM work differently with regards to
> inheritance: as mentioned in the preceding patch, for qdev the parent
> was (had to be) identified by name and could be anywhere in the struct;
> for QOM the parent is a subset of the struct from the start and it's
> supposed to be accessed through the struct type that provides the
> fields, the usual way to get such a pointer is through
> OBJECT_CHECK()-derived cast macros.
It makes sense if you go from parent to child. Even in C++
you don't use dynamic_cast to go from child to parent.
> In the snippet above I chose to use the macro directly since there was
> no second use but I can introduce a variable if preferred.
> What I see now though is that this code snippet creating the PCIBus
> should be moved up into the SysBus init function.
>
> I'll wait with posting a v4 til I hear what exactly to rebase it on.
> As for further renaming/splitting wishes this is what I referenced:
> http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg03069.html
>
> In particular, did you expect me to rename PCIHostState to PCIHostBridge
> / PCIHostBridgeState everywhere? pci_host.c to pci_host_bridge.c? Or did
> you just mean the new PCI_HOST_BRIDGE macros like I did in this v3?
>
> Regards,
> Andreas
Yes just the new macros.
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 13:34 ` Michael S. Tsirkin
@ 2012-07-05 13:42 ` Andreas Färber
2012-07-05 13:54 ` Anthony Liguori
1 sibling, 0 replies; 33+ messages in thread
From: Andreas Färber @ 2012-07-05 13:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jbaron, qemu-devel, Alexander Graf, Andreas Färber,
"list@suse.de@zmta06.collab.prod.int.phx2.redhat.com"@suse.de:New
World, anthony, pbonzini, open
Am 05.07.2012 15:34, schrieb Michael S. Tsirkin:
> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
>>> 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.
>>
>> As per Anthony the parent field in the QOM instance structs is not
>> supposed to be touched (cf. object.h). We mark it /*< private >*/ so
>> that it doesn't even show up in gtk-doc documentation.
>
> PCIHostState is not private here. And if it were, it won't be
> of any use because compiler does not read gtk-doc documentation
> and neither do developers.
>
>> If it is unused,
>> its name becomes irrelevant and could even be "reserved" if we so
>> wanted. Renaming it to whatever proves that all old references are gone.
>
> Adding arbitrary rules like that only seems to make code future proof.
> People will not remember and will use the field anyway,
> then when you try to change it there will be work to be done.
> So let's do the work when we really need it.
>
>> Background is that qdev and QOM work differently with regards to
>> inheritance: as mentioned in the preceding patch, for qdev the parent
>> was (had to be) identified by name and could be anywhere in the struct;
>> for QOM the parent is a subset of the struct from the start and it's
>> supposed to be accessed through the struct type that provides the
>> fields, the usual way to get such a pointer is through
>> OBJECT_CHECK()-derived cast macros.
>
> It makes sense if you go from parent to child. Even in C++
> you don't use dynamic_cast to go from child to parent.
Which is the core point: In C++ you don't need to go through some parent
field, you access the fields and methods directly through the child
variable. In C that's not possible. Sticking to the specific type that
has the fields you want to access allows you to code C++-style.
Anyway, I am not doing these changes here because I have great arguments
foo and bar for it but because this is QOM that so far no one opposed
when Anthony introduced it. Please discuss that with Anthony. My
interest is having it done consistently and not having legacy code lying
around that gets copied by new contributors.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 13:34 ` Michael S. Tsirkin
2012-07-05 13:42 ` Andreas Färber
@ 2012-07-05 13:54 ` Anthony Liguori
2012-07-05 14:15 ` Michael S. Tsirkin
1 sibling, 1 reply; 33+ messages in thread
From: Anthony Liguori @ 2012-07-05 13:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jbaron, Alexander Graf, qemu-devel, Andreas Färber,
"list@suse.de@zmta06.collab.prod.int.phx2.redhat.com":
New World, pbonzini, open, Andreas Färber
On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
>>> 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.
>>
>> As per Anthony the parent field in the QOM instance structs is not
>> supposed to be touched (cf. object.h). We mark it /*< private>*/ so
>> that it doesn't even show up in gtk-doc documentation.
>
> PCIHostState is not private here. And if it were, it won't be
> of any use because compiler does not read gtk-doc documentation
> and neither do developers.
>
>> If it is unused,
>> its name becomes irrelevant and could even be "reserved" if we so
>> wanted. Renaming it to whatever proves that all old references are gone.
>
> Adding arbitrary rules like that only seems to make code future proof.
> People will not remember and will use the field anyway,
> then when you try to change it there will be work to be done.
> So let's do the work when we really need it.
>
>> Background is that qdev and QOM work differently with regards to
>> inheritance: as mentioned in the preceding patch, for qdev the parent
>> was (had to be) identified by name and could be anywhere in the struct;
>> for QOM the parent is a subset of the struct from the start and it's
>> supposed to be accessed through the struct type that provides the
>> fields, the usual way to get such a pointer is through
>> OBJECT_CHECK()-derived cast macros.
>
> It makes sense if you go from parent to child. Even in C++
> you don't use dynamic_cast to go from child to parent.
But you use static cast. Casting is not the same thing as dereferencing a
member. IOW:
Foo *foo = (Foo *)bar;
Is very different than:
Foo *foo = &bar.foo;
Using cast macros makes the code an awful lot more readable because it tells the
reader more.
Foo *foo = FOO(bar);
Tells the user that we're casting bar to the a type Foo. OTOH:
Foo *foo = &bar.foo;
Doesn't tell the user anything. A FOO() macro is consistent regardless of how
the type is implemented too. It gets even worse when you are going up multiple
levels:
DeviceState *dev = &bar.host.qdev;
Is unintelligible whereas:
DeviceState *dev = DEVICE(bar);
Tells you exactly what's happening.
But really, this isn't the place to debate this. QOM has been around for a
while. Consistency is important and this is how things are done in QOM.
If you want to revisit this style, you should start a separate thread about it
and we can talk about it. But this patch is consistent with the current
infrastructure.
Regards,
Anthony Liguori
>
>
>> In the snippet above I chose to use the macro directly since there was
>> no second use but I can introduce a variable if preferred.
>> What I see now though is that this code snippet creating the PCIBus
>> should be moved up into the SysBus init function.
>>
>> I'll wait with posting a v4 til I hear what exactly to rebase it on.
>> As for further renaming/splitting wishes this is what I referenced:
>> http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg03069.html
>>
>> In particular, did you expect me to rename PCIHostState to PCIHostBridge
>> / PCIHostBridgeState everywhere? pci_host.c to pci_host_bridge.c? Or did
>> you just mean the new PCI_HOST_BRIDGE macros like I did in this v3?
>>
>> Regards,
>> Andreas
>
> Yes just the new macros.
>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 13:54 ` Anthony Liguori
@ 2012-07-05 14:15 ` Michael S. Tsirkin
2012-07-05 15:00 ` Andreas Färber
0 siblings, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-05 14:15 UTC (permalink / raw)
To: Anthony Liguori
Cc: jbaron, Alexander Graf, qemu-devel, Andreas Färber,
"list@suse.de@zmta06.collab.prod.int.phx2.redhat.com":
New World, pbonzini, open, Andreas Färber
On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote:
> On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
> >On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
> >>Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
> >>>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.
> >>
> >>As per Anthony the parent field in the QOM instance structs is not
> >>supposed to be touched (cf. object.h). We mark it /*< private>*/ so
> >>that it doesn't even show up in gtk-doc documentation.
> >
> >PCIHostState is not private here. And if it were, it won't be
> >of any use because compiler does not read gtk-doc documentation
> >and neither do developers.
> >
> >>If it is unused,
> >>its name becomes irrelevant and could even be "reserved" if we so
> >>wanted. Renaming it to whatever proves that all old references are gone.
> >
> >Adding arbitrary rules like that only seems to make code future proof.
> >People will not remember and will use the field anyway,
> >then when you try to change it there will be work to be done.
> >So let's do the work when we really need it.
> >
> >>Background is that qdev and QOM work differently with regards to
> >>inheritance: as mentioned in the preceding patch, for qdev the parent
> >>was (had to be) identified by name and could be anywhere in the struct;
> >>for QOM the parent is a subset of the struct from the start and it's
> >>supposed to be accessed through the struct type that provides the
> >>fields, the usual way to get such a pointer is through
> >>OBJECT_CHECK()-derived cast macros.
> >
> >It makes sense if you go from parent to child. Even in C++
> >you don't use dynamic_cast to go from child to parent.
>
> But you use static cast. Casting is not the same thing as
> dereferencing a member. IOW:
>
> Foo *foo = (Foo *)bar;
>
> Is very different than:
>
> Foo *foo = &bar.foo;
>
> Using cast macros makes the code an awful lot more readable because
> it tells the reader more.
>
> Foo *foo = FOO(bar);
>
> Tells the user that we're casting bar to the a type Foo. OTOH:
>
> Foo *foo = &bar.foo;
>
> Doesn't tell the user anything. A FOO() macro is consistent
> regardless of how the type is implemented too. It gets even worse
> when you are going up multiple levels:
>
> DeviceState *dev = &bar.host.qdev;
>
> Is unintelligible whereas:
>
> DeviceState *dev = DEVICE(bar);
>
> Tells you exactly what's happening.
>
> But really, this isn't the place to debate this. QOM has been
> around for a while. Consistency is important and this is how things
> are done in QOM.
It's important but not the most important thing.
It does not make sense to do casts *everywhere*.
Do it where it makes sense.
> If you want to revisit this style, you should start a separate
> thread about it and we can talk about it. But this patch is
> consistent with the current infrastructure.
>
> Regards,
>
> Anthony Liguori
So far QOM was a win.
You examples look better with a macro. Patch 13 looks better:
- PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
+ PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
this is an improvement: devices do not want to deal with sysbus.
The code above that is being changed looks better without.
This is why this thread is about the specific patch and not
general QOM.
> >
> >
> >>In the snippet above I chose to use the macro directly since there was
> >>no second use but I can introduce a variable if preferred.
> >>What I see now though is that this code snippet creating the PCIBus
> >>should be moved up into the SysBus init function.
> >>
> >>I'll wait with posting a v4 til I hear what exactly to rebase it on.
> >>As for further renaming/splitting wishes this is what I referenced:
> >>http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg03069.html
> >>
> >>In particular, did you expect me to rename PCIHostState to PCIHostBridge
> >>/ PCIHostBridgeState everywhere? pci_host.c to pci_host_bridge.c? Or did
> >>you just mean the new PCI_HOST_BRIDGE macros like I did in this v3?
> >>
> >>Regards,
> >>Andreas
> >
> >Yes just the new macros.
> >
> >>--
> >>SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >>GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> >>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 14:15 ` Michael S. Tsirkin
@ 2012-07-05 15:00 ` Andreas Färber
2012-07-05 15:23 ` Michael S. Tsirkin
0 siblings, 1 reply; 33+ messages in thread
From: Andreas Färber @ 2012-07-05 15:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: pbonzini, jbaron, qemu-devel, Anthony Liguori, Alexander Graf
(Dropping some borked CCs)
Am 05.07.2012 16:15, schrieb Michael S. Tsirkin:
> On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote:
>> On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
>>> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
>>>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
>>>>> 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.
>>>>
>>>> As per Anthony the parent field in the QOM instance structs is not
>>>> supposed to be touched (cf. object.h). We mark it /*< private>*/ so
>>>> that it doesn't even show up in gtk-doc documentation.
>>>
>>> PCIHostState is not private here. And if it were, it won't be
>>> of any use because compiler does not read gtk-doc documentation
>>> and neither do developers.
>>>
>>>> If it is unused,
>>>> its name becomes irrelevant and could even be "reserved" if we so
>>>> wanted. Renaming it to whatever proves that all old references are gone.
>>>
>>> Adding arbitrary rules like that only seems to make code future proof.
>>> People will not remember and will use the field anyway,
>>> then when you try to change it there will be work to be done.
>>> So let's do the work when we really need it.
>>>
>>>> Background is that qdev and QOM work differently with regards to
>>>> inheritance: as mentioned in the preceding patch, for qdev the parent
>>>> was (had to be) identified by name and could be anywhere in the struct;
>>>> for QOM the parent is a subset of the struct from the start and it's
>>>> supposed to be accessed through the struct type that provides the
>>>> fields, the usual way to get such a pointer is through
>>>> OBJECT_CHECK()-derived cast macros.
>>>
>>> It makes sense if you go from parent to child. Even in C++
>>> you don't use dynamic_cast to go from child to parent.
>>
>> But you use static cast. Casting is not the same thing as
>> dereferencing a member. IOW:
>>
>> Foo *foo = (Foo *)bar;
>>
>> Is very different than:
>>
>> Foo *foo = &bar.foo;
>>
>> Using cast macros makes the code an awful lot more readable because
>> it tells the reader more.
>>
>> Foo *foo = FOO(bar);
>>
>> Tells the user that we're casting bar to the a type Foo. OTOH:
>>
>> Foo *foo = &bar.foo;
>>
>> Doesn't tell the user anything. A FOO() macro is consistent
>> regardless of how the type is implemented too. It gets even worse
>> when you are going up multiple levels:
>>
>> DeviceState *dev = &bar.host.qdev;
>>
>> Is unintelligible whereas:
>>
>> DeviceState *dev = DEVICE(bar);
>>
>> Tells you exactly what's happening.
>>
>> But really, this isn't the place to debate this. QOM has been
>> around for a while. Consistency is important and this is how things
>> are done in QOM.
>
> It's important but not the most important thing.
> It does not make sense to do casts *everywhere*.
> Do it where it makes sense.
>
>> If you want to revisit this style, you should start a separate
>> thread about it and we can talk about it. But this patch is
>> consistent with the current infrastructure.
>>
>> Regards,
>>
>> Anthony Liguori
>
> So far QOM was a win.
> You examples look better with a macro. Patch 13 looks better:
> - PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
> + PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
> this is an improvement: devices do not want to deal with sysbus.
>
> The code above that is being changed looks better without.
Do you want me to apologize for my macro misuse now? I apologize. :)
Replace PCI_HOST_BRIDGE(s)->bus = b; with
PCI_HOST_BRIDGE *phb = PCI_HOST_BRIDGE(s);
phb->bus = b;
in your mind and it matches exactly what you liked better above, no?
> This is why this thread is about the specific patch and not
> general QOM.
You're basically saying, QOM was a win but don't use it here. That's a
contradiction and thus a general QOM issue since that paradigm not only
applies here: Either we need to design all structs so that their fields
have nice names to access directly as you suggest, or nobody must access
the parent field as Anthony suggests. Having a wild mix of both at
maintainers' gusto is not a good solution.
Arguably we can just leave out this last patch if it is so
controversial. However, the split is arbitrary since I backwards-coded
this series, moving code snippets to earlier individual patches using
checkout -p, i.e. patches 1-11 have this final code design in mind and
thus went from SYS_BUS_DEVICE() to PCIHostState above rather than
through parent_obj. The reason I couldn't do it there directly is that
we didn't have the PCI_HOST_BRIDGE() macro before patch 13.
I also based patch 14 on the assumption that i440fx will get more state
fields when Anthony goes ahead with his QOM Pin series, otherwise we
could just scratch the I440FXState typedef (same discussion as for the
qtest herbivore test case).
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges
2012-07-05 15:00 ` Andreas Färber
@ 2012-07-05 15:23 ` Michael S. Tsirkin
0 siblings, 0 replies; 33+ messages in thread
From: Michael S. Tsirkin @ 2012-07-05 15:23 UTC (permalink / raw)
To: Andreas Färber
Cc: pbonzini, jbaron, qemu-devel, Anthony Liguori, Alexander Graf
On Thu, Jul 05, 2012 at 05:00:00PM +0200, Andreas Färber wrote:
> (Dropping some borked CCs)
>
> Am 05.07.2012 16:15, schrieb Michael S. Tsirkin:
> > On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote:
> >> On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
> >>> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
> >>>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
> >>>>> 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.
> >>>>
> >>>> As per Anthony the parent field in the QOM instance structs is not
> >>>> supposed to be touched (cf. object.h). We mark it /*< private>*/ so
> >>>> that it doesn't even show up in gtk-doc documentation.
> >>>
> >>> PCIHostState is not private here. And if it were, it won't be
> >>> of any use because compiler does not read gtk-doc documentation
> >>> and neither do developers.
> >>>
> >>>> If it is unused,
> >>>> its name becomes irrelevant and could even be "reserved" if we so
> >>>> wanted. Renaming it to whatever proves that all old references are gone.
> >>>
> >>> Adding arbitrary rules like that only seems to make code future proof.
> >>> People will not remember and will use the field anyway,
> >>> then when you try to change it there will be work to be done.
> >>> So let's do the work when we really need it.
> >>>
> >>>> Background is that qdev and QOM work differently with regards to
> >>>> inheritance: as mentioned in the preceding patch, for qdev the parent
> >>>> was (had to be) identified by name and could be anywhere in the struct;
> >>>> for QOM the parent is a subset of the struct from the start and it's
> >>>> supposed to be accessed through the struct type that provides the
> >>>> fields, the usual way to get such a pointer is through
> >>>> OBJECT_CHECK()-derived cast macros.
> >>>
> >>> It makes sense if you go from parent to child. Even in C++
> >>> you don't use dynamic_cast to go from child to parent.
> >>
> >> But you use static cast. Casting is not the same thing as
> >> dereferencing a member. IOW:
> >>
> >> Foo *foo = (Foo *)bar;
> >>
> >> Is very different than:
> >>
> >> Foo *foo = &bar.foo;
> >>
> >> Using cast macros makes the code an awful lot more readable because
> >> it tells the reader more.
> >>
> >> Foo *foo = FOO(bar);
> >>
> >> Tells the user that we're casting bar to the a type Foo. OTOH:
> >>
> >> Foo *foo = &bar.foo;
> >>
> >> Doesn't tell the user anything. A FOO() macro is consistent
> >> regardless of how the type is implemented too. It gets even worse
> >> when you are going up multiple levels:
> >>
> >> DeviceState *dev = &bar.host.qdev;
> >>
> >> Is unintelligible whereas:
> >>
> >> DeviceState *dev = DEVICE(bar);
> >>
> >> Tells you exactly what's happening.
> >>
> >> But really, this isn't the place to debate this. QOM has been
> >> around for a while. Consistency is important and this is how things
> >> are done in QOM.
> >
> > It's important but not the most important thing.
> > It does not make sense to do casts *everywhere*.
> > Do it where it makes sense.
> >
> >> If you want to revisit this style, you should start a separate
> >> thread about it and we can talk about it. But this patch is
> >> consistent with the current infrastructure.
> >>
> >> Regards,
> >>
> >> Anthony Liguori
> >
> > So far QOM was a win.
> > You examples look better with a macro. Patch 13 looks better:
> > - PCIHostState *phb = FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s->pcihost));
> > + PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
> > this is an improvement: devices do not want to deal with sysbus.
> >
> > The code above that is being changed looks better without.
>
> Do you want me to apologize for my macro misuse now? I apologize. :)
> Replace PCI_HOST_BRIDGE(s)->bus = b; with
> PCI_HOST_BRIDGE *phb = PCI_HOST_BRIDGE(s);
> phb->bus = b;
> in your mind and it matches exactly what you liked better above, no?
No, what I liked above is that we hide sysbus from devices.
> > This is why this thread is about the specific patch and not
> > general QOM.
>
> You're basically saying, QOM was a win but don't use it here. That's a
> contradiction and thus a general QOM issue since that paradigm not only
> applies here: Either we need to design all structs so that their fields
> have nice names to access directly as you suggest, or nobody must access
> the parent field as Anthony suggests. Having a wild mix of both at
> maintainers' gusto is not a good solution.
With time we'll end up with a mix anyway since compiler does not enforce
no direct access.
> Arguably we can just leave out this last patch if it is so
> controversial. However, the split is arbitrary since I backwards-coded
> this series, moving code snippets to earlier individual patches using
> checkout -p, i.e. patches 1-11 have this final code design in mind and
> thus went from SYS_BUS_DEVICE() to PCIHostState above rather than
> through parent_obj. The reason I couldn't do it there directly is that
> we didn't have the PCI_HOST_BRIDGE() macro before patch 13.
> I also based patch 14 on the assumption that i440fx will get more state
> fields when Anthony goes ahead with his QOM Pin series, otherwise we
> could just scratch the I440FXState typedef (same discussion as for the
> qtest herbivore test case).
>
> Andreas
The real problem is that devices have to tweak pcihost->bus and that
is because we don't model has-a relationship between pci host bridge and
bus well: pci host bridge has a pci bus. As long as that remains true
it seems to me the more explicit it is the better, so it's easier to
fix.
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
^ permalink raw reply [flat|nested] 33+ messages in thread