From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46749) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbWEc-0006od-Vi for qemu-devel@nongnu.org; Wed, 30 Oct 2013 09:55:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VbWEU-0002XQ-KI for qemu-devel@nongnu.org; Wed, 30 Oct 2013 09:54:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4677) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbWEU-0002XF-CW for qemu-devel@nongnu.org; Wed, 30 Oct 2013 09:54:50 -0400 From: Markus Armbruster References: <1383062897-30084-1-git-send-email-armbru@redhat.com> <1383062897-30084-6-git-send-email-armbru@redhat.com> <1383126348.4734.19.camel@localhost.localdomain> <87sivjlz52.fsf@blackfin.pond.sub.org> <527105B9.3010104@suse.de> Date: Wed, 30 Oct 2013 14:54:41 +0100 In-Reply-To: <527105B9.3010104@suse.de> ("Andreas =?utf-8?Q?F=C3=A4rber=22?= =?utf-8?Q?'s?= message of "Wed, 30 Oct 2013 14:12:25 +0100") Message-ID: <87d2mmc19a.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 05/10] pci-host: Consistently set cannot_instantiate_with_device_add_yet List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: kwolf@redhat.com, peter.maydell@linaro.org, Marcel Apfelbaum , qemu-devel@nongnu.org, borntraeger@de.ibm.com, anthony@codemonkey.ws Andreas F=C3=A4rber writes: > Am 30.10.2013 13:30, schrieb Markus Armbruster: >> Marcel Apfelbaum writes: >>=20 >>> On Tue, 2013-10-29 at 17:08 +0100, armbru@redhat.com wrote: >>>> From: Markus Armbruster >>>> >>>> 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 >>>> --- >>>> 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 =3D PCI_DEVICE_CLASS(klass); >>>> + DeviceClass *dc =3D DEVICE_CLASS(klass); >>>>=20=20 >>>> k->init =3D gt64120_pci_init; >>>> k->vendor_id =3D PCI_VENDOR_ID_MARVELL; >>>> k->device_id =3D PCI_DEVICE_ID_MARVELL_GT6412X; >>>> k->revision =3D 0x10; >>>> k->class_id =3D 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 =3D true; >>> I noticed that all class_id in this patch are PCI_CLASS_BRIDGE_HOST. >>=20 >> Correct. >>=20 >>> 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? >>=20 >> My understanding of QOM is rather limited, so take the following with >> due skepticism. >>=20 >> 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.