[PATCH v3 0/3] Nested VMX: fix bugs when reading VMX MSRs.

[PATCH v3 0/3] Nested VMX: fix bugs when reading VMX MSRs.

[Yang Zhang]

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

The following patches fix the issues which existing in current vmx msr reading logic

Yang Zhang (3):
  Nested VMX: Check VMX capability before read VMX related MSRs.
  Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR
  Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

 xen/arch/x86/hvm/vmx/vmcs.c      |    3 +-
 xen/arch/x86/hvm/vmx/vvmx.c      |   73 ++++++++++++++++++++++++++++++++++++--
 xen/include/asm-x86/cpufeature.h |    1 +
 xen/include/asm-x86/processor.h  |    1 +
 4 files changed, 74 insertions(+), 4 deletions(-)

[PATCH v3 1/3] Nested VMX: Check VMX capability before read VMX related MSRs.

[Yang Zhang]

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

VMX MSRs only available when the CPU support the VMX feature. In addition,
VMX_TRUE* MSRs only available when bit 55 of VMX_BASIC MSR is set.

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

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 31347bb..f8d2a9d 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -78,6 +78,7 @@ static DEFINE_PER_CPU(struct list_head, active_vmcs_list);
 static DEFINE_PER_CPU(bool_t, vmxon);
 
 static u32 vmcs_revision_id __read_mostly;
+u32 vmx_basic_msr_low, vmx_basic_msr_high;
 
 static void __init vmx_display_features(void)
 {
@@ -133,7 +134,7 @@ static bool_t cap_check(const char *name, u32 expected, u32 saw)
 
 static int vmx_init_vmcs_config(void)
 {
-    u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
+    u32 min, opt;
     u32 _vmx_pin_based_exec_control;
     u32 _vmx_cpu_based_exec_control;
     u32 _vmx_secondary_exec_control = 0;
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index cecc72f..554d5c0 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -31,6 +31,7 @@
 static DEFINE_PER_CPU(u64 *, vvmcs_buf);
 
 static void nvmx_purge_vvmcs(struct vcpu *v);
+extern u32 vmx_basic_msr_high;
 
 #define VMCS_BUF_SIZE 100
 
@@ -1814,12 +1815,33 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
 int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
 {
     struct vcpu *v = current;
+    unsigned int eax, ebx, ecx, edx;
     u64 data = 0, host_data = 0;
     int r = 1;
 
     if ( !nestedhvm_enabled(v->domain) )
         return 0;
 
+    /* VMX capablity MSRs are available only when guest supports VMX. */
+    hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx);
+    if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) )
+        return 0;
+
+    /*
+     * Those MSRs are available only when bit 55 of
+     * MSR_IA32_VMX_BASIC is set.
+     */
+    switch (msr) {
+    case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
+    case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
+    case MSR_IA32_VMX_TRUE_EXIT_CTLS:
+    case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
+        if ( !(vmx_basic_msr_high & VMX_BASIC_DEFAULT1_ZERO >> 32) )
+            return 0;
+    default:
+        break;
+    }
+
     rdmsrl(msr, host_data);
 
     /*
-- 
1.7.1

Re: [PATCH v3 1/3] Nested VMX: Check VMX capability before read VMX related MSRs.

[Jan Beulich]

>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@intel.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -78,6 +78,7 @@ static DEFINE_PER_CPU(struct list_head, active_vmcs_list);
>  static DEFINE_PER_CPU(bool_t, vmxon);
>  
>  static u32 vmcs_revision_id __read_mostly;
> +u32 vmx_basic_msr_low, vmx_basic_msr_high;

This should be a single 64-bit variable.

> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -31,6 +31,7 @@
>  static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>  
>  static void nvmx_purge_vvmcs(struct vcpu *v);
> +extern u32 vmx_basic_msr_high;

No declarations in source files please; they belong into header files
(which ought to be included by producer and consumer, so that
eventual inconsistencies can be caught by the compiler).

> +    /*
> +     * Those MSRs are available only when bit 55 of
> +     * MSR_IA32_VMX_BASIC is set.
> +     */
> +    switch (msr) {

Coding style.

> +    case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
> +    case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
> +    case MSR_IA32_VMX_TRUE_EXIT_CTLS:
> +    case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> +        if ( !(vmx_basic_msr_high & VMX_BASIC_DEFAULT1_ZERO >> 32) )

With the switch to a single 64-bit variable this will get cleaned up
anyway, but for the future: This would need full parenthesization.

Jan

Re: [PATCH v3 1/3] Nested VMX: Check VMX capability before read VMX related MSRs.

[Andrew Cooper]

On 11/09/13 03:52, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> VMX MSRs only available when the CPU support the VMX feature. In addition,
> VMX_TRUE* MSRs only available when bit 55 of VMX_BASIC MSR is set.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c |    3 ++-
>  xen/arch/x86/hvm/vmx/vvmx.c |   22 ++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 31347bb..f8d2a9d 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -78,6 +78,7 @@ static DEFINE_PER_CPU(struct list_head, active_vmcs_list);
>  static DEFINE_PER_CPU(bool_t, vmxon);
>  
>  static u32 vmcs_revision_id __read_mostly;
> +u32 vmx_basic_msr_low, vmx_basic_msr_high;
>  
>  static void __init vmx_display_features(void)
>  {
> @@ -133,7 +134,7 @@ static bool_t cap_check(const char *name, u32 expected, u32 saw)
>  
>  static int vmx_init_vmcs_config(void)
>  {
> -    u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
> +    u32 min, opt;
>      u32 _vmx_pin_based_exec_control;
>      u32 _vmx_cpu_based_exec_control;
>      u32 _vmx_secondary_exec_control = 0;
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index cecc72f..554d5c0 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -31,6 +31,7 @@
>  static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>  
>  static void nvmx_purge_vvmcs(struct vcpu *v);
> +extern u32 vmx_basic_msr_high;
>  
>  #define VMCS_BUF_SIZE 100
>  
> @@ -1814,12 +1815,33 @@ int nvmx_handle_invvpid(struct cpu_user_regs *regs)
>  int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
>  {
>      struct vcpu *v = current;
> +    unsigned int eax, ebx, ecx, edx;
>      u64 data = 0, host_data = 0;
>      int r = 1;
>  
>      if ( !nestedhvm_enabled(v->domain) )
>          return 0;
>  
> +    /* VMX capablity MSRs are available only when guest supports VMX. */
> +    hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx);
> +    if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) )
> +        return 0;
> +
> +    /*
> +     * Those MSRs are available only when bit 55 of
> +     * MSR_IA32_VMX_BASIC is set.
> +     */
> +    switch (msr) {
> +    case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
> +    case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
> +    case MSR_IA32_VMX_TRUE_EXIT_CTLS:
> +    case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> +        if ( !(vmx_basic_msr_high & VMX_BASIC_DEFAULT1_ZERO >> 32) )
> +            return 0;
> +    default:
> +        break;

default:
    break;

Is completely useless here.

~Andrew

> +    }
> +
>      rdmsrl(msr, host_data);
>  
>      /*

Re: [PATCH v3 1/3] Nested VMX: Check VMX capability before read VMX related MSRs.

[Zhang, Yang Z]

Andrew Cooper wrote on 2013-09-11:
> On 11/09/13 03:52, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> VMX MSRs only available when the CPU support the VMX feature. In addition,
>> VMX_TRUE* MSRs only available when bit 55 of VMX_BASIC MSR is set.
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  xen/arch/x86/hvm/vmx/vmcs.c |    3 ++-
>>  xen/arch/x86/hvm/vmx/vvmx.c |   22 ++++++++++++++++++++++
>>  2 files changed, 24 insertions(+), 1 deletions(-)
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 31347bb..f8d2a9d 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -78,6 +78,7 @@ static DEFINE_PER_CPU(struct list_head,
> active_vmcs_list);
>>  static DEFINE_PER_CPU(bool_t, vmxon);
>>  
>>  static u32 vmcs_revision_id __read_mostly;
>> +u32 vmx_basic_msr_low, vmx_basic_msr_high;
>> 
>>  static void __init vmx_display_features(void)
>>  {
>> @@ -133,7 +134,7 @@ static bool_t cap_check(const char *name, u32
>> expected, u32 saw)
>> 
>>  static int vmx_init_vmcs_config(void)
>>  {
>> -    u32 vmx_basic_msr_low, vmx_basic_msr_high, min, opt;
>> +    u32 min, opt;
>>      u32 _vmx_pin_based_exec_control;
>>      u32 _vmx_cpu_based_exec_control;
>>      u32 _vmx_secondary_exec_control = 0;
>> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
>> index cecc72f..554d5c0 100644
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -31,6 +31,7 @@
>>  static DEFINE_PER_CPU(u64 *, vvmcs_buf);
>>  
>>  static void nvmx_purge_vvmcs(struct vcpu *v);
>> +extern u32 vmx_basic_msr_high;
>> 
>>  #define VMCS_BUF_SIZE 100
>> @@ -1814,12 +1815,33 @@ int nvmx_handle_invvpid(struct cpu_user_regs
> *regs)
>>  int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
>>  {
>>      struct vcpu *v = current; +    unsigned int eax, ebx, ecx, edx;
>>      u64 data = 0, host_data = 0; int r = 1;
>>      
>>      if ( !nestedhvm_enabled(v->domain) )
>>          return 0;
>> +    /* VMX capablity MSRs are available only when guest supports VMX. */
>> +    hvm_cpuid(0x1, &eax, &ebx, &ecx, &edx);
>> +    if ( !(ecx & cpufeat_mask(X86_FEATURE_VMXE)) )
>> +        return 0;
>> +
>> +    /*
>> +     * Those MSRs are available only when bit 55 of
>> +     * MSR_IA32_VMX_BASIC is set.
>> +     */
>> +    switch (msr) {
>> +    case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
>> +    case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
>> +    case MSR_IA32_VMX_TRUE_EXIT_CTLS:
>> +    case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
>> +        if ( !(vmx_basic_msr_high & VMX_BASIC_DEFAULT1_ZERO >> 32) )
>> +            return 0;
>> +    default:
>> +        break;
> 
> default:
>     break;
> Is completely useless here.

Yes. I added them here to let the code look like much neatly. Compiler will ignore them.


Best regards,
Yang

[PATCH v3 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR

[Yang Zhang]

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

The bit 31 of revision_id will set to 1 if vmcs shadowing enabled. And
according intel SDM, the bit 31 of IA32_VMX_BASIC MSR is always 0. So we
cannot set low 32 bit of IA32_VMX_BASIC to revision_id directly. Must clear
the bit 31 to 0.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 554d5c0..48cfbc6 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1850,7 +1850,7 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
     switch (msr) {
     case MSR_IA32_VMX_BASIC:
         data = (host_data & (~0ul << 32)) |
-               ((v->arch.hvm_vmx.vmcs)->vmcs_revision_id);
+               (v->arch.hvm_vmx.vmcs->vmcs_revision_id & 0x7fffffff);
         break;
     case MSR_IA32_VMX_PINBASED_CTLS:
     case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
-- 
1.7.1

Re: [PATCH v3 2/3] Nested VMX: Clear bit 31 of IA32_VMX_BASIC MSR

[Jan Beulich]

>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@intel.com> wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> The bit 31 of revision_id will set to 1 if vmcs shadowing enabled. And
> according intel SDM, the bit 31 of IA32_VMX_BASIC MSR is always 0. So we
> cannot set low 32 bit of IA32_VMX_BASIC to revision_id directly. Must clear
> the bit 31 to 0.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This already got applied yesterday - no need resending.

Jan

[PATCH v3 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

[Yang Zhang]

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

Currently, it use hardcode value for IA32_VMX_CR4_FIXED1. This is wrong.
We should check guest's cpuid to know which bits are writeable in CR4 by guest
and allow the guest to set the corresponding bit only when guest has the feature.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c      |   49 ++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/cpufeature.h |    1 +
 xen/include/asm-x86/processor.h  |    1 +
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 48cfbc6..f9c7832 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1947,8 +1947,53 @@ int nvmx_msr_read_intercept(unsigned int msr, u64 *msr_content)
         data = X86_CR4_VMXE;
         break;
     case MSR_IA32_VMX_CR4_FIXED1:
-        /* allow 0-settings except SMXE */
-        data = 0x267ff & ~X86_CR4_SMXE;
+        if ( edx & cpufeat_mask(X86_FEATURE_VME) )
+            data |= X86_CR4_VME | X86_CR4_PVI;
+        if ( edx & cpufeat_mask(X86_FEATURE_TSC) )
+            data |= X86_CR4_TSD;
+        if ( edx & cpufeat_mask(X86_FEATURE_DE) )
+            data |= X86_CR4_DE;
+        if ( edx & cpufeat_mask(X86_FEATURE_PSE) )
+            data |= X86_CR4_PSE;
+        if ( edx & cpufeat_mask(X86_FEATURE_PAE) )
+            data |= X86_CR4_PAE;
+        if ( edx & cpufeat_mask(X86_FEATURE_MCE) )
+            data |= X86_CR4_MCE;
+        if ( edx & cpufeat_mask(X86_FEATURE_PGE) )
+            data |= X86_CR4_PGE;
+        if ( edx & cpufeat_mask(X86_FEATURE_FXSR) )
+            data |= X86_CR4_OSFXSR;
+        if ( edx & cpufeat_mask(X86_FEATURE_XMM) )
+            data |= X86_CR4_OSXMMEXCPT;
+        if ( ecx & cpufeat_mask(X86_FEATURE_VMXE) )
+            data |= X86_CR4_VMXE;
+        if ( ecx & cpufeat_mask(X86_FEATURE_SMXE) )
+            data |= X86_CR4_SMXE;
+        if ( ecx & cpufeat_mask(X86_FEATURE_PCID) )
+            data |= X86_CR4_PCIDE;
+        if ( ecx & cpufeat_mask(X86_FEATURE_XSAVE) )
+            data |= X86_CR4_OSXSAVE;
+
+        hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx);
+        if ( eax >= 0x7 )
+        {
+            ecx = 0;
+            hvm_cpuid(0x7, &ecx, &ebx, &ecx, &edx);
+            if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) )
+                data |= X86_CR4_FSGSBASE;
+            if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) )
+                data |= X86_CR4_SMEP;
+            if ( ebx & cpufeat_mask(X86_FEATURE_SMAP) )
+                data |= X86_CR4_SMAP;
+
+            if ( eax >= 0xa )
+            {
+                hvm_cpuid(0xa, &eax, &ebx, &ecx, &edx);
+                /* Check whether guest has the perf monitor feature. */
+                if ( (eax & 0xff) && (eax & 0xff00) )
+                    data |= X86_CR4_PCE;
+            }
+        }
         break;
     case MSR_IA32_VMX_MISC:
         /* Do not support CR3-target feature now */
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 065c265..73d5cb6 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -148,6 +148,7 @@
 #define X86_FEATURE_INVPCID	(7*32+10) /* Invalidate Process Context ID */
 #define X86_FEATURE_RTM 	(7*32+11) /* Restricted Transactional Memory */
 #define X86_FEATURE_NO_FPU_SEL 	(7*32+13) /* FPU CS/DS stored as zero */
