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

Re: [Xen-devel] [PATCH 3/3] x86/HVM: make hvm_efer_valid() honor guest features



On 08/01/15 15:23, Jan Beulich wrote:
Following the earlier similar change validating CR4 modifications.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1672,20 +1672,53 @@ 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,
+                             bool_t restore)
 {
-    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 ( !restore && !is_hardware_domain(v->domain) )
+    {
+        unsigned int level;
+
+        ASSERT(v == current);
+        hvm_cpuid(0x80000000, &level, NULL, NULL, NULL);
+        if ( level >= 0x80000001 )
+            hvm_cpuid(0x80000001, NULL, NULL, &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) )
+        return 0;

The LME/LMA handling more complicated than this.

LMA is read-only on Intel, but specified as read-write on AMD, with the requirement that if it doesn't match the value generated by hardware, a #GP fault will occur.  I believe this actually means it is read-only on AMD as well.

LMA only gets set by hardware after paging is enabled and the processor switches properly into long mode, which means that there is a window between setting LME and setting CR0.PG where LMA should read as 0.

I think hvm_efer_valid() also needs the current EFER and CR0 to work out what the current LMA should be, and reject any attempt to change it.

+
+    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)) )

This is going to cause an issue for the restore case, as the HVM PARAMs follow the architectural state.

I don't believe it is reasonable for nestedhvm_enabled() to disagree with cpufeat_mask(X86_FEATURE_{SVM,VMX}), or for the toolstack to be able to yank nestedhvm while the VM is active.

Looking at the checks when setting HVM_PARAM_NESTEDHVM, neither of the guest feature flags are actually checked before setting up the nested infrastructure.

Having said all of this, don't appear to be any migration records associated with nested state (hardware-loaded VMCS/VMCB pointers, currently nested?) which leads me to suspect that a domain actually using nested virt is not going to survive a migration, so it might be acceptable to fudge the checking of SVME for now.

~Andrew

+        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 +1796,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 +1826,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, 1) )
     {
         printk(XENLOG_G_ERR "HVM%d restore: bad EFER %#" PRIx64 "\n",
                d->domain_id, ctxt.msr_efer);
@@ -2936,12 +2966,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, 0) )
     {
         gdprintk(XENLOG_WARNING, "Trying to set reserved bit in "
                  "EFER: %#"PRIx64"\n", value);





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

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

 


Rackspace

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