From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35951) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Smnty-0004PL-7V for qemu-devel@nongnu.org; Thu, 05 Jul 2012 11:23:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Smntr-0003yj-An for qemu-devel@nongnu.org; Thu, 05 Jul 2012 11:23:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5957) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Smntr-0003yN-2J for qemu-devel@nongnu.org; Thu, 05 Jul 2012 11:23:23 -0400 Date: Thu, 5 Jul 2012 18:23:37 +0300 From: "Michael S. Tsirkin" Message-ID: <20120705152337.GC31257@redhat.com> References: <1341422373-13614-1-git-send-email-afaerber@suse.de> <1341422373-13614-15-git-send-email-afaerber@suse.de> <20120704211717.GC27653@redhat.com> <20120704212614.GA27711@redhat.com> <4FF4C4EC.2090404@suse.de> <20120705133431.GF30572@redhat.com> <4FF59C8D.8040706@codemonkey.ws> <20120705141556.GA31257@redhat.com> <4FF5ABF0.9070004@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <4FF5ABF0.9070004@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: pbonzini@redhat.com, jbaron@redhat.com, qemu-devel@nongnu.org, Anthony Liguori , Alexander Graf On Thu, Jul 05, 2012 at 05:00:00PM +0200, Andreas F=E4rber wrote: > (Dropping some borked CCs) >=20 > Am 05.07.2012 16:15, schrieb Michael S. Tsirkin: > > On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote: > >> On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote: > >>> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas F=E4rber wrote: > >>>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin: > >>>>> On Thu, Jul 05, 2012 at 12:17:17AM +0300, Michael S. Tsirkin wrot= e: > >>>>>> On Wed, Jul 04, 2012 at 07:19:33PM +0200, Andreas F=E4rber wrote= : > >>>>>>> Uglify the parent field to enforce QOM-style access via casts. > >>>>>>> Don't just typedef PCIHostState, either use it directly or embe= d it. > >>>>>>> > >>>>>>> Signed-off-by: Andreas F=E4rber > >>>>>>> --- > >>>>>>> hw/alpha_typhoon.c | 4 ++-- > >>>>>>> hw/dec_pci.c | 2 +- > >>>>>>> hw/grackle_pci.c | 2 +- > >>>>>>> hw/gt64xxx.c | 26 ++++++++++++++++---------- > >>>>>>> hw/piix_pci.c | 6 ++++-- > >>>>>>> hw/ppc4xx_pci.c | 8 +++++--- > >>>>>>> hw/ppce500_pci.c | 2 +- > >>>>>>> hw/prep_pci.c | 8 +++++--- > >>>>>>> hw/spapr_pci.c | 12 +++++++----- > >>>>>>> hw/spapr_pci.h | 2 +- > >>>>>>> hw/unin_pci.c | 14 +++++++------- > >>>>>>> 11 files changed, 50 insertions(+), 36 deletions(-) > >>>>>>> > >>>>>>> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c > >>>>>>> index 58025a3..955d628 100644 > >>>>>>> --- a/hw/alpha_typhoon.c > >>>>>>> +++ b/hw/alpha_typhoon.c > >>>>>>> @@ -46,7 +46,7 @@ typedef struct TyphoonPchip { > >>>>>>> OBJECT_CHECK(TyphoonState, (obj), TYPE_TYPHOON_PCI_HOST_BR= IDGE) > >>>>>>> > >>>>>>> typedef struct TyphoonState { > >>>>>>> - PCIHostState host; > >>>>>>> + PCIHostState parent_obj; > >>>>>>> > >>>>>>> TyphoonCchip cchip; > >>>>>>> TyphoonPchip pchip; > >>>>>>> @@ -770,7 +770,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, I= SABus **isa_bus, > >>>>>>> b =3D pci_register_bus(dev, "pci", > >>>>>>> typhoon_set_irq, sys_map_irq, s, > >>>>>>> &s->pchip.reg_mem, addr_space_io, 0, = 64); > >>>>>>> - s->host.bus =3D b; > >>>>>>> + PCI_HOST_BRIDGE(s)->bus =3D b; > >>>>>>> > >>>>>>> /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.00= 00, 64MB. */ > >>>>>>> memory_region_init_io(&s->pchip.reg_iack,&alpha_pci_iack_o= ps, b, > >>>>>> > >>>>>> Sorry I don't understand. > >>>>>> why are we making code ugly apparently intentionally? > >>>>> > >>>>> Just to clarify: replacing upcasts which are always safe > >>>>> with downcasts which can fail is what I consider especially ugly. > >>>> > >>>> As per Anthony the parent field in the QOM instance structs is not > >>>> supposed to be touched (cf. object.h). We mark it /*< private>*/ = so > >>>> that it doesn't even show up in gtk-doc documentation. > >>> > >>> PCIHostState is not private here. And if it were, it won't be > >>> of any use because compiler does not read gtk-doc documentation > >>> and neither do developers. > >>> > >>>> If it is unused, > >>>> its name becomes irrelevant and could even be "reserved" if we so > >>>> wanted. Renaming it to whatever proves that all old references are= gone. > >>> > >>> Adding arbitrary rules like that only seems to make code future pro= of. > >>> People will not remember and will use the field anyway, > >>> then when you try to change it there will be work to be done. > >>> So let's do the work when we really need it. > >>> > >>>> Background is that qdev and QOM work differently with regards to > >>>> inheritance: as mentioned in the preceding patch, for qdev the par= ent > >>>> was (had to be) identified by name and could be anywhere in the st= ruct; > >>>> for QOM the parent is a subset of the struct from the start and it= 's > >>>> supposed to be accessed through the struct type that provides the > >>>> fields, the usual way to get such a pointer is through > >>>> OBJECT_CHECK()-derived cast macros. > >>> > >>> It makes sense if you go from parent to child. Even in C++ > >>> you don't use dynamic_cast to go from child to parent. > >> > >> But you use static cast. Casting is not the same thing as > >> dereferencing a member. IOW: > >> > >> Foo *foo =3D (Foo *)bar; > >> > >> Is very different than: > >> > >> Foo *foo =3D &bar.foo; > >> > >> Using cast macros makes the code an awful lot more readable because > >> it tells the reader more. > >> > >> Foo *foo =3D FOO(bar); > >> > >> Tells the user that we're casting bar to the a type Foo. OTOH: > >> > >> Foo *foo =3D &bar.foo; > >> > >> Doesn't tell the user anything. A FOO() macro is consistent > >> regardless of how the type is implemented too. It gets even worse > >> when you are going up multiple levels: > >> > >> DeviceState *dev =3D &bar.host.qdev; > >> > >> Is unintelligible whereas: > >> > >> DeviceState *dev =3D DEVICE(bar); > >> > >> Tells you exactly what's happening. > >> > >> But really, this isn't the place to debate this. QOM has been > >> around for a while. Consistency is important and this is how things > >> are done in QOM. > >=20 > > It's important but not the most important thing. > > It does not make sense to do casts *everywhere*. > > Do it where it makes sense. > >=20 > >> If you want to revisit this style, you should start a separate > >> thread about it and we can talk about it. But this patch is > >> consistent with the current infrastructure. > >> > >> Regards, > >> > >> Anthony Liguori > >=20 > > So far QOM was a win. > > You examples look better with a macro. Patch 13 looks better: > > - PCIHostState *phb =3D FROM_SYSBUS(PCIHostState, SYS_BUS_DEVICE(s= ->pcihost)); > > + PCIHostState *phb =3D PCI_HOST_BRIDGE(s->pcihost); > > this is an improvement: devices do not want to deal with sysbus. > >=20 > > The code above that is being changed looks better without. >=20 > Do you want me to apologize for my macro misuse now? I apologize. :) > Replace PCI_HOST_BRIDGE(s)->bus =3D b; with > PCI_HOST_BRIDGE *phb =3D PCI_HOST_BRIDGE(s); > phb->bus =3D b; > in your mind and it matches exactly what you liked better above, no? No, what I liked above is that we hide sysbus from devices. > > This is why this thread is about the specific patch and not > > general QOM. >=20 > You're basically saying, QOM was a win but don't use it here. That's a > contradiction and thus a general QOM issue since that paradigm not only > applies here: Either we need to design all structs so that their fields > have nice names to access directly as you suggest, or nobody must acces= s > the parent field as Anthony suggests. Having a wild mix of both at > maintainers' gusto is not a good solution. With time we'll end up with a mix anyway since compiler does not enforce no direct access. > Arguably we can just leave out this last patch if it is so > controversial. However, the split is arbitrary since I backwards-coded > this series, moving code snippets to earlier individual patches using > checkout -p, i.e. patches 1-11 have this final code design in mind and > thus went from SYS_BUS_DEVICE() to PCIHostState above rather than > through parent_obj. The reason I couldn't do it there directly is that > we didn't have the PCI_HOST_BRIDGE() macro before patch 13. > I also based patch 14 on the assumption that i440fx will get more state > fields when Anthony goes ahead with his QOM Pin series, otherwise we > could just scratch the I440FXState typedef (same discussion as for the > qtest herbivore test case). >=20 > Andreas The real problem is that devices have to tweak pcihost->bus and that is because we don't model has-a relationship between pci host bridge and bus well: pci host bridge has a pci bus. As long as that remains true it seems to me the more explicit it is the better, so it's easier to fix. > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg >=20