Re: [Qemu-devel] [PATCH 0/6] refactor PC machine, i440fx and piix3 to take advantage of QOM

[Anthony Liguori]

On 03/26/2012 02:30 PM, Jan Kiszka wrote:
> On 2012-03-26 19:33, Anthony Liguori wrote:
> > On 03/26/2012 07:20 AM, Jan Kiszka wrote:
> > > On 2012-03-26 04:06, Wanpeng Li wrote:
> > > > From: Anthony Liguori<address@hidden>
> > > >
> > > >
> > > > This series aggressively refactors the PC machine initialization to be more
> > > > modelled and less ad-hoc.  The highlights of this series are:
> > > >
> > > >    1) Things like -m and -bios-name are now device model properties
> > > >
> > > >    2) The i440fx and piix3 are now modelled in a thorough fashion
> > > >
> > > >    3) Most of the chipset features of the piix3 are modelled through composition
> > > >
> > > >    4) i440fx_init is trivialized to creating devices and setting properties
> > > >
> > > >    5) convert MemoryRegion to QOM
> > > >
> > > >    6) convert PCI host bridge to QOM
> > > >
> > > > The point (4) is the most important one.  As we refactor in this fashion,
> > > > we should quickly get to the point where machine->init disappears completely in
> > > > favor of just creating a handful of devices.
> > > >
> > > > The two stage initialization of QOM is important here.  instance_init() is when
> > > > composed devices are created which means that after you've created a device, all
> > > > of its children are visible in the device model.  This lets you set properties
> > > > of the parent and its children.
> > > >
> > > > realize() (which is still called DeviceState::init today) will be called right
> > > > before the guest starts up for the first time.
> > >
> > > While I see the value of the overall direction, I still disagree on
> > > making internal data structures of HPET, RTC and 8254 publicly
> > > available. That's a wrong step back. I'm sure there are smarter
> > > solutions, alse as there were some proposals back then in the original
> > > thread.
> >
> > I'm not fully decided myself.  A couple things are clear to me though:
> >
> > 1) We must expose type proper types in header files.  We need there to be a
> > globally accessible RTCState type and functions that operate on it.
>
> I'm not sure what "proper type" means in this context, but I'm quite
> sure that there should be no need for poking into the internal of the
> class outside of mc146818rtc.c.

It needs to be at least a forward reference.  So we can avoid stuff like:

int apic_accept_pic_intr(DeviceState *s);

It should be:

int apic_accept_pic_intr(APICState *s);

So we can make use of the lovely type checking provided by the compiler to us.

> We even abstracted the specifics of the
> RTC away when it is embedded into a super-IO and interacts with an HPET.
> If QOM requires such poking, then that requirement should be reassessed.

There are a couple of ways to make types private while still having forward 
declarations.  None of them are straight forward.  That's why I suggest we save 
this for another day.

> > 2) We can simplify memory management by knowing the size of the type in the
> > header files too.
>
> Is this more than a malloc-free pair?
>
> > Since this is an easily refactorable thing to look at later, I think we should
> > start with extracting the types.
>
> My worry is that those three refactorings set bad examples for others.
> So I'd like to avoid such back and forth if possible.

I'm not really worried about it.  It's so easier to refactor this later.  Why 
rush it now?

Regards,

Anthony Liguori

> Jan