Xen devel - Aug 2013 - [PATCH] Nested VMX: Allow to set CR4.OSXSAVE if guest has xsave feature

From: Yang Zhang <yang.z.zhang@Intel.com>

We exposed the xsave feature to guest, but we didn''t allow guest
to set CR4.OSXSAVE when guest running in nested mode. This will
cause win 7 guest fail to use XP mode. In this patch, we allow guest
to set CR4.OSXSAVE in nested mode when it has the xsave feature.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index cecc72f..879af58 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1815,6 +1815,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64
*msr_content)
 {
     struct vcpu *v = current;
     u64 data = 0, host_data = 0;
+    unsigned int eax, ebx, ecx, edx;
     int r = 1;
 
     if ( !nestedhvm_enabled(v->domain) )
@@ -1925,8 +1926,13 @@ int nvmx_msr_read_intercept(unsigned int msr, u64
*msr_content)
         data = X86_CR4_VMXE;
         break;
     case MSR_IA32_VMX_CR4_FIXED1:
+        data = 0x267ff;
+        /* Allow to set OSXSAVE if guest has xsave feature. */
+        hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx);
+        if ( ecx & cpufeat_mask(X86_FEATURE_XSAVE) )
+            data |= X86_CR4_OSXSAVE;
         /* allow 0-settings except SMXE */
-        data = 0x267ff & ~X86_CR4_SMXE;
+        data &= ~X86_CR4_SMXE;
         break;
     case MSR_IA32_VMX_MISC:
         /* Do not support CR3-target feature now */
-- 
1.7.1

>>> On 23.08.13 at 04:26, Yang Zhang <yang.z.zhang@intel.com>
wrote:
>      case MSR_IA32_VMX_CR4_FIXED1:
> +        data = 0x267ff;
> +        /* Allow to set OSXSAVE if guest has xsave feature. */
> +        hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx);
> +        if ( ecx & cpufeat_mask(X86_FEATURE_XSAVE) )
> +            data |= X86_CR4_OSXSAVE;
>          /* allow 0-settings except SMXE */
> -        data = 0x267ff & ~X86_CR4_SMXE;
> +        data &= ~X86_CR4_SMXE;
While fine with me in general, please take the opportunity and
eliminate the bogus literal 0x267ff (which will then also make it
unnecessary to explicitly remove X86_CR4_SMXE from the mask,
and will make obvious that the mask likely is missing
X86_CR4_FSGSBASE and X86_CR4_SMEP). I also wonder why
the OSXSAVE bit needs to be gated by a virtual CPUID check,
while all other bits don''t: Is it possible that this code needs a
wider overhaul?

Jan

Jan Beulich wrote on 2013-08-23:
>>>> On 23.08.13 at 04:26, Yang Zhang <yang.z.zhang@intel.com>
wrote:
>>      case MSR_IA32_VMX_CR4_FIXED1:
>> +        data = 0x267ff;
>> +        /* Allow to set OSXSAVE if guest has xsave feature. */
>> +        hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx);
>> +        if ( ecx & cpufeat_mask(X86_FEATURE_XSAVE) )
>> +            data |= X86_CR4_OSXSAVE;
>>          /* allow 0-settings except SMXE */
>> -        data = 0x267ff & ~X86_CR4_SMXE;
>> +        data &= ~X86_CR4_SMXE;
> 
> While fine with me in general, please take the opportunity and
> eliminate the bogus literal 0x267ff (which will then also make it
> unnecessary to explicitly remove X86_CR4_SMXE from the mask, and will
> make obvious that the mask likely is missing X86_CR4_FSGSBASE and
> X86_CR4_SMEP). I also wonder why the OSXSAVE bit needs to be gated by
> a virtual CPUID check, while all other bits
> don''t: Is it possible that this code needs a wider overhaul?
Agree. Current nested code seems a little ugly in some logics (not just here, I
also saw some hack logics in handling interrupt). Since the nested virtulization
is still not very mature, so my current focus is to improve the stability and
bug fixing. But for this, I will send out patch to refine them.


Best regards,
Yang