On Tue, Mar 10, 2015 at 02:30:34PM +0100, Andreas Färber wrote: > Am 10.03.2015 um 14:24 schrieb Eduardo Habkost: > > On Tue, Mar 10, 2015 at 01:51:26PM +0100, Andreas Färber wrote: > >> Am 10.03.2015 um 13:42 schrieb Eduardo Habkost: > >>> On Tue, Mar 10, 2015 at 12:50:07PM +0100, Andreas Färber wrote: > >>>> Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: > >>>>> Instead of passing icc_bridge from the PC initialization code to > >>>>> cpu_x86_create(), make the PC initialization code attach the CPU to > >>>>> icc_bridge. > >>>>> > >>>>> The only difference here is that icc_bridge attachment will now be done > >>>>> after x86_cpu_parse_featurestr() is called. But this shouldn't make any > >>>>> difference, as property setters shouldn't depend on icc_bridge. > >>>>> > >>>>> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> > >>>> > >>>> Looks okay to me, > >>>> > >>>> Reviewed-by: Andreas Färber <afaer...@suse.de> > >>>> > >>>> But using this smaller patch will still make inlining pc_new_cpu(), > >>>> where you are moving it to, bigger diffstat-wise (WIP). > >>> > >>> I just see this it as a reason to not inline pc_new_cpu(). :) > >> > >> Did you actually read my code? cpu_x86_create() does not allow in-place > >> initialization, in addition to doing the realized=true. > > > > I hadn't, sorry, I was simply thinking in general terms, where I > > wouldn't want to inline a function if that would mean duplicating code. > > > > I did read the qom-cpu-x86 code now, and I still don't see why you would > > want to duplicate existing pc_new_cpu() code that is needed on both call > > sites. I assume there is a way to avoid code duplication and still allow > > in-place initialization. > > > > Anyway, this discussion about diff sizes and duplication will be > > obsolete as soon as we apply a new version of the icc-bus removal patch. > > So I would prefer to discuss the details once we have a new patch > > My series (and Bharata's) cannot wait for some ominous new ICC removal > patch, which you yourself said would take some time to review and test...
I have no idea about timing and ordering, really. It was just a guess, because the ICC removal was already in the list for review. > > In short, if the only thing in common is setting apic-id and the icc > bridge parent, is that really worth having a function for? (Care to join > #qemu?) I am not sure until I see the actual patch. Before I see it, I believe icc-bridge and apic-id is probably worth a common function. If it's just for apic-id, probably not. But that's all speaking in general terms, and once I see the actual patch I may be easily convinced that inlining will be better in this case. :) (I will be in #qemu in 1 hour.) -- Eduardo
