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

[Jan Kiszka]

On 2012-03-26 21:35, Anthony Liguori wrote:
> 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.

I do not disagree. A pointer is harmless.

> 
>> 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?

You rush changing the current layout, not me. :)

Jan

signature.asc
Description: OpenPGP digital signature