[PATCH v4] x86/HVM: make hvm_efer_valid() honor guest features

[PATCH v4] x86/HVM: make hvm_efer_valid() honor guest features

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4532 bytes --]

Following the earlier similar change validating CR4 modifications.

Note that this requires a non-obvious adjustment to hvm_cpuid(): On
32-bit hosts/tool stacks, the SYSCALL feature flag will be seen clear
on Intel CPUs, yet 64-bit guests legitimately expect the feature for
be available. Mimic hardware behavior: When executed from a 64-bit
code segment, force the feature flag set.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Drop hvm_cpuid() adjustment and use hvm_funcs.cpuid_intercept()
    instead for leaf 0x80000001.
v3: Drop cr0_pg > 0 test for LMA/LME check: This would need to be >= 0,
    which is then redundant with the check for EFER_LMA (getting
    cleared when cr0_pg gets passed a negative value). Force SYSCALL
    feature flag on when guest is in 64-bit mode.
v2: consider CR0.PG during restore when checking EFER.LMA

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1672,20 +1672,58 @@ static int hvm_save_cpu_ctxt(struct doma
     return 0;
 }
 
-static bool_t hvm_efer_valid(struct domain *d,
-                             uint64_t value, uint64_t efer_validbits)
+static bool_t hvm_efer_valid(const struct vcpu *v, uint64_t value,
+                             signed int cr0_pg)
 {
-    if ( nestedhvm_enabled(d) && cpu_has_svm )
-        efer_validbits |= EFER_SVME;
+    unsigned int ext1_ecx = 0, ext1_edx = 0;
 
-    return !((value & ~efer_validbits) ||
-             ((sizeof(long) != 8) && (value & EFER_LME)) ||
-             (!cpu_has_svm && (value & EFER_SVME)) ||
-             (!cpu_has_nx && (value & EFER_NX)) ||
-             (!cpu_has_syscall && (value & EFER_SCE)) ||
-             (!cpu_has_lmsl && (value & EFER_LMSLE)) ||
-             (!cpu_has_ffxsr && (value & EFER_FFXSE)) ||
-             ((value & (EFER_LME|EFER_LMA)) == EFER_LMA));
+    if ( cr0_pg < 0 && !is_hardware_domain(v->domain) )
+    {
+        unsigned int level;
+
+        ASSERT(v == current);
+        hvm_cpuid(0x80000000, &level, NULL, NULL, NULL);
+        if ( level >= 0x80000001 )
+        {
+            unsigned int dummy;
+
+            level = 0x80000001;
+            hvm_funcs.cpuid_intercept(&level, &dummy, &ext1_ecx, &ext1_edx);
+        }
+    }
+    else
+    {
+        ext1_edx = boot_cpu_data.x86_capability[X86_FEATURE_LM / 32];
+        ext1_ecx = boot_cpu_data.x86_capability[X86_FEATURE_SVM / 32];
+    }
+
+    if ( (value & EFER_SCE) &&
+         !(ext1_edx & cpufeat_mask(X86_FEATURE_SYSCALL)) )
+        return 0;
+
+    if ( (value & (EFER_LME | EFER_LMA)) &&
+         !(ext1_edx & cpufeat_mask(X86_FEATURE_LM)) )
+        return 0;
+
+    if ( (value & EFER_LMA) && (!(value & EFER_LME) || !cr0_pg) )
+        return 0;
+
+    if ( (value & EFER_NX) && !(ext1_edx & cpufeat_mask(X86_FEATURE_NX)) )
+        return 0;
+
+    if ( (value & EFER_SVME) &&
+         (!(ext1_ecx & cpufeat_mask(X86_FEATURE_SVM)) ||
+          !nestedhvm_enabled(v->domain)) )
+        return 0;
+
+    if ( (value & EFER_LMSLE) && !cpu_has_lmsl )
+        return 0;
+
+    if ( (value & EFER_FFXSE) &&
+         !(ext1_edx & cpufeat_mask(X86_FEATURE_FFXSR)) )
+        return 0;
+
+    return 1;
 }
 
 /* These reserved bits in lower 32 remain 0 after any load of CR0 */
