On Fri, 12 Apr 2013 10:13:13 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Fri, Apr 12, 2013 at 12:20:45PM +0200, Igor Mammedov wrote:
> > On Thu, 11 Apr 2013 16:12:33 -0300
> > Eduardo Habkost <ehabk...@redhat.com> wrote:
> >
> > > On Thu, Apr 11, 2013 at 04:51:50PM +0200, Igor Mammedov wrote:
> > > > ... and use it from board level to set APIC ID for CPUs
> > > > it creates.
> > > >
> > > > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > > > Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
> > > > ---
> > > > Note:
> > > > * pc_new_cpu() function will be reused later in CPU hot-plug hook.
> > > >
> > > > v3:
> > > > * user error_propagate() in property setter
> > > > v2:
> > > > * use generic cpu_exists() instead of custom one
> > > > * make apic-id dynamic property, so it won't be possible to use it
> > > > as global property, since it's instance specific
> > > > ---
> > > > hw/i386/pc.c | 25 ++++++++++++++++++++++++-
> > > > target-i386/cpu.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 66 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 8d75b34..3adf294 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -869,9 +869,29 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,
> > > > int level)
> > > > }
> > > > }
> > > >
> > > > +static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error
> > > > **errp)
> > > > +{
> > > > + X86CPU *cpu;
> > > > +
> > > > + cpu = cpu_x86_create(cpu_model, errp);
> > > > + if (!cpu) {
> > > > + return;
> > > > + }
> > > > +
> > > > + object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp);
> > > > + object_property_set_bool(OBJECT(cpu), true, "realized", errp);
> > > > +
> > > > + if (error_is_set(errp)) {
> > > > + if (cpu != NULL) {
> > > > + object_unref(OBJECT(cpu));
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > void pc_cpus_init(const char *cpu_model)
> > > > {
> > > > int i;
> > > > + Error *error = NULL;
> > > >
> > > > /* init CPUs */
> > > > if (cpu_model == NULL) {
> > > > @@ -883,7 +903,10 @@ void pc_cpus_init(const char *cpu_model)
> > > > }
> > > >
> > > > for (i = 0; i < smp_cpus; i++) {
> > > > - if (!cpu_x86_init(cpu_model)) {
> > > > + pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error);
> > > > + if (error) {
> > > > + fprintf(stderr, "%s\n", error_get_pretty(error));
> > > > + error_free(error);
> > > > exit(1);
> > > > }
> > > > }
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 9cca031..4ddc18a 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -1272,6 +1272,45 @@ static void x86_cpuid_set_tsc_freq(Object *obj,
> > > > Visitor *v, void *opaque,
> > > > cpu->env.tsc_khz = value / 1000;
> > > > }
> > > >
> > > > +static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void
> > > > *opaque,
> > > > + const char *name, Error **errp)
> > > > +{
> > > > + X86CPU *cpu = X86_CPU(obj);
> > > > + int64_t value = cpu->env.cpuid_apic_id;
> > > > +
> > > > + visit_type_int(v, &value, name, errp);
> > > > +}
> > > > +
> > > > +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void
> > > > *opaque,
> > > > + const char *name, Error **errp)
> > > > +{
> > > > + X86CPU *cpu = X86_CPU(obj);
> > > > + const int64_t min = 0;
> > > > + const int64_t max = UINT32_MAX;
> > > > + Error *error = NULL;
> > > > + int64_t value;
> > > > +
> > > > + visit_type_int(v, &value, name, &error);
> > > > + if (error) {
> > > > + error_propagate(errp, error);
> > > > + return;
> > > > + }
> > > > + if (value < min || value > max) {
> > > > + error_setg(&error, "Property %s.%s doesn't take value %" PRId64
> > > > + " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
> > > > + object_get_typename(obj), name, value, min, max);
> > > > + error_propagate(errp, error);
> > > > + return;
> > > > + }
> > > > +
> > > > + if (cpu_exists(value)) {
> > > > + error_setg(&error, "CPU with APIC ID %" PRIi64 " exists",
> > > > value);
> > > > + error_propagate(errp, error);
> > >
> > > What about implementing this check after implementing the
> > > /machine/icc-bridge links? Then we can simply check if
> > > "/machine/icc-bridge/cpus[ID]" is already set.
> > because it's more generic and won't break even if link location changes
>
> Well, if link location changed, we could change the cpu_exists()
> implementation. We don't even need to hardcode the icc-bridge path, we
> could simply follow appropriate links if we set them up.
>
> My problem with cpu_exists() is that it is based on global state,
> instead of simply using the links between bus/devices to find out if the
> IDs are being allocated correctly. We could make it work like PCI: devfn
> conflicts are solved simply by looking at the list of devices in the
> specific PCIBus where the device is being added. See
> do_pci_register_device().
>
> That said, I am not against the current approach if we don't have the
> necessary bus/links modelled yet. But if we are going to add icc-bridge,
> we could benefit from it to implement cpu_exists().
Well,check has to be done here anyway, regardless how cpu_exists() implemented.
BTW:
This discussion and below belongs more to 9/19 patch that implements
cpu_exists() than here.
>
> >
> > >
> > > (And then icc-bridge could be responsible for ensuring APIC ID
> > > uniqueness, not the CPU object itself)
> > Yep, with cpu-add id could be/is verified before setting property, but if
> > one considers device_add or fictional -device cpuxxx on cmd line, then won't
> > be an external check for this. This check takes care about these use cases.
>
> That's why I believe the CPU has to calculate the APIC ID based on
> signaling coming from other devices/buses, instead of allowing the APIC
You mean by board not by CPU, I suppose.
> ID to be directly specified in the device_add/-device options. That's
> how real CPUs work: as the CPU manufacturer doesn't know what will be
> the package ID of the CPU, the APIC IDs are not hardcoded in the CPU;
> they are calculated based on the CPU topology and some socket identifier
> signal coming from the board.
that is why apic_id has been made a property, to be set from outside.
>
> Comparing it with PCI again: devfn of PCI devices can be specified
> manually, but if it is not present you just need to look at the PCI bus
> to find out the next available devfn.
18/19 gives an option to lookup available APIC IDs in a target specific way.
If we come up with a better target agnostic way to lookup ID, we can always
add it on top of currently proposed implementation.
> Based on that, I thought that icc-bridge was going be the component
> responsible for the APIC ID allocation logic, considering that it
> already contains APIC-ID-based links to the CPU objects.
in 19/19 APIC IDs are allocated by board for each possible CPU, icc-bridge is
just convenient place for storing links now. If machine was QOM object, I'd
probably place it there.
>
>
> >
> > >
> > > > + return;
> > > > + }
> > > > + cpu->env.cpuid_apic_id = value;
> > > > +}
> > > > +
> > > > static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char
> > > > *name)
> > > > {
> > > > x86_def_t *def;
> > > > @@ -2259,6 +2298,9 @@ static void x86_cpu_initfn(Object *obj)
> > > > object_property_add(obj, "tsc-frequency", "int",
> > > > x86_cpuid_get_tsc_freq,
> > > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > > > + object_property_add(obj, "apic-id", "int",
> > > > + x86_cpuid_get_apic_id,
> > > > + x86_cpuid_set_apic_id, NULL, NULL, NULL);
> > > >
> > > > env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
> > > >
> > > > --
> > > > 1.8.2
> > > >
> > >
> > > --
> > > Eduardo
> > >
> >
> >
> > --
> > Regards,
> > Igor
>
> --
> Eduardo
--
Regards,
Igor
