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

[Blue Swirl]

On Wed, Apr 27, 2011 at 8:32 PM, Jan Kiszka <address@hidden> wrote:
> On 2011-04-27 19:17, Blue Swirl wrote:
>> On Wed, Apr 27, 2011 at 10:11 AM, Jan Kiszka <address@hidden> wrote:
>>> On 2011-04-26 21:59, Blue Swirl wrote:
>>>> On Tue, Apr 26, 2011 at 9:55 PM, Jan Kiszka <address@hidden> wrote:
>>>>> On 2011-04-26 20:00, Blue Swirl wrote:
>>>>>> On Tue, Apr 26, 2011 at 11:50 AM, Jan Kiszka <address@hidden> 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_*.

Well, cpu_{s,g}et_apic_base() are in the wrong place.

TPR is shared between CR8 and APIC, currently it is handled by APIC so
the functions could be renamed.

>>
>>> 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.

Perhaps we could start deprecating old versions. For example, v0.16
could deprecate v0.14 or older and v0.17 drop support for them and
deprecate v0.15.

>
>>
>>>
>>> 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).

Perhaps the check should be moved to CPU level then.