qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH qom-next 07/12] target-i386: move cpu halted dec


From: Andreas Färber
Subject: Re: [Qemu-devel] [PATCH qom-next 07/12] target-i386: move cpu halted decision into x86_cpu_reset
Date: Wed, 30 May 2012 16:54:27 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120421 Thunderbird/12.0

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<address@hidden>
>>>
>>> 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<address@hidden>
>>>
>>> Signed-off-by: Igor Mammedov<address@hidden>
>>> ---
>>>   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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]