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.
