[PATCH 0/2] i386/cpu: Two minor fixes for x86_cpu_enable_xsave_components()

[PATCH 0/2] i386/cpu: Two minor fixes for x86_cpu_enable_xsave_components()

[Xiaoyao Li]

The two bugs were introduced when xsaves feature was added by commit
301e90675c3f ("target/i386: Enable support for XSAVES based features").

Xiaoyao Li (2):
  i386/cpu: Clear FEAT_XSAVE_XSS_LO/HI leafs when CPUID_EXT_XSAVE is not
    available
  i386/cpu: Mask with XCR0/XSS mask for FEAT_XSAVE_XCR0_HI and
    FEAT_XSAVE_XSS_HI leafs

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

-- 
2.34.1

[PATCH 1/2] i386/cpu: Clear FEAT_XSAVE_XSS_LO/HI leafs when CPUID_EXT_XSAVE is not available

[Xiaoyao Li]

Leaf FEAT_XSAVE_XSS_LO and FEAT_XSAVE_XSS_HI also need to be cleared
when CPUID_EXT_XSAVE is not set.

Fixes: 301e90675c3f ("target/i386: Enable support for XSAVES based features")
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 2524881ce245..b445e2957c4f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6926,6 +6926,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
     if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
         env->features[FEAT_XSAVE_XCR0_LO] = 0;
         env->features[FEAT_XSAVE_XCR0_HI] = 0;
+        env->features[FEAT_XSAVE_XSS_LO] = 0;
+        env->features[FEAT_XSAVE_XSS_HI] = 0;
         return;
     }
 
-- 
2.34.1

[PATCH 2/2] i386/cpu: Mask with XCR0/XSS mask for FEAT_XSAVE_XCR0_HI and FEAT_XSAVE_XSS_HI leafs

[Xiaoyao Li]

The value of FEAT_XSAVE_XCR0_HI leaf and FEAT_XSAVE_XSS_HI leaf also
need to be masked by XCR0 and XSS mask respectively, to make it
logically correct.

Fixes: 301e90675c3f ("target/i386: Enable support for XSAVES based features")
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b445e2957c4f..a5c08944a483 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6946,9 +6946,9 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
     }
 
     env->features[FEAT_XSAVE_XCR0_LO] = mask & CPUID_XSTATE_XCR0_MASK;
-    env->features[FEAT_XSAVE_XCR0_HI] = mask >> 32;
+    env->features[FEAT_XSAVE_XCR0_HI] = (mask & CPUID_XSTATE_XCR0_MASK) >> 32;
     env->features[FEAT_XSAVE_XSS_LO] = mask & CPUID_XSTATE_XSS_MASK;
