| Message ID | 1375323463-32747-2-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On 08/01/2013 12:17 PM, Andreas Färber wrote: > The object argument is currently unused and may be used to optimize the > class lookup when needed. > > Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > include/qom/object.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/qom/object.h b/include/qom/object.h > index 23fc048..a8e71dc 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -511,6 +511,16 @@ struct TypeInfo > OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) > > /** > + * OBJECT_GET_PARENT_CLASS: > + * @obj: The object to obtain the parent class for. > + * @name: The QOM typename of @obj. > + * > + * Returns the parent class for a given object of a specific class. > + */ > +#define OBJECT_GET_PARENT_CLASS(obj, name) \ > + object_class_get_parent(object_class_by_name(name)) > + > +/** > * InterfaceInfo: > * @type: The name of the interface. > * > Has anyone ever tried to use this macro?
Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: > On 08/01/2013 12:17 PM, Andreas Färber wrote: >> The object argument is currently unused and may be used to optimize the >> class lookup when needed. >> >> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> include/qom/object.h | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/include/qom/object.h b/include/qom/object.h >> index 23fc048..a8e71dc 100644 >> --- a/include/qom/object.h >> +++ b/include/qom/object.h >> @@ -511,6 +511,16 @@ struct TypeInfo >> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >> >> /** >> + * OBJECT_GET_PARENT_CLASS: >> + * @obj: The object to obtain the parent class for. >> + * @name: The QOM typename of @obj. >> + * >> + * Returns the parent class for a given object of a specific class. >> + */ >> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >> + object_class_get_parent(object_class_by_name(name)) >> + >> +/** >> * InterfaceInfo: >> * @type: The name of the interface. >> * >> > > Has anyone ever tried to use this macro? Since you're asking me, obviously later in this virtio series it's used and in the IndustryPack series as well. I'm not aware of anyone else having used it yet - I'm still waiting for review feedback from Peter Cr. and/or Anthony (or you!) before I put it on qom-next. Andreas
On Thu, Aug 1, 2013 at 12:17 PM, Andreas Färber <afaerber@suse.de> wrote: > The object argument is currently unused and may be used to optimize the > class lookup when needed. > > Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Signed-off-by: Andreas Färber <afaerber@suse.de> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> One minor nit about comments below but im happy as is. Thanks. > --- > include/qom/object.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/qom/object.h b/include/qom/object.h > index 23fc048..a8e71dc 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -511,6 +511,16 @@ struct TypeInfo > OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) > > /** > + * OBJECT_GET_PARENT_CLASS: > + * @obj: The object to obtain the parent class for. > + * @name: The QOM typename of @obj. This is actually a little more complicated than this comment suggests. You are actually obtaining the parent class of @name for use by object@ obj. @name only needs to be an ancestor of @obj's concrete type and not necessarily the concrete class. Regards, Peter > + * > + * Returns the parent class for a given object of a specific class. > + */ > +#define OBJECT_GET_PARENT_CLASS(obj, name) \ > + object_class_get_parent(object_class_by_name(name)) > + > +/** > * InterfaceInfo: > * @type: The name of the interface. > * > -- > 1.8.1.4 > >
On 08/06/2013 06:33 PM, Andreas Färber wrote: > Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>> The object argument is currently unused and may be used to optimize the >>> class lookup when needed. >>> >>> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>> --- >>> include/qom/object.h | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/include/qom/object.h b/include/qom/object.h >>> index 23fc048..a8e71dc 100644 >>> --- a/include/qom/object.h >>> +++ b/include/qom/object.h >>> @@ -511,6 +511,16 @@ struct TypeInfo >>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>> >>> /** >>> + * OBJECT_GET_PARENT_CLASS: >>> + * @obj: The object to obtain the parent class for. >>> + * @name: The QOM typename of @obj. >>> + * >>> + * Returns the parent class for a given object of a specific class. >>> + */ >>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>> + object_class_get_parent(object_class_by_name(name)) >>> + >>> +/** >>> * InterfaceInfo: >>> * @type: The name of the interface. >>> * >>> >> >> Has anyone ever tried to use this macro? > > Since you're asking me, obviously later in this virtio series it's used > and in the IndustryPack series as well. > > I'm not aware of anyone else having used it yet - I'm still waiting for > review feedback from Peter Cr. and/or Anthony (or you!) before I put it > on qom-next. Still do not understand what "obj" is doing here. This what I would suggest: #define OBJECT_GET_PARENT_CLASS(obj, name) \ object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) @name here is just to make sure we are at the right level of the class hierarchy. And use it like this: static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) { XICSStateClass *xsc = XICS_COMMON_CLASS( OBJECT_GET_PARENT_CLASS(OBJECT(icp), TYPE_KVM_XICS)); ... Here both source and destination classes are checked, everyone must be happy :)
Hi, On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 08/06/2013 06:33 PM, Andreas Färber wrote: >> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>>> The object argument is currently unused and may be used to optimize the >>>> class lookup when needed. >>>> >>>> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>> --- >>>> include/qom/object.h | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/include/qom/object.h b/include/qom/object.h >>>> index 23fc048..a8e71dc 100644 >>>> --- a/include/qom/object.h >>>> +++ b/include/qom/object.h >>>> @@ -511,6 +511,16 @@ struct TypeInfo >>>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>>> >>>> /** >>>> + * OBJECT_GET_PARENT_CLASS: >>>> + * @obj: The object to obtain the parent class for. >>>> + * @name: The QOM typename of @obj. >>>> + * >>>> + * Returns the parent class for a given object of a specific class. >>>> + */ >>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>> + object_class_get_parent(object_class_by_name(name)) >>>> + >>>> +/** >>>> * InterfaceInfo: >>>> * @type: The name of the interface. >>>> * >>>> >>> >>> Has anyone ever tried to use this macro? >> >> Since you're asking me, obviously later in this virtio series it's used >> and in the IndustryPack series as well. >> >> I'm not aware of anyone else having used it yet - I'm still waiting for >> review feedback from Peter Cr. and/or Anthony (or you!) before I put it >> on qom-next. > > > Still do not understand what "obj" is doing here. The object is currently where cast cache optimizations are implemented. So having a handle to it is useful if ever these get-parent operations end up in fast paths and we need to do one of Anthony's caching tricks. > This what I would suggest: > > #define OBJECT_GET_PARENT_CLASS(obj, name) \ > object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) > You have changed the semantic from what Andreas has implemented. This will always return the parent of the concrete class, whereas to solve the save-override-call problem you need to get the parent of abstract level that is overriding the function (not always the concrete class). > @name here is just to make sure we are at the right level of the class > hierarchy. > Its @name that is actually important. Its more than just assertive, variation of @name for the same object will and should return different results (aside from just checking). The demonstrative case for this requirement is TYPE_ARM_CPU, which has a realize fn that needs to call the parent version as implemented by TYPE_CPU. ARM CPUs however have a whole bunch of concrete classes inheriting from TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only be able to get a handle to the parent of the concrete class (i.e. TYPE_ARM_CPU) and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as intended. Regards, Peter > And use it like this: > > static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > { > XICSStateClass *xsc = XICS_COMMON_CLASS( > OBJECT_GET_PARENT_CLASS(OBJECT(icp), TYPE_KVM_XICS)); > ... > > Here both source and destination classes are checked, everyone must be happy :) > > > -- > Alexey >
On 08/07/2013 02:20 PM, Peter Crosthwaite wrote: > Hi, > > On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> On 08/06/2013 06:33 PM, Andreas Färber wrote: >>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>>> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>>>> The object argument is currently unused and may be used to optimize the >>>>> class lookup when needed. >>>>> >>>>> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>> --- >>>>> include/qom/object.h | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>>> >>>>> diff --git a/include/qom/object.h b/include/qom/object.h >>>>> index 23fc048..a8e71dc 100644 >>>>> --- a/include/qom/object.h >>>>> +++ b/include/qom/object.h >>>>> @@ -511,6 +511,16 @@ struct TypeInfo >>>>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>>>> >>>>> /** >>>>> + * OBJECT_GET_PARENT_CLASS: >>>>> + * @obj: The object to obtain the parent class for. >>>>> + * @name: The QOM typename of @obj. >>>>> + * >>>>> + * Returns the parent class for a given object of a specific class. >>>>> + */ >>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>> + object_class_get_parent(object_class_by_name(name)) >>>>> + >>>>> +/** >>>>> * InterfaceInfo: >>>>> * @type: The name of the interface. >>>>> * >>>>> >>>> >>>> Has anyone ever tried to use this macro? >>> >>> Since you're asking me, obviously later in this virtio series it's used >>> and in the IndustryPack series as well. >>> >>> I'm not aware of anyone else having used it yet - I'm still waiting for >>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it >>> on qom-next. >> >> >> Still do not understand what "obj" is doing here. > > The object is currently where cast cache optimizations are > implemented. So having a handle to it is useful if ever these > get-parent operations end up in fast paths and we need to do one of > Anthony's caching tricks. > >> This what I would suggest: >> >> #define OBJECT_GET_PARENT_CLASS(obj, name) \ >> object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) >> > > You have changed the semantic from what Andreas has implemented. This > will always return the parent of the concrete class, whereas to solve > the save-override-call problem you need to get the parent of abstract > level that is overriding the function (not always the concrete class). > >> @name here is just to make sure we are at the right level of the class >> hierarchy. >> > > Its @name that is actually important. Its more than just assertive, > variation of @name for the same object will and should return > different results (aside from just checking). The demonstrative case > for this requirement is TYPE_ARM_CPU, which has a realize fn that > needs to call the parent version as implemented by TYPE_CPU. ARM CPUs > however have a whole bunch of concrete classes inheriting from > TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only > be able to get a handle to the parent of the concrete class (i.e. > TYPE_ARM_CPU) This is what I would really (really) expect in this situation. > and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as > intended. Oh. Then it is not get_parent() at all, this is get_grand_parent. realize() of TYPE_ARM_CPU_CORTEXTA9 (example) calls its parent (TYPE_ARM_CPU) realize() and that realize() calls its parent (TYPE_CPU) realize. It some level (for example, TYPE_ARM_CPU) does not want to implement realize(), then pointer to realize() from the upper class should be copied into TYPE_ARM_CPU's type info struct. class_init() callbacks are called for every class in the chain, so if some class_init() won't write to realize() pointer, it should be what the parent initialized it to, no? What do I miss here? > Regards, > Peter > >> And use it like this: >> >> static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) >> { >> XICSStateClass *xsc = XICS_COMMON_CLASS( >> OBJECT_GET_PARENT_CLASS(OBJECT(icp), TYPE_KVM_XICS)); >> ... >> >> Here both source and destination classes are checked, everyone must be happy :) >> >> >> -- >> Alexey >>
Am 07.08.2013 07:38, schrieb Alexey Kardashevskiy: > On 08/07/2013 02:20 PM, Peter Crosthwaite wrote: >> Hi, >> >> On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>> On 08/06/2013 06:33 PM, Andreas Färber wrote: >>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>>>> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>>>>> The object argument is currently unused and may be used to optimize the >>>>>> class lookup when needed. >>>>>> >>>>>> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>>> --- >>>>>> include/qom/object.h | 10 ++++++++++ >>>>>> 1 file changed, 10 insertions(+) >>>>>> >>>>>> diff --git a/include/qom/object.h b/include/qom/object.h >>>>>> index 23fc048..a8e71dc 100644 >>>>>> --- a/include/qom/object.h >>>>>> +++ b/include/qom/object.h >>>>>> @@ -511,6 +511,16 @@ struct TypeInfo >>>>>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>>>>> >>>>>> /** >>>>>> + * OBJECT_GET_PARENT_CLASS: >>>>>> + * @obj: The object to obtain the parent class for. >>>>>> + * @name: The QOM typename of @obj. >>>>>> + * >>>>>> + * Returns the parent class for a given object of a specific class. >>>>>> + */ >>>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>>> + object_class_get_parent(object_class_by_name(name)) >>>>>> + >>>>>> +/** >>>>>> * InterfaceInfo: >>>>>> * @type: The name of the interface. >>>>>> * >>>>>> >>>>> >>>>> Has anyone ever tried to use this macro? >>>> >>>> Since you're asking me, obviously later in this virtio series it's used >>>> and in the IndustryPack series as well. >>>> >>>> I'm not aware of anyone else having used it yet - I'm still waiting for >>>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it >>>> on qom-next. >>> >>> >>> Still do not understand what "obj" is doing here. >> >> The object is currently where cast cache optimizations are >> implemented. So having a handle to it is useful if ever these >> get-parent operations end up in fast paths and we need to do one of >> Anthony's caching tricks. >> >>> This what I would suggest: >>> >>> #define OBJECT_GET_PARENT_CLASS(obj, name) \ >>> object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) >>> >> >> You have changed the semantic from what Andreas has implemented. This >> will always return the parent of the concrete class, whereas to solve >> the save-override-call problem you need to get the parent of abstract >> level that is overriding the function (not always the concrete class). >> >>> @name here is just to make sure we are at the right level of the class >>> hierarchy. >>> >> >> Its @name that is actually important. Its more than just assertive, >> variation of @name for the same object will and should return >> different results (aside from just checking). The demonstrative case >> for this requirement is TYPE_ARM_CPU, which has a realize fn that >> needs to call the parent version as implemented by TYPE_CPU. ARM CPUs >> however have a whole bunch of concrete classes inheriting from >> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only >> be able to get a handle to the parent of the concrete class (i.e. >> TYPE_ARM_CPU) > > This is what I would really (really) expect in this situation. > >> and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as >> intended. > > Oh. Then it is not get_parent() at all, this is get_grand_parent. No. It is parent of TYPE_ARM_CPU, as specified by the name argument. obj serves for possible optimization, as demonstrated by an earlier patch by Peter that avoids the hashtable lookup by iterating over obj's parent classes. If we pass obj down through our macros then such changes can be done in one central place rather than reviewing and changing the whole code base. > realize() of TYPE_ARM_CPU_CORTEXTA9 (example) calls its parent > (TYPE_ARM_CPU) realize() and that realize() calls its parent (TYPE_CPU) > realize. > > It some level (for example, TYPE_ARM_CPU) does not want to implement > realize(), then pointer to realize() from the upper class should be copied > into TYPE_ARM_CPU's type info struct. class_init() callbacks are called for > every class in the chain, so if some class_init() won't write to realize() > pointer, it should be what the parent initialized it to, no? > > What do I miss here? You are missing that we do implement realize for TYPE_ARM_CPU, not for its derived types. So we want to call TYPE_CPU's realize from there, not some random realize determined by what type dev happens to have. Andreas
On 08/07/2013 03:45 PM, Andreas Färber wrote: > Am 07.08.2013 07:38, schrieb Alexey Kardashevskiy: >> On 08/07/2013 02:20 PM, Peter Crosthwaite wrote: >>> Hi, >>> >>> On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>>> On 08/06/2013 06:33 PM, Andreas Färber wrote: >>>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>>>>> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>>>>>> The object argument is currently unused and may be used to optimize the >>>>>>> class lookup when needed. >>>>>>> >>>>>>> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>>>> --- >>>>>>> include/qom/object.h | 10 ++++++++++ >>>>>>> 1 file changed, 10 insertions(+) >>>>>>> >>>>>>> diff --git a/include/qom/object.h b/include/qom/object.h >>>>>>> index 23fc048..a8e71dc 100644 >>>>>>> --- a/include/qom/object.h >>>>>>> +++ b/include/qom/object.h >>>>>>> @@ -511,6 +511,16 @@ struct TypeInfo >>>>>>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>>>>>> >>>>>>> /** >>>>>>> + * OBJECT_GET_PARENT_CLASS: >>>>>>> + * @obj: The object to obtain the parent class for. >>>>>>> + * @name: The QOM typename of @obj. >>>>>>> + * >>>>>>> + * Returns the parent class for a given object of a specific class. >>>>>>> + */ >>>>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>>>> + object_class_get_parent(object_class_by_name(name)) >>>>>>> + >>>>>>> +/** >>>>>>> * InterfaceInfo: >>>>>>> * @type: The name of the interface. >>>>>>> * >>>>>>> >>>>>> >>>>>> Has anyone ever tried to use this macro? >>>>> >>>>> Since you're asking me, obviously later in this virtio series it's used >>>>> and in the IndustryPack series as well. >>>>> >>>>> I'm not aware of anyone else having used it yet - I'm still waiting for >>>>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it >>>>> on qom-next. >>>> >>>> >>>> Still do not understand what "obj" is doing here. >>> >>> The object is currently where cast cache optimizations are >>> implemented. So having a handle to it is useful if ever these >>> get-parent operations end up in fast paths and we need to do one of >>> Anthony's caching tricks. >>> >>>> This what I would suggest: >>>> >>>> #define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>> object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) >>>> >>> >>> You have changed the semantic from what Andreas has implemented. This >>> will always return the parent of the concrete class, whereas to solve >>> the save-override-call problem you need to get the parent of abstract >>> level that is overriding the function (not always the concrete class). >>> >>>> @name here is just to make sure we are at the right level of the class >>>> hierarchy. >>>> >>> >>> Its @name that is actually important. Its more than just assertive, >>> variation of @name for the same object will and should return >>> different results (aside from just checking). The demonstrative case >>> for this requirement is TYPE_ARM_CPU, which has a realize fn that >>> needs to call the parent version as implemented by TYPE_CPU. ARM CPUs >>> however have a whole bunch of concrete classes inheriting from >>> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only >>> be able to get a handle to the parent of the concrete class (i.e. >>> TYPE_ARM_CPU) >> >> This is what I would really (really) expect in this situation. >> >>> and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as >>> intended. >> >> Oh. Then it is not get_parent() at all, this is get_grand_parent. > > No. It is parent of TYPE_ARM_CPU, as specified by the name argument. Here I lost you. You really want finalize() of TYPE_CPU, no matter how many classes are between TYPE_CPU and your final ARM CPU class. So call it directly, why do you need this workaround with get_parent if it is not really a parent of the current obj and you do not trust to what you get in finalize callback field of all intermediate classes (TYPE_CPU_ARM in this example)? Again, I am not arguing, I really that dump and do not understand :) > obj serves for possible optimization, as demonstrated by an earlier > patch by Peter that avoids the hashtable lookup by iterating over obj's > parent classes. If we pass obj down through our macros then such changes > can be done in one central place rather than reviewing and changing the > whole code base. >> realize() of TYPE_ARM_CPU_CORTEXTA9 (example) calls its parent >> (TYPE_ARM_CPU) realize() and that realize() calls its parent (TYPE_CPU) >> realize. >> >> It some level (for example, TYPE_ARM_CPU) does not want to implement >> realize(), then pointer to realize() from the upper class should be copied >> into TYPE_ARM_CPU's type info struct. class_init() callbacks are called for >> every class in the chain, so if some class_init() won't write to realize() >> pointer, it should be what the parent initialized it to, no? >> >> What do I miss here? > > You are missing that we do implement realize for TYPE_ARM_CPU, not for > its derived types. So we want to call TYPE_CPU's realize from there, not > some random realize determined by what type dev happens to have. How is it random? Inheritance is strict. It is CPU -> ARM_CPU -> A9_ARM_CPU so their finalize() should point to something reasonable or inherit from a parent.
Am 07.08.2013 05:36, schrieb Alexey Kardashevskiy: > This what I would suggest: > > #define OBJECT_GET_PARENT_CLASS(obj, name) \ > object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) > > @name here is just to make sure we are at the right level of the class > hierarchy. > > And use it like this: > > static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu) > { > XICSStateClass *xsc = XICS_COMMON_CLASS( > OBJECT_GET_PARENT_CLASS(OBJECT(icp), TYPE_KVM_XICS)); > ... > > Here both source and destination classes are checked, everyone must be happy :) In addition to what Peter has explained, I recommend against using my macro inline, just like we don't use OBJECT_CHECK() inline. Doing so keeps the uses of TYPE_KVM_XICS together and lets us exchange the implementation more easily should the need arise. Btw COMMON_XICS_CLASS() would be a more specific name. (Yes, ARM GIC is a counter-example.) Andreas
On Wed, Aug 7, 2013 at 3:53 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 08/07/2013 03:45 PM, Andreas Färber wrote: >> Am 07.08.2013 07:38, schrieb Alexey Kardashevskiy: >>> On 08/07/2013 02:20 PM, Peter Crosthwaite wrote: >>>> Hi, >>>> >>>> On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>>>> On 08/06/2013 06:33 PM, Andreas Färber wrote: >>>>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>>>>>> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>>>>>>> The object argument is currently unused and may be used to optimize the >>>>>>>> class lookup when needed. >>>>>>>> >>>>>>>> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>>>>> --- >>>>>>>> include/qom/object.h | 10 ++++++++++ >>>>>>>> 1 file changed, 10 insertions(+) >>>>>>>> >>>>>>>> diff --git a/include/qom/object.h b/include/qom/object.h >>>>>>>> index 23fc048..a8e71dc 100644 >>>>>>>> --- a/include/qom/object.h >>>>>>>> +++ b/include/qom/object.h >>>>>>>> @@ -511,6 +511,16 @@ struct TypeInfo >>>>>>>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>>>>>>> >>>>>>>> /** >>>>>>>> + * OBJECT_GET_PARENT_CLASS: >>>>>>>> + * @obj: The object to obtain the parent class for. >>>>>>>> + * @name: The QOM typename of @obj. >>>>>>>> + * >>>>>>>> + * Returns the parent class for a given object of a specific class. >>>>>>>> + */ >>>>>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>>>>> + object_class_get_parent(object_class_by_name(name)) >>>>>>>> + >>>>>>>> +/** >>>>>>>> * InterfaceInfo: >>>>>>>> * @type: The name of the interface. >>>>>>>> * >>>>>>>> >>>>>>> >>>>>>> Has anyone ever tried to use this macro? >>>>>> >>>>>> Since you're asking me, obviously later in this virtio series it's used >>>>>> and in the IndustryPack series as well. >>>>>> >>>>>> I'm not aware of anyone else having used it yet - I'm still waiting for >>>>>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it >>>>>> on qom-next. >>>>> >>>>> >>>>> Still do not understand what "obj" is doing here. >>>> >>>> The object is currently where cast cache optimizations are >>>> implemented. So having a handle to it is useful if ever these >>>> get-parent operations end up in fast paths and we need to do one of >>>> Anthony's caching tricks. >>>> >>>>> This what I would suggest: >>>>> >>>>> #define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>> object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) >>>>> >>>> >>>> You have changed the semantic from what Andreas has implemented. This >>>> will always return the parent of the concrete class, whereas to solve >>>> the save-override-call problem you need to get the parent of abstract >>>> level that is overriding the function (not always the concrete class). >>>> >>>>> @name here is just to make sure we are at the right level of the class >>>>> hierarchy. >>>>> >>>> >>>> Its @name that is actually important. Its more than just assertive, >>>> variation of @name for the same object will and should return >>>> different results (aside from just checking). The demonstrative case >>>> for this requirement is TYPE_ARM_CPU, which has a realize fn that >>>> needs to call the parent version as implemented by TYPE_CPU. ARM CPUs >>>> however have a whole bunch of concrete classes inheriting from >>>> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only >>>> be able to get a handle to the parent of the concrete class (i.e. >>>> TYPE_ARM_CPU) >>> >>> This is what I would really (really) expect in this situation. >>> >>>> and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as >>>> intended. >>> >>> Oh. Then it is not get_parent() at all, this is get_grand_parent. >> >> No. It is parent of TYPE_ARM_CPU, as specified by the name argument. > > Here I lost you. You really want finalize() of TYPE_CPU, no matter how many > classes are between TYPE_CPU and your final ARM CPU class. No, we want the immediate parent of the TYPE_ARM_CPU class which is not set in stone. Directly refing TYPE_CPU makes it difficult for anyone trying to re-organise the QOM heirachy. The idea behind this approach was that TYPE_ARM_CPU::realize does not make assumptions about the classes above it (except that someone in the ancestry is TYPE_DEVICE for the definition of realize) nor the classes below it (as already discussed). For example, if someone decides to implement TYPE_CPU_FOO (abstract child of TYPE_CPU) and reparents TYPE_ARM_CPU to this, then without need-for-change TYPE_ARM_CPU will now call TYPE_CPU_FOO:realize rather than inadvertently skipping over levels to a grandparent implementation. It can be done your proposed way, but re-organising the heirachy will potentially require change patterns that are avoided with this approach. So call it > directly, why do you need this workaround with get_parent if it is not > really a parent of the current obj and you do not trust to what you get in This is probably terminology confusion. Its a parent of a class not an object. The documentation and naming maybe needs work? Regards, Peter
Am 07.08.2013 07:53, schrieb Alexey Kardashevskiy: > On 08/07/2013 03:45 PM, Andreas Färber wrote: >> Am 07.08.2013 07:38, schrieb Alexey Kardashevskiy: >>> On 08/07/2013 02:20 PM, Peter Crosthwaite wrote: >>>> Hi, >>>> >>>> On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>>>> On 08/06/2013 06:33 PM, Andreas Färber wrote: >>>>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>>>>>> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>>>>>>> The object argument is currently unused and may be used to optimize the >>>>>>>> class lookup when needed. >>>>>>>> >>>>>>>> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>>>>> --- >>>>>>>> include/qom/object.h | 10 ++++++++++ >>>>>>>> 1 file changed, 10 insertions(+) >>>>>>>> >>>>>>>> diff --git a/include/qom/object.h b/include/qom/object.h >>>>>>>> index 23fc048..a8e71dc 100644 >>>>>>>> --- a/include/qom/object.h >>>>>>>> +++ b/include/qom/object.h >>>>>>>> @@ -511,6 +511,16 @@ struct TypeInfo >>>>>>>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>>>>>>> >>>>>>>> /** >>>>>>>> + * OBJECT_GET_PARENT_CLASS: >>>>>>>> + * @obj: The object to obtain the parent class for. >>>>>>>> + * @name: The QOM typename of @obj. >>>>>>>> + * >>>>>>>> + * Returns the parent class for a given object of a specific class. >>>>>>>> + */ >>>>>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>>>>> + object_class_get_parent(object_class_by_name(name)) >>>>>>>> + >>>>>>>> +/** >>>>>>>> * InterfaceInfo: >>>>>>>> * @type: The name of the interface. >>>>>>>> * >>>>>>>> >>>>>>> >>>>>>> Has anyone ever tried to use this macro? >>>>>> >>>>>> Since you're asking me, obviously later in this virtio series it's used >>>>>> and in the IndustryPack series as well. >>>>>> >>>>>> I'm not aware of anyone else having used it yet - I'm still waiting for >>>>>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it >>>>>> on qom-next. >>>>> >>>>> >>>>> Still do not understand what "obj" is doing here. >>>> >>>> The object is currently where cast cache optimizations are >>>> implemented. So having a handle to it is useful if ever these >>>> get-parent operations end up in fast paths and we need to do one of >>>> Anthony's caching tricks. >>>> >>>>> This what I would suggest: >>>>> >>>>> #define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>> object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) >>>>> >>>> >>>> You have changed the semantic from what Andreas has implemented. This >>>> will always return the parent of the concrete class, whereas to solve >>>> the save-override-call problem you need to get the parent of abstract >>>> level that is overriding the function (not always the concrete class). >>>> >>>>> @name here is just to make sure we are at the right level of the class >>>>> hierarchy. >>>>> >>>> >>>> Its @name that is actually important. Its more than just assertive, >>>> variation of @name for the same object will and should return >>>> different results (aside from just checking). The demonstrative case >>>> for this requirement is TYPE_ARM_CPU, which has a realize fn that >>>> needs to call the parent version as implemented by TYPE_CPU. ARM CPUs >>>> however have a whole bunch of concrete classes inheriting from >>>> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only >>>> be able to get a handle to the parent of the concrete class (i.e. >>>> TYPE_ARM_CPU) >>> >>> This is what I would really (really) expect in this situation. >>> >>>> and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as >>>> intended. >>> >>> Oh. Then it is not get_parent() at all, this is get_grand_parent. >> >> No. It is parent of TYPE_ARM_CPU, as specified by the name argument. > > Here I lost you. You really want finalize() of TYPE_CPU, no matter how many > classes are between TYPE_CPU and your final ARM CPU class. Correct. > So call it > directly, why do you need this workaround with get_parent if it is not > really a parent of the current obj and you do not trust to what you get in > finalize callback field of all intermediate classes (TYPE_CPU_ARM in this > example)? > > Again, I am not arguing, I really that dump and do not understand :) This macro represents Java's "super" or C#'s "base" (which refer to the method's parent type, not the object pointer's) rather than C++'s SomeType:: (which refers to some concrete type down the multi-inheritence chain and is compile-checked in C++ but is independent of whether it's an immediate or indirect parent type). I.e., class ArmCpu { protected override void realize(DeviceState dev, ref Error err) { base.realize(dev, err); ... } } rather than void ARMCPU::realize(DeviceState *dev, Error &err) { CPU::realize(dev, err); // what if we get a 32bitCPU parent? ... } >>> realize() of TYPE_ARM_CPU_CORTEXTA9 (example) calls its parent >>> (TYPE_ARM_CPU) realize() and that realize() calls its parent (TYPE_CPU) >>> realize. >>> >>> It some level (for example, TYPE_ARM_CPU) does not want to implement >>> realize(), then pointer to realize() from the upper class should be copied >>> into TYPE_ARM_CPU's type info struct. class_init() callbacks are called for >>> every class in the chain, so if some class_init() won't write to realize() >>> pointer, it should be what the parent initialized it to, no? >>> >>> What do I miss here? >> >> You are missing that we do implement realize for TYPE_ARM_CPU, not for >> its derived types. So we want to call TYPE_CPU's realize from there, not >> some random realize determined by what type dev happens to have. > > How is it random? Inheritance is strict. It is CPU -> ARM_CPU -> A9_ARM_CPU > so their finalize() should point to something reasonable or inherit from a > parent. Inheritence can change by modifying .parent in TypeInfo. I have done so several times for 1.6 to allow proper FOO() casts for types sharing a state struct and you have done it for common XICS, too, so it is not just theory. If that happens, not relying on the parent type's constant across code is beneficial; as long as the type a function is for does not change (i.e., not xics_realize -> common_xics_realize) then the macro can be reused unchanged. If you hardcode TYPE_* inline, then someone needs to go through and check all that code, making review and changes harder. In case you were not aware, read the documentation Peter Ch. just fixed on overriding QOM methods in include/qom/object.h to see how Anthony originally asked us to do things. This macro (and its predecessors) try to simplify that process by avoiding adding *Class classes per type GObject-style, which does not scale well for leaf types (such as cortex-a9-arm-cpu). If Anthony does not object to this approach, then we should update the documentation and I should pick up and queue Peter Cr.'s ISA and ARM patches. Andreas
On 08/07/2013 04:10 PM, Peter Crosthwaite wrote: > On Wed, Aug 7, 2013 at 3:53 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> On 08/07/2013 03:45 PM, Andreas Färber wrote: >>> Am 07.08.2013 07:38, schrieb Alexey Kardashevskiy: >>>> On 08/07/2013 02:20 PM, Peter Crosthwaite wrote: >>>>> Hi, >>>>> >>>>> On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>>>>> On 08/06/2013 06:33 PM, Andreas Färber wrote: >>>>>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>>>>>>> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>>>>>>>> The object argument is currently unused and may be used to optimize the >>>>>>>>> class lookup when needed. >>>>>>>>> >>>>>>>>> Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de> >>>>>>>>> --- >>>>>>>>> include/qom/object.h | 10 ++++++++++ >>>>>>>>> 1 file changed, 10 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/include/qom/object.h b/include/qom/object.h >>>>>>>>> index 23fc048..a8e71dc 100644 >>>>>>>>> --- a/include/qom/object.h >>>>>>>>> +++ b/include/qom/object.h >>>>>>>>> @@ -511,6 +511,16 @@ struct TypeInfo >>>>>>>>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>>>>>>>> >>>>>>>>> /** >>>>>>>>> + * OBJECT_GET_PARENT_CLASS: >>>>>>>>> + * @obj: The object to obtain the parent class for. >>>>>>>>> + * @name: The QOM typename of @obj. >>>>>>>>> + * >>>>>>>>> + * Returns the parent class for a given object of a specific class. >>>>>>>>> + */ >>>>>>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>>>>>> + object_class_get_parent(object_class_by_name(name)) >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> * InterfaceInfo: >>>>>>>>> * @type: The name of the interface. >>>>>>>>> * >>>>>>>>> >>>>>>>> >>>>>>>> Has anyone ever tried to use this macro? >>>>>>> >>>>>>> Since you're asking me, obviously later in this virtio series it's used >>>>>>> and in the IndustryPack series as well. >>>>>>> >>>>>>> I'm not aware of anyone else having used it yet - I'm still waiting for >>>>>>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it >>>>>>> on qom-next. >>>>>> >>>>>> >>>>>> Still do not understand what "obj" is doing here. >>>>> >>>>> The object is currently where cast cache optimizations are >>>>> implemented. So having a handle to it is useful if ever these >>>>> get-parent operations end up in fast paths and we need to do one of >>>>> Anthony's caching tricks. >>>>> >>>>>> This what I would suggest: >>>>>> >>>>>> #define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>>> object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) >>>>>> >>>>> >>>>> You have changed the semantic from what Andreas has implemented. This >>>>> will always return the parent of the concrete class, whereas to solve >>>>> the save-override-call problem you need to get the parent of abstract >>>>> level that is overriding the function (not always the concrete class). >>>>> >>>>>> @name here is just to make sure we are at the right level of the class >>>>>> hierarchy. >>>>>> >>>>> >>>>> Its @name that is actually important. Its more than just assertive, >>>>> variation of @name for the same object will and should return >>>>> different results (aside from just checking). The demonstrative case >>>>> for this requirement is TYPE_ARM_CPU, which has a realize fn that >>>>> needs to call the parent version as implemented by TYPE_CPU. ARM CPUs >>>>> however have a whole bunch of concrete classes inheriting from >>>>> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only >>>>> be able to get a handle to the parent of the concrete class (i.e. >>>>> TYPE_ARM_CPU) >>>> >>>> This is what I would really (really) expect in this situation. >>>> >>>>> and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as >>>>> intended. >>>> >>>> Oh. Then it is not get_parent() at all, this is get_grand_parent. >>> >>> No. It is parent of TYPE_ARM_CPU, as specified by the name argument. >> >> Here I lost you. You really want finalize() of TYPE_CPU, no matter how many >> classes are between TYPE_CPU and your final ARM CPU class. > > No, we want the immediate parent of the TYPE_ARM_CPU class which is > not set in stone. > > Directly refing TYPE_CPU makes it difficult for > anyone trying to re-organise the QOM heirachy. The idea behind this > approach was that TYPE_ARM_CPU::realize does not make assumptions > about the classes above it (except that someone in the ancestry is > TYPE_DEVICE for the definition of realize) nor the classes below it > (as already discussed). > > For example, if someone decides to implement TYPE_CPU_FOO (abstract > child of TYPE_CPU) and reparents TYPE_ARM_CPU to this, then without > need-for-change TYPE_ARM_CPU will now call TYPE_CPU_FOO:realize rather > than inadvertently skipping over levels to a grandparent > implementation. It can be done your proposed way, but re-organising > the heirachy will potentially require change patterns that are avoided > with this approach. > > So call it >> directly, why do you need this workaround with get_parent if it is not >> really a parent of the current obj and you do not trust to what you get in > > This is probably terminology confusion. Its a parent of a class not an > object. The documentation and naming maybe needs work? Oh. Right. It is confusion. I get it now. For some reason I decided that a specific ARM CPU has its own realize() but it is defined in TYPE_CPU_ARM. Sorry.
diff --git a/include/qom/object.h b/include/qom/object.h index 23fc048..a8e71dc 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -511,6 +511,16 @@ struct TypeInfo OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) /** + * OBJECT_GET_PARENT_CLASS: + * @obj: The object to obtain the parent class for. + * @name: The QOM typename of @obj. + * + * Returns the parent class for a given object of a specific class. + */ +#define OBJECT_GET_PARENT_CLASS(obj, name) \ + object_class_get_parent(object_class_by_name(name)) + +/** * InterfaceInfo: * @type: The name of the interface. *
The object argument is currently unused and may be used to optimize the class lookup when needed. Inspired-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Signed-off-by: Andreas Färber <afaerber@suse.de> --- include/qom/object.h | 10 ++++++++++ 1 file changed, 10 insertions(+)