Re: [Qemu-devel] [RFC PATCH v1 0/7] QOM Super class access

[Michael S. Tsirkin]

On Tue, Jun 18, 2013 at 08:35:22PM +1000, Peter Crosthwaite wrote:
> Hi Michael,
> 
> On Tue, Jun 18, 2013 at 8:23 PM, Michael S. Tsirkin <address@hidden> wrote:
> > On Tue, Jun 18, 2013 at 07:43:11PM +1000, address@hidden wrote:
> >> From: Peter Crosthwaite <address@hidden>
> >>
> >>
> >> This series enables QOM super class access and demostrates some usages.
> >> Replaces the save->override->call via FooClass technique, to reduce
> >> some of the boiler plate in recently fully QOMified devices.
> >>
> >> Applied the change to ARM CPU, MB CPU and some of Andreas's recently
> >> QOMified i386 devices, all which have the save->override->call issue.
> >> ARMCPU I've done a brief test on and seems to work.
> >>
> >> ARM CPU was particularly difficult, as it has 3 layers of heirachy,
> >> where a non-concrete class (TYPE_ARM_CPU) need to super class itself
> >> (to TYPE_CPU). This sees the need for super-classers to specify their
> >> expected base class level. See patches for illustration.
> >>
> >> The main future work to the series is to apply the change pattern to
> >> the reset of the tree
> >
> > Looks good to me overall.
> > Some nits:
> > - Super is an immediate parent in java and python.
> 
> s/Super/parent might be the go. But it is designed to work like
> py/java. Its the immediate parent of the specified level, and it is
> analogous to the java super.
> 
> > - One of the design points of QOM is that it let
> >   you ignore which class is a parent and which is a child.
> >   All casts look the same.
> >
> > So, why do we need the new APIs with _SUPER?
> 
> The SUPER APIs have a nice consistent appearance with the GET_CLASS
> and FOO_CLASS APIs and they are likely to live alongside each other in
> QOM fns.
> 
> > What's wrong with simple
> >         object_class_by_name()
> > and casting to that?
> >
> 
> There a performance consequence - object_class_by_name is a lookup,
> whereas this approach is able to just walk the pointer through the
> inheritance heirachy till it hits.

None of these uses has a chance to be at all performance
sensitive.


> Tying it to an object also brings
> into play the possibility of a cast cache should that be needed.

Doesn't make sense to me. This is object construction,
how many classes do you expect there to be so
this would affect qemu startup speed noticeably?
1000000?

> Thanks for the review.
> 
> Regards,
> Peter
> 
> >
> >>
> >> Peter Crosthwaite (7):
> >>   target-arm/cpu.c: delete un-needed instance/class sizes
> >>   qom: Add super class accessor
> >>   qdev-core: Introduce DEVICE super class cast macros
> >>   qom/cpu: Introduce CPU super class cast macros
> >>   target-arm: Remove ARMCPUClass
> >>   target-microblaze: Remove MicroblazeCPUClass
> >>   i8254: Remove [KVM]PITClass
> >>
> >>  hw/i386/kvm/i8254.c         | 17 ++---------------
> >>  hw/timer/i8254.c            | 16 ++--------------
> >>  include/hw/qdev-core.h      |  4 ++++
> >>  include/qom/cpu.h           |  4 ++++
> >>  include/qom/object.h        | 18 ++++++++++++++++++
> >>  qom/object.c                | 15 +++++++++++++++
> >>  target-arm/cpu-qom.h        | 20 --------------------
> >>  target-arm/cpu.c            | 16 +++++-----------
> >>  target-microblaze/cpu-qom.h | 20 --------------------
> >>  target-microblaze/cpu.c     | 13 ++++---------
> >>  10 files changed, 54 insertions(+), 89 deletions(-)
> >>
> >> --
> >> 1.8.3.rc1.44.gb387c77.dirty
> >