Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges

[Michael S. Tsirkin]

On Thu, Jul 05, 2012 at 05:00:00PM +0200, Andreas Färber wrote:
> (Dropping some borked CCs)
> 
> 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ärber wrote:
> >>>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
> >>>>> On Thu, Jul 05, 2012 at 12:17:17AM +0300, Michael S. Tsirkin wrote:
> >>>>>> On Wed, Jul 04, 2012 at 07:19:33PM +0200, Andreas Färber wrote:
> >>>>>>> Uglify the parent field to enforce QOM-style access via casts.
> >>>>>>> Don't just typedef PCIHostState, either use it directly or embed it.
> >>>>>>>
> >>>>>>> Signed-off-by: Andreas Färber<address@hidden>
> >>>>>>> ---
> >>>>>>>  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_BRIDGE)
> >>>>>>>
> >>>>>>>  typedef struct TyphoonState {
> >>>>>>> -    PCIHostState host;
> >>>>>>> +    PCIHostState parent_obj;
> >>>>>>>
> >>>>>>>      TyphoonCchip cchip;
> >>>>>>>      TyphoonPchip pchip;
> >>>>>>> @@ -770,7 +770,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus 
> >>>>>>> **isa_bus,
> >>>>>>>      b = pci_register_bus(dev, "pci",
> >>>>>>>                           typhoon_set_irq, sys_map_irq, s,
> >>>>>>>                           &s->pchip.reg_mem, addr_space_io, 0, 64);
> >>>>>>> -    s->host.bus = b;
> >>>>>>> +    PCI_HOST_BRIDGE(s)->bus = b;
> >>>>>>>
> >>>>>>>      /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 
> >>>>>>> 64MB.  */
> >>>>>>>      memory_region_init_io(&s->pchip.reg_iack,&alpha_pci_iack_ops, 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 proof.
> >>> 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 parent
> >>>> was (had to be) identified by name and could be anywhere in the struct;
> >>>> 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 = (Foo *)bar;
> >>
> >> Is very different than:
> >>
> >> Foo *foo = &bar.foo;
> >>
> >> Using cast macros makes the code an awful lot more readable because
> >> it tells the reader more.
> >>
> >> Foo *foo = FOO(bar);
> >>
> >> Tells the user that we're casting bar to the a type Foo.  OTOH:
> >>
> >> Foo *foo = &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 = &bar.host.qdev;
> >>
> >> Is unintelligible whereas:
> >>
> >> DeviceState *dev = 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.
> > 
> > It's important but not the most important thing.
> > It does not make sense to do casts *everywhere*.
> > Do it where it makes sense.
> > 
> >> 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
> > 
> > So far QOM was a win.
> > You examples look better with a macro. Patch 13 looks better:
> > -    PCIHostState *phb = FROM_SYSBUS(PCIHostState, 
> > SYS_BUS_DEVICE(s->pcihost));
> > +    PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
> > this is an improvement: devices do not want to deal with sysbus.
> > 
> > The code above that is being changed looks better without.
> 
> Do you want me to apologize for my macro misuse now? I apologize. :)
> Replace PCI_HOST_BRIDGE(s)->bus = b; with
> PCI_HOST_BRIDGE *phb = PCI_HOST_BRIDGE(s);
> phb->bus = 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.
> 
> 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 access
> 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).
> 
> 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.

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>