Re: [PATCH] return default values for apic probe functions.

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

Glauber Costa wrote:
> On Fri, Apr 17, 2009 at 10:22 AM, Glauber Costa <glommer@gmail.com> wrote:
>>> Even on sunny days, this collides with QEMU commit #7048. :)
>>>
>>> Does Intel specify what non-existent MSRs should return, ie. is your
>>> version still correct if !s->apicbase means that there is actually no
>>> APIC? And does kvm depend on the default base? If so, I would say:
>>> provide a patch against upstream.
>> hummm, I missed this one going in.
>> But sadly, your patch also breaks cpu hotplug. Not a segfault anymore, but the
>> VM will freeze instead of shutting down, if we ask too. It does not even respond
>> to ^C anymore.
>>
>> By leaving your patch as is, and changing the apic base return to
>>
>>   return s ? s->apicbase : (0xfee00000 | MSR_IA32_APICBASE_ENABLE);
>>
>> fixes the issue.
>>
> After reading the manual, my understanding is that only the flag must
> be set. I tried,
> and in fact:
> 
>    return s ? s->apicbase :  MSR_IA32_APICBASE_ENABLE;
> 
> still fixes it.
> If it works for you, I believe this is a good solution, and will send
> a descriptive patch.
> If we ever read the apic as disabled, we will have problems enabling
> it again. So for
> my test case, kvm should never see a disabled apic.
> 
> For yours, you'll still see the apic base address as zero.
> 
> what do you think?
> 

My use case (you can try it yourself: -M isapc) will likely already be
fine if there is no segv on accidentally reading that msr. I was more
concerned about the correct value according to the spec - if that case
isn't simply "undefined".

On the other hand, I didn't get the precise race yet, and my feeling
about this patch as a fix for something else than an inexistent apic is
not that good. But it's just a feeling.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux