Re: [Qemu-ppc] [PATCH v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[Cédric Le Goater]

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.

>> 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.  


Thanks,

C.