Re: [Xen-devel] [PATCH v4] x86/AMD: Add support for AMD's OSVW feature in guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.02.12 at 17:48, Boris Ostrovsky <boris.ostrovsky@xxxxxxx> wrote:
> On 02/03/12 11:25, Jan Beulich wrote:
>>>>> On 03.02.12 at 17:13, Boris Ostrovsky<boris.ostrovsky@xxxxxxx>  wrote:
>>> On 02/03/12 02:53, Jan Beulich wrote:
>>>>>>> On 02.02.12 at 21:29, Boris Ostrovsky<boris.ostrovsky@xxxxxxx>   wrote:
>>>>> On 02/02/12 08:22, Jan Beulich wrote:
>>>>>> As I was about to apply this to my local tree to give it a try, I had
>>>>>> to realize that the microcode integration is still not correct: There
>>>>>> is (at least from an abstract perspective) no guarantee for
>>>>>> cpu_request_microcode() to be called on all CPUs, yet you want
>>>>>> svm_host_osvw_init() to be re-run on all of them. If you choose
>>>>>> to not deal with this in a formally correct way, it should be stated
>>>>>> so in a code comment (to lower the risk of surprises when someone
>>>>>> touches that code) that this is not possible in practice because
>>>>>> collect_cpu_info() won't currently fail for CPUs of interest.
>>>>>
>>>>> What if svm_host_osvw_init() is called from collect_cpu_info()? There
>>>>> may be cases when svm_host_osvw_init() is called multiple times for the
>>>>> same cpu but that should be harmless (and the routine will be renamed to
>>>>> svm_host_osvw_update()).
>>>>
>>>> Wouldn't that result in workaround bits that might get cleared with
>>>> the pending microcode update to get (and remain) set, as they're
>>>> being or-ed together over all invocations of the function after any
>>>> svm_host_osvw_reset()?
>>>
>>>
>>> I think that would be an OK but not optimal situation: more bits will
>>> end up being set than necessary, meaning that workarounds will need to
>>> be applied where they may not be required. But that should not affect
>>> correctness. I am not sure it's worth optimizing for this case since I
>>> think onlining a core while doing a microcode update is a rather
>>> uncommon occurrence.
>>
>> How is that related to CPU onlining? If you put the call into
>> collect_cpu_info(), that'll affect all CPUs currently online as well
>> (and, as I said, negate the effect of possibly cleared OSVW bits
>> that the pending microcode updated might have). The only case
>> where calling svm_host_osvw_init() from collect_cpu_info() would
>> be correct is if the latter returns an error. Which, as I noted
>> before, can only happen for non-AMD or pre-Fam10 CPUs (on
>> all of which calling the function is pointless). So we're back to me
>> asking that this simply be explained in a code comment in lieu of
>> a proper abstraction of the situation.
> 
> 
> OK, I misunderstood your original comment then. I thought you were 
> talking about a race between microcode update and onlining a core.

That was a second concern, easily taken care of by adding locking
in _init() and _reset().

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel