Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links

[Igor Mammedov]

On Thu, 11 Apr 2013 14:19:37 -0300
Eduardo Habkost <address@hidden> wrote:

> On Thu, Apr 11, 2013 at 04:51:57PM +0200, Igor Mammedov wrote:
> > ... and leave links for not present CPUs empty.
> > 
> > It will allow users to query for possible APIC IDs and use them
> > with cpu-add QMP command.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> 
> I don't see anything wrong with having icc-bridge links as well, but I
> would really like to have a target-independent namespace with links,
> that could be used to query for the available/valid CPU IDs for cpu-add
> commands instead of icc-bridge. The IDs on that namespace could be
> considered completely opaque.

Considering that -numa in present state is not compatible with cpu-add
and that all CPU ID in this case are are sequence [0..maxcpus-1], this
patch could be dropped without any harm. libvirt could just use
numbers from this sequence like it's doing with current cpu_set without
any ID discovery. 

So, I've postponed target independent until we have -numa reworked,
then we could have /machine/node/socket/cpu containers with links.
The problem that needs to be solved, is the links storage ownership.
Who should allocate and own it? If machine was QOM object already,
I'd go with machine but it's not yet.

> 
> > ---
> > v2:
> >  * s/get_firmware_id/get_arch_id/ due to rebase
> >  * rename cpu_add_notifier to cpu_added_notifier &
> >    icc_bridge_cpu_add_req -> icc_bridge_cpued_add_req
> > ---
> >  hw/cpu/icc_bus.c          | 46 
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/i386/pc.c              |  9 +++++++--
> >  include/hw/i386/icc_bus.h |  2 ++
> >  3 files changed, 55 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/cpu/icc_bus.c b/hw/cpu/icc_bus.c
> > index ab9623d..5c0b9d4 100644
> > --- a/hw/cpu/icc_bus.c
> > +++ b/hw/cpu/icc_bus.c
> > @@ -18,6 +18,7 @@
> >   */
> >  #include "hw/i386/icc_bus.h"
> >  #include "hw/sysbus.h"
> > +#include "sysemu/sysemu.h"
> >  
> >  static void icc_bus_initfn(Object *obj)
> >  {
> > @@ -61,15 +62,39 @@ typedef struct ICCBridgeState {
> >      SysBusDevice busdev;
> >      MemoryRegion apic_container;
> >      MemoryRegion ioapic_container;
> > +    Notifier cpu_added_notifier;
> > +    Object **links;
> >  } ICCBridgeState;
> >  #define ICC_BRIGDE(obj) OBJECT_CHECK(ICCBridgeState, (obj), 
> > TYPE_ICC_BRIDGE)
> >  
> >  
> > +void icc_bridge_set_cpu_link(Object *bridge, Object *cpu_obj)
> > +{
> > +    gchar *name;
> > +    Error *error = NULL;
> > +    CPUState *cpu = CPU(cpu_obj);
> > +    int64_t id = CPU_GET_CLASS(cpu)->get_arch_id(cpu);
> > +
> > +    name = g_strdup_printf("cpu[%" PRIu32 "]", 
> > x86_cpu_apic_id_from_index(id));
> > +    object_property_set_link(bridge, cpu_obj, name, &error);
> > +    g_free(name);
> > +
> > +    g_assert(error == NULL);
> > +}
> > +
> > +static void icc_bridge_cpu_added_req(Notifier *n, void *opaque)
> > +{
> > +    ICCBridgeState *s = container_of(n, ICCBridgeState, 
> > cpu_added_notifier);
> > +
> > +    icc_bridge_set_cpu_link(OBJECT(s), OBJECT(opaque));
> > +}
> > +
> >  static void icc_bridge_initfn(Object *obj)
> >  {
> >      ICCBridgeState *s = ICC_BRIGDE(obj);
> >      SysBusDevice *sb = SYS_BUS_DEVICE(obj);
> >      ICCBus *ibus;
> > +    int i;
> >  
> >      ibus = ICC_BUS(qbus_create(TYPE_ICC_BUS, DEVICE(obj), "icc-bus"));
> >  
> > @@ -85,12 +110,33 @@ static void icc_bridge_initfn(Object *obj)
> >      memory_region_init(&s->ioapic_container, "icc-ioapic-container", 
> > 0x1000);
> >      sysbus_init_mmio(sb, &s->ioapic_container);
> >      ibus->ioapic_address_space = &s->ioapic_container;
> > +
> > +    s->links = g_malloc0(sizeof(Object *) * max_cpus);
> > +    for (i = 0; i < max_cpus; i++) {
> > +        gchar *cpu_name;
> > +
> > +        cpu_name = g_strdup_printf("cpu[%" PRIu32 "]",
> > +                                   x86_cpu_apic_id_from_index(i));
> > +        object_property_add_link(obj, cpu_name, TYPE_CPU, &s->links[i], 
> > NULL);
> > +        g_free(cpu_name);
> > +    }
> > +
> > +    s->cpu_added_notifier.notify = icc_bridge_cpu_added_req;
> > +    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> > +}
> > +
> > +static void icc_bridge_fini(Object *obj)
> > +{
> > +    ICCBridgeState *s = ICC_BRIGDE(obj);
> > +
> > +    g_free(s->links);
> >  }
> >  
> >  static const TypeInfo icc_bridge_info = {
> >      .name  = "icc-bridge",
> >      .parent = TYPE_SYS_BUS_DEVICE,
> >      .instance_init  = icc_bridge_initfn,
> > +    .instance_finalize  = icc_bridge_fini,
> >      .instance_size  = sizeof(ICCBridgeState),
> >  };
> >  
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 6d5e164..ada235c 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -870,7 +870,8 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
> > level)
> >      }
> >  }
> >  
> > -static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, Error 
> > **errp)
> > +static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> > +                          SysBusDevice *icc_bridge, Error **errp)
> >  {
> >      X86CPU *cpu;
> >  
> > @@ -882,6 +883,10 @@ static X86CPU *pc_new_cpu(const char *cpu_model, 
> > int64_t apic_id, Error **errp)
> >      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
> >      object_property_set_bool(OBJECT(cpu), true, "realized", errp);
> >  
> > +    if (icc_bridge != NULL) {
> > +        icc_bridge_set_cpu_link(OBJECT(icc_bridge), OBJECT(cpu));
> > +    }
> > +
> >      if (error_is_set(errp)) {
> >          if (cpu != NULL) {
> >              object_unref(OBJECT(cpu));
> > @@ -911,7 +916,7 @@ void pc_cpus_init(const char *cpu_model)
> >                                                   TYPE_ICC_BRIDGE, NULL));
> >  
> >      for (i = 0; i < smp_cpus; i++) {
> > -        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> > +        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), ib, 
> > &error);
> >          if (error) {
> >              fprintf(stderr, "%s\n", error_get_pretty(error));
> >              error_free(error);
> > diff --git a/include/hw/i386/icc_bus.h b/include/hw/i386/icc_bus.h
> > index 69a0278..bc31cd9 100644
> > --- a/include/hw/i386/icc_bus.h
> > +++ b/include/hw/i386/icc_bus.h
> > @@ -49,5 +49,7 @@ typedef struct ICCDeviceClass {
> >  
> >  #define TYPE_ICC_BRIDGE "icc-bridge"
> >  
> > +void icc_bridge_set_cpu_link(Object *bridge, Object *cpu);
> > +
> >  #endif /* CONFIG_USER_ONLY */
> >  #endif
> > -- 
> > 1.8.2
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor