| Message ID | 1383062897-30084-6-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: > From: Markus Armbruster <armbru@redhat.com> > > Many PCI host bridges consist of a sysbus device and a PCI device. > You need both for the thing to work. Arguably, these bridges should > be modelled as a single, composite devices instead of pairs of > seemingly independent devices you can only use together, but we're not > there, yet. > > Since the sysbus part can't be instantiated with device_add, yet, > permitting it with the PCI part is useless. We shouldn't offer > useless options to the user, so let's set > cannot_instantiate_with_device_add_yet for them. > > It's already set for Bonito, grackle, i440FX, and raven. Document > why. > > Set it for the others: dec-21154, e500-host-bridge, gt64120_pci, mch, > pbm-pci, ppc4xx-host-bridge, sh_pci_host, u3-agp, uni-north-agp, > uni-north-internal-pci, uni-north-pci, and versatile_pci_host. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/mips/gt64xxx_pci.c | 6 ++++++ > hw/pci-bridge/dec.c | 6 ++++++ > hw/pci-host/apb.c | 6 ++++++ > hw/pci-host/bonito.c | 6 +++++- > hw/pci-host/grackle.c | 6 +++++- > hw/pci-host/piix.c | 6 +++++- > hw/pci-host/ppce500.c | 5 +++++ > hw/pci-host/prep.c | 6 +++++- > hw/pci-host/q35.c | 5 +++++ > hw/pci-host/uninorth.c | 24 ++++++++++++++++++++++++ > hw/pci-host/versatile.c | 6 ++++++ > hw/ppc/ppc4xx_pci.c | 5 +++++ > hw/sh4/sh_pci.c | 6 ++++++ > 13 files changed, 89 insertions(+), 4 deletions(-) > > diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c > index 3da2e67..6398514 100644 > --- a/hw/mips/gt64xxx_pci.c > +++ b/hw/mips/gt64xxx_pci.c > @@ -1151,12 +1151,18 @@ static int gt64120_pci_init(PCIDevice *d) > static void gt64120_pci_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = gt64120_pci_init; > k->vendor_id = PCI_VENDOR_ID_MARVELL; > k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X; > k->revision = 0x10; > k->class_id = PCI_CLASS_BRIDGE_HOST; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; I noticed that all class_id in this patch are PCI_CLASS_BRIDGE_HOST. What do you think about a different approach: check class_id in parent class init func and set the flag according to it? It corresponds to your idea of changing only sysbus base class. Here we don't have a "natural" base class, but we can use class_id. What do you think? Marcel > } > > static const TypeInfo gt64120_pci_info = { > diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c > index e5e3be8..a6ca940 100644 > --- a/hw/pci-bridge/dec.c > +++ b/hw/pci-bridge/dec.c > @@ -116,6 +116,7 @@ static int dec_21154_pci_host_init(PCIDevice *d) > static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = dec_21154_pci_host_init; > k->vendor_id = PCI_VENDOR_ID_DEC; > @@ -123,6 +124,11 @@ static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data) > k->revision = 0x02; > k->class_id = PCI_CLASS_BRIDGE_PCI; > k->is_bridge = 1; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo dec_21154_pci_host_info = { > diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c > index 92f289f..1b399dd 100644 > --- a/hw/pci-host/apb.c > +++ b/hw/pci-host/apb.c > @@ -516,11 +516,17 @@ static int pbm_pci_host_init(PCIDevice *d) > static void pbm_pci_host_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = pbm_pci_host_init; > k->vendor_id = PCI_VENDOR_ID_SUN; > k->device_id = PCI_DEVICE_ID_SUN_SABRE; > k->class_id = PCI_CLASS_BRIDGE_HOST; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo pbm_pci_host_info = { > diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c > index bfb9820..902441f 100644 > --- a/hw/pci-host/bonito.c > +++ b/hw/pci-host/bonito.c > @@ -806,8 +806,12 @@ static void bonito_class_init(ObjectClass *klass, void *data) > k->revision = 0x01; > k->class_id = PCI_CLASS_BRIDGE_HOST; > dc->desc = "Host bridge"; > - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */ > dc->vmsd = &vmstate_bonito; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo bonito_info = { > diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c > index c178375..7d95821 100644 > --- a/hw/pci-host/grackle.c > +++ b/hw/pci-host/grackle.c > @@ -130,7 +130,11 @@ static void grackle_pci_class_init(ObjectClass *klass, void *data) > k->device_id = PCI_DEVICE_ID_MOTOROLA_MPC106; > k->revision = 0x00; > k->class_id = PCI_CLASS_BRIDGE_HOST; > - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */ > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo grackle_pci_info = { > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c > index 21ffe97..8089fd6 100644 > --- a/hw/pci-host/piix.c > +++ b/hw/pci-host/piix.c > @@ -698,8 +698,12 @@ static void i440fx_class_init(ObjectClass *klass, void *data) > k->revision = 0x02; > k->class_id = PCI_CLASS_BRIDGE_HOST; > dc->desc = "Host bridge"; > - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */ > dc->vmsd = &vmstate_i440fx; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo i440fx_info = { > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c > index f00793d..c80b7cb 100644 > --- a/hw/pci-host/ppce500.c > +++ b/hw/pci-host/ppce500.c > @@ -387,6 +387,11 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data) > k->device_id = PCI_DEVICE_ID_MPC8533E; > k->class_id = PCI_CLASS_PROCESSOR_POWERPC; > dc->desc = "Host bridge"; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo e500_host_bridge_info = { > diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c > index ebc40c6..042dc8f 100644 > --- a/hw/pci-host/prep.c > +++ b/hw/pci-host/prep.c > @@ -198,7 +198,11 @@ static void raven_class_init(ObjectClass *klass, void *data) > k->class_id = PCI_CLASS_BRIDGE_HOST; > dc->desc = "PReP Host Bridge - Motorola Raven"; > dc->vmsd = &vmstate_raven; > - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */ > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo raven_info = { > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index ad703a4..4dd75c6 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -390,6 +390,11 @@ static void mch_class_init(ObjectClass *klass, void *data) > k->device_id = PCI_DEVICE_ID_INTEL_Q35_MCH; > k->revision = MCH_HOST_BRIDGE_REVISION_DEFAULT; > k->class_id = PCI_CLASS_BRIDGE_HOST; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo mch_info = { > diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c > index 91530cd..ae04be5 100644 > --- a/hw/pci-host/uninorth.c > +++ b/hw/pci-host/uninorth.c > @@ -351,12 +351,18 @@ static int unin_internal_pci_host_init(PCIDevice *d) > static void unin_main_pci_host_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = unin_main_pci_host_init; > k->vendor_id = PCI_VENDOR_ID_APPLE; > k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI; > k->revision = 0x00; > k->class_id = PCI_CLASS_BRIDGE_HOST; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo unin_main_pci_host_info = { > @@ -369,12 +375,18 @@ static const TypeInfo unin_main_pci_host_info = { > static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = u3_agp_pci_host_init; > k->vendor_id = PCI_VENDOR_ID_APPLE; > k->device_id = PCI_DEVICE_ID_APPLE_U3_AGP; > k->revision = 0x00; > k->class_id = PCI_CLASS_BRIDGE_HOST; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo u3_agp_pci_host_info = { > @@ -387,12 +399,18 @@ static const TypeInfo u3_agp_pci_host_info = { > static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = unin_agp_pci_host_init; > k->vendor_id = PCI_VENDOR_ID_APPLE; > k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_AGP; > k->revision = 0x00; > k->class_id = PCI_CLASS_BRIDGE_HOST; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo unin_agp_pci_host_info = { > @@ -405,12 +423,18 @@ static const TypeInfo unin_agp_pci_host_info = { > static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = unin_internal_pci_host_init; > k->vendor_id = PCI_VENDOR_ID_APPLE; > k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_I_PCI; > k->revision = 0x00; > k->class_id = PCI_CLASS_BRIDGE_HOST; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo unin_internal_pci_host_info = { > diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c > index 6b28929..71ff0de 100644 > --- a/hw/pci-host/versatile.c > +++ b/hw/pci-host/versatile.c > @@ -467,11 +467,17 @@ static int versatile_pci_host_init(PCIDevice *d) > static void versatile_pci_host_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = versatile_pci_host_init; > k->vendor_id = PCI_VENDOR_ID_XILINX; > k->device_id = PCI_DEVICE_ID_XILINX_XC2VP30; > k->class_id = PCI_CLASS_PROCESSOR_CO; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo versatile_pci_host_info = { > diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c > index d2d6f65..4cb7851 100644 > --- a/hw/ppc/ppc4xx_pci.c > +++ b/hw/ppc/ppc4xx_pci.c > @@ -380,6 +380,11 @@ static void ppc4xx_host_bridge_class_init(ObjectClass *klass, void *data) > k->vendor_id = PCI_VENDOR_ID_IBM; > k->device_id = PCI_DEVICE_ID_IBM_440GX; > k->class_id = PCI_CLASS_BRIDGE_OTHER; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo ppc4xx_host_bridge_info = { > diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c > index e81176a..a2f6d9e 100644 > --- a/hw/sh4/sh_pci.c > +++ b/hw/sh4/sh_pci.c > @@ -162,10 +162,16 @@ static int sh_pci_host_init(PCIDevice *d) > static void sh_pci_host_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + DeviceClass *dc = DEVICE_CLASS(klass); > > k->init = sh_pci_host_init; > k->vendor_id = PCI_VENDOR_ID_HITACHI; > k->device_id = PCI_DEVICE_ID_HITACHI_SH7751R; > + /* > + * PCI-facing part of the host bridge, not usable without the > + * host-facing part, which can't be device_add'ed, yet. > + */ > + dc->cannot_instantiate_with_device_add_yet = true; > } > > static const TypeInfo sh_pci_host_info = {
Marcel Apfelbaum <marcel.a@redhat.com> writes: > On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: >> From: Markus Armbruster <armbru@redhat.com> >> >> Many PCI host bridges consist of a sysbus device and a PCI device. >> You need both for the thing to work. Arguably, these bridges should >> be modelled as a single, composite devices instead of pairs of >> seemingly independent devices you can only use together, but we're not >> there, yet. >> >> Since the sysbus part can't be instantiated with device_add, yet, >> permitting it with the PCI part is useless. We shouldn't offer >> useless options to the user, so let's set >> cannot_instantiate_with_device_add_yet for them. >> >> It's already set for Bonito, grackle, i440FX, and raven. Document >> why. >> >> Set it for the others: dec-21154, e500-host-bridge, gt64120_pci, mch, >> pbm-pci, ppc4xx-host-bridge, sh_pci_host, u3-agp, uni-north-agp, >> uni-north-internal-pci, uni-north-pci, and versatile_pci_host. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/mips/gt64xxx_pci.c | 6 ++++++ >> hw/pci-bridge/dec.c | 6 ++++++ >> hw/pci-host/apb.c | 6 ++++++ >> hw/pci-host/bonito.c | 6 +++++- >> hw/pci-host/grackle.c | 6 +++++- >> hw/pci-host/piix.c | 6 +++++- >> hw/pci-host/ppce500.c | 5 +++++ >> hw/pci-host/prep.c | 6 +++++- >> hw/pci-host/q35.c | 5 +++++ >> hw/pci-host/uninorth.c | 24 ++++++++++++++++++++++++ >> hw/pci-host/versatile.c | 6 ++++++ >> hw/ppc/ppc4xx_pci.c | 5 +++++ >> hw/sh4/sh_pci.c | 6 ++++++ >> 13 files changed, 89 insertions(+), 4 deletions(-) >> >> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c >> index 3da2e67..6398514 100644 >> --- a/hw/mips/gt64xxx_pci.c >> +++ b/hw/mips/gt64xxx_pci.c >> @@ -1151,12 +1151,18 @@ static int gt64120_pci_init(PCIDevice *d) >> static void gt64120_pci_class_init(ObjectClass *klass, void *data) >> { >> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >> + DeviceClass *dc = DEVICE_CLASS(klass); >> >> k->init = gt64120_pci_init; >> k->vendor_id = PCI_VENDOR_ID_MARVELL; >> k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X; >> k->revision = 0x10; >> k->class_id = PCI_CLASS_BRIDGE_HOST; >> + /* >> + * PCI-facing part of the host bridge, not usable without the >> + * host-facing part, which can't be device_add'ed, yet. >> + */ >> + dc->cannot_instantiate_with_device_add_yet = true; > I noticed that all class_id in this patch are PCI_CLASS_BRIDGE_HOST. Correct. > What do you think about a different approach: check class_id > in parent class init func and set the flag according to it? > It corresponds to your idea of changing only sysbus base class. > Here we don't have a "natural" base class, but we can use class_id. > What do you think? My understanding of QOM is rather limited, so take the following with due skepticism. I'm afraid the parent's class_init() runs before the child's, and therefore can't see class_id PCI_CLASS_BRIDGE_HOST set by the child's class_init(). To factor common initialization code, I figure I'd have to splice in an abstract TYPE_PCI_HOST_BRIDGE between TYPE_PCI_DEVICE and the host bridge types such as this one. Might make sense, but it's a bit more than I bargained for in this series :)
Am 30.10.2013 13:30, schrieb Markus Armbruster: > Marcel Apfelbaum <marcel.a@redhat.com> writes: > >> On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: >>> From: Markus Armbruster <armbru@redhat.com> >>> >>> Many PCI host bridges consist of a sysbus device and a PCI device. >>> You need both for the thing to work. Arguably, these bridges should >>> be modelled as a single, composite devices instead of pairs of >>> seemingly independent devices you can only use together, but we're not >>> there, yet. >>> >>> Since the sysbus part can't be instantiated with device_add, yet, >>> permitting it with the PCI part is useless. We shouldn't offer >>> useless options to the user, so let's set >>> cannot_instantiate_with_device_add_yet for them. >>> >>> It's already set for Bonito, grackle, i440FX, and raven. Document >>> why. >>> >>> Set it for the others: dec-21154, e500-host-bridge, gt64120_pci, mch, >>> pbm-pci, ppc4xx-host-bridge, sh_pci_host, u3-agp, uni-north-agp, >>> uni-north-internal-pci, uni-north-pci, and versatile_pci_host. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> hw/mips/gt64xxx_pci.c | 6 ++++++ >>> hw/pci-bridge/dec.c | 6 ++++++ >>> hw/pci-host/apb.c | 6 ++++++ >>> hw/pci-host/bonito.c | 6 +++++- >>> hw/pci-host/grackle.c | 6 +++++- >>> hw/pci-host/piix.c | 6 +++++- >>> hw/pci-host/ppce500.c | 5 +++++ >>> hw/pci-host/prep.c | 6 +++++- >>> hw/pci-host/q35.c | 5 +++++ >>> hw/pci-host/uninorth.c | 24 ++++++++++++++++++++++++ >>> hw/pci-host/versatile.c | 6 ++++++ >>> hw/ppc/ppc4xx_pci.c | 5 +++++ >>> hw/sh4/sh_pci.c | 6 ++++++ >>> 13 files changed, 89 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c >>> index 3da2e67..6398514 100644 >>> --- a/hw/mips/gt64xxx_pci.c >>> +++ b/hw/mips/gt64xxx_pci.c >>> @@ -1151,12 +1151,18 @@ static int gt64120_pci_init(PCIDevice *d) >>> static void gt64120_pci_class_init(ObjectClass *klass, void *data) >>> { >>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>> + DeviceClass *dc = DEVICE_CLASS(klass); >>> >>> k->init = gt64120_pci_init; >>> k->vendor_id = PCI_VENDOR_ID_MARVELL; >>> k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X; >>> k->revision = 0x10; >>> k->class_id = PCI_CLASS_BRIDGE_HOST; >>> + /* >>> + * PCI-facing part of the host bridge, not usable without the >>> + * host-facing part, which can't be device_add'ed, yet. >>> + */ >>> + dc->cannot_instantiate_with_device_add_yet = true; >> I noticed that all class_id in this patch are PCI_CLASS_BRIDGE_HOST. > > Correct. > >> What do you think about a different approach: check class_id >> in parent class init func and set the flag according to it? >> It corresponds to your idea of changing only sysbus base class. >> Here we don't have a "natural" base class, but we can use class_id. >> What do you think? > > My understanding of QOM is rather limited, so take the following with > due skepticism. > > I'm afraid the parent's class_init() runs before the child's, and > therefore can't see class_id PCI_CLASS_BRIDGE_HOST set by the child's > class_init(). Right. > To factor common initialization code, I figure I'd have to splice in an > abstract TYPE_PCI_HOST_BRIDGE between TYPE_PCI_DEVICE and the host > bridge types such as this one. Might make sense, but it's a bit more > than I bargained for in this series :) I don't quite follow: We already have an abstract TYPE_PCI_HOST_BRIDGE in between TYPE_SYS_BUS_DEVICE and the concrete host bridge. You mean a base type TYPE_PCI_HOST_BRIDGE_DEVICE for the PCIDevice representing the controller on the PCIBus it exposes? Regards, Andreas
Andreas Färber <afaerber@suse.de> writes: > Am 30.10.2013 13:30, schrieb Markus Armbruster: >> Marcel Apfelbaum <marcel.a@redhat.com> writes: >> >>> On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: >>>> From: Markus Armbruster <armbru@redhat.com> >>>> >>>> Many PCI host bridges consist of a sysbus device and a PCI device. >>>> You need both for the thing to work. Arguably, these bridges should >>>> be modelled as a single, composite devices instead of pairs of >>>> seemingly independent devices you can only use together, but we're not >>>> there, yet. >>>> >>>> Since the sysbus part can't be instantiated with device_add, yet, >>>> permitting it with the PCI part is useless. We shouldn't offer >>>> useless options to the user, so let's set >>>> cannot_instantiate_with_device_add_yet for them. >>>> >>>> It's already set for Bonito, grackle, i440FX, and raven. Document >>>> why. >>>> >>>> Set it for the others: dec-21154, e500-host-bridge, gt64120_pci, mch, >>>> pbm-pci, ppc4xx-host-bridge, sh_pci_host, u3-agp, uni-north-agp, >>>> uni-north-internal-pci, uni-north-pci, and versatile_pci_host. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>>> hw/mips/gt64xxx_pci.c | 6 ++++++ >>>> hw/pci-bridge/dec.c | 6 ++++++ >>>> hw/pci-host/apb.c | 6 ++++++ >>>> hw/pci-host/bonito.c | 6 +++++- >>>> hw/pci-host/grackle.c | 6 +++++- >>>> hw/pci-host/piix.c | 6 +++++- >>>> hw/pci-host/ppce500.c | 5 +++++ >>>> hw/pci-host/prep.c | 6 +++++- >>>> hw/pci-host/q35.c | 5 +++++ >>>> hw/pci-host/uninorth.c | 24 ++++++++++++++++++++++++ >>>> hw/pci-host/versatile.c | 6 ++++++ >>>> hw/ppc/ppc4xx_pci.c | 5 +++++ >>>> hw/sh4/sh_pci.c | 6 ++++++ >>>> 13 files changed, 89 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c >>>> index 3da2e67..6398514 100644 >>>> --- a/hw/mips/gt64xxx_pci.c >>>> +++ b/hw/mips/gt64xxx_pci.c >>>> @@ -1151,12 +1151,18 @@ static int gt64120_pci_init(PCIDevice *d) >>>> static void gt64120_pci_class_init(ObjectClass *klass, void *data) >>>> { >>>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); >>>> + DeviceClass *dc = DEVICE_CLASS(klass); >>>> >>>> k->init = gt64120_pci_init; >>>> k->vendor_id = PCI_VENDOR_ID_MARVELL; >>>> k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X; >>>> k->revision = 0x10; >>>> k->class_id = PCI_CLASS_BRIDGE_HOST; >>>> + /* >>>> + * PCI-facing part of the host bridge, not usable without the >>>> + * host-facing part, which can't be device_add'ed, yet. >>>> + */ >>>> + dc->cannot_instantiate_with_device_add_yet = true; >>> I noticed that all class_id in this patch are PCI_CLASS_BRIDGE_HOST. >> >> Correct. >> >>> What do you think about a different approach: check class_id >>> in parent class init func and set the flag according to it? >>> It corresponds to your idea of changing only sysbus base class. >>> Here we don't have a "natural" base class, but we can use class_id. >>> What do you think? >> >> My understanding of QOM is rather limited, so take the following with >> due skepticism. >> >> I'm afraid the parent's class_init() runs before the child's, and >> therefore can't see class_id PCI_CLASS_BRIDGE_HOST set by the child's >> class_init(). > > Right. > >> To factor common initialization code, I figure I'd have to splice in an >> abstract TYPE_PCI_HOST_BRIDGE between TYPE_PCI_DEVICE and the host >> bridge types such as this one. Might make sense, but it's a bit more >> than I bargained for in this series :) > > I don't quite follow: We already have an abstract TYPE_PCI_HOST_BRIDGE > in between TYPE_SYS_BUS_DEVICE and the concrete host bridge. You mean a > base type TYPE_PCI_HOST_BRIDGE_DEVICE for the PCIDevice representing the > controller on the PCIBus it exposes? Yes. Sorry for the poor choice of name; I should've checked I got a new one.
On Wed, 2013-10-30 at 14:54 +0100, Markus Armbruster wrote: > Andreas Färber <afaerber@suse.de> writes: > > > Am 30.10.2013 13:30, schrieb Markus Armbruster: > >> Marcel Apfelbaum <marcel.a@redhat.com> writes: > >> > >>> On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: > >>>> From: Markus Armbruster <armbru@redhat.com> > >>>> > >>>> Many PCI host bridges consist of a sysbus device and a PCI device. > >>>> You need both for the thing to work. Arguably, these bridges should > >>>> be modelled as a single, composite devices instead of pairs of > >>>> seemingly independent devices you can only use together, but we're not > >>>> there, yet. > >>>> > >>>> Since the sysbus part can't be instantiated with device_add, yet, > >>>> permitting it with the PCI part is useless. We shouldn't offer > >>>> useless options to the user, so let's set > >>>> cannot_instantiate_with_device_add_yet for them. > >>>> > >>>> It's already set for Bonito, grackle, i440FX, and raven. Document > >>>> why. > >>>> > >>>> Set it for the others: dec-21154, e500-host-bridge, gt64120_pci, mch, > >>>> pbm-pci, ppc4xx-host-bridge, sh_pci_host, u3-agp, uni-north-agp, > >>>> uni-north-internal-pci, uni-north-pci, and versatile_pci_host. > >>>> > >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >>>> --- > >>>> hw/mips/gt64xxx_pci.c | 6 ++++++ > >>>> hw/pci-bridge/dec.c | 6 ++++++ > >>>> hw/pci-host/apb.c | 6 ++++++ > >>>> hw/pci-host/bonito.c | 6 +++++- > >>>> hw/pci-host/grackle.c | 6 +++++- > >>>> hw/pci-host/piix.c | 6 +++++- > >>>> hw/pci-host/ppce500.c | 5 +++++ > >>>> hw/pci-host/prep.c | 6 +++++- > >>>> hw/pci-host/q35.c | 5 +++++ > >>>> hw/pci-host/uninorth.c | 24 ++++++++++++++++++++++++ > >>>> hw/pci-host/versatile.c | 6 ++++++ > >>>> hw/ppc/ppc4xx_pci.c | 5 +++++ > >>>> hw/sh4/sh_pci.c | 6 ++++++ > >>>> 13 files changed, 89 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c > >>>> index 3da2e67..6398514 100644 > >>>> --- a/hw/mips/gt64xxx_pci.c > >>>> +++ b/hw/mips/gt64xxx_pci.c > >>>> @@ -1151,12 +1151,18 @@ static int gt64120_pci_init(PCIDevice *d) > >>>> static void gt64120_pci_class_init(ObjectClass *klass, void *data) > >>>> { > >>>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > >>>> + DeviceClass *dc = DEVICE_CLASS(klass); > >>>> > >>>> k->init = gt64120_pci_init; > >>>> k->vendor_id = PCI_VENDOR_ID_MARVELL; > >>>> k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X; > >>>> k->revision = 0x10; > >>>> k->class_id = PCI_CLASS_BRIDGE_HOST; > >>>> + /* > >>>> + * PCI-facing part of the host bridge, not usable without the > >>>> + * host-facing part, which can't be device_add'ed, yet. > >>>> + */ > >>>> + dc->cannot_instantiate_with_device_add_yet = true; > >>> I noticed that all class_id in this patch are PCI_CLASS_BRIDGE_HOST. > >> > >> Correct. > >> > >>> What do you think about a different approach: check class_id > >>> in parent class init func and set the flag according to it? > >>> It corresponds to your idea of changing only sysbus base class. > >>> Here we don't have a "natural" base class, but we can use class_id. > >>> What do you think? > >> > >> My understanding of QOM is rather limited, so take the following with > >> due skepticism. > >> > >> I'm afraid the parent's class_init() runs before the child's, and > >> therefore can't see class_id PCI_CLASS_BRIDGE_HOST set by the child's > >> class_init(). > > > > Right. So the only way would be a base class. > > > >> To factor common initialization code, I figure I'd have to splice in an > >> abstract TYPE_PCI_HOST_BRIDGE between TYPE_PCI_DEVICE and the host > >> bridge types such as this one. Might make sense, but it's a bit more > >> than I bargained for in this series :) Yes, I suppose I wouldn't do it either only for this flag that will eventually disappear, so Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > I don't quite follow: We already have an abstract TYPE_PCI_HOST_BRIDGE > > in between TYPE_SYS_BUS_DEVICE and the concrete host bridge. You mean a > > base type TYPE_PCI_HOST_BRIDGE_DEVICE for the PCIDevice representing the > > controller on the PCIBus it exposes? > > Yes. Sorry for the poor choice of name; I should've checked I got a new > one.
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c index 3da2e67..6398514 100644 --- a/hw/mips/gt64xxx_pci.c +++ b/hw/mips/gt64xxx_pci.c @@ -1151,12 +1151,18 @@ static int gt64120_pci_init(PCIDevice *d) static void gt64120_pci_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = gt64120_pci_init; k->vendor_id = PCI_VENDOR_ID_MARVELL; k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X; k->revision = 0x10; k->class_id = PCI_CLASS_BRIDGE_HOST; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo gt64120_pci_info = { diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c index e5e3be8..a6ca940 100644 --- a/hw/pci-bridge/dec.c +++ b/hw/pci-bridge/dec.c @@ -116,6 +116,7 @@ static int dec_21154_pci_host_init(PCIDevice *d) static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = dec_21154_pci_host_init; k->vendor_id = PCI_VENDOR_ID_DEC; @@ -123,6 +124,11 @@ static void dec_21154_pci_host_class_init(ObjectClass *klass, void *data) k->revision = 0x02; k->class_id = PCI_CLASS_BRIDGE_PCI; k->is_bridge = 1; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo dec_21154_pci_host_info = { diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c index 92f289f..1b399dd 100644 --- a/hw/pci-host/apb.c +++ b/hw/pci-host/apb.c @@ -516,11 +516,17 @@ static int pbm_pci_host_init(PCIDevice *d) static void pbm_pci_host_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = pbm_pci_host_init; k->vendor_id = PCI_VENDOR_ID_SUN; k->device_id = PCI_DEVICE_ID_SUN_SABRE; k->class_id = PCI_CLASS_BRIDGE_HOST; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo pbm_pci_host_info = { diff --git a/hw/pci-host/bonito.c b/hw/pci-host/bonito.c index bfb9820..902441f 100644 --- a/hw/pci-host/bonito.c +++ b/hw/pci-host/bonito.c @@ -806,8 +806,12 @@ static void bonito_class_init(ObjectClass *klass, void *data) k->revision = 0x01; k->class_id = PCI_CLASS_BRIDGE_HOST; dc->desc = "Host bridge"; - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */ dc->vmsd = &vmstate_bonito; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo bonito_info = { diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c index c178375..7d95821 100644 --- a/hw/pci-host/grackle.c +++ b/hw/pci-host/grackle.c @@ -130,7 +130,11 @@ static void grackle_pci_class_init(ObjectClass *klass, void *data) k->device_id = PCI_DEVICE_ID_MOTOROLA_MPC106; k->revision = 0x00; k->class_id = PCI_CLASS_BRIDGE_HOST; - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */ + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo grackle_pci_info = { diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index 21ffe97..8089fd6 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -698,8 +698,12 @@ static void i440fx_class_init(ObjectClass *klass, void *data) k->revision = 0x02; k->class_id = PCI_CLASS_BRIDGE_HOST; dc->desc = "Host bridge"; - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */ dc->vmsd = &vmstate_i440fx; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo i440fx_info = { diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index f00793d..c80b7cb 100644 --- a/hw/pci-host/ppce500.c +++ b/hw/pci-host/ppce500.c @@ -387,6 +387,11 @@ static void e500_host_bridge_class_init(ObjectClass *klass, void *data) k->device_id = PCI_DEVICE_ID_MPC8533E; k->class_id = PCI_CLASS_PROCESSOR_POWERPC; dc->desc = "Host bridge"; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo e500_host_bridge_info = { diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c index ebc40c6..042dc8f 100644 --- a/hw/pci-host/prep.c +++ b/hw/pci-host/prep.c @@ -198,7 +198,11 @@ static void raven_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_BRIDGE_HOST; dc->desc = "PReP Host Bridge - Motorola Raven"; dc->vmsd = &vmstate_raven; - dc->cannot_instantiate_with_device_add_yet = true; /* FIXME explain why */ + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo raven_info = { diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index ad703a4..4dd75c6 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -390,6 +390,11 @@ static void mch_class_init(ObjectClass *klass, void *data) k->device_id = PCI_DEVICE_ID_INTEL_Q35_MCH; k->revision = MCH_HOST_BRIDGE_REVISION_DEFAULT; k->class_id = PCI_CLASS_BRIDGE_HOST; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo mch_info = { diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c index 91530cd..ae04be5 100644 --- a/hw/pci-host/uninorth.c +++ b/hw/pci-host/uninorth.c @@ -351,12 +351,18 @@ static int unin_internal_pci_host_init(PCIDevice *d) static void unin_main_pci_host_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = unin_main_pci_host_init; k->vendor_id = PCI_VENDOR_ID_APPLE; k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI; k->revision = 0x00; k->class_id = PCI_CLASS_BRIDGE_HOST; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo unin_main_pci_host_info = { @@ -369,12 +375,18 @@ static const TypeInfo unin_main_pci_host_info = { static void u3_agp_pci_host_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = u3_agp_pci_host_init; k->vendor_id = PCI_VENDOR_ID_APPLE; k->device_id = PCI_DEVICE_ID_APPLE_U3_AGP; k->revision = 0x00; k->class_id = PCI_CLASS_BRIDGE_HOST; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo u3_agp_pci_host_info = { @@ -387,12 +399,18 @@ static const TypeInfo u3_agp_pci_host_info = { static void unin_agp_pci_host_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = unin_agp_pci_host_init; k->vendor_id = PCI_VENDOR_ID_APPLE; k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_AGP; k->revision = 0x00; k->class_id = PCI_CLASS_BRIDGE_HOST; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo unin_agp_pci_host_info = { @@ -405,12 +423,18 @@ static const TypeInfo unin_agp_pci_host_info = { static void unin_internal_pci_host_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = unin_internal_pci_host_init; k->vendor_id = PCI_VENDOR_ID_APPLE; k->device_id = PCI_DEVICE_ID_APPLE_UNI_N_I_PCI; k->revision = 0x00; k->class_id = PCI_CLASS_BRIDGE_HOST; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo unin_internal_pci_host_info = { diff --git a/hw/pci-host/versatile.c b/hw/pci-host/versatile.c index 6b28929..71ff0de 100644 --- a/hw/pci-host/versatile.c +++ b/hw/pci-host/versatile.c @@ -467,11 +467,17 @@ static int versatile_pci_host_init(PCIDevice *d) static void versatile_pci_host_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = versatile_pci_host_init; k->vendor_id = PCI_VENDOR_ID_XILINX; k->device_id = PCI_DEVICE_ID_XILINX_XC2VP30; k->class_id = PCI_CLASS_PROCESSOR_CO; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo versatile_pci_host_info = { diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c index d2d6f65..4cb7851 100644 --- a/hw/ppc/ppc4xx_pci.c +++ b/hw/ppc/ppc4xx_pci.c @@ -380,6 +380,11 @@ static void ppc4xx_host_bridge_class_init(ObjectClass *klass, void *data) k->vendor_id = PCI_VENDOR_ID_IBM; k->device_id = PCI_DEVICE_ID_IBM_440GX; k->class_id = PCI_CLASS_BRIDGE_OTHER; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo ppc4xx_host_bridge_info = { diff --git a/hw/sh4/sh_pci.c b/hw/sh4/sh_pci.c index e81176a..a2f6d9e 100644 --- a/hw/sh4/sh_pci.c +++ b/hw/sh4/sh_pci.c @@ -162,10 +162,16 @@ static int sh_pci_host_init(PCIDevice *d) static void sh_pci_host_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + DeviceClass *dc = DEVICE_CLASS(klass); k->init = sh_pci_host_init; k->vendor_id = PCI_VENDOR_ID_HITACHI; k->device_id = PCI_DEVICE_ID_HITACHI_SH7751R; + /* + * PCI-facing part of the host bridge, not usable without the + * host-facing part, which can't be device_add'ed, yet. + */ + dc->cannot_instantiate_with_device_add_yet = true; } static const TypeInfo sh_pci_host_info = {