[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links |
Date: |
Fri, 12 Apr 2013 09:44:09 -0300 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Apr 12, 2013 at 12:01:03PM +0200, Igor Mammedov wrote:
> 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.
But it's not -numa that makes APIC ID probing necessary, it's
non-power-of-2 core/thread counts on -smp (that make APIC IDs not match
CPU indexes).
"Don't use CPU hotplug with -numa" is easy to be understood by users and
by libvirt, but "don't use CPU hotplug with non-power-of-2 cores/threads
counts" is harder to explain.
>
> 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.
If we use CPU index as argument to cpu-add, we don't need to handle all
those problems right now, we don't need to expose an APIC ID discovery
interface, we make it work even with non-power-of-2 cores/threads
counts, and we make it work with -numa.
So, my big question is: why are we trying so hard to avoid using CPU
indexes as argument to cpu-add, if it's so much easier, and it is an
obvious solution that makes the interface target-independent without any
extra effort?
>
> >
> > > ---
> > > 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
--
Eduardo
- Re: [Qemu-devel] [PATCH 05/19] cpu: resume CPU from CPUClass.cpu_common_realizefn() when it is hot-plugged, (continued)
- [Qemu-devel] [PATCH 01/19] target-i386: split out CPU creation and features parsing into cpu_x86_create(), Igor Mammedov, 2013/04/11
- [Qemu-devel] [PATCH 16/19] target-i386: move IOAPIC to ICC bus, Igor Mammedov, 2013/04/11
- [Qemu-devel] [PATCH 12/19] introduce ICC bus/device/bridge, Igor Mammedov, 2013/04/11
- [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links, Igor Mammedov, 2013/04/11
- Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links, Eduardo Habkost, 2013/04/11
- Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links, Igor Mammedov, 2013/04/12
- Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links,
Eduardo Habkost <=
- Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links, Igor Mammedov, 2013/04/15
- Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links, Eduardo Habkost, 2013/04/15
- Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links, Igor Mammedov, 2013/04/15
- Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links, Eduardo Habkost, 2013/04/15
- Re: [Qemu-devel] [PATCH 18/19] target-i386: expose all possible CPUs as /machine/icc-bridge/cpu[0..N] links, Igor Mammedov, 2013/04/15
[Qemu-devel] [PATCH 19/19] add cpu-add qmp command and implement CPU hot-add for target-i386, Igor Mammedov, 2013/04/11