Re: [Qemu-devel] [PATCH v3 06/13] pc: apic_common: extend APIC ID property to 32bit
On Tue, 18 Oct 2016 12:14:43 -0200 Eduardo Habkostwrote: > On Tue, Oct 18, 2016 at 04:01:56PM +0200, Igor Mammedov wrote: > > On Tue, 18 Oct 2016 10:59:17 -0200 > > Eduardo Habkost wrote: > > > > > On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote: > > > > On Tue, 18 Oct 2016 08:56:28 -0200 > > > > Eduardo Habkost wrote: > > > > > > > > > On Thu, Oct 13, 2016 at 11:52:40AM +0200, 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 > > > > > > --- > > > > > > v3: > > > > > >keep original behaviour where 'id' is readonly after > > > > > >object is realized (pbonzini) > > > > > > --- > > > > > [...] > > > > > > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > > > > > > index 8d01c9c..30f2af0 100644 > > > > > > --- a/hw/intc/apic_common.c > > > > > > +++ b/hw/intc/apic_common.c > > > > > > @@ -428,7 +429,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), > > > > > > @@ -437,6 +437,49 @@ 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, , errp); > > > > > > +} > > > > > > > > > > Who exactly is going to read this property and require this logic > > > > > to be in the property getter? > > > > As it's set/read only from CPU we don't actually have to expose it > > > > as property. > > > > However, I've kept it as read/write property because it has already > > > > been this way and been exposed to external users as some magic property. > > > > Not sure is anyone cares. > > > > > > > > > > > > > Do we really need to expose this to the outside as a magic > > > > > property that changes depending on hardware state? Returning > > > > > initial_apic_id sounds much simpler. > > > > Well that's what it is now, so I've kept current behavior. > > > > If we decide to change property behavior or drop it altogether > > > > I can do it on top. > > > > > > > > > > I agree to make them static properties as follow-up patch. This > > > way, if the change breaks anything we can revert only that patch. > > Static property won't work here as it should show APIC ID > > depending on current CPU mode. > > My suggestion is to _not_ show a different ID depending on the > current mode, to keep it simple. We can just have > "initial-apic-id" and "id" properties. > > But, anyway, I didn't mean to suggest static properties > specifically. Where I say "static property" above, please read > "simple property that returns just a simple struct field and > don't need custom getter/setter code". That means either using a > static property (in case we still want it to be writeable, which > I doubt) or using object_property_add_uint*_ptr(). > > > > > I could make it readonly property on respin, > > and set apic.id/initial_apic_id directly from CPU. > > Change would be > >- apic_common_set_id() > >+ apic_common::set_apic_id() callback > > It won't get us less LOC, more likely it will take even more > > code to do so. > > As it's in this patch, it's at least consistent in a way > > values are get/set. And effectively it's readonly due to check: > > Don't worry about doing it on the respin. I'm OK with keeping the > current version by now, and change it in a follow-up patch. > > > > > +if (dev->realized) { > > +qdev_prop_set_after_realize(dev, name, errp); > > +return; > > +} > > > > as external user can see only realized apic device. > > That's true. But we could avoid all the extra getter/setter code > if we just use a static property or a read-only > object_property_add_uint*_ptr() property. > > (All I say above are suggestions for a follow-up patch. I'm OK > with keeping the existing behavior like you do in this patch, to > keep this series simple.) Ok, I'll respin this patch as is and think/try the way you are suggesting in follow up.
Re: [Qemu-devel] [PATCH v3 06/13] pc: apic_common: extend APIC ID property to 32bit
On Tue, Oct 18, 2016 at 04:01:56PM +0200, Igor Mammedov wrote: > On Tue, 18 Oct 2016 10:59:17 -0200 > Eduardo Habkostwrote: > > > On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote: > > > On Tue, 18 Oct 2016 08:56:28 -0200 > > > Eduardo Habkost wrote: > > > > > > > On Thu, Oct 13, 2016 at 11:52:40AM +0200, 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 > > > > > --- > > > > > v3: > > > > >keep original behaviour where 'id' is readonly after > > > > >object is realized (pbonzini) > > > > > --- > > > > [...] > > > > > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > > > > > index 8d01c9c..30f2af0 100644 > > > > > --- a/hw/intc/apic_common.c > > > > > +++ b/hw/intc/apic_common.c > > > > > @@ -428,7 +429,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), > > > > > @@ -437,6 +437,49 @@ 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, , errp); > > > > > +} > > > > > > > > Who exactly is going to read this property and require this logic > > > > to be in the property getter? > > > As it's set/read only from CPU we don't actually have to expose it > > > as property. > > > However, I've kept it as read/write property because it has already > > > been this way and been exposed to external users as some magic property. > > > Not sure is anyone cares. > > > > > > > > > > Do we really need to expose this to the outside as a magic > > > > property that changes depending on hardware state? Returning > > > > initial_apic_id sounds much simpler. > > > Well that's what it is now, so I've kept current behavior. > > > If we decide to change property behavior or drop it altogether > > > I can do it on top. > > > > > > > I agree to make them static properties as follow-up patch. This > > way, if the change breaks anything we can revert only that patch. > Static property won't work here as it should show APIC ID > depending on current CPU mode. My suggestion is to _not_ show a different ID depending on the current mode, to keep it simple. We can just have "initial-apic-id" and "id" properties. But, anyway, I didn't mean to suggest static properties specifically. Where I say "static property" above, please read "simple property that returns just a simple struct field and don't need custom getter/setter code". That means either using a static property (in case we still want it to be writeable, which I doubt) or using object_property_add_uint*_ptr(). > > I could make it readonly property on respin, > and set apic.id/initial_apic_id directly from CPU. > Change would be >- apic_common_set_id() >+ apic_common::set_apic_id() callback > It won't get us less LOC, more likely it will take even more > code to do so. > As it's in this patch, it's at least consistent in a way > values are get/set. And effectively it's readonly due to check: Don't worry about doing it on the respin. I'm OK with keeping the current version by now, and change it in a follow-up patch. > > +if (dev->realized) { > +qdev_prop_set_after_realize(dev, name, errp); > +return; > +} > > as external user can see only realized apic device. That's true. But we could avoid all the extra getter/setter code if we just use a static property or a read-only object_property_add_uint*_ptr() property. (All I say above are suggestions for a follow-up patch. I'm OK with keeping the existing behavior like you do in this patch, to keep this series simple.) -- Eduardo
Re: [Qemu-devel] [PATCH v3 06/13] pc: apic_common: extend APIC ID property to 32bit
On Tue, 18 Oct 2016 10:59:17 -0200 Eduardo Habkostwrote: > On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote: > > On Tue, 18 Oct 2016 08:56:28 -0200 > > Eduardo Habkost wrote: > > > > > On Thu, Oct 13, 2016 at 11:52:40AM +0200, 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 > > > > --- > > > > v3: > > > >keep original behaviour where 'id' is readonly after > > > >object is realized (pbonzini) > > > > --- > > > [...] > > > > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > > > > index 8d01c9c..30f2af0 100644 > > > > --- a/hw/intc/apic_common.c > > > > +++ b/hw/intc/apic_common.c > > > > @@ -428,7 +429,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), > > > > @@ -437,6 +437,49 @@ 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, , errp); > > > > +} > > > > > > Who exactly is going to read this property and require this logic > > > to be in the property getter? > > As it's set/read only from CPU we don't actually have to expose it > > as property. > > However, I've kept it as read/write property because it has already > > been this way and been exposed to external users as some magic property. > > Not sure is anyone cares. > > > > > > > Do we really need to expose this to the outside as a magic > > > property that changes depending on hardware state? Returning > > > initial_apic_id sounds much simpler. > > Well that's what it is now, so I've kept current behavior. > > If we decide to change property behavior or drop it altogether > > I can do it on top. > > > > I agree to make them static properties as follow-up patch. This > way, if the change breaks anything we can revert only that patch. Static property won't work here as it should show APIC ID depending on current CPU mode. I could make it readonly property on respin, and set apic.id/initial_apic_id directly from CPU. Change would be - apic_common_set_id() + apic_common::set_apic_id() callback It won't get us less LOC, more likely it will take even more code to do so. As it's in this patch, it's at least consistent in a way values are get/set. And effectively it's readonly due to check: +if (dev->realized) { +qdev_prop_set_after_realize(dev, name, errp); +return; +} as external user can see only realized apic device.
Re: [Qemu-devel] [PATCH v3 06/13] pc: apic_common: extend APIC ID property to 32bit
On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote: > On Tue, 18 Oct 2016 08:56:28 -0200 > Eduardo Habkostwrote: > > > On Thu, Oct 13, 2016 at 11:52:40AM +0200, 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 > > > --- > > > v3: > > >keep original behaviour where 'id' is readonly after > > >object is realized (pbonzini) > > > --- > > [...] > > > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > > > index 8d01c9c..30f2af0 100644 > > > --- a/hw/intc/apic_common.c > > > +++ b/hw/intc/apic_common.c > > > @@ -428,7 +429,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), > > > @@ -437,6 +437,49 @@ 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, , errp); > > > +} > > > > Who exactly is going to read this property and require this logic > > to be in the property getter? > As it's set/read only from CPU we don't actually have to expose it > as property. > However, I've kept it as read/write property because it has already > been this way and been exposed to external users as some magic property. > Not sure is anyone cares. > > > > Do we really need to expose this to the outside as a magic > > property that changes depending on hardware state? Returning > > initial_apic_id sounds much simpler. > Well that's what it is now, so I've kept current behavior. > If we decide to change property behavior or drop it altogether > I can do it on top. > I agree to make them static properties as follow-up patch. This way, if the change breaks anything we can revert only that patch. -- Eduardo
Re: [Qemu-devel] [PATCH v3 06/13] pc: apic_common: extend APIC ID property to 32bit
On Tue, 18 Oct 2016 08:56:28 -0200 Eduardo Habkostwrote: > On Thu, Oct 13, 2016 at 11:52:40AM +0200, 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 > > --- > > v3: > >keep original behaviour where 'id' is readonly after > >object is realized (pbonzini) > > --- > [...] > > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > > index 8d01c9c..30f2af0 100644 > > --- a/hw/intc/apic_common.c > > +++ b/hw/intc/apic_common.c > > @@ -428,7 +429,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), > > @@ -437,6 +437,49 @@ 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, , errp); > > +} > > Who exactly is going to read this property and require this logic > to be in the property getter? As it's set/read only from CPU we don't actually have to expose it as property. However, I've kept it as read/write property because it has already been this way and been exposed to external users as some magic property. Not sure is anyone cares. > Do we really need to expose this to the outside as a magic > property that changes depending on hardware state? Returning > initial_apic_id sounds much simpler. Well that's what it is now, so I've kept current behavior. If we decide to change property behavior or drop it altogether I can do it on top. > > > + > > +static void apic_common_set_id(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > +APICCommonState *s = APIC_COMMON(obj); > > +DeviceState *dev = DEVICE(obj); > > +Error *local_err = NULL; > > +int64_t value; > > + > > +if (dev->realized) { > > +qdev_prop_set_after_realize(dev, name, errp); > > +return; > > +} > > + > > +visit_type_int(v, name, , _err); > > +if (local_err) { > > +error_propagate(errp, local_err); > > +return; > > +} > > + > > +s->initial_apic_id = value; > > +s->id = (uint8_t)value; > > Do we really need to change s->id here too? Won't it be set > automatically to initial_apic_id on reset? it will after following patch, [PATCH v3 07/13] pc: apic_common: restore APIC ID to initial ID on reset but I'd prefer to set it here as well to make consistent and clear where it comes from. > I'm asking this because making it read/write only initial_apic_id > would make it easier to eventually convert the property to a > field-based getter/setter API (maybe even keep it using the > static property system). If we don't care about keeping current behavior then I can make it static property like it used to be. > Or, even better: do we really need a writeable property named > "id" at all? Is there any valid use case for the user to set it > directly? if user does it, it likely would make VM unsable, I don't see a way for user to dot it though as he/she is going to see APIC object only in realized state where it's forbidden to change property. > We could make the code that creates the APIC set > apic->initial_apic_id directly (or use a clearer > "initial-apic-id" property name). So we probably fine with making it readonly (still not static but a bit less code in this patch) and set apic_id directly by calling new callback. apic_common::set_apic_id(uint32_t new_apic_id){ this->id = this->initial_apic_id = new_apic_id; } > > +} > > + > > +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); > > If you are going to add new properties, please register them > using object_class_property_add*(). Sure, will fix. > > > +} > > + > > static void apic_common_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -456,6 +499,7 @@ static const TypeInfo apic_common_type = { > > .name = TYPE_APIC_COMMON, > > .parent = TYPE_DEVICE, > > .instance_size = sizeof(APICCommonState), > > +.instance_init = apic_common_initfn, > >
Re: [Qemu-devel] [PATCH v3 06/13] pc: apic_common: extend APIC ID property to 32bit
On Thu, Oct 13, 2016 at 11:52:40AM +0200, 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> --- > v3: >keep original behaviour where 'id' is readonly after >object is realized (pbonzini) > --- [...] > diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c > index 8d01c9c..30f2af0 100644 > --- a/hw/intc/apic_common.c > +++ b/hw/intc/apic_common.c > @@ -428,7 +429,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), > @@ -437,6 +437,49 @@ 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, , errp); > +} Who exactly is going to read this property and require this logic to be in the property getter? Do we really need to expose this to the outside as a magic property that changes depending on hardware state? Returning initial_apic_id sounds much simpler. > + > +static void apic_common_set_id(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > +APICCommonState *s = APIC_COMMON(obj); > +DeviceState *dev = DEVICE(obj); > +Error *local_err = NULL; > +int64_t value; > + > +if (dev->realized) { > +qdev_prop_set_after_realize(dev, name, errp); > +return; > +} > + > +visit_type_int(v, name, , _err); > +if (local_err) { > +error_propagate(errp, local_err); > +return; > +} > + > +s->initial_apic_id = value; > +s->id = (uint8_t)value; Do we really need to change s->id here too? Won't it be set automatically to initial_apic_id on reset? I'm asking this because making it read/write only initial_apic_id would make it easier to eventually convert the property to a field-based getter/setter API (maybe even keep it using the static property system). Or, even better: do we really need a writeable property named "id" at all? Is there any valid use case for the user to set it directly? We could make the code that creates the APIC set apic->initial_apic_id directly (or use a clearer "initial-apic-id" property name). > +} > + > +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); If you are going to add new properties, please register them using object_class_property_add*(). > +} > + > static void apic_common_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -456,6 +499,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 13505ab..b4b4342 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2872,7 +2872,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error > **errp) >OBJECT(cpu->apic_state), _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; > -- > 2.7.4 > -- Eduardo
[Qemu-devel] [PATCH v3 06/13] pc: apic_common: extend APIC ID property to 32bit
ACPI ID is 32 bit wide on CPUs with x2APIC support. Extend 'id' property to support it. Signed-off-by: Igor Mammedov--- v3: keep original behaviour where 'id' is readonly after object is realized (pbonzini) --- include/hw/i386/apic_internal.h | 3 ++- target-i386/cpu.h | 1 + hw/intc/apic_common.c | 46 - target-i386/cpu.c | 2 +- 4 files changed, 49 insertions(+), 3 deletions(-) diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h index cdd11fb..1209eb4 100644 --- a/include/hw/i386/apic_internal.h +++ b/include/hw/i386/apic_internal.h @@ -160,7 +160,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 e645698..6303d65 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 (0xfU<<12) #define MSR_IA32_FEATURE_CONTROL0x003a #define MSR_TSC_ADJUST 0x003b diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c index 8d01c9c..30f2af0 100644 --- a/hw/intc/apic_common.c +++ b/hw/intc/apic_common.c @@ -22,6 +22,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" @@ -428,7 +429,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), @@ -437,6 +437,49 @@ 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, , errp); +} + +static void apic_common_set_id(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ +APICCommonState *s = APIC_COMMON(obj); +DeviceState *dev = DEVICE(obj); +Error *local_err = NULL; +int64_t value; + +if (dev->realized) { +qdev_prop_set_after_realize(dev, name, errp); +return; +} + +visit_type_int(v, name, , _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); @@ -456,6 +499,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 13505ab..b4b4342 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2872,7 +2872,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp) OBJECT(cpu->apic_state), _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; -- 2.7.4