[v1] cpu/i386: update xsave components after CPUID filtering

[PATCH] cpu/i386: update xsave components after CPUID filtering

Posted by Huanyu ZHAI 1 year, 5 months ago

Subject: [PATCH] cpu/i386: update xsave components after CPUID filtering

On i386 platform, CPUID data are setup through three consecutive steps: CPU model definition, expansion and filtering.
XSAVE components are enabled during the expansion stage, by checking if they are enabled in CPUID. However, it is still
probable that some XSAVE features will be enabled/disabled during the filtering stage and the XSAVE components left unchanged.
Inconsistency between XSAVE features and enabled XSAVE components can lead to problems on some Linux guests in the absence of
the following patch in the kernel:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1452368.html

A simple case to reproduce this problem is to start a SUSE 12 SP3 guest with cpu model set to Skylake-Server:
$ qemu-system-x86_64 -cpu Skylake-Server ...

In the SUSE 12 SP3 guest, one can observe that PKRU will be enabled without Intel PKU's presence.
That's because on platform with Skylake-Server cpus, Intel PKU is disabled during x86_cpu_filter_features(),
but the XSAVE PKRU bit was enabled by x86_cpu_expand_features().

Signed-off-by: Huanyu ZHAI zhaihuanyu@huawei.com<mailto:zhaihuanyu@huawei.com>
Signed-off-by: Xin Wang wangxinxin.wang@huawei.com<mailto:wangxinxin.wang@huawei.com>
---
target/i386/cpu.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 22b681ca37..2ee574cf05 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6362,6 +6362,9 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
             mark_unavailable_features(cpu, FEAT_7_0_EBX, CPUID_7_0_EBX_INTEL_PT, prefix);
         }
     }
+
+    /* Update XSAVE components again based on the filtered CPU feature flags */
+    x86_cpu_enable_xsave_components(cpu);
}

static void x86_cpu_hyperv_realize(X86CPU *cpu)
--
2.27.0

Re: [PATCH] cpu/i386: update xsave components after CPUID filtering

Posted by Paolo Bonzini 1 year, 2 months ago

On 11/23/22 19:17, Huanyu ZHAI wrote:
> Subject: [PATCH] cpu/i386: update xsave components after CPUID filtering
> 
> On i386 platform, CPUID data are setup through three consecutive steps: 
> CPU model definition, expansion and filtering.
> 
> XSAVE components are enabled during the expansion stage, by checking if 
> they are enabled in CPUID. However, it is still
> 
> probable that some XSAVE features will be enabled/disabled during the 
> filtering stage and the XSAVE components left unchanged.
> 
> Inconsistency between XSAVE features and enabled XSAVE components can 
> lead to problems on some Linux guests in the absence of

The patch is correct, but I think you can also remove the existing call 
to x86_cpu_enable_xsave_components().  Can you test that?

Thanks,

Paolo

> the following patch in the kernel:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1452368.html <https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1452368.html>
> 
> A simple case to reproduce this problem is to start a SUSE 12 SP3 guest 
> with cpu model set to Skylake-Server:
> 
> $ qemu-system-x86_64 -cpu Skylake-Server ...
> 
> In the SUSE 12 SP3 guest, one can observe that PKRU will be enabled 
> without Intel PKU's presence.
> 
> That's because on platform with Skylake-Server cpus, Intel PKU is 
> disabled during x86_cpu_filter_features(),
> 
> but the XSAVE PKRU bit was enabled by x86_cpu_expand_features().
> 
> Signed-off-by: Huanyu ZHAI zhaihuanyu@huawei.com 
> <mailto:zhaihuanyu@huawei.com>
> 
> Signed-off-by: Xin Wang wangxinxin.wang@huawei.com 
> <mailto:wangxinxin.wang@huawei.com>
> 
> ---
> 
> target/i386/cpu.c | 3 +++
> 
> 1 file changed, 3 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> 
> index 22b681ca37..2ee574cf05 100644
> 
> --- a/target/i386/cpu.c
> 
> +++ b/target/i386/cpu.c
> 
> @@ -6362,6 +6362,9 @@ static void x86_cpu_filter_features(X86CPU *cpu, 
> bool verbose)
> 
>               mark_unavailable_features(cpu, FEAT_7_0_EBX, 
> CPUID_7_0_EBX_INTEL_PT, prefix);
> 
>           }
> 
>       }
> 
> +
> 
> +    /* Update XSAVE components again based on the filtered CPU feature 
> flags */
> 
> +    x86_cpu_enable_xsave_components(cpu);
> 
> }
> 
> static void x86_cpu_hyperv_realize(X86CPU *cpu)
> 
> -- 
> 
> 2.27.0
>

Re: [PATCH] cpu/i386: update xsave components after CPUID filtering

Posted by Dr. David Alan Gilbert 1 year, 2 months ago

* Huanyu ZHAI (huanyu.zhai@outlook.com) wrote:
> Subject: [PATCH] cpu/i386: update xsave components after CPUID filtering
> 
> On i386 platform, CPUID data are setup through three consecutive steps: CPU model definition, expansion and filtering.
> XSAVE components are enabled during the expansion stage, by checking if they are enabled in CPUID. However, it is still
> probable that some XSAVE features will be enabled/disabled during the filtering stage and the XSAVE components left unchanged.
> Inconsistency between XSAVE features and enabled XSAVE components can lead to problems on some Linux guests in the absence of
> the following patch in the kernel:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1452368.html
> 
> A simple case to reproduce this problem is to start a SUSE 12 SP3 guest with cpu model set to Skylake-Server:
> $ qemu-system-x86_64 -cpu Skylake-Server ...
> 
> In the SUSE 12 SP3 guest, one can observe that PKRU will be enabled without Intel PKU's presence.
> That's because on platform with Skylake-Server cpus, Intel PKU is disabled during x86_cpu_filter_features(),
> but the XSAVE PKRU bit was enabled by x86_cpu_expand_features().

I just spotted this when trying to clear out my mail folder;
this isn't a 'trivial'!

I'm not sure if it's right or not, but I've cc'd in Paolo and Marcelo as
x86 maintainers.

Dave

> Signed-off-by: Huanyu ZHAI zhaihuanyu@huawei.com<mailto:zhaihuanyu@huawei.com>
> Signed-off-by: Xin Wang wangxinxin.wang@huawei.com<mailto:wangxinxin.wang@huawei.com>
> ---
> target/i386/cpu.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 22b681ca37..2ee574cf05 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6362,6 +6362,9 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>              mark_unavailable_features(cpu, FEAT_7_0_EBX, CPUID_7_0_EBX_INTEL_PT, prefix);
>          }
>      }
> +
> +    /* Update XSAVE components again based on the filtered CPU feature flags */
> +    x86_cpu_enable_xsave_components(cpu);
> }
> 
> static void x86_cpu_hyperv_realize(X86CPU *cpu)
> --
> 2.27.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK