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

[Boris Ostrovsky <boris.ostrovsky@amd.com>]

On 01/18/12 04:50, Jan Beulich wrote:
>>>> On 17.01.12 at 18:54, Boris Ostrovsky<boris.ostrovsky@amd.com>  wrote:
>> On 01/17/12 03:26, Jan Beulich wrote:
>>>>>> On 16.01.12 at 22:11, Boris Ostrovsky<boris.ostrovsky@amd.com>   wrote:
>>>> --- a/tools/libxc/xc_cpuid_x86.c	Mon Jan 09 16:01:44 2012 +0100
>>>> +++ b/tools/libxc/xc_cpuid_x86.c	Mon Jan 16 22:08:09 2012 +0100
>>>> @@ -108,6 +108,7 @@ static void amd_xc_cpuid_policy(
>>>>                        bitmaskof(X86_FEATURE_SSE4A) |
>>>>                        bitmaskof(X86_FEATURE_MISALIGNSSE) |
>>>>                        bitmaskof(X86_FEATURE_3DNOWPREFETCH) |
>>>> +		    bitmaskof(X86_FEATURE_OSVW) |
>>>
>>> Indentation.
>>>
>>> Also, is this really meaningful to PV guests?
>>
>> Does amd_xc_cpuid_policy() get called for PV guests? I thought this is
>> HVM path only.
>
> In that case I simply misplaced the comment.
>
>> xc_cpuid_pv_policy() is where clear_bit(X86_FEATURE_OSVW, regs[2]) was
>> removed to handle PV.
>>
>> 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.

>>>> +    vcpu->arch.amd.osvw.length = (osvw_length >= 3) ? (osvw_length) : 3
>>>> +    vcpu->arch.amd.osvw.status = osvw_status&   ~(6ULL);
>>>> +
>>>> +    /*
>>>> +     * By increasing VCPU's osvw.length to 3 we are telling the guest that
>>>> +     * all osvw.status bits inside that length, including bit 0 (which is
>>>> +     * reserved for erratum 298), are valid. However, if host processor's
>>>> +     * osvw_len is 0 then osvw_status[0] carries no information. We need to
>>>> +     * be conservative here and therefore we tell the guest that erratum
>> 298
>>>> +     * is present (because we really don't know).
>>>> +     */
>>>> +    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.


Thanks.
-boris