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

[Keir Fraser <keir.xen@gmail.com>]

On 19/01/2012 08:10, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 18.01.12 at 19:26, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
>> On 01/18/12 04:50, Jan Beulich wrote:
>>>>>> On 17.01.12 at 18:54, Boris Ostrovsky<boris.ostrovsky@amd.com>  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.

Well, the general principle of exposing OSVW to PV guests doesn't seem
terrible. It's just the current specific motivation for exposing to HVM
guests does not apply to PV guests.

Are *any* of the current OSVW defined bits at all useful or applicable to PV
guests?

 -- Keir

>>>>>> +    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@lists.xensource.com
> http://lists.xensource.com/xen-devel