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<aligu...@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.
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
