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

[Andreas Färber]

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)
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.

>>>>> +
>>>>> +    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.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg