[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Cédric Le Goater
Subject: Re: [Qemu-ppc] [PATCH v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models
Date: Wed, 20 Jun 2018 07:29:37 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2

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.  



reply via email to

[Prev in Thread] Current Thread [Next in Thread]