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

[Blue Swirl]

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.

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

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