From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38463) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SgkZP-0006Co-Gw for qemu-devel@nongnu.org; Mon, 18 Jun 2012 18:37:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SgkZN-000120-GV for qemu-devel@nongnu.org; Mon, 18 Jun 2012 18:37:15 -0400 Date: Tue, 19 Jun 2012 01:37:06 +0300 From: "Michael S. Tsirkin" Message-ID: <20120618223706.GA1863@redhat.com> References: <1339343875-6958-1-git-send-email-afaerber@suse.de> <1339343875-6958-2-git-send-email-afaerber@suse.de> <20120618182854.GB29700@redhat.com> <4FDFA129.5070203@suse.de> <4FDFAA64.9090502@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <4FDFAA64.9090502@codemonkey.ws> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 1/2] pci_host: Turn into SysBus-derived QOM type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Wanpeng Li , Anthony Liguori , qemu-ppc , Andreas =?iso-8859-1?Q?F=E4rber?= , qemu-devel@nongnu.org On Mon, Jun 18, 2012 at 05:23:32PM -0500, Anthony Liguori wrote: > On 06/18/2012 04:44 PM, Andreas F=E4rber wrote: > >Am 18.06.2012 20:28, schrieb Michael S. Tsirkin: > >>On Sun, Jun 10, 2012 at 05:57:54PM +0200, Andreas F=E4rber wrote: > >>>From: Andreas F=E4rber > >>> > >>>Allows us to access PCIHostState QOM-style with PCI_HOST() macro. > >>> > >>>Update PReP Raven PCI to derive from this type. > >>> > >>>Signed-off-by: Anthony Liguori > >>>Signed-off-by: Wanpeng Li > >>>Signed-off-by: Andreas F=E4rber > >>>Reviewed-by: Anthony Liguori > >> > >>Question: this is really a pci host *bridge*. > >>We are calling this PCIHost internally for brevity > >>but QOM hierarchy is user-visible, right? > > > >That's a good question... I would say it's not user-visible today unle= ss > >we instantiate TYPE_PCI_HOST directly, in which case its value > >"pci-host" would be visible through the "type" property that got > >introduced on qom-next. My CPU modeling for instance is based on the > >assumption that we can introduce intermediate types later as a > >user-invisible implementation detail. >=20 > Yes, we need to formulate a support statement for the 1.2 release. >=20 > My general thinking is: >=20 > 1) Properties will remain ABI compatible. A property will not > change it's type or semantics over time. An example is link/child > properties. A link will always remain a link but the link subtype > made be made more specific over time. Likewise with child > properties. >=20 > 2) A property may be removed and new properties may be added. At will? That's a very weak statement. > Applications should always gracefully handle the non-existence of a > property. They can fail gracefully but what else can they do? > 3) Since paths are composed of properties, they are subject to the > same rules. That is, an absolute path will always have the same > semantics as long as that path is still valid. >=20 > 4) No guarantee is made about the stability of relative paths. >=20 > Types are really just another form of properties here so changing > the type of an object provided that we don't violate ABI rules is > okay. Let's invent internal types that we don't expect user to know about. We have x- for properties, let's do the same here. > I actually think it's okay for a typename to change even if it's > exposed by -device. If something is using -device, it ought to > probe for the existence of a type before using -device. >=20 > Regards, >=20 > Anthony Liguori Then it'll fail gracefully but it can't work. > > > >>>--- > >>> hw/pci_host.c | 11 +++++++++++ > >>> hw/pci_host.h | 3 +++ > >>> hw/prep_pci.c | 4 ++-- > >>> 3 files changed, 16 insertions(+), 2 deletions(-) > >>> > >>>diff --git a/hw/pci_host.c b/hw/pci_host.c > >>>index 8041778..347bfa6 100644 > >>>--- a/hw/pci_host.c > >>>+++ b/hw/pci_host.c > >>>@@ -165,4 +165,15 @@ const MemoryRegionOps pci_host_data_be_ops =3D = { > >>> .endianness =3D DEVICE_BIG_ENDIAN, > >>> }; > >>> > >>>+static const TypeInfo pci_host_type_info =3D { > >>>+ .name =3D TYPE_PCI_HOST, > >>>+ .parent =3D TYPE_SYS_BUS_DEVICE, > >>>+ .instance_size =3D sizeof(PCIHostState), > >>>+}; > > > >It would in fact be better to set .abstract =3D true, I guess. > > > >Anyway, I think for sPAPR I translated PHB to PCI_HOST_BRIDGE already, > >so TYPE_PCI_HOST_BRIDGE "pci-host-bridge" and PCI_HOST_BRIDGE() would > >fit in nicely with the otherwise clear and verbose QOM naming. But I'd > >rather not rename PCIHostState everywhere... do you agree? Or would yo= u > >want to have it as PCIHostBridge or PCIHostBridgeState for consistency= ? > > > >If we use TYPE_PCI_HOST_BRIDGE I should also add _BRIDGE for some of t= he > >derived types, but we can't change the user-visible "-pcihost" type na= me > >there for backwards compatibility. > > > >Any further wishes? Should the second patch be split up in some way? > > > >Andreas > > > >>>+ > >>>+static void pci_host_register_types(void) > >>>+{ > >>>+ type_register_static(&pci_host_type_info); > >>>+} > >>> > >>>+type_init(pci_host_register_types) > >[snip] > >