[PATCH 0/2] Two Fixes for xsave compoent features

[PATCH 0/2] Two Fixes for xsave compoent features

[Xiaoyao Li]

Two simple fixes for XSAVE component features, please see each one
for details.

Xiaoyao Li (2):
  i386/cpu: Clear FEAT_XSAVE_COMP_{LO,HI} when XSAVE is not available
  i386/cpu: Mask off unsupported XSAVE components

 target/i386/cpu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.18.4

[PATCH 1/2] i386/cpu: Clear FEAT_XSAVE_COMP_{LO, HI} when XSAVE is not available

[Xiaoyao Li]

Per Intel SDM vol 1, 13.2, if CPUID.1:ECX.XSAVE[bit 26] is 0, the
processor provides no further enumeration through CPUID function 0DH.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1e5123251d74..f5f11603e805 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6261,6 +6261,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
     uint64_t mask;
 
     if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
+        env->features[FEAT_XSAVE_COMP_LO] = 0;
+        env->features[FEAT_XSAVE_COMP_HI] = 0;
         return;
     }
 
-- 
2.18.4

[PATCH 2/2] i386/cpu: Mask off unsupported XSAVE components

[Xiaoyao Li]

When setting up XSAVE components, it needs to mask off those unsupported
by KVM.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f5f11603e805..efc92334b7b1 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6274,8 +6274,10 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
         }
     }
 
-    env->features[FEAT_XSAVE_COMP_LO] = mask;
-    env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
+    env->features[FEAT_XSAVE_COMP_LO] = mask &
+        x86_cpu_get_supported_feature_word(FEAT_XSAVE_COMP_LO, cpu->migratable);
+    env->features[FEAT_XSAVE_COMP_HI] = (mask >> 32) &
+        x86_cpu_get_supported_feature_word(FEAT_XSAVE_COMP_HI, cpu->migratable);
 }
 
 /***** Steps involved on loading and filtering CPUID data
-- 
2.18.4

Re: [PATCH 2/2] i386/cpu: Mask off unsupported XSAVE components

[Eduardo Habkost]

On Thu, Jul 16, 2020 at 04:20:19PM +0800, Xiaoyao Li wrote:
> When setting up XSAVE components, it needs to mask off those unsupported
> by KVM.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>

We must never disable CPUID features silently based on host
capabilities, otherwise we can't guarantee guest ABI stability
when migrating to another host.  Filtering of features should
involve a call to mark_unavailable_features() (or some equivalent
mechanism) so we can report the missing features properly through
QMP.

Could you explain what's the bug you are trying to fix?  The loop
at x86_cpu_filter_features() is already supposed to disable
features unsupported by the host.

> ---
>  target/i386/cpu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index f5f11603e805..efc92334b7b1 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6274,8 +6274,10 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>          }
>      }
>  
> -    env->features[FEAT_XSAVE_COMP_LO] = mask;
> -    env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
> +    env->features[FEAT_XSAVE_COMP_LO] = mask &
> +        x86_cpu_get_supported_feature_word(FEAT_XSAVE_COMP_LO, cpu->migratable);
> +    env->features[FEAT_XSAVE_COMP_HI] = (mask >> 32) &
> +        x86_cpu_get_supported_feature_word(FEAT_XSAVE_COMP_HI, cpu->migratable);
>  }
>  
>  /***** Steps involved on loading and filtering CPUID data
> -- 
> 2.18.4
> 

-- 
Eduardo

Re: [PATCH 1/2] i386/cpu: Clear FEAT_XSAVE_COMP_{LO,HI} when XSAVE is not available

[Eduardo Habkost]

On Thu, Jul 16, 2020 at 04:20:18PM +0800, Xiaoyao Li wrote:
> Per Intel SDM vol 1, 13.2, if CPUID.1:ECX.XSAVE[bit 26] is 0, the
> processor provides no further enumeration through CPUID function 0DH.

Can you explain what's the bug you are trying to fix?
env->features[FEAT_XSAVE_COMP_*] is already initialized as zero.

> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1e5123251d74..f5f11603e805 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6261,6 +6261,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>      uint64_t mask;
>  
>      if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> +        env->features[FEAT_XSAVE_COMP_LO] = 0;
> +        env->features[FEAT_XSAVE_COMP_HI] = 0;
>          return;
>      }
>  
> -- 
> 2.18.4
> 

-- 
Eduardo

Re: [PATCH 1/2] i386/cpu: Clear FEAT_XSAVE_COMP_{LO, HI} when XSAVE is not available

[Xiaoyao Li]

On 7/16/2020 11:15 PM, Eduardo Habkost wrote:
> On Thu, Jul 16, 2020 at 04:20:18PM +0800, Xiaoyao Li wrote:
>> Per Intel SDM vol 1, 13.2, if CPUID.1:ECX.XSAVE[bit 26] is 0, the
>> processor provides no further enumeration through CPUID function 0DH.
> 
> Can you explain what's the bug you are trying to fix?
> env->features[FEAT_XSAVE_COMP_*] is already initialized as zero.

When "-cpu host", it's not zero I think.

> 
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   target/i386/cpu.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 1e5123251d74..f5f11603e805 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6261,6 +6261,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>>       uint64_t mask;
>>   
>>       if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
>> +        env->features[FEAT_XSAVE_COMP_LO] = 0;
>> +        env->features[FEAT_XSAVE_COMP_HI] = 0;
>>           return;
>>       }
>>   
>> -- 
>> 2.18.4
>>
> 

Re: [PATCH 2/2] i386/cpu: Mask off unsupported XSAVE components

[Xiaoyao Li]

On 7/16/2020 11:14 PM, Eduardo Habkost wrote:
> On Thu, Jul 16, 2020 at 04:20:19PM +0800, Xiaoyao Li wrote:
>> When setting up XSAVE components, it needs to mask off those unsupported
>> by KVM.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> We must never disable CPUID features silently based on host
> capabilities, otherwise we can't guarantee guest ABI stability
> when migrating to another host.  Filtering of features should
> involve a call to mark_unavailable_features() (or some equivalent
> mechanism) so we can report the missing features properly through
> QMP.
> 
> Could you explain what's the bug you are trying to fix?  The loop
> at x86_cpu_filter_features() is already supposed to disable
> features unsupported by the host.

Sorry, I forgot x86_cpu_filter_features() totally when code inspection.

>> ---
>>   target/i386/cpu.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index f5f11603e805..efc92334b7b1 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6274,8 +6274,10 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>>           }
>>       }
>>   
>> -    env->features[FEAT_XSAVE_COMP_LO] = mask;
>> -    env->features[FEAT_XSAVE_COMP_HI] = mask >> 32;
>> +    env->features[FEAT_XSAVE_COMP_LO] = mask &
>> +        x86_cpu_get_supported_feature_word(FEAT_XSAVE_COMP_LO, cpu->migratable);
>> +    env->features[FEAT_XSAVE_COMP_HI] = (mask >> 32) &
>> +        x86_cpu_get_supported_feature_word(FEAT_XSAVE_COMP_HI, cpu->migratable);
>>   }
>>   
>>   /***** Steps involved on loading and filtering CPUID data
>> -- 
>> 2.18.4
>>
> 

Re: [PATCH 1/2] i386/cpu: Clear FEAT_XSAVE_COMP_{LO, HI} when XSAVE is not available

[Paolo Bonzini]

On 16/07/20 10:20, Xiaoyao Li wrote:
> Per Intel SDM vol 1, 13.2, if CPUID.1:ECX.XSAVE[bit 26] is 0, the
> processor provides no further enumeration through CPUID function 0DH.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1e5123251d74..f5f11603e805 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6261,6 +6261,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>      uint64_t mask;
>  
>      if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> +        env->features[FEAT_XSAVE_COMP_LO] = 0;
> +        env->features[FEAT_XSAVE_COMP_HI] = 0;
>          return;
>      }
>  
> 

Queued this patch, noting in the commit message that it affects "-cpu
host,-xsave".

Paolo