Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller

[Alexey Kardashevskiy]

On 08/01/2013 01:07 PM, Andreas Färber wrote:
> Am 01.08.2013 04:08, schrieb Alexey Kardashevskiy:
>> On 08/01/2013 11:29 AM, Andreas Färber wrote:
>>> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy:
>>>> On 08/01/2013 05:52 AM, Andreas Färber wrote:
>>>>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>>>>>> +/*
>>>>>> + * XICS-KVM
>>>>>> + */
>>>>>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>>>>>> +{
>>>>>> +    CPUState *cs;
>>>>>> +    ICPState *ss;
>>>>>> +    XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
>>>>>> +            OBJECT(icp), TYPE_XICS_KVM);
>>>>>> +    XICSStateClass *xics_info = 
>>>>>> XICS_CLASS(object_class_by_name(TYPE_XICS));
>>>>>
>>>>> Are you intentionally accessing that class by name rather than using
>>>>> XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?
>>>>
>>>>
>>>> This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterwards,
>>>> i.e. "call parent method". XICS_GET_CLASS will return XICS_KVM class but
>>>> not XICS, no?
>>>
>>> OK, then I'll CC you on my upcoming virtio v2 series that introduces a
>>> more comprehensable macro for this purpose: I would/will recommend to
>>> use a local macro KVM_XICS_GET_PARENT_CLASS(obj) - where you could move
>>> your current inline implementation - to make more obvious that it's not
>>> a mistake.
>>
>> Oh. So. This has to wait till that virtio thing gets to upstream. Correct?
> 
> Not quite, there is no dependency on virtio.
> 
> a) You could do
> #define KVM_XICS_GET_PARENT_CLASS(obj) \
>     object_class_by_name(TYPE_KVM_XICS)

TYPE_KVM_XICS or TYPE_XICS?


> for now, where you do #define TYPE_KVM_XICS etc.
> Note that this is a proposal and not a rule. So far we agreed that
> adding classes with fields was not working well for variable depths of
> hierarchy and that open-coding the access was not ideal either. Nothing
> more, nothing less.
> 
> b) You could cherry-pick just http://patchwork.ozlabs.org/patch/263863/
> to update the implementation of a), but since that has not yet been
> acked I would advise against doing that immediately.
> But upstream is in freeze for the next two weeks anyway, so no need to rush.
> 
> c) You should participate in the review of Peter's and my proposals
> rather than silently inventing your own solution. :)
> Advantage of our approaches is hardcoding the current type rather than
> the parent's outside of TypeInfo.

I do not understand most of the stuff you do for QOM. I mean I know
C++/RTTI pretty good and even worse - I still remember M$ DCOM/OLE2 but I
do not know to what extent you want to implement that C++ in QEMU :)



>>>>>> +
>>>>>> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>>>>> +    for (i = 0; i < icp->nr_servers; i++) {
>>>>>> +        char buffer[32];
>>>>>> +        object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>>>>> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>>>>> +        object_property_add_child(OBJECT(icp), buffer, 
>>>>>> OBJECT(&icp->ss[i]), NULL);
>>>>>> +        qdev_init_nofail(DEVICE(&icp->ss[i]));
>>>>>
>>>>> object_property_set_bool()
>>>>
>>>>
>>>> ? Anthony did XICS refactoring recently and that has qdev_init_nofail().
>>>
>>> Nobody is perfect. ;)
>>
>> That's ok, my question is more about whether I should use set_bool here and
>> leave emulated XICS as is or you expect me to fix emulated XICS as well and
>> post an additional patch or what?
> 
> If that is so then yes, cleaning up your existing emulation in a patch
> before this one would be a good idea.
> 
>>>>> Is there no way to split this into
>>>>> instance_init and realize?
>>>>
>>>> Why would we want to split?
>>>
>>> Because realize is too late to create new devices: With our targetted
>>> late, recursive realization model it will not be possible to see and
>>> modify such objects from management interface - only before realize.
>>>
>>> I even have a patch on the list that would assert when that happens
>>> during final recursive realization.
>>
>>
>> So most this stuff has to go to instance_init and since there is no way to
>> prevent parent's instance_init from being called, you are basically forcing
>> me to introduce an abstract XICS class and inherit emulated XICS and KVM
>> XICS from it. Besides that, I do not any use of it. Is that correct?
> 
> Sorry, I don't follow. x86 and arm do use an abstract base class, e500
> doesn't iirc. But whether instance_init or realize, the parent's
> implementation will/should be called.

Openpic does not but its init() does not do much and openpic does not have
child devices but XICS does now (after Anthony's rework).

Unlike instance_init(), parent's realize() is not called automatically,
this was the main reason why I put everything into realize().


-- 
Alexey