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

[Glauber Costa <glommer@gmail.com>]

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

I'm not sure about what the manual says (will check now), but I
believe if we ever try to
read from apic, we should return a meaningful value. Can you verify if
this also works for your
test case?

>
>>
>>  void cpu_set_apic_tpr(CPUX86State *env, uint8_t val)
>> @@ -314,7 +319,10 @@ void cpu_set_apic_tpr(CPUX86State *env, uint8_t val)
>>  uint8_t cpu_get_apic_tpr(CPUX86State *env)
>>  {
>>      APICState *s = env->apic_state;
>> -    return s->tpr >> 4;
>> +    if (s)
>> +        return s->tpr >> 4;
>> +    else
>> +        return 0;
>>  }
>>
>>  /* return -1 if no bit is set */
>
> This is already upstream.

Yeah, and the rest of your patch is totally ok for me.


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."