Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class

[Nikunj A Dadhania]

Cédric Le Goater <address@hidden> writes:

> On 09/20/2016 08:29 AM, Nikunj A Dadhania wrote:
>> Cédric Le Goater <address@hidden> writes:
>> 
>>> On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote:
>>>> Cédric Le Goater <address@hidden> writes:
>>>>
>>>>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
>>>>>> From: Benjamin Herrenschmidt <address@hidden>
>>>>>>
>>>>>> The existing implementation remains same and ics-base is introduced. The
>>>>>> type name "ics" is retained, and all the related functions renamed as
>>>>>> ics_simple_*
>>>>>>
>>>>>> This will allow different implementations for the source controllers
>>>>>> such as the MSI support of PHB3 on Power8 which uses in-memory state
>>>>>> tables for example.
>>>>>
>>>>> A couple of issues below regarding the class helpers,
>>>>>
>>>>>> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
>>>>>> Signed-off-by: Nikunj A Dadhania <address@hidden>
>>>>>> ---
>>>>>>  hw/intc/trace-events  |  10 ++--
>>>>>>  hw/intc/xics.c        | 143 
>>>>>> +++++++++++++++++++++++++++++++-------------------
>>>>>>  hw/intc/xics_kvm.c    |  10 ++--
>>>>>>  hw/intc/xics_spapr.c  |  28 +++++-----
>>>>>>  include/hw/ppc/xics.h |  23 +++++---
>>>>>>  5 files changed, 131 insertions(+), 83 deletions(-)
>
>>>>>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
>>>>>>  #define XISR(ss)   (((ss)->xirr) & XISR_MASK)
>>>>>>  #define CPPR(ss)   (((ss)->xirr) >> 24)
>>>>>>  
>>>>>> -static void ics_reject(ICSState *ics, int nr);
>>>>>> -static void ics_resend(ICSState *ics, int server);
>>>>>> -static void ics_eoi(ICSState *ics, int nr);
>>>>>> +static void ics_reject(ICSState *ics, uint32_t nr)
>>>>>> +{
>>>>>> +    ICSStateClass *k = ICS_GET_CLASS(ics);
>>>>>
>>>>> Shouldn't that be ICS_BASE_GET_CLASS()
>>>>
>>>> The class hierarchy is something like this:
>>>>
>>>> ICS_BASE -> ICS_SIMPLE -> ICS_KVM
>>>
>>> yes. but if we called  ics_* with an instance of an ics class which is 
>>> not a ICS_SIMPLE class that will break.
>> 
>> Correct
>> 
>>> ICSStateClass is the baseclass, whenever we call methods on a ICSState* 
>>> object, we should use the method defines in ICSStateClass.
>> 
>> Hmm, in that case I need to initialize base class methods in
>> instance_init of ics_simple
>
> yes but this is done, no ? I see : 
>
> static void ics_simple_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     ICSStateClass *isc = ICS_BASE_CLASS(klass);

Right.

Currently, we have this:

+    ICSStateClass *isc = ICS_CLASS(klass);

>
>     dc->realize = ics_simple_realize;
>     dc->vmsd = &vmstate_ics_simple;
>     dc->reset = ics_simple_reset;
>     isc->post_load = ics_simple_post_load;
>     isc->reject = ics_simple_reject;
>     isc->resend = ics_simple_resend;
>     isc->eoi = ics_simple_eoi;

Regards
Nikunj