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

[Cédric Le Goater]

On 06/25/2018 08:36 AM, David Gibson wrote:
> 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.

That is one way to do it yes, even if the common pattern used in other 
systems is more like the above. I don't have strong preferences.
 
We should see what is driving our needs. The most complex so far is to 
be able on P9 to switch from XICS to XIVE under KVM. 

> The reason is that having different object classes for devices based
> on accelerator is unusual and kind of dangerous. 

I agree.

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

Here is a summary of the KVM needs which applies to both XICS and XIVE,
when not activated both at the same time. 

The source and presenter objects need extra handlers to save/restore and 
synchronize KVM/QEMU states :

  - pre_save()
  - post_load()
  - synchronize_state()

The source object needs :

  - to create the KVM IRQ device (and surrounding support, VMAs for XIVE), 
  - to set up a different qemu_irq handler using the KVM device 

The presenter needs  

  - to attach the KVM vCPU to the KVM IRQ device 
  - a cpu hotplug/unplug tracking mechanism

C.