Re: [Qemu-devel] [PATCH] target-i386: Initialize CPUState::halted in cpu_reset

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-04-27 19:17, Blue Swirl wrote:
> On Wed, Apr 27, 2011 at 10:11 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-04-26 21:59, Blue Swirl wrote:
>>> On Tue, Apr 26, 2011 at 9:55 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> On 2011-04-26 20:00, Blue Swirl wrote:
>>>>> On Tue, Apr 26, 2011 at 11:50 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>> Instead of having an extra reset function at machine level and special
>>>>>> code for processing INIT, move the initialization of halted into the
>>>>>> cpu reset handler.
>>>>>
>>>>> Nack. A CPU is designated as a BSP at board level. CPUs do not need to
>>>>> know about this at all.
>>>>
>>>> That's why we have cpu_is_bsp() in pc.c.
>>>>
>>>> Obviously, every CPU (which includes the APIC) must know if it is
>>>> supposed to be BP or AP. It would be unable to enter the right state
>>>> after reset otherwise (e.g. Intel SDM 9.1). cpu_is_bsp() is basically
>>>> reporting the result of the MP init protocol in condensed from.
>>>
>>> Intel 64 and IA-32 Architectures Software Developer’s Manual vol 3A,
>>> 7.5.1 says that the protocol result is stored in APIC MSR. I think we
>>> should be using that instead. For example, the board could call
>>> cpu_designate_bsp() to set the BSP flag in MSR. Then cpu_is_bsp()
>>> would only check the MSR, which naturally belongs to the CPU/APIC
>>> domain.
>>
>> Something like this? The original patch has to be rebased on top.
> 
> How about not deleting cpu_is_bsp() but moving it to apic.c, as a
> check for the BSP flag? That would simplify the patches a bit.

Maybe as an inline helper.

But all this apic cpu_* helpers are not really beautiful. Logically,
they should take a CPUState, not an APIC. Or they should be called apic_*.

> 
>> I'm still struggling how to deal with apicbase long-term. I doesn't
>> belong to the APIC, but it's saved/restored there. Guess we should move
>> it to the CPU's vmstate. OTOH, changing vmstates only for the sake of
>> minor refactorings is also not very attractive.
> 
> CPU should be the correct place. You could wait until the vmstate is
> changed anyway, or be the first.

Changing is not a big issue. But we will only add code this way,
unfortunately not just move it around: we will still have to load and
sync the apicbase for older versions.

> 
>>
>> Jan
>>
>> ---
>>  hw/apic.c            |   18 +++++++++++++-----
>>  hw/apic.h            |    2 +-
>>  hw/pc.c              |   14 +++++++-------
>>  target-i386/helper.c |    3 ++-
>>  target-i386/kvm.c    |    5 +++--
>>  5 files changed, 26 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 9febf40..31ac6cd 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -318,7 +318,7 @@ uint64_t cpu_get_apic_base(DeviceState *d)
>>
>>     trace_cpu_get_apic_base(s ? (uint64_t)s->apicbase: 0);
>>
>> -    return s ? s->apicbase : 0;
>> +    return s ? s->apicbase : MSR_IA32_APICBASE_BSP;
> 
> This does not look OK.

Required for APIC-less mode (otherwise there would be no BSP).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux