[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target-i386: Initialize CPUState::halted in cpu
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH] target-i386: Initialize CPUState::halted in cpu_reset |
Date: |
Wed, 27 Apr 2011 19:32:35 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
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_*.
>
>> 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