On Wed, 2013-10-30 at 14:54 +0100, Markus Armbruster wrote:
> Andreas Färber <afaer...@suse.de> writes:
>
> > Am 30.10.2013 13:30, schrieb Markus Armbruster:
> >> Marcel Apfelbaum <marce...@redhat.com> writes:
> >>
> >>> On Tue, 2013-10-29 at 17:08 +0100, arm...@redhat.com wrote:
> >>>> From: Markus Armbruster <arm...@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 <arm...@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 <marce...@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.
