Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Eduardo Habkost]

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 <address@hidden>
> >>>>
> >>>> Looks okay to me,
> >>>>
> >>>> Reviewed-by: Andreas Färber <address@hidden>
> >>>>
> >>>> 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