+#define X86_FEATURE_SMAP	(7*32+ 20) /* Supervisor Mode Access Prevention */
 
 #define cpu_has(c, bit)		test_bit(bit, (c)->x86_capability)
 #define boot_cpu_has(bit)	test_bit(bit, boot_cpu_data.x86_capability)
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 5cdacc7..893afa3 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -87,6 +87,7 @@
 #define X86_CR4_PCIDE		0x20000 /* enable PCID */
 #define X86_CR4_OSXSAVE	0x40000 /* enable XSAVE/XRSTOR */
 #define X86_CR4_SMEP		0x100000/* enable SMEP */
+#define X86_CR4_SMAP		0x200000/* enable SMAP */
 
 /*
  * Trap/fault mnemonics.
-- 
1.7.1

Re: [PATCH v3 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

[Jan Beulich]

>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@intel.com> wrote:
> +        hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx);
> +        if ( eax >= 0x7 )

Not a need to re-submit this patch, but for the future I'd recommend
trying to avoid deep scope nesting where possible. Here that's
pretty simple: If for whatever reason you dislike using the switch()
approach I suggested in the v2 review, invert the condition above,
and make the body of the if() simply "break;".

> +        {
> +            ecx = 0;
> +            hvm_cpuid(0x7, &ecx, &ebx, &ecx, &edx);

That's confusing: A casual reader might assume that the first use
of &ecx is actually a typo. Either use a dummy variable, or at
least be consistent and funnel _all_ unused output into the same
variable (i.e. hvm_cpuid(0x7, &ecx, &ebx, &ecx, &ecx)). But
again, the switch() approach suggested earlier would avoid all
that.

In the end, with more uses of hvm_cpuid() appearing, we may
want to make the function tolerate NULL inputs to allow to make
clear at the call site which of the outputs aren't cared about.

> +            if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) )
> +                data |= X86_CR4_FSGSBASE;
> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) )
> +                data |= X86_CR4_SMEP;
> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMAP) )
> +                data |= X86_CR4_SMAP;
> +
> +            if ( eax >= 0xa )
> +            {

Same here then.

Jan

> +                hvm_cpuid(0xa, &eax, &ebx, &ecx, &edx);
> +                /* Check whether guest has the perf monitor feature. */
> +                if ( (eax & 0xff) && (eax & 0xff00) )
> +                    data |= X86_CR4_PCE;
> +            }
> +        }

