[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 01/18/12 04:50, Jan Beulich wrote:
On 17.01.12 at 18:54, Boris Ostrovsky<boris.ostrovsky@xxxxxxx>  wrote:
On 01/17/12 03:26, Jan Beulich wrote:
On 16.01.12 at 22:11, Boris Ostrovsky<boris.ostrovsky@xxxxxxx>   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) |


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


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.