From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55404) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbWqA-00053Y-CV for qemu-devel@nongnu.org; Wed, 30 Oct 2013 10:33:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VbWq5-0007ML-G8 for qemu-devel@nongnu.org; Wed, 30 Oct 2013 10:33:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6134) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbWq5-0007KZ-8D for qemu-devel@nongnu.org; Wed, 30 Oct 2013 10:33:41 -0400 Message-ID: <1383143451.4734.30.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Wed, 30 Oct 2013 16:30:51 +0200 In-Reply-To: <87d2mmc19a.fsf@blackfin.pond.sub.org> 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> <87d2mmc19a.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 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: Markus Armbruster Cc: kwolf@redhat.com, peter.maydell@linaro.org, qemu-devel@nongnu.org, borntraeger@de.ibm.com, anthony@codemonkey.ws, Andreas =?ISO-8859-1?Q?F=E4rber?= On Wed, 2013-10-30 at 14:54 +0100, Markus Armbruster wrote: > Andreas F=C3=A4rber writes: >=20 > > 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 shou= ld > >>>> 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, m= ch, > >>>> 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 > >>>> 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 wit= h > >> 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. 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 mor= e > >> than I bargained for in this series :) Yes, I suppose I wouldn't do it either only for this flag that will event= ually disappear, so Reviewed-by: Marcel Apfelbaum > > > > I don't quite follow: We already have an abstract TYPE_PCI_HOST_BRIDG= E > > 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? >=20 > Yes. Sorry for the poor choice of name; I should've checked I got a ne= w > one.