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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.01.12 at 19:26, Boris Ostrovsky <boris.ostrovsky@xxxxxxx> wrote:
> On 01/18/12 04:50, Jan Beulich wrote:
>>>>> On 17.01.12 at 18:54, Boris Ostrovsky<boris.ostrovsky@xxxxxxx>  wrote:
>>> I believe this (i.e. OSVW changes) is meaningful for PV. Taking erratum
>>> 400 as an example -- we don't need a Linux PV guest reading an MSR
>>> before going to idle (in amd_e400_idle()).
>>
>> It is bogus in the first place if a pv guest does so - after all, not doing
>> stuff like this is the nature of being pv.
> 
> It was actually a bad example --- the guest is not using amd_e400_idle() 
> and so there are no extra MSR accesses.
> 
> However, without this change OSVW bit will not show up in the guest's 
> CPUID and I think guests should be able to see it. One could argue 
> whether or not we should mask off status bits for the two errata (400 
> and 415) since they are not currently used; I'd mask them off just to be 
> consistent with HVM, it won't affect anything.

I continue to think otherwise - knowing of and dealing with this is
supposed to be entirely hidden from PV guests, unless and until
you can provide a counter example. Therefore I am likely to nak
this part of future revisions of the patch (which Keir could
certainly override), up to and including ripping out the PV part (and
adjusting the rest accordingly) if I would go for committing it.

>>>>> +    if (osvw_length == 0&&   boot_cpu_data.x86 == 0x10)
>>>>
>>>> Why do you check the family here? If osvw_length can only ever be
>>>> zero on Fam10, then the first check is sufficient. If osvw_length can
>>>> be zero on other than Fam10 (no matter whether we bail early older
>>>> CPUs), then the check is actually wrong.
>>>
>>> 10h is the only family affected by this erratum.
>>
>> What is "this erratum" here? My comment was to suggest that either
>> of the two conditions can likely be eliminated for being redundant.
> 
> ("This erratum" is erratum 298, which is bit 0)
> 
> If osvw_length!=0 then we don't need to do anything since bit 0 is 
> already valid.
> 
> If osvw_length==0 then -- since we just made the guest's version of this 
> register 3 and thus turned bit 0 from being invalid to valid -- we need 
> to do something about bit 0. But the bit can only be set on family 10h, 
> thus both checks.

Okay, got you. However, the whole thing will become superfluous
anyway if you bail on family less than 0x10 right at the start of the
function.

Btw., one more comment on the change to init_amd(): You will likely
need to distinguish BP and AP cases here - on the BP you want to
plainly write the values read from the MSRs to the global variables,
but on APs (with possibly different settings) you need to work
towards a setting of the global variables that apply to all of the
CPUs. This is not just for the (theoretical only?) hotplug case, but
particularly the one of a soft-offlined CPU that had its microcode
updated already in a way affecting the OSVW bits and is
subsequently being brought back online.

Which reminds you and me that the patch is missing integration with
the microcode update loader (as that one may alter the OSVW bits
iiuc).

Jan


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