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.