@@ -1763,7 +1801,6 @@ static int hvm_load_cpu_ctxt(struct doma
     struct vcpu *v;
     struct hvm_hw_cpu ctxt;
     struct segment_register seg;
-    uint64_t efer_validbits;
 
     /* Which vcpu is this? */
     vcpuid = hvm_load_instance(h);
@@ -1794,9 +1831,7 @@ static int hvm_load_cpu_ctxt(struct doma
         return -EINVAL;
     }
 
-    efer_validbits = EFER_FFXSE | EFER_LMSLE | EFER_LME | EFER_LMA
-                   | EFER_NX | EFER_SCE;
-    if ( !hvm_efer_valid(d, ctxt.msr_efer, efer_validbits) )
+    if ( !hvm_efer_valid(v, ctxt.msr_efer, MASK_EXTR(ctxt.cr0, X86_CR0_PG)) )
     {
         printk(XENLOG_G_ERR "HVM%d restore: bad EFER %#" PRIx64 "\n",
                d->domain_id, ctxt.msr_efer);
@@ -2936,12 +2971,10 @@ err:
 int hvm_set_efer(uint64_t value)
 {
     struct vcpu *v = current;
-    uint64_t efer_validbits;
 
     value &= ~EFER_LMA;
 
-    efer_validbits = EFER_FFXSE | EFER_LMSLE | EFER_LME | EFER_NX | EFER_SCE;
-    if ( !hvm_efer_valid(v->domain, value, efer_validbits) )
+    if ( !hvm_efer_valid(v, value, -1) )
     {
         gdprintk(XENLOG_WARNING, "Trying to set reserved bit in "
                  "EFER: %#"PRIx64"\n", value);



[-- Attachment #2: x86-HVM-refine-EFER-reserved-bits-checks.patch --]
[-- Type: text/plain, Size: 4583 bytes --]

x86/HVM: make hvm_efer_valid() honor guest features

Following the earlier similar change validating CR4 modifications.

Note that this requires a non-obvious adjustment to hvm_cpuid(): On
32-bit hosts/tool stacks, the SYSCALL feature flag will be seen clear
on Intel CPUs, yet 64-bit guests legitimately expect the feature for
be available. Mimic hardware behavior: When executed from a 64-bit
code segment, force the feature flag set.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Drop hvm_cpuid() adjustment and use hvm_funcs.cpuid_intercept()
    instead for leaf 0x80000001.
v3: Drop cr0_pg > 0 test for LMA/LME check: This would need to be >= 0,
    which is then redundant with the check for EFER_LMA (getting
    cleared when cr0_pg gets passed a negative value). Force SYSCALL
    feature flag on when guest is in 64-bit mode.
v2: consider CR0.PG during restore when checking EFER.LMA

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1672,20 +1672,58 @@ static int hvm_save_cpu_ctxt(struct doma
     return 0;
 }
 
-static bool_t hvm_efer_valid(struct domain *d,
-                             uint64_t value, uint64_t efer_validbits)
+static bool_t hvm_efer_valid(const struct vcpu *v, uint64_t value,
+                             signed int cr0_pg)
 {
-    if ( nestedhvm_enabled(d) && cpu_has_svm )
-        efer_validbits |= EFER_SVME;
+    unsigned int ext1_ecx = 0, ext1_edx = 0;
 
-    return !((value & ~efer_validbits) ||
-             ((sizeof(long) != 8) && (value & EFER_LME)) ||
-             (!cpu_has_svm && (value & EFER_SVME)) ||
-             (!cpu_has_nx && (value & EFER_NX)) ||
-             (!cpu_has_syscall && (value & EFER_SCE)) ||
-             (!cpu_has_lmsl && (value & EFER_LMSLE)) ||
-             (!cpu_has_ffxsr && (value & EFER_FFXSE)) ||
-             ((value & (EFER_LME|EFER_LMA)) == EFER_LMA));
+    if ( cr0_pg < 0 && !is_hardware_domain(v->domain) )
+    {
+        unsigned int level;
+
+        ASSERT(v == current);
+        hvm_cpuid(0x80000000, &level, NULL, NULL, NULL);
+        if ( level >= 0x80000001 )
+        {
+            unsigned int dummy;
+
+            level = 0x80000001;
+            hvm_funcs.cpuid_intercept(&level, &dummy, &ext1_ecx, &ext1_edx);
+        }
+    }
+    else
+    {
+        ext1_edx = boot_cpu_data.x86_capability[X86_FEATURE_LM / 32];
+        ext1_ecx = boot_cpu_data.x86_capability[X86_FEATURE_SVM / 32];
+    }
+
+    if ( (value & EFER_SCE) &&
+         !(ext1_edx & cpufeat_mask(X86_FEATURE_SYSCALL)) )
+        return 0;
+
+    if ( (value & (EFER_LME | EFER_LMA)) &&
+         !(ext1_edx & cpufeat_mask(X86_FEATURE_LM)) )
+        return 0;
+
+    if ( (value & EFER_LMA) && (!(value & EFER_LME) || !cr0_pg) )
+        return 0;
+
+    if ( (value & EFER_NX) && !(ext1_edx & cpufeat_mask(X86_FEATURE_NX)) )
+        return 0;
+
+    if ( (value & EFER_SVME) &&
+         (!(ext1_ecx & cpufeat_mask(X86_FEATURE_SVM)) ||
+          !nestedhvm_enabled(v->domain)) )
+        return 0;
+
+    if ( (value & EFER_LMSLE) && !cpu_has_lmsl )
+        return 0;
+
+    if ( (value & EFER_FFXSE) &&
+         !(ext1_edx & cpufeat_mask(X86_FEATURE_FFXSR)) )
+        return 0;
+
+    return 1;
 }
 
 /* These reserved bits in lower 32 remain 0 after any load of CR0 */
@@ -1763,7 +1801,6 @@ static int hvm_load_cpu_ctxt(struct doma
     struct vcpu *v;
     struct hvm_hw_cpu ctxt;
     struct segment_register seg;
-    uint64_t efer_validbits;
 
     /* Which vcpu is this? */
     vcpuid = hvm_load_instance(h);
@@ -1794,9 +1831,7 @@ static int hvm_load_cpu_ctxt(struct doma
         return -EINVAL;
     }
 
-    efer_validbits = EFER_FFXSE | EFER_LMSLE | EFER_LME | EFER_LMA
-                   | EFER_NX | EFER_SCE;
-    if ( !hvm_efer_valid(d, ctxt.msr_efer, efer_validbits) )
+    if ( !hvm_efer_valid(v, ctxt.msr_efer, MASK_EXTR(ctxt.cr0, X86_CR0_PG)) )
     {
         printk(XENLOG_G_ERR "HVM%d restore: bad EFER %#" PRIx64 "\n",
                d->domain_id, ctxt.msr_efer);
@@ -2936,12 +2971,10 @@ err:
 int hvm_set_efer(uint64_t value)
 {
     struct vcpu *v = current;
-    uint64_t efer_validbits;
 
     value &= ~EFER_LMA;
 
-    efer_validbits = EFER_FFXSE | EFER_LMSLE | EFER_LME | EFER_NX | EFER_SCE;
-    if ( !hvm_efer_valid(v->domain, value, efer_validbits) )
+    if ( !hvm_efer_valid(v, value, -1) )
     {
         gdprintk(XENLOG_WARNING, "Trying to set reserved bit in "
                  "EFER: %#"PRIx64"\n", value);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v4] x86/HVM: make hvm_efer_valid() honor guest features

[Andrew Cooper]

On 19/01/15 15:12, Jan Beulich wrote:
> Following the earlier similar change validating CR4 modifications.
>
> Note that this requires a non-obvious adjustment to hvm_cpuid(): On
> 32-bit hosts/tool stacks, the SYSCALL feature flag will be seen clear
> on Intel CPUs, yet 64-bit guests legitimately expect the feature for
> be available. Mimic hardware behavior: When executed from a 64-bit
> code segment, force the feature flag set.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I finally have the debugging out of XenRT as to why this is still
failing for windows (including my improvements to the error message
which I will post in due course).

d58v1 Invalid EFER update: 0 -> 0x901 - SCE without feature

When windows brings up its second cpu, it appears to set up EFER
completely in one go, which means it will still be in protected mode at
this point.  It sets SCE in the knowledge that the cpu is 64bit, and
this indicates that it is valid to set EFER.SCE on 64-bit Intel cpus
even the feature would not appear via cpuid.

Undoing the v3 -> v4 change wrt hvm_cpuid() fixes the issue, and I rerun
the test suite like this.

I suggest that the easiest course of action is to simply ignore this
Intelism wrt EFER.SCE, and add it to the big bucket of edge cases that
we can't quite virtualise correctly.

On the other hand, I wonder whether it is permitted because LME is set
at the same time?  It might be that SCE is permitted if and only if LME
has been verified as ok.

~Andrew

Re: [PATCH v4] x86/HVM: make hvm_efer_valid() honor guest features

[Jan Beulich]

>>> On 21.01.15 at 21:08, <andrew.cooper3@citrix.com> wrote:
> On 19/01/15 15:12, Jan Beulich wrote:
>> Following the earlier similar change validating CR4 modifications.
>>
>> Note that this requires a non-obvious adjustment to hvm_cpuid(): On
>> 32-bit hosts/tool stacks, the SYSCALL feature flag will be seen clear
>> on Intel CPUs, yet 64-bit guests legitimately expect the feature for
>> be available. Mimic hardware behavior: When executed from a 64-bit
>> code segment, force the feature flag set.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I finally have the debugging out of XenRT as to why this is still
> failing for windows (including my improvements to the error message
> which I will post in due course).
> 
> d58v1 Invalid EFER update: 0 -> 0x901 - SCE without feature
> 
> When windows brings up its second cpu, it appears to set up EFER
> completely in one go, which means it will still be in protected mode at
> this point.  It sets SCE in the knowledge that the cpu is 64bit, and
> this indicates that it is valid to set EFER.SCE on 64-bit Intel cpus
> even the feature would not appear via cpuid.
> 
> Undoing the v3 -> v4 change wrt hvm_cpuid() fixes the issue, and I rerun
> the test suite like this.

Now that's odd - v4 only changes _where_ the SYSCALL feature
flag gets set, not the dependency on CS.L. Or are you saying
that this vCPU hotplug issue is independent of the 32-bit tool
stack one that v3 fixed, and hence it is a problem that the VMX
code actively clears the feature flag?

> I suggest that the easiest course of action is to simply ignore this
> Intelism wrt EFER.SCE, and add it to the big bucket of edge cases that
> we can't quite virtualise correctly.
> 
> On the other hand, I wonder whether it is permitted because LME is set
> at the same time?  It might be that SCE is permitted if and only if LME
> has been verified as ok.

And I think I'll go that route - permit SCE when the feature flag is
set, or would be set after entering long mode when enabling long
mode at the same time.

Jan

Re: [PATCH v4] x86/HVM: make hvm_efer_valid() honor guest features

[Andrew Cooper]

On 22/01/15 09:02, Jan Beulich wrote:
>>>> On 21.01.15 at 21:08, <andrew.cooper3@citrix.com> wrote:
>> On 19/01/15 15:12, Jan Beulich wrote:
>>> Following the earlier similar change validating CR4 modifications.
>>>
>>> Note that this requires a non-obvious adjustment to hvm_cpuid(): On
>>> 32-bit hosts/tool stacks, the SYSCALL feature flag will be seen clear
>>> on Intel CPUs, yet 64-bit guests legitimately expect the feature for
>>> be available. Mimic hardware behavior: When executed from a 64-bit
>>> code segment, force the feature flag set.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> I finally have the debugging out of XenRT as to why this is still
>> failing for windows (including my improvements to the error message
>> which I will post in due course).
>>
>> d58v1 Invalid EFER update: 0 -> 0x901 - SCE without feature
>>
>> When windows brings up its second cpu, it appears to set up EFER
>> completely in one go, which means it will still be in protected mode at
>> this point.  It sets SCE in the knowledge that the cpu is 64bit, and
>> this indicates that it is valid to set EFER.SCE on 64-bit Intel cpus
>> even the feature would not appear via cpuid.
>>
>> Undoing the v3 -> v4 change wrt hvm_cpuid() fixes the issue, and I rerun
>> the test suite like this.
> Now that's odd - v4 only changes _where_ the SYSCALL feature
> flag gets set, not the dependency on CS.L. Or are you saying
> that this vCPU hotplug issue is independent of the 32-bit tool
> stack one that v3 fixed, and hence it is a problem that the VMX
> code actively clears the feature flag?

vmx_cpuid_intercept() unconditionally sets or clears the SYSCALL feature
depending on whether the current vcpu is in a CS.L segment.

Therefore, when hvm_valid_efer() queries ext1_edx (currently running in
32bit mode, setting up for long mode), the syscall feature bit is clear,
which causes the SCE check to fail.

~Andrew

>
>> I suggest that the easiest course of action is to simply ignore this
>> Intelism wrt EFER.SCE, and add it to the big bucket of edge cases that
>> we can't quite virtualise correctly.
>>
>> On the other hand, I wonder whether it is permitted because LME is set
>> at the same time?  It might be that SCE is permitted if and only if LME
>> has been verified as ok.
> And I think I'll go that route - permit SCE when the feature flag is
> set, or would be set after entering long mode when enabling long
> mode at the same time.
>
> Jan
>