Re: [Xen-devel] [PATCH v2 16/25] x86/pv: Use per-domain policy information in pv_cpuid()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/01/17 15:50, Boris Ostrovsky wrote:
> On 01/12/2017 10:31 AM, Andrew Cooper wrote:
>> On 12/01/17 15:22, Boris Ostrovsky wrote:
>>>>      case 0x80000001:
>>>> -        c &= pv_featureset[FEATURESET_e1c];
>>>> -        d &= pv_featureset[FEATURESET_e1d];
>>>> +        c = p->extd.e1c;
>>> This appears to crash guests Intel, at least for dom0.
>> Is this a PVH dom0?  I can't see from this snippet which function you
>> are in.
> No, this is normal PV dom0.
>
> I may have gone too far trimming the patch. It's this chunk:
>
>
> @@ -1291,15 +1281,15 @@ void pv_cpuid(struct cpu_user_regs *regs)
>          }
>  
>          case 1:
> -            a &= pv_featureset[FEATURESET_Da1];
> +            a = p->xstate.Da1;
>              b = c = d = 0;
>              break;
>          }
>          break;
>  
>      case 0x80000001:
> -        c &= pv_featureset[FEATURESET_e1c];
> -        d &= pv_featureset[FEATURESET_e1d];
> +        c = p->extd.e1c;
> +        d = p->extd.e1d;
>
>
>>> p->extd.e1c is 0x3 and bit 1 is reserved on Intel.
>>> I haven't traced it yet to exact place that causes dom0 to crash but
>>> clearing this bit make dom0 boot.
>> The logic immediately below the snippet should clean out the common bits
>> if vendor != AMD.  Do we perhaps have a bad vendor setting?
>>
> -bash-4.1# ./cpuid 0
> CPUID 0x00000000: eax = 0x0000000d ebx = 0x756e6547 ecx = 0x6c65746e edx
> = 0x49656e69
> -bash-4.1#
>
> This is machine that I run my nightly tests on and it failed this
> morning so it's not a new HW.
>
> As far as adjusting the bits based on vendor --- don't you only do this
> for edx:
>
> arch/x86/cpuid.c: pv_cpuid():
>
>    case 0x80000001:
>         res->c = p->extd.e1c;
>         res->c &= ~2U; // My workaround
>         res->d = p->extd.e1d;
>
>         /* If not emulating AMD, clear the duplicated features in e1d. */
>         if ( currd->arch.x86_vendor != X86_VENDOR_AMD )
>             res->d &= ~CPUID_COMMON_1D_FEATURES;

Ahh! found it.  This is a side effect of starting to generate the dom0
policy in Xen.

Can you try this patch?

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index b685874..1e5013d 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -164,14 +164,6 @@ static void __init calculate_pv_max_policy(void)
     /* Unconditionally claim to be able to set the hypervisor bit. */
     __set_bit(X86_FEATURE_HYPERVISOR, pv_featureset);
 
-    /*
-     * Allow the toolstack to set HTT, X2APIC and CMP_LEGACY.  These bits
-     * affect how to interpret topology information in other cpuid leaves.
-     */
-    __set_bit(X86_FEATURE_HTT, pv_featureset);
-    __set_bit(X86_FEATURE_X2APIC, pv_featureset);
-    __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset);
-
     sanitise_featureset(pv_featureset);
     cpuid_featureset_to_policy(pv_featureset, p);
 }
@@ -199,14 +191,6 @@ static void __init calculate_hvm_max_policy(void)
     __set_bit(X86_FEATURE_HYPERVISOR, hvm_featureset);
 
     /*
-     * Allow the toolstack to set HTT, X2APIC and CMP_LEGACY.  These bits
-     * affect how to interpret topology information in other cpuid leaves.
-     */
-    __set_bit(X86_FEATURE_HTT, hvm_featureset);
-    __set_bit(X86_FEATURE_X2APIC, hvm_featureset);
-    __set_bit(X86_FEATURE_CMP_LEGACY, hvm_featureset);
-
-    /*
      * Xen can provide an APIC emulation to HVM guests even if the
host's APIC
      * isn't enabled.
      */
@@ -301,6 +285,14 @@ void recalculate_cpuid_policy(struct domain *d)
     }
 
     /*
+     * Allow the toolstack to set HTT, X2APIC and CMP_LEGACY.  These bits
+     * affect how to interpret topology information in other cpuid leaves.
+     */
+    __set_bit(X86_FEATURE_HTT, max_fs);
+    __set_bit(X86_FEATURE_X2APIC, max_fs);
+    __set_bit(X86_FEATURE_CMP_LEGACY, max_fs);
+
+    /*
      * 32bit PV domains can't use any Long Mode features, and cannot use
      * SYSCALL on non-AMD hardware.
      */


The toolstack fudge is still necessary for PV guests (where faulting
isn't in use), and still necessary for HVM guests until I fix topology
representation, but we shouldn't be exposing them by default on hardware
which lacks the appropriate bits.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel