Re: [Qemu-devel] [PATCH for-2.10 03/23] hw/arm/virt: use machine->possible_cpus for storing possible topology info

[Igor Mammedov]

On Tue, 25 Apr 2017 16:28:27 +0200
Andrew Jones <address@hidden> wrote:

> On Wed, Mar 22, 2017 at 02:32:28PM +0100, Igor Mammedov wrote:
> > for now precalculate and store mp_afinity in possible_cpus
> > as ARM cpus don't have socket/core/thread-id properties yet.
> > In follow patches possible_cpus will be used for storing
> > and setting NUMA node mapping and replace legacy bitmap
> > based numa_info[node_id].node_cpu/numa_get_node_for_cpu()
> > 
> > For the lack of better idea, this patch cannibalizes
> > possible_cpus.cpus[x].props.thread_id so that
> > *_cpu_index_to_props() callback could return addressable
> > by props CPU which will used by machine_set_cpu_numa_node()
> > in follow up patches to assign a CPU to node. But
> > cannibalizing is fine for now as that thread_id isn't exposed
> > to users (no hotpluggable_cpus callback support for ARM yet)
> > and it will be used only internally until 'device_add cpu'
> > is supported where we can decide on which properties to use.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  hw/arm/virt.c | 39 ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 36 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 484754e..4de46b1 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1237,6 +1237,7 @@ static void machvirt_init(MachineState *machine)
> >  {
> >      VirtMachineState *vms = VIRT_MACHINE(machine);
> >      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
> > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> >      qemu_irq pic[NUM_IRQS];
> >      MemoryRegion *sysmem = get_system_memory();
> >      MemoryRegion *secure_sysmem = NULL;
> > @@ -1360,10 +1361,16 @@ static void machvirt_init(MachineState *machine)
> >          exit(1);
> >      }
> >  
> > -    for (n = 0; n < smp_cpus; n++) {
> > -        Object *cpuobj = object_new(typename);
> > +    mc->possible_cpu_arch_ids(machine);
> 
> Hmm, this is a bit too subtle for my taste. Making this call, which by
> name implies it returns the possible CPU Arch-IDs (which indeed it does),
> but without assigning its return value to anything, looks wrong. I see
> the reason to do so, though, is because on first call it will do the
> allocation. I'd prefer a wrapper
> 
>  static void virt_alloc_possible_cpus(MachineState *ms)
>  {
>     /* First invocation does allocation and initialization. */
>     (void)MACHINE_GET_CLASS(ms)->possible_cpu_arch_ids(ms);
>  }
Ok, I'll make it generic
machine_alloc_possible_cpus(MachineState *ms)
and use it for x86/ppc as well

> 
> > +    for (n = 0; n < machine->possible_cpus->len; n++) {
> > +        Object *cpuobj;
> >  
> > -        object_property_set_int(cpuobj, virt_idx2mp_affinity(vms, n),
> > +        if (n >= smp_cpus) {
> > +            break;
> > +        }
> > +
> > +        cpuobj = object_new(typename);
> > +        object_property_set_int(cpuobj, 
> > machine->possible_cpus->cpus[n].arch_id,
> >                                  "mp-affinity", NULL);
> >  
> >          if (!vms->secure) {
> > @@ -1543,6 +1550,31 @@ static void virt_set_gic_version(Object *obj, const 
> > char *value, Error **errp)
> >      }
> >  }
> >  
> > +static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> > +{
> > +    int n;
> > +    VirtMachineState *vms = VIRT_MACHINE(ms);
> > +
> > +    if (ms->possible_cpus) {
> > +        assert(ms->possible_cpus->len == max_cpus);
> > +        return ms->possible_cpus;
> > +    }
> > +
> > +    ms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
> > +                                  sizeof(CPUArchId) * max_cpus);
> > +    ms->possible_cpus->len = max_cpus;
> > +    for (n = 0; n < ms->possible_cpus->len; n++) {
> > +        ms->possible_cpus->cpus[n].arch_id =
> > +            virt_idx2mp_affinity(vms, n);
> > +        ms->possible_cpus->cpus[n].props.has_thread_id = true;
> > +        ms->possible_cpus->cpus[n].props.thread_id = n;
> > +
> > +        /* TODO: add 'has_node/node' here to describe
> > +           to which node core belongs */
> > +    }
> > +    return ms->possible_cpus;
> > +}
> > +
> >  static void virt_machine_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > @@ -1559,6 +1591,7 @@ static void virt_machine_class_init(ObjectClass *oc, 
> > void *data)
> >      mc->pci_allow_0_address = true;
> >      /* We know we will never create a pre-ARMv7 CPU which needs 1K pages */
> >      mc->minimum_page_bits = 12;
> > +    mc->possible_cpu_arch_ids = virt_possible_cpu_arch_ids;
> >  }
> >  
> >  static const TypeInfo virt_machine_info = {
> > -- 
> > 2.7.4
> > 
> > 
> 
> Thanks,
> drew