Am 30.05.2012 16:13, schrieb Igor Mammedov:
> On 05/30/2012 02:11 PM, Andreas Färber wrote:
>> Am 30.05.2012 00:10, schrieb Igor Mammedov:
>>> From: Igor Mammedov<niall...@gmail.com>
>>>
>>> MP initialization protocol differs between cpu families, and for P6 and
>>> onward models it is up to CPU to decide if it will be BSP using this
>>> protocol, so try to model this. However there is no point in
>>> implementing
>>> MP initialization protocol in qemu. Thus first CPU is always marked
>>> as BSP.
>>>
>>> This patch:
>>> - moves decision to designate BSP from board into cpu, making cpu
>>> self-sufficient in this regard. Later it will allow to cleanup hw/pc.c
>>> and remove cpu_reset and wrappers from there.
>>> - stores flag that CPU is BSP in IA32_APIC_BASE to model behavior
>>> described in Inted SDM vol 3a part 1 chapter 8.4.1
>>> - uses MSR_IA32_APICBASE_BSP flag in apic_base for checking if cpu
>>> is BSP
>>>
>>> patch is based on Jan Kiszka's proposal:
>>> http://thread.gmane.org/gmane.comp.emulators.qemu/100806
>>>
>>> v2:
>>> - fix build for i386-linux-user
>>> spotted-by: Peter Maydell<peter.mayd...@linaro.org>
>>>
>>> Signed-off-by: Igor Mammedov<imamm...@redhat.com>
>>> ---
>>> hw/apic.h | 2 +-
>>> hw/apic_common.c | 18 ++++++++++++------
>>> hw/pc.c | 9 ---------
>>> target-i386/cpu.c | 9 +++++++++
>>> target-i386/helper.c | 1 -
>>> target-i386/kvm.c | 5 +++--
>>> 6 files changed, 25 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/hw/apic.h b/hw/apic.h
>>> index 62179ce..d961ed4 100644
>>> --- a/hw/apic.h
>>> +++ b/hw/apic.h
>>> @@ -20,9 +20,9 @@ void apic_init_reset(DeviceState *s);
>>> void apic_sipi(DeviceState *s);
>>> void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
>>> TPRAccess access);
>>> +void apic_designate_bsp(DeviceState *d);
>>>
>>> /* pc.c */
>>> -int cpu_is_bsp(CPUX86State *env);
>>> DeviceState *cpu_get_current_apic(void);
>>>
>>> #endif
>>> diff --git a/hw/apic_common.c b/hw/apic_common.c
>>> index 46a9ff7..98ad6f0 100644
>>> --- a/hw/apic_common.c
>>> +++ b/hw/apic_common.c
>>> @@ -43,8 +43,8 @@ uint64_t cpu_get_apic_base(DeviceState *d)
>>> trace_cpu_get_apic_base((uint64_t)s->apicbase);
>>> return s->apicbase;
>>> } else {
>>> - trace_cpu_get_apic_base(0);
>>> - return 0;
>>> + trace_cpu_get_apic_base(MSR_IA32_APICBASE_BSP);
>>> + return MSR_IA32_APICBASE_BSP;
>>> }
>>> }
>>>
>>> @@ -201,22 +201,28 @@ void apic_init_reset(DeviceState *d)
>>> s->timer_expiry = -1;
>>> }
>>>
>>> +void apic_designate_bsp(DeviceState *d)
>>> +{
>>> + if (d) {
>>
>> This check looks odd. The function call is already guarded with
>> cpu_index == 0. What other case would lead to d == NULL and can't we
>> check that at the call site?
>
> cpu_index == 0 doesn't imply that cpu has apic hence the check. And we
> for sure
> can check it call site, would you like to do it there?
> PS:
> there are many checks for this condition in APIC code, so may be it should
> be there style-wise at least?
What I referred to as odd was the indentation of the actual implementation.
So I'd be fine with either checking at the call site or
if (d == NULL) {
return;
}
...
>>> + APICCommonState *s = APIC_COMMON(d);
>>
>> If this is a QOM cast macro, it will implicitly assert d != NULL, no?
>
> it actually will lead to null pointer dereference, like this:
> OBJECT_CHECK(type, obj, name) \
> ((type *)object_dynamic_cast_assert(OBJECT(obj), (name)))
> => object_dynamic_cast(obj, typename)
> => object_is_type(obj, target_type)
> => type_is_ancestor(obj->class->type, target_type);
> ^^^
Mind to send a patch fixing that? :-)
I'd suppose the _is_... functions should probably return false and the
cast should assert. Anthony?
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
