Re: [Qemu-devel] [PATCH v3 03/19] pc: extract CPU lookup into a separate function

[Igor Mammedov]

On Mon, 11 Jul 2016 23:28:19 -0300
Eduardo Habkost <address@hidden> wrote:

> On Wed, Jul 06, 2016 at 08:20:39AM +0200, Igor Mammedov wrote:
> > it will be reused in the next patch at pre_plug time
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > v2:
> >   - rename pc_find_cpu() into pc_find_cpu_slot() and add comment to it
> >      Eduardo Habkost <address@hidden>
> > ---
> >  hw/i386/pc.c | 29 ++++++++++++++++++++++-------
> >  1 file changed, 22 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7ab06c2..3c42d84 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1675,11 +1675,30 @@ static int pc_apic_cmp(const void *a, const void *b)
> >     return apic_a->arch_id - apic_b->arch_id;
> >  }
> >  
> > +/* returns pointer to CPUArchId descriptor that matches CPU's apic_id
> > + * in pcms->possible_cpus->cpus, if pcms->possible_cpus->cpus has no
> > + * entry correponding to CPU's apic_id returns NULL.
> > + */
> > +static CPUArchId *pc_find_cpu_slot(PCMachineState *pcms, CPUState *cpu,
> > +                                   int *idx)
> > +{
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +    CPUArchId apic_id, *found_cpu;
> > +
> > +    apic_id.arch_id = cc->get_arch_id(CPU(cpu));
> > +    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> > +        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
> > +        pc_apic_cmp);
> > +    if (found_cpu && idx) {
> > +        *idx = found_cpu - pcms->possible_cpus->cpus;  
> 
> Do you have a use case for the idx parameter? I don't see it
> being used for anything in this series.
I've used to use idx but forgot about it on respin.

it was intended to avoid adhoc index calculations at call sites if needed.

And well, I've used "cpu_slot - pcms->possible_cpus->cpus" instead of idx.
So lets leave this patch as is and I'll fix the next patch to use idx.

> 
> > +    }
> > +    return found_cpu;
> > +}
> > +
> >  static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> >                          DeviceState *dev, Error **errp)
> >  {
> > -    CPUClass *cc = CPU_GET_CLASS(dev);
> > -    CPUArchId apic_id, *found_cpu;
> > +    CPUArchId *found_cpu;
> >      HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > @@ -1703,11 +1722,7 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> >      /* increment the number of CPUs */
> >      rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
> >  
> > -    apic_id.arch_id = cc->get_arch_id(CPU(dev));
> > -    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
> > -        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
> > -        pc_apic_cmp);
> > -    assert(found_cpu);
> > +    found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> >      found_cpu->cpu = CPU(dev);
> >  out:
> >      error_propagate(errp, local_err);
> > -- 
> > 2.7.0
> >   
>