Re: [Qemu-devel] [PATCH v2 05/10] pci-host: Consistently set cannot_instantiate_with_device_add_yet

[Markus Armbruster <armbru@redhat.com>]

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.