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

[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?


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


> The point is, this is an object, and in realize you shouldn't abort but
> set errp and leave error printing and handling to your caller. The QOM
> API as opposed to qdev works with an Error object that you can
> error_propagate() to your caller.
> 
> (Also using qdev_* for something that is new-style QOM is ugly IMO.)
> 
>>> Where does icp->nr_servers come from?
>>
>> Via properties in try_create_xics() (hw/ppc/spapr.c).
> 
> Sounds tricky... Peter introduced static array properties for a similar
> purpose, I believe. Don't know if that would help here.
> 
>>
>>> 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?


-- 
Alexey