-    env->features[FEAT_XSAVE_XSS_HI] = mask >> 32;
+    env->features[FEAT_XSAVE_XSS_HI] = (mask & CPUID_XSTATE_XSS_MASK) >> 32;
 }
 
 /***** Steps involved on loading and filtering CPUID data
-- 
2.34.1

Re: [PATCH 0/2] i386/cpu: Two minor fixes for x86_cpu_enable_xsave_components()

[Zhao Liu]

Hi Xiaoyao,

On Mon, Jan 15, 2024 at 04:13:23AM -0500, Xiaoyao Li wrote:
> Date: Mon, 15 Jan 2024 04:13:23 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH 0/2] i386/cpu: Two minor fixes for
>  x86_cpu_enable_xsave_components()
> X-Mailer: git-send-email 2.34.1
> 
> The two bugs were introduced when xsaves feature was added by commit
> 301e90675c3f ("target/i386: Enable support for XSAVES based features").

Could you please provide more details about reproducing these two bugs?
If I'm able, I'd be glad to help you to test and verify them.

Regards,
Zhao

> 
> Xiaoyao Li (2):
>   i386/cpu: Clear FEAT_XSAVE_XSS_LO/HI leafs when CPUID_EXT_XSAVE is not
>     available
>   i386/cpu: Mask with XCR0/XSS mask for FEAT_XSAVE_XCR0_HI and
>     FEAT_XSAVE_XSS_HI leafs
> 
>  target/i386/cpu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH 0/2] i386/cpu: Two minor fixes for x86_cpu_enable_xsave_components()

[Xiaoyao Li]

On 1/16/2024 10:19 PM, Zhao Liu wrote:
> Hi Xiaoyao,
> 
> On Mon, Jan 15, 2024 at 04:13:23AM -0500, Xiaoyao Li wrote:
>> Date: Mon, 15 Jan 2024 04:13:23 -0500
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>> Subject: [PATCH 0/2] i386/cpu: Two minor fixes for
>>   x86_cpu_enable_xsave_components()
>> X-Mailer: git-send-email 2.34.1
>>
>> The two bugs were introduced when xsaves feature was added by commit
>> 301e90675c3f ("target/i386: Enable support for XSAVES based features").
> 
> Could you please provide more details about reproducing these two bugs?
> If I'm able, I'd be glad to help you to test and verify them.

There are potential bugs and currently we don't have test step to 
trigger it. Because for patch 1, KVM doesn't support arch-lbr 
virtualization yet, which is the first user in QEMU of xss. Once KVM 
merges the arch-lbr series, using "-cpu xxx,+arch-lbr,-xsave" can expose 
arch-lbr to guest, which violates the architectural behavior of xfeatures.

For patch2, current code just happens to work correctly because there is 
not xfeature in upper 32-bit get defined yet. But I think make the code 
logically correct is important and we shouldn't depend on the 
happened-to-work code.


> Regards,
> Zhao
> 
>>
>> Xiaoyao Li (2):
>>    i386/cpu: Clear FEAT_XSAVE_XSS_LO/HI leafs when CPUID_EXT_XSAVE is not
>>      available
>>    i386/cpu: Mask with XCR0/XSS mask for FEAT_XSAVE_XCR0_HI and
>>      FEAT_XSAVE_XSS_HI leafs
>>
>>   target/i386/cpu.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH 1/2] i386/cpu: Clear FEAT_XSAVE_XSS_LO/HI leafs when CPUID_EXT_XSAVE is not available

[Yang, Weijiang]

On 1/15/2024 5:13 PM, Li, Xiaoyao wrote:
> Leaf FEAT_XSAVE_XSS_LO and FEAT_XSAVE_XSS_HI also need to be cleared
> when CPUID_EXT_XSAVE is not set.
>
> Fixes: 301e90675c3f ("target/i386: Enable support for XSAVES based features")
> 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 2524881ce245..b445e2957c4f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6926,6 +6926,8 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>       if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
>           env->features[FEAT_XSAVE_XCR0_LO] = 0;
>           env->features[FEAT_XSAVE_XCR0_HI] = 0;
> +        env->features[FEAT_XSAVE_XSS_LO] = 0;
> +        env->features[FEAT_XSAVE_XSS_HI] = 0;
>           return;
>       }

Thanks for fixing this!
Reviewed-by: Yang Weijiang <weijiang.yang@intel.com>

>   

Re: [PATCH 2/2] i386/cpu: Mask with XCR0/XSS mask for FEAT_XSAVE_XCR0_HI and FEAT_XSAVE_XSS_HI leafs

[Yang, Weijiang]

On 1/15/2024 5:13 PM, Li, Xiaoyao wrote:
> The value of FEAT_XSAVE_XCR0_HI leaf and FEAT_XSAVE_XSS_HI leaf also
> need to be masked by XCR0 and XSS mask respectively, to make it
> logically correct.
>
> Fixes: 301e90675c3f ("target/i386: Enable support for XSAVES based features")
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   target/i386/cpu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b445e2957c4f..a5c08944a483 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6946,9 +6946,9 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>       }
>   
>       env->features[FEAT_XSAVE_XCR0_LO] = mask & CPUID_XSTATE_XCR0_MASK;
> -    env->features[FEAT_XSAVE_XCR0_HI] = mask >> 32;
> +    env->features[FEAT_XSAVE_XCR0_HI] = (mask & CPUID_XSTATE_XCR0_MASK) >> 32;
>       env->features[FEAT_XSAVE_XSS_LO] = mask & CPUID_XSTATE_XSS_MASK;
> -    env->features[FEAT_XSAVE_XSS_HI] = mask >> 32;
> +    env->features[FEAT_XSAVE_XSS_HI] = (mask & CPUID_XSTATE_XSS_MASK) >> 32;
>   }

Thanks for fixing this!
Reviewed-by: Yang Weijiang <weijiang.yang@intel.com>

>   
>   /***** Steps involved on loading and filtering CPUID data

Re: [PATCH 0/2] i386/cpu: Two minor fixes for x86_cpu_enable_xsave_components()

[Paolo Bonzini]

Queued, thanks.

Paolo