Re: [PATCH v3 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

[Zhang, Yang Z]

Jan Beulich wrote on 2013-09-11:
>>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@intel.com> wrote:
>> +        hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx);
>> +        if ( eax >= 0x7 )
> 
> Not a need to re-submit this patch, but for the future I'd recommend
> trying to avoid deep scope nesting where possible. Here that's pretty
> simple: If for whatever reason you dislike using the switch() approach
> I suggested in the v2 review, invert the condition above, and make the
> body of the if() simply "break;".
> 
>> +        {
>> +            ecx = 0;
>> +            hvm_cpuid(0x7, &ecx, &ebx, &ecx, &edx);
> 
> That's confusing: A casual reader might assume that the first use of
> &ecx is actually a typo. Either use a dummy variable, or at least be
> consistent and funnel _all_ unused output into the same variable (i.e.
> hvm_cpuid(0x7, &ecx, &ebx, &ecx, &ecx)). But again, the switch()
> approach suggested earlier would avoid all that.
> 
> In the end, with more uses of hvm_cpuid() appearing, we may want to
> make the function tolerate NULL inputs to allow to make clear at the
> call site which of the outputs aren't cared about.
> 
>> +            if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) )
>> +                data |= X86_CR4_FSGSBASE;
>> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) )
>> +                data |= X86_CR4_SMEP;
>> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMAP) )
>> +                data |= X86_CR4_SMAP;
>> +
>> +            if ( eax >= 0xa )
>> +            {
> 
> Same here then.
> 
> Jan
> 
>> +                hvm_cpuid(0xa, &eax, &ebx, &ecx, &edx);
>> +                /* Check whether guest has the perf monitor feature. */
>> +                if ( (eax & 0xff) && (eax & 0xff00) )
>> +                    data |= X86_CR4_PCE;
>> +            }
>> +        }
>
Should I resend this or just the first patch is enough?

Best regards,
Yang

Re: [PATCH v3 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

[Jan Beulich]

>>> On 17.09.13 at 04:25, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2013-09-11:
>>>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@intel.com> wrote:
>>> +        hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx);
>>> +        if ( eax >= 0x7 )
>> 
>> Not a need to re-submit this patch, but for the future I'd recommend
>> trying to avoid deep scope nesting where possible. Here that's pretty
>> simple: If for whatever reason you dislike using the switch() approach
>> I suggested in the v2 review, invert the condition above, and make the
>> body of the if() simply "break;".
>> 
>>> +        {
>>> +            ecx = 0;
>>> +            hvm_cpuid(0x7, &ecx, &ebx, &ecx, &edx);
>> 
>> That's confusing: A casual reader might assume that the first use of
>> &ecx is actually a typo. Either use a dummy variable, or at least be
>> consistent and funnel _all_ unused output into the same variable (i.e.
>> hvm_cpuid(0x7, &ecx, &ebx, &ecx, &ecx)). But again, the switch()
>> approach suggested earlier would avoid all that.
>> 
>> In the end, with more uses of hvm_cpuid() appearing, we may want to
>> make the function tolerate NULL inputs to allow to make clear at the
>> call site which of the outputs aren't cared about.
>> 
>>> +            if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) )
>>> +                data |= X86_CR4_FSGSBASE;
>>> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) )
>>> +                data |= X86_CR4_SMEP;
>>> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMAP) )
>>> +                data |= X86_CR4_SMAP;
>>> +
>>> +            if ( eax >= 0xa )
>>> +            {
>> 
>> Same here then.
>> 
>> Jan
>> 
>>> +                hvm_cpuid(0xa, &eax, &ebx, &ecx, &edx);
>>> +                /* Check whether guest has the perf monitor feature. */
>>> +                if ( (eax & 0xff) && (eax & 0xff00) )
>>> +                    data |= X86_CR4_PCE;
>>> +            }
>>> +        }
>>
> Should I resend this or just the first patch is enough?

I case it came over ambiguously: The first comment alone was
meant to be indicated to be no reason for resubmitting. The
later comments, however, I would really want to see addressed
(if you don't, I'll likely end up polishing this, which would then still
want to be reviewed/tested by you again).

Jan

Re: [PATCH v3 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

[Zhang, Yang Z]

Jan Beulich wrote on 2013-09-11:
>>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@intel.com> wrote:
>> +        hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx);
>> +        if ( eax >= 0x7 )
> 
> Not a need to re-submit this patch, but for the future I'd recommend
> trying to avoid deep scope nesting where possible. Here that's pretty
> simple: If for whatever reason you dislike using the switch() approach
> I suggested in the v2 review, invert the condition above, and make the
> body of the if() simply "break;".
Can you give an example of how to use switch() instead if() here? I cannot find out a nice switch() to simply the logic.

> 
>> +        {
>> +            ecx = 0;
>> +            hvm_cpuid(0x7, &ecx, &ebx, &ecx, &edx);
> 
> That's confusing: A casual reader might assume that the first use of
> &ecx is actually a typo. Either use a dummy variable, or at least be
> consistent and funnel _all_ unused output into the same variable (i.e.
> hvm_cpuid(0x7, &ecx, &ebx, &ecx, &ecx)). But again, the switch()
> approach suggested earlier would avoid all that.
> 
> In the end, with more uses of hvm_cpuid() appearing, we may want to
> make the function tolerate NULL inputs to allow to make clear at the
> call site which of the outputs aren't cared about.
> 
>> +            if ( ebx & cpufeat_mask(X86_FEATURE_FSGSBASE) )
>> +                data |= X86_CR4_FSGSBASE;
>> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMEP) )
>> +                data |= X86_CR4_SMEP;
>> +            if ( ebx & cpufeat_mask(X86_FEATURE_SMAP) )
>> +                data |= X86_CR4_SMAP;
>> +
>> +            if ( eax >= 0xa )
>> +            {
> 
> Same here then.
> 
> Jan
> 
>> +                hvm_cpuid(0xa, &eax, &ebx, &ecx, &edx);
>> +                /* Check whether guest has the perf monitor feature. */
>> +                if ( (eax & 0xff) && (eax & 0xff00) )
>> +                    data |= X86_CR4_PCE;
>> +            }
>> +        }
>


Best regards,
Yang

Re: [PATCH v3 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

[Jan Beulich]

>>> On 22.09.13 at 07:34, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2013-09-11:
>>>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@intel.com> wrote:
>>> +        hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx);
>>> +        if ( eax >= 0x7 )
>> 
>> Not a need to re-submit this patch, but for the future I'd recommend
>> trying to avoid deep scope nesting where possible. Here that's pretty
>> simple: If for whatever reason you dislike using the switch() approach
>> I suggested in the v2 review, invert the condition above, and make the
>> body of the if() simply "break;".
> Can you give an example of how to use switch() instead if() here? I cannot 
> find out a nice switch() to simply the logic.

Please see the v4 series I just sent out.

Jan

Re: [PATCH v3 3/3] Nested VMX: Fix IA32_VMX_CR4_FIXED1 msr emulation

[Zhang, Yang Z]

Jan Beulich wrote on 2013-09-23:
>>>> On 22.09.13 at 07:34, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2013-09-11:
>>>>>> On 11.09.13 at 04:52, Yang Zhang <yang.z.zhang@intel.com> wrote:
>>>> +        hvm_cpuid(0x0, &eax, &ebx, &ecx, &edx);
>>>> +        if ( eax >= 0x7 )
>>> 
>>> Not a need to re-submit this patch, but for the future I'd
>>> recommend trying to avoid deep scope nesting where possible. Here
>>> that's pretty
>>> simple: If for whatever reason you dislike using the switch()
>>> approach I suggested in the v2 review, invert the condition above,
>>> and make the body of the if() simply "break;".
>> Can you give an example of how to use switch() instead if() here? I
>> cannot find out a nice switch() to simply the logic.
> 
> Please see the v4 series I just sent out.
I see it. It looks great. Thanks for the cleanup.

Best regards,
Yang