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

["Andreas Färber" <afaerber@suse.de>]

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.

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

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.

>>> +    }
>>> +    return;
>>> +
>>> +fail:
>>> +    kvmppc_define_rtas_token(0, "ibm,set-xive");
>>> +    kvmppc_define_rtas_token(0, "ibm,get-xive");
>>> +    kvmppc_define_rtas_token(0, "ibm,int-on");
>>> +    kvmppc_define_rtas_token(0, "ibm,int-off");
>>> +    return;
>>> +}
>>> +
>>> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>>> +    XICSStateClass *k = XICS_CLASS(oc);
>>
>> xsc?
> 
> 
> Whyyyy? Many places keep using "k".

Many places also still use "klass" and you took the new "oc". ;)

There's no "k" in XICSStateClass, and we adopted using the initial
letters of the name components, like dc for DeviceClass and oc for
ObjectClass above, just like we sort variables from abstract to
concrete. That way, when the type hierarchy gets changed you can more
naturally add variables.
It's not a requirement, but there's no real reason to call it "k"
either, since "c" unlike "class" is not a reserved word in C++. :)

Cheers,
Andreas

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