| Message ID | 1474548655-157373-8-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 22/09/2016 14:50, Igor Mammedov wrote: > ACPI ID is 32 bit wide on CPUs with x2APIC support. > Extend 'id' property to support it. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > include/hw/i386/apic_internal.h | 3 ++- > target-i386/cpu.h | 1 + > hw/intc/apic_common.c | 40 +++++++++++++++++++++++++++++++++++++++- > target-i386/cpu.c | 2 +- > 4 files changed, 43 insertions(+), 3 deletions(-) > > diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h > index 06c4e9f..c79b080 100644 > --- a/include/hw/i386/apic_internal.h > +++ b/include/hw/i386/apic_internal.h > @@ -156,7 +156,8 @@ struct APICCommonState { > MemoryRegion io_memory; > X86CPU *cpu; > uint32_t apicbase; > - uint8_t id; > + uint8_t id; /* legacy APIC ID */ > + uint32_t initial_apic_id; > uint8_t version; > uint8_t arb_id; > uint8_t tpr; > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 27af9c3..f364e8e 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -325,6 +325,7 @@ > #define MSR_IA32_APICBASE 0x1b > #define MSR_IA32_APICBASE_BSP (1<<8) > #define MSR_IA32_APICBASE_ENABLE (1<<11) > +#define MSR_IA32_APICBASE_EXTD (1 << 10) > #define MSR_IA32_APICBASE_BASE (0xfffffU<<12) > #define MSR_IA32_FEATURE_CONTROL 0x0000003a > #define MSR_TSC_ADJUST 0x0000003b > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > index 14ac43c..125af9d 100644 > --- a/hw/intc/apic_common.c > +++ b/hw/intc/apic_common.c > @@ -21,6 +21,7 @@ > #include "qapi/error.h" > #include "qemu-common.h" > #include "cpu.h" > +#include "qapi/visitor.h" > #include "hw/i386/apic.h" > #include "hw/i386/apic_internal.h" > #include "trace.h" > @@ -427,7 +428,6 @@ static const VMStateDescription vmstate_apic_common = { > }; > > static Property apic_properties_common[] = { > - DEFINE_PROP_UINT8("id", APICCommonState, id, -1), > DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14), > DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT, > true), > @@ -436,6 +436,43 @@ static Property apic_properties_common[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static void apic_common_get_id(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + APICCommonState *s = APIC_COMMON(obj); > + int64_t value; > + > + value = s->apicbase & MSR_IA32_APICBASE_EXTD ? s->initial_apic_id : s->id; Why not just return initial_apic_id? This is the meaning the property had before your patch. > + visit_type_int(v, name, &value, errp); > +} > + > +static void apic_common_set_id(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + APICCommonState *s = APIC_COMMON(obj); > + Error *local_err = NULL; > + int64_t value; > + > + visit_type_int(v, name, &value, &local_err); ... so I think you should fail here if the device has been realized already. Or even better, just do this: - DEFINE_PROP_UINT8("id", APICCommonState, id, -1), + DEFINE_PROP_UINT32("id", APICCommonState, initial_apic_id, -1), then in the realize method you assign initial_apic_id to id. Otherwise series looks good. Paolo > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + s->initial_apic_id = value; > + s->id = (uint8_t)value; > +} > + > +static void apic_common_initfn(Object *obj) > +{ > + APICCommonState *s = APIC_COMMON(obj); > + > + s->id = s->initial_apic_id = -1; > + object_property_add(obj, "id", "int", > + apic_common_get_id, > + apic_common_set_id, NULL, NULL, NULL); > +} > + > static void apic_common_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -455,6 +492,7 @@ static const TypeInfo apic_common_type = { > .name = TYPE_APIC_COMMON, > .parent = TYPE_DEVICE, > .instance_size = sizeof(APICCommonState), > + .instance_init = apic_common_initfn, > .class_size = sizeof(APICCommonClass), > .class_init = apic_common_class_init, > .abstract = true, > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index db12728..c7cbc88 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2877,7 +2877,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > OBJECT(cpu->apic_state), &error_abort); > object_unref(OBJECT(cpu->apic_state)); > > - qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id); > + qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id); > /* TODO: convert to link<> */ > apic = APIC_COMMON(cpu->apic_state); > apic->cpu = cpu; >
On Thu, 22 Sep 2016 16:37:04 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 22/09/2016 14:50, Igor Mammedov wrote: > > ACPI ID is 32 bit wide on CPUs with x2APIC support. > > Extend 'id' property to support it. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > include/hw/i386/apic_internal.h | 3 ++- > > target-i386/cpu.h | 1 + > > hw/intc/apic_common.c | 40 +++++++++++++++++++++++++++++++++++++++- > > target-i386/cpu.c | 2 +- > > 4 files changed, 43 insertions(+), 3 deletions(-) > > > > diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h > > index 06c4e9f..c79b080 100644 > > --- a/include/hw/i386/apic_internal.h > > +++ b/include/hw/i386/apic_internal.h > > @@ -156,7 +156,8 @@ struct APICCommonState { > > MemoryRegion io_memory; > > X86CPU *cpu; > > uint32_t apicbase; > > - uint8_t id; > > + uint8_t id; /* legacy APIC ID */ > > + uint32_t initial_apic_id; > > uint8_t version; > > uint8_t arb_id; > > uint8_t tpr; > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index 27af9c3..f364e8e 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -325,6 +325,7 @@ > > #define MSR_IA32_APICBASE 0x1b > > #define MSR_IA32_APICBASE_BSP (1<<8) > > #define MSR_IA32_APICBASE_ENABLE (1<<11) > > +#define MSR_IA32_APICBASE_EXTD (1 << 10) > > #define MSR_IA32_APICBASE_BASE (0xfffffU<<12) > > #define MSR_IA32_FEATURE_CONTROL 0x0000003a > > #define MSR_TSC_ADJUST 0x0000003b > > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > > index 14ac43c..125af9d 100644 > > --- a/hw/intc/apic_common.c > > +++ b/hw/intc/apic_common.c > > @@ -21,6 +21,7 @@ > > #include "qapi/error.h" > > #include "qemu-common.h" > > #include "cpu.h" > > +#include "qapi/visitor.h" > > #include "hw/i386/apic.h" > > #include "hw/i386/apic_internal.h" > > #include "trace.h" > > @@ -427,7 +428,6 @@ static const VMStateDescription vmstate_apic_common = { > > }; > > > > static Property apic_properties_common[] = { > > - DEFINE_PROP_UINT8("id", APICCommonState, id, -1), > > DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14), > > DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT, > > true), > > @@ -436,6 +436,43 @@ static Property apic_properties_common[] = { > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > +static void apic_common_get_id(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + APICCommonState *s = APIC_COMMON(obj); > > + int64_t value; > > + > > + value = s->apicbase & MSR_IA32_APICBASE_EXTD ? s->initial_apic_id : s->id; > > Why not just return initial_apic_id? This is the meaning the property > had before your patch. initial_apic_id is immutable but 'id' could be changed at runtime by guest in xAPIC mode so returned value depends on xAPIC/x2APIC mode so I'm just following spec here. > > > + visit_type_int(v, name, &value, errp); > > +} > > + > > +static void apic_common_set_id(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + APICCommonState *s = APIC_COMMON(obj); > > + Error *local_err = NULL; > > + int64_t value; > > + > > + visit_type_int(v, name, &value, &local_err); > > ... so I think you should fail here if the device has been realized already. > > Or even better, just do this: > > - DEFINE_PROP_UINT8("id", APICCommonState, id, -1), > + DEFINE_PROP_UINT32("id", APICCommonState, initial_apic_id, -1), > > then in the realize method you assign initial_apic_id to id. > > Otherwise series looks good. > > Paolo > > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + s->initial_apic_id = value; > > + s->id = (uint8_t)value; > > +} > > + > > +static void apic_common_initfn(Object *obj) > > +{ > > + APICCommonState *s = APIC_COMMON(obj); > > + > > + s->id = s->initial_apic_id = -1; > > + object_property_add(obj, "id", "int", > > + apic_common_get_id, > > + apic_common_set_id, NULL, NULL, NULL); > > +} > > + > > static void apic_common_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -455,6 +492,7 @@ static const TypeInfo apic_common_type = { > > .name = TYPE_APIC_COMMON, > > .parent = TYPE_DEVICE, > > .instance_size = sizeof(APICCommonState), > > + .instance_init = apic_common_initfn, > > .class_size = sizeof(APICCommonClass), > > .class_init = apic_common_class_init, > > .abstract = true, > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index db12728..c7cbc88 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -2877,7 +2877,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) > > OBJECT(cpu->apic_state), &error_abort); > > object_unref(OBJECT(cpu->apic_state)); > > > > - qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id); > > + qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id); > > /* TODO: convert to link<> */ > > apic = APIC_COMMON(cpu->apic_state); > > apic->cpu = cpu; > >
On 22/09/2016 18:00, Igor Mammedov wrote: > > Why not just return initial_apic_id? This is the meaning the property > > had before your patch. > > initial_apic_id is immutable but 'id' could be changed at runtime by guest in xAPIC mode > so returned value depends on xAPIC/x2APIC mode Understood, but this is just a possibly poorly-named property. "id" (e.g. from info qtree as opposed to info lapic) used to be the initial APIC ID always, even in x2APIC mode. Not a big deal, but thought I'd mention it since you can keep using static properties. Paolo > so I'm just following spec here.
On Thu, 22 Sep 2016 18:16:47 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 22/09/2016 18:00, Igor Mammedov wrote: > > > Why not just return initial_apic_id? This is the meaning the property > > > had before your patch. > > > > initial_apic_id is immutable but 'id' could be changed at runtime by guest in xAPIC mode > > so returned value depends on xAPIC/x2APIC mode > > Understood, but this is just a possibly poorly-named property. "id" > (e.g. from info qtree as opposed to info lapic) used to be the initial > APIC ID always, even in x2APIC mode. 'info qtree' doesn't show CPUs anymore (since ICC bus has been removed), but if it were it would show effective APIC ID. Same applie[ds] for reading property value with qom-get. > > Not a big deal, but thought I'd mention it since you can keep using > static properties. PS: changing initial APIC ID from guest probably wouldn't work anyway and beak somewhere else, so we could just continue to ignore it and use static properties for now if you prefer. > > Paolo > > > so I'm just following spec here.
On 26/09/2016 13:10, Igor Mammedov wrote: > On Thu, 22 Sep 2016 18:16:47 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 22/09/2016 18:00, Igor Mammedov wrote: >>>> Why not just return initial_apic_id? This is the meaning the property >>>> had before your patch. >>> >>> initial_apic_id is immutable but 'id' could be changed at runtime by guest in xAPIC mode >>> so returned value depends on xAPIC/x2APIC mode >> >> Understood, but this is just a possibly poorly-named property. "id" >> (e.g. from info qtree as opposed to info lapic) used to be the initial >> APIC ID always, even in x2APIC mode. > > 'info qtree' doesn't show CPUs anymore (since ICC bus has been removed), > but if it were it would show effective APIC ID. Same applie[ds] for > reading property value with qom-get. Oh, thanks for correcting me then. >> Not a big deal, but thought I'd mention it since you can keep using >> static properties. > > changing initial APIC ID from guest probably wouldn't work anyway > and beak somewhere else, so we could just continue to ignore > it and use static properties for now if you prefer. No big deal---I'll let other reviewers chime in. Paolo
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h index 06c4e9f..c79b080 100644 --- a/include/hw/i386/apic_internal.h +++ b/include/hw/i386/apic_internal.h @@ -156,7 +156,8 @@ struct APICCommonState { MemoryRegion io_memory; X86CPU *cpu; uint32_t apicbase; - uint8_t id; + uint8_t id; /* legacy APIC ID */ + uint32_t initial_apic_id; uint8_t version; uint8_t arb_id; uint8_t tpr; diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 27af9c3..f364e8e 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -325,6 +325,7 @@ #define MSR_IA32_APICBASE 0x1b #define MSR_IA32_APICBASE_BSP (1<<8) #define MSR_IA32_APICBASE_ENABLE (1<<11) +#define MSR_IA32_APICBASE_EXTD (1 << 10) #define MSR_IA32_APICBASE_BASE (0xfffffU<<12) #define MSR_IA32_FEATURE_CONTROL 0x0000003a #define MSR_TSC_ADJUST 0x0000003b diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c index 14ac43c..125af9d 100644 --- a/hw/intc/apic_common.c +++ b/hw/intc/apic_common.c @@ -21,6 +21,7 @@ #include "qapi/error.h" #include "qemu-common.h" #include "cpu.h" +#include "qapi/visitor.h" #include "hw/i386/apic.h" #include "hw/i386/apic_internal.h" #include "trace.h" @@ -427,7 +428,6 @@ static const VMStateDescription vmstate_apic_common = { }; static Property apic_properties_common[] = { - DEFINE_PROP_UINT8("id", APICCommonState, id, -1), DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14), DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT, true), @@ -436,6 +436,43 @@ static Property apic_properties_common[] = { DEFINE_PROP_END_OF_LIST(), }; +static void apic_common_get_id(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + APICCommonState *s = APIC_COMMON(obj); + int64_t value; + + value = s->apicbase & MSR_IA32_APICBASE_EXTD ? s->initial_apic_id : s->id; + visit_type_int(v, name, &value, errp); +} + +static void apic_common_set_id(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + APICCommonState *s = APIC_COMMON(obj); + Error *local_err = NULL; + int64_t value; + + visit_type_int(v, name, &value, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + + s->initial_apic_id = value; + s->id = (uint8_t)value; +} + +static void apic_common_initfn(Object *obj) +{ + APICCommonState *s = APIC_COMMON(obj); + + s->id = s->initial_apic_id = -1; + object_property_add(obj, "id", "int", + apic_common_get_id, + apic_common_set_id, NULL, NULL, NULL); +} + static void apic_common_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -455,6 +492,7 @@ static const TypeInfo apic_common_type = { .name = TYPE_APIC_COMMON, .parent = TYPE_DEVICE, .instance_size = sizeof(APICCommonState), + .instance_init = apic_common_initfn, .class_size = sizeof(APICCommonClass), .class_init = apic_common_class_init, .abstract = true, diff --git a/target-i386/cpu.c b/target-i386/cpu.c index db12728..c7cbc88 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2877,7 +2877,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) OBJECT(cpu->apic_state), &error_abort); object_unref(OBJECT(cpu->apic_state)); - qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id); + qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id); /* TODO: convert to link<> */ apic = APIC_COMMON(cpu->apic_state); apic->cpu = cpu;
ACPI ID is 32 bit wide on CPUs with x2APIC support. Extend 'id' property to support it. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- include/hw/i386/apic_internal.h | 3 ++- target-i386/cpu.h | 1 + hw/intc/apic_common.c | 40 +++++++++++++++++++++++++++++++++++++++- target-i386/cpu.c | 2 +- 4 files changed, 43 insertions(+), 3 deletions(-)