| Message ID | 1365691918-30594-12-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <imammedo@redhat.com> > Reviewed-by: Paolo Bonzini <pbonzini@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. (And then icc-bridge could be responsible for ensuring APIC ID uniqueness, not the CPU object itself) > + 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 >
On Thu, 11 Apr 2013 16:12:33 -0300 Eduardo Habkost <ehabkost@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 <imammedo@redhat.com> > > Reviewed-by: Paolo Bonzini <pbonzini@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 > > (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. > > > + 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 >
On Fri, Apr 12, 2013 at 12:20:45PM +0200, Igor Mammedov wrote: > On Thu, 11 Apr 2013 16:12:33 -0300 > Eduardo Habkost <ehabkost@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 <imammedo@redhat.com> > > > Reviewed-by: Paolo Bonzini <pbonzini@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(). > > > > > (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 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. 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. 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. > > > > > > + 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
On Fri, 12 Apr 2013 10:13:13 -0300 Eduardo Habkost <ehabkost@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 <ehabkost@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 <imammedo@redhat.com> > > > > Reviewed-by: Paolo Bonzini <pbonzini@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
On Fri, Apr 12, 2013 at 05:46:42PM +0200, Igor Mammedov wrote: > On Fri, 12 Apr 2013 10:13:13 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: [...] > > > > > +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. OK. > > > > > > > > > > > > > > (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. Actually I am not 100% sure. For example: we could simply provide the socket/core/thread IDs (or links to socket/core objects?) to the CPU object, and the CPU object could calculate the APIC ID itself. > > > 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. True. I believe the conflict here is: we want other objects to set the APIC ID (be it the board, or socket/core objects), but at the same time it would be interesting to not expose the APIC ID outside QEMU. Being too flexible regarding the APIC ID is more likely to cause problems later. That said, I don't mind having a "apic-id" property because it is easier to simply expose it directly. But do you agree that: 1) we don't really need to expose it to be set from outside QEMU; 2) we shouldn't require it to be set from outside QEMU; 3) we should recommend users to not try to fiddle it with? > > > > > 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. I was just wondering if we could solve both problems at the same time time: if we have a convenient place to store APIC-ID-based links, we are getting a more efficient APIC ID allocation/check system for free. Especially considering how inefficient is the method used by cpu_exists(). It looks like the problem with my approach is that the QOM APIC-ID-based links are target-specific, but cpu_exists() is target-independent. If both were target-specific or both were target-independent, we could easily reuse the same mechanism for both, but that's not the case now.
在 2013-04-12五的 13:29 -0300，Eduardo Habkost写道： > On Fri, Apr 12, 2013 at 05:46:42PM +0200, Igor Mammedov wrote: > > On Fri, 12 Apr 2013 10:13:13 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > [...] > > > > > > +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. > > OK. > > > > > > > > > > > > > > > > > > > > (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. > > Actually I am not 100% sure. For example: we could simply provide the > socket/core/thread IDs (or links to socket/core objects?) to the CPU > object, and the CPU object could calculate the APIC ID itself. > > > > > > 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. > > True. I believe the conflict here is: we want other objects to set the > APIC ID (be it the board, or socket/core objects), ideally, we really prefer that, but, you know, QEMU machine implementation seems can't feed us enough to let us decide CPU ID as real hardware board, and neither in future for QEMU doesn't like to emulate many complex signals that connect devices. so, I think, this property is just as well as we can do, maybe we can't expect too much :-). > but at the same time > it would be interesting to not expose the APIC ID outside QEMU. Being > too flexible regarding the APIC ID is more likely to cause problems > later. > > That said, I don't mind having a "apic-id" property because it is easier > to simply expose it directly. But do you agree that: 1) we don't really > need to expose it to be set from outside QEMU; 2) we shouldn't require > it to be set from outside QEMU; 3) we should recommend users to not try > to fiddle it with? > > > > > > > > > > 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. > > I was just wondering if we could solve both problems at the same time > time: if we have a convenient place to store APIC-ID-based links, we are > getting a more efficient APIC ID allocation/check system for free. > Especially considering how inefficient is the method used by > cpu_exists(). > > It looks like the problem with my approach is that the QOM APIC-ID-based > links are target-specific, but cpu_exists() is target-independent. If > both were target-specific or both were target-independent, we could > easily reuse the same mechanism for both, but that's not the case now. >
On Fri, 12 Apr 2013 13:29:40 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Apr 12, 2013 at 05:46:42PM +0200, Igor Mammedov wrote: > > On Fri, 12 Apr 2013 10:13:13 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > [...] > > > > > > +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. > > OK. > > > > > > > > > > > > > > > > > > > > (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. > > Actually I am not 100% sure. For example: we could simply provide the > socket/core/thread IDs (or links to socket/core objects?) to the CPU > object, and the CPU object could calculate the APIC ID itself. > > > > > > 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. > > True. I believe the conflict here is: we want other objects to set the > APIC ID (be it the board, or socket/core objects), but at the same time > it would be interesting to not expose the APIC ID outside QEMU. Being > too flexible regarding the APIC ID is more likely to cause problems > later. > > That said, I don't mind having a "apic-id" property because it is easier > to simply expose it directly. But do you agree that: 1) we don't really > need to expose it to be set from outside QEMU; 2) we shouldn't require > it to be set from outside QEMU; 3) we should recommend users to not try > to fiddle it with? Due to nature of per thread CPU hotplug, management will have to specify some kind of ID to specify which CPU is being plugged. Management really doesn't/shouldn't care what this ID is. > > > > > > > > > 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. > > I was just wondering if we could solve both problems at the same time > time: if we have a convenient place to store APIC-ID-based links, we are > getting a more efficient APIC ID allocation/check system for free. > Especially considering how inefficient is the method used by > cpu_exists(). Like I said, when we come up with target-independent links interface, we can easily switch implementation of cpu_exists(). Right now inefficiency here isn't issue since it's used on slow path only. > > It looks like the problem with my approach is that the QOM APIC-ID-based > links are target-specific, but cpu_exists() is target-independent. If > both were target-specific or both were target-independent, we could > easily reuse the same mechanism for both, but that's not the case now. > cpu_exists() is target-independent but it look-ups CPU by target specific ID, that target provides via get_arch_id() hook. So for target-i386 it will be APIC ID.
On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote: [...] > > > > 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. > > > > True. I believe the conflict here is: we want other objects to set the > > APIC ID (be it the board, or socket/core objects), but at the same time > > it would be interesting to not expose the APIC ID outside QEMU. Being > > too flexible regarding the APIC ID is more likely to cause problems > > later. > > > > That said, I don't mind having a "apic-id" property because it is easier > > to simply expose it directly. But do you agree that: 1) we don't really > > need to expose it to be set from outside QEMU; 2) we shouldn't require > > it to be set from outside QEMU; 3) we should recommend users to not try > > to fiddle it with? > Due to nature of per thread CPU hotplug, management will have to specify some > kind of ID to specify which CPU is being plugged. Management really > doesn't/shouldn't care what this ID is. As long as management really doesn't/shouldn't care what the ID is, exposing the APIC ID in the form of an opaque CPU identifier wouldn't be a problem to me. I just wanted to make clarify if we agree that messing with the APIC ID directly won't be recommended and that the "apic-id" property will be for QEMU internal use only.
On Mon, 15 Apr 2013 10:45:24 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote: > [...] > > > > > 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. > > > > > > True. I believe the conflict here is: we want other objects to set the > > > APIC ID (be it the board, or socket/core objects), but at the same time > > > it would be interesting to not expose the APIC ID outside QEMU. Being > > > too flexible regarding the APIC ID is more likely to cause problems > > > later. > > > > > > That said, I don't mind having a "apic-id" property because it is easier > > > to simply expose it directly. But do you agree that: 1) we don't really > > > need to expose it to be set from outside QEMU; 2) we shouldn't require > > > it to be set from outside QEMU; 3) we should recommend users to not try > > > to fiddle it with? > > Due to nature of per thread CPU hotplug, management will have to specify > > some kind of ID to specify which CPU is being plugged. Management really > > doesn't/shouldn't care what this ID is. > > As long as management really doesn't/shouldn't care what the ID is, > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be > a problem to me. I just wanted to make clarify if we agree that messing > with the APIC ID directly won't be recommended and that the "apic-id" > property will be for QEMU internal use only. On contrary, it's useful external feature, x86 guests see only APIC ID, since it's the only ID they [should] know about. So guest aware mgmt could definitely use apic_id propery to correlate CPU in guest with QEMU view of them.
On Mon, Apr 15, 2013 at 04:34:41PM +0200, Igor Mammedov wrote: > On Mon, 15 Apr 2013 10:45:24 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote: > > [...] > > > > > > 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. > > > > > > > > True. I believe the conflict here is: we want other objects to set the > > > > APIC ID (be it the board, or socket/core objects), but at the same time > > > > it would be interesting to not expose the APIC ID outside QEMU. Being > > > > too flexible regarding the APIC ID is more likely to cause problems > > > > later. > > > > > > > > That said, I don't mind having a "apic-id" property because it is easier > > > > to simply expose it directly. But do you agree that: 1) we don't really > > > > need to expose it to be set from outside QEMU; 2) we shouldn't require > > > > it to be set from outside QEMU; 3) we should recommend users to not try > > > > to fiddle it with? > > > Due to nature of per thread CPU hotplug, management will have to specify > > > some kind of ID to specify which CPU is being plugged. Management really > > > doesn't/shouldn't care what this ID is. > > > > As long as management really doesn't/shouldn't care what the ID is, > > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be > > a problem to me. I just wanted to make clarify if we agree that messing > > with the APIC ID directly won't be recommended and that the "apic-id" > > property will be for QEMU internal use only. > On contrary, it's useful external feature, x86 guests see only APIC ID, since > it's the only ID they [should] know about. So guest aware mgmt could > definitely use apic_id propery to correlate CPU in guest with QEMU view of > them. You're right, _reading_ the APIC ID is very useful. I am worried about _setting_ it from external code.
On Mon, 15 Apr 2013 11:49:40 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 15, 2013 at 04:34:41PM +0200, Igor Mammedov wrote: > > On Mon, 15 Apr 2013 10:45:24 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote: > > > [...] > > > > > > > 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. > > > > > > > > > > True. I believe the conflict here is: we want other objects to set the > > > > > APIC ID (be it the board, or socket/core objects), but at the same time > > > > > it would be interesting to not expose the APIC ID outside QEMU. Being > > > > > too flexible regarding the APIC ID is more likely to cause problems > > > > > later. > > > > > > > > > > That said, I don't mind having a "apic-id" property because it is easier > > > > > to simply expose it directly. But do you agree that: 1) we don't really > > > > > need to expose it to be set from outside QEMU; 2) we shouldn't require > > > > > it to be set from outside QEMU; 3) we should recommend users to not try > > > > > to fiddle it with? > > > > Due to nature of per thread CPU hotplug, management will have to specify > > > > some kind of ID to specify which CPU is being plugged. Management really > > > > doesn't/shouldn't care what this ID is. > > > > > > As long as management really doesn't/shouldn't care what the ID is, > > > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be > > > a problem to me. I just wanted to make clarify if we agree that messing > > > with the APIC ID directly won't be recommended and that the "apic-id" > > > property will be for QEMU internal use only. > > On contrary, it's useful external feature, x86 guests see only APIC ID, since > > it's the only ID they [should] know about. So guest aware mgmt could > > definitely use apic_id propery to correlate CPU in guest with QEMU view of > > them. > > You're right, _reading_ the APIC ID is very useful. I am worried about > _setting_ it from external code. currently it's not possible since cpu-add doesn't allow to set any properties. We will need setting it for device_add though. By then, I guess some way to check that it's valid would be enough, otherwise hot-plugged CPU will be out of scope of MADT and guest would ignore it or through an error. > > -- > Eduardo >
On Mon, Apr 15, 2013 at 10:27:57PM +0200, Igor Mammedov wrote: > On Mon, 15 Apr 2013 11:49:40 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Apr 15, 2013 at 04:34:41PM +0200, Igor Mammedov wrote: > > > On Mon, 15 Apr 2013 10:45:24 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote: > > > > [...] > > > > > > > > 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. > > > > > > > > > > > > True. I believe the conflict here is: we want other objects to set the > > > > > > APIC ID (be it the board, or socket/core objects), but at the same time > > > > > > it would be interesting to not expose the APIC ID outside QEMU. Being > > > > > > too flexible regarding the APIC ID is more likely to cause problems > > > > > > later. > > > > > > > > > > > > That said, I don't mind having a "apic-id" property because it is easier > > > > > > to simply expose it directly. But do you agree that: 1) we don't really > > > > > > need to expose it to be set from outside QEMU; 2) we shouldn't require > > > > > > it to be set from outside QEMU; 3) we should recommend users to not try > > > > > > to fiddle it with? > > > > > Due to nature of per thread CPU hotplug, management will have to specify > > > > > some kind of ID to specify which CPU is being plugged. Management really > > > > > doesn't/shouldn't care what this ID is. > > > > > > > > As long as management really doesn't/shouldn't care what the ID is, > > > > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be > > > > a problem to me. I just wanted to make clarify if we agree that messing > > > > with the APIC ID directly won't be recommended and that the "apic-id" > > > > property will be for QEMU internal use only. > > > On contrary, it's useful external feature, x86 guests see only APIC ID, since > > > it's the only ID they [should] know about. So guest aware mgmt could > > > definitely use apic_id propery to correlate CPU in guest with QEMU view of > > > them. > > > > You're right, _reading_ the APIC ID is very useful. I am worried about > > _setting_ it from external code. > > currently it's not possible since cpu-add doesn't allow to set any properties. > > We will need setting it for device_add though. Not necessarily. That's why I am insisting on an interface based on links/topology, not based on a raw "apic-id" property: instead of setting apic-id directly, we could just require that the CPU be attached to the right socket/core objects, and the APIC ID would be magically calculated correctly. Or we could just let the right socket/core/thread IDs to be set as properties, and apic-id could be calculated based on that. There are many ways to expose an abstraction that's simpler to use and less likely to cause problems. > By then, I guess some way to check that it's valid would be enough, otherwise > hot-plugged CPU will be out of scope of MADT and guest would ignore it or > through an error. >
On Mon, 15 Apr 2013 17:49:20 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 15, 2013 at 10:27:57PM +0200, Igor Mammedov wrote: > > On Mon, 15 Apr 2013 11:49:40 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Apr 15, 2013 at 04:34:41PM +0200, Igor Mammedov wrote: > > > > On Mon, 15 Apr 2013 10:45:24 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Mon, Apr 15, 2013 at 03:37:00PM +0200, Igor Mammedov wrote: > > > > > [...] > > > > > > > > > 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. > > > > > > > > > > > > > > True. I believe the conflict here is: we want other objects to set the > > > > > > > APIC ID (be it the board, or socket/core objects), but at the same time > > > > > > > it would be interesting to not expose the APIC ID outside QEMU. Being > > > > > > > too flexible regarding the APIC ID is more likely to cause problems > > > > > > > later. > > > > > > > > > > > > > > That said, I don't mind having a "apic-id" property because it is easier > > > > > > > to simply expose it directly. But do you agree that: 1) we don't really > > > > > > > need to expose it to be set from outside QEMU; 2) we shouldn't require > > > > > > > it to be set from outside QEMU; 3) we should recommend users to not try > > > > > > > to fiddle it with? > > > > > > Due to nature of per thread CPU hotplug, management will have to specify > > > > > > some kind of ID to specify which CPU is being plugged. Management really > > > > > > doesn't/shouldn't care what this ID is. > > > > > > > > > > As long as management really doesn't/shouldn't care what the ID is, > > > > > exposing the APIC ID in the form of an opaque CPU identifier wouldn't be > > > > > a problem to me. I just wanted to make clarify if we agree that messing > > > > > with the APIC ID directly won't be recommended and that the "apic-id" > > > > > property will be for QEMU internal use only. > > > > On contrary, it's useful external feature, x86 guests see only APIC ID, since > > > > it's the only ID they [should] know about. So guest aware mgmt could > > > > definitely use apic_id propery to correlate CPU in guest with QEMU view of > > > > them. > > > > > > You're right, _reading_ the APIC ID is very useful. I am worried about > > > _setting_ it from external code. > > > > currently it's not possible since cpu-add doesn't allow to set any properties. > > > > We will need setting it for device_add though. > > Not necessarily. That's why I am insisting on an interface based on > links/topology, not based on a raw "apic-id" property: instead of > setting apic-id directly, we could just require that the CPU be attached > to the right socket/core objects, and the APIC ID would be magically > calculated correctly. > > Or we could just let the right socket/core/thread IDs to be set as > properties, and apic-id could be calculated based on that. There are > many ways to expose an abstraction that's simpler to use and less likely > to cause problems. if we have tree like this: /nodes/0../sockets/0../cores/0../threads/0.. user will have to parse all branches IDs to collect node/socket/core/thread IDs before adding CPU. If use APIC ID on thread level then user is required to get only it and no magic would be required to calculate it, because board has already calculated it in advance. BTW: it'd be nicer to refuse wrong id earlier at property setting time then postponing it till realize when it would be possible to check that node/socket/core/thread info provided earlier was correct. > > > > By then, I guess some way to check that it's valid would be enough, otherwise > > hot-plugged CPU will be out of scope of MADT and guest would ignore it or > > through an error. > > > > -- > Eduardo
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); + 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);