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

[Andreas Färber]

Am 06.08.2013 14:06, schrieb Alexey Kardashevskiy:
> On 08/06/2013 08:12 PM, Andreas Färber wrote:
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index 1066f69..e5889e9 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -35,6 +35,9 @@
>>>  #define TYPE_XICS "xics"
>>>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>>>  
>>> +#define TYPE_KVM_XICS "xics-kvm"
>>> +#define KVM_XICS(obj) OBJECT_CHECK(KVMXICSState, (obj), TYPE_KVM_XICS)
>>> +
>>>  #define XICS_COMMON_CLASS(klass) \
>>>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
>>>  #define XICS_CLASS(klass) \
>>> @@ -44,6 +47,9 @@
>>>  #define XICS_GET_CLASS(obj) \
>>>       OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
>>>  
>>> +#define XICS_GET_PARENT_CLASS(obj) \
>>> +    (XICSStateClass *) object_class_get_parent(object_get_class(obj))
>>
>> This is dangerous in that it takes the parent class of whatever type the
>> obj argument turns out to have. This has been discussed in lengths in
>> the context of Peter C.'s proposal for ISA/ARM realize cleanup.
> 
> How is it dangerous? I perfectly know what I call it with. And simple run
> will crash QEMU if something is wrong.

I did not say it was wrong or an error, I said it's dangerous. The
reason is that unlike C++ we do not have VTables in QOM:
KVM_XICS(obj) === XICS(obj) === obj.
I.e., object_get_class() always returns the actual, most specific type
rather than a type corresponding to the variable you are using it through.

So if someone derives TYPE_MY_KVM_XICS from your TYPE_KVM_XICS then the
function will silently do something different than it was doing before.
Same if someone inserts TYPE_VIRTUAL_XICS between TYPE_XICS and
TYPE_KVM_XICS.

The thought of both my macro (and Peter C.'s last version) and my
comments here is to keep the type information in a central place and to
make, say, a kvm_xics_realize C function behave like a KVMXICS::realize
C++ function would.

>> This should therefore be a macro per type specifying the TYPE_*, either
>> for object_class_by_name() or to my proposed macro which abstracts the
>> implementation to core QOM code.
> 
> Please, be more specific. What type should be used in macro here -
> XICS_COMMON or KVM_XICS? I asked already in the other thread but somehow
> you missed it.

Answered it now! ;)

> Neither really makes sense for me as I believe that
> get_parent is exactly for the cases (and this is the case) when I do not
> want to know exact parent type and I already know the current type. Thanks.

Correct. You do not know, you are making implicit assumptions that I
would like to avoid for safety reasons in favor of explicit
parent-of-my-type. That allows both to derive further types and to
insert intermediate types when using object_class_get_parent() the way I do.

>> XICS_CLASS() would be nicer than open-coding, but why cast here?
>> DeviceClass can be needed just as well.
> 
> I do not need DeviceClass, at all, anywhere. I need a pointer to a my
> specific cpu_setup callback, this is all about it.

You might not do so now - you might if there is common stuff to do in
DeviceClass::realize().

But that is something I suggest to discuss on my macro patch instead, as
it is a general design question for our parent-related macros. I also
suggest to search the archives for Peter's last two proposals that got
some discussion of corner cases and intentions as well.

Andreas

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