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

[Anthony Liguori <anthony@codemonkey.ws>]

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<aliguori@us.ibm.com>
>>>>
>>>>
>>>> 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