On Wed, Jun 20, 2018 at 07:29:37AM +0200, Cédric Le Goater wrote:
> On 06/20/2018 02:56 AM, David Gibson wrote:
> > On Tue, Jun 19, 2018 at 07:24:44AM +0200, Cédric Le Goater wrote:
> >>
> >>>>>  typedef struct PnvChipClass {
> >>>>>      /*< private >*/
> >>>>> @@ -75,6 +95,7 @@ typedef struct PnvChipClass {
> >>>>>  
> >>>>>      hwaddr       xscom_base;
> >>>>>  
> >>>>> +    void (*realize)(PnvChip *chip, Error **errp);
> >>>>
> >>>> This looks the wrong way round from how things are usually done.
> >>>> Rather than having the base class realize() call the subclass specific
> >>>> realize hook, it's more usual for the subclass to set the
> >>>> dc->realize() and have it call a k->parent_realize() to call up the
> >>>> chain.  grep for device_class_set_parent_realize() for some more
> >>>> examples.
> >>>
> >>> Ah. That is more to my liking. There are a couple of models following
> >>> the wrong object pattern, xics, vio. I will check them.
> >>
> >> So XICS is causing some head-aches because the ics-kvm model inherits
> >> from ics-simple which inherits from ics-base. so we have a grand-parent
> >> class to handle.
> > 
> > Ok.  I mean, we should probably switch ics around to use the
> > parent_realize model, rather than the backwards one it does now.  But
> > it's not immediately obvious to me why having a grandparent class
> > breaks things.
> 
> If you follow the common realize pattern, you end up with a recursive 
> loop with one of the realize routine. I didn't dig much the issue.

Hmm.

> >> if we could affiliate ics-kvm directly to ics-base it would make the 
> >> family affair easier. we need to check migration though.
> > 
> > But that said, I've been thinking for a while that it might make sense
> > to fold ics-kvm into ics-base.  It seems very risky to have two
> > different object classes that are supposed to have guest-identical
> > behaviour.  Certainly adding any migratable state to one not the other
> > would be horribly wrong.
> 
> yes. clearly. something like bellow would be better:
> 
>                    +---------------+
>                    |     ICS       |
>          +---------+  common/base  +--------+
>          |         +---------------+        |
>          |                                  |
>          | spapr                    spapr   |
>          |  pnv                             |
>   +------v--------+                +--------v--------+
>   |     ICS       |                |       ICS       |
>   |  simple/QEMU  |                |       KVM       |
>   +---------------+                +-----------------+
> 
> with only some reset and realize handling in the subclasses. The only
> extra field we could add under the KVM class is the KVM XICS device fd.  

I think that would be an improvement over what we have, yes.  However,
it's not what I actually had in mind.  In fact I was planning on
getting rid of the KVM specific subclass entirely and merging it into
the base/simple classes with explicit if (kvm) logic.

The reason is that having different object classes for devices based
on accelerator is unusual and kind of dangerous.  We get away with it
because we don't have any migration information that gets tied to the
object class name - but that's a pretty non-obvious restriction that
would be easy to break in future.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson