At present, in KVM, x86_ext_save_areas[] is initialized based on Host
CPUIDs.

Except AMX xstates, both user & supervisor xstates have their
information exposed in KVM_GET_SUPPORTED_CPUID. Therefore, their entries
in x86_ext_save_areas[] should be filled based on KVM support.

For AMX xstates (XFEATURE_MASK_XTILE_DATA and XFEATURE_MASK_XTILE_CFG),
KVM doesn't report their details before they (mainly
XSTATE_XTILE_DATA_MASK) get permission on host. But this happens within
the function kvm_request_xsave_components(), after the current
initialization. So still fill AMX entries with Host CPUIDs.

In addition, drop a check: "if (eax != 0)" when assert the assert the
size of xstate. In fact, this check is incorrect, since any valid
xstate should have non-zero size of xstate area.

Tested-by: Farrah Chen <farrah.chen@intel.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Changes Since v3:
 - New commit.
---
 target/i386/cpu.h         |  3 +++
 target/i386/kvm/kvm-cpu.c | 23 +++++++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 3d74afc5a8e7..f065527757c4 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -609,6 +609,9 @@ typedef enum X86Seg {
 
 #define XSTATE_DYNAMIC_MASK             (XSTATE_XTILE_DATA_MASK)
 
+#define XSTATE_XTILE_MASK               (XSTATE_XTILE_DATA_MASK | \
+                                         XSTATE_XTILE_CFG_MASK)
+
 #define ESA_FEATURE_ALIGN64_BIT         1
 #define ESA_FEATURE_XFD_BIT             2
 
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index 9c25b5583955..2e2d47d2948a 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -136,7 +136,7 @@ static void kvm_cpu_max_instance_init(X86CPU *cpu)
 static void kvm_cpu_xsave_init(void)
 {
     static bool first = true;
-    uint32_t eax, ebx, ecx, edx;
+    uint32_t eax, ebx, ecx, unused;
     int i;
 
     if (!first) {
@@ -154,12 +154,23 @@ static void kvm_cpu_xsave_init(void)
         if (!esa->size) {
             continue;
         }
-        host_cpuid(0xd, i, &eax, &ebx, &ecx, &edx);
-        if (eax != 0) {
-            assert(esa->size == eax);
-            esa->offset = ebx;
-            esa->ecx = ecx;
+
+        /*
+         * AMX xstates are supported in KVM_GET_SUPPORTED_CPUID only when
+         * XSTATE_XTILE_DATA_MASK gets guest permission in
+         * kvm_request_xsave_components().
+         */
+        if (!((1 << i) & XSTATE_XTILE_MASK)) {
+            eax = kvm_arch_get_supported_cpuid(kvm_state, 0xd, i, R_EAX);
+            ebx = kvm_arch_get_supported_cpuid(kvm_state, 0xd, i, R_EBX);
+            ecx = kvm_arch_get_supported_cpuid(kvm_state, 0xd, i, R_ECX);
+        } else {
+            host_cpuid(0xd, i, &eax, &ebx, &ecx, &unused);
         }
+
+        assert(esa->size == eax);
+        esa->offset = ebx;
+        esa->ecx = ecx;
     }
 }
 
-- 
2.34.1

On 11/18/25 04:42, Zhao Liu wrote:
> At present, in KVM, x86_ext_save_areas[] is initialized based on Host
> CPUIDs.
> 
> Except AMX xstates, both user & supervisor xstates have their
> information exposed in KVM_GET_SUPPORTED_CPUID. Therefore, their entries
> in x86_ext_save_areas[] should be filled based on KVM support.
> 
> For AMX xstates (XFEATURE_MASK_XTILE_DATA and XFEATURE_MASK_XTILE_CFG),
> KVM doesn't report their details before they (mainly
> XSTATE_XTILE_DATA_MASK) get permission on host. But this happens within
> the function kvm_request_xsave_components(), after the current
> initialization. So still fill AMX entries with Host CPUIDs.
> 
> In addition, drop a check: "if (eax != 0)" when assert the assert the
> size of xstate. In fact, this check is incorrect, since any valid
> xstate should have non-zero size of xstate area.
> 
> Tested-by: Farrah Chen <farrah.chen@intel.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
> Changes Since v3:
>   - New commit.
> ---
>   target/i386/cpu.h         |  3 +++
>   target/i386/kvm/kvm-cpu.c | 23 +++++++++++++++++------
>   2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 3d74afc5a8e7..f065527757c4 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -609,6 +609,9 @@ typedef enum X86Seg {
>   
>   #define XSTATE_DYNAMIC_MASK             (XSTATE_XTILE_DATA_MASK)
>   
> +#define XSTATE_XTILE_MASK               (XSTATE_XTILE_DATA_MASK | \
> +                                         XSTATE_XTILE_CFG_MASK)
> +
>   #define ESA_FEATURE_ALIGN64_BIT         1
>   #define ESA_FEATURE_XFD_BIT             2
>   
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index 9c25b5583955..2e2d47d2948a 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -136,7 +136,7 @@ static void kvm_cpu_max_instance_init(X86CPU *cpu)
>   static void kvm_cpu_xsave_init(void)
>   {
>       static bool first = true;
> -    uint32_t eax, ebx, ecx, edx;
> +    uint32_t eax, ebx, ecx, unused;
>       int i;
>   
>       if (!first) {
> @@ -154,12 +154,23 @@ static void kvm_cpu_xsave_init(void)
>           if (!esa->size) {
>               continue;
>           }
> -        host_cpuid(0xd, i, &eax, &ebx, &ecx, &edx);
> -        if (eax != 0) {
> -            assert(esa->size == eax);
> -            esa->offset = ebx;
> -            esa->ecx = ecx;
> +
> +        /*
> +         * AMX xstates are supported in KVM_GET_SUPPORTED_CPUID only when
> +         * XSTATE_XTILE_DATA_MASK gets guest permission in
> +         * kvm_request_xsave_components().
> +         */
> +        if (!((1 << i) & XSTATE_XTILE_MASK)) {

This should be XSTATE_DYNAMIC_MASK, but I don't like getting the 
information differently.  My understanding is that this is useful for 
the next patch, but I don't understand well why the next patch is 
needed. The commit message doesn't help.

Can you move the call to kvm_cpu_xsave_init() after 
x86_cpu_enable_xsave_components()?  Is it used anywhere before the CPU 
is running?

Paolo

> +            eax = kvm_arch_get_supported_cpuid(kvm_state, 0xd, i, R_EAX);
> +            ebx = kvm_arch_get_supported_cpuid(kvm_state, 0xd, i, R_EBX);
> +            ecx = kvm_arch_get_supported_cpuid(kvm_state, 0xd, i, R_ECX);
> +        } else {
> +            host_cpuid(0xd, i, &eax, &ebx, &ecx, &unused);
>           }
> +
> +        assert(esa->size == eax);
> +        esa->offset = ebx;
> +        esa->ecx = ecx;
>       }
>   }
>

Hi Paolo,

> > +        /*
> > +         * AMX xstates are supported in KVM_GET_SUPPORTED_CPUID only when
> > +         * XSTATE_XTILE_DATA_MASK gets guest permission in
> > +         * kvm_request_xsave_components().
> > +         */
> > +        if (!((1 << i) & XSTATE_XTILE_MASK)) {
> 
> This should be XSTATE_DYNAMIC_MASK, 

XSTATE_DYNAMIC_MASK covers both XSTATE_XTILE_DATA_MASK and
XSTATE_XTILE_CFG_MASK, and XSTATE_DYNAMIC_MASK only has
XSTATE_XTILE_DATA_MASK.

On KVM side, kvm_get_filtered_xcr0() will mask off XTILE_CFG if
XTILE_DATA is filtered out.

So from this KVM logic, at current point, getting XTILE_CFG information
seems meaningless. Therefore I skip both XTILE_DATA and XTILE_CFG.

> but I don't like getting the information differently.  My understanding is
> that this is useful for the next patch, but I don't understand well why
> the next patch is needed. The commit message doesn't help.

My bad, my commit messages are not orginized well. Both patch 6 & 7 are
serving patch 8. Please let me explain in detail:

* In 0xD encoding, Arch LBR is checked specially, that's not needed.
  Before 0xD encoding, any dependencies check should be done. So there's
  patch 8 to drop such special check for Arch LBR.

  But there're 2 differences should be clarified before patch 8.

  - Arch LBR xstate CPUID is filled by x86_cpu_get_supported_cpuid(),
    and other xstates in 0xD is filled based on x86_ext_save_areas[].

    Ideally, all xstates CPUIDs should be from x86_ext_save_areas[] and
    x86_ext_save_areas[] is initialized by accelerators.

    So there's patch 7 to make ARCH LBR also use x86_ext_save_areas[] to
    encode its CPUID.

  - Arch LBR gets its xstate information by
    x86_cpu_get_supported_cpuid(), and other xstates (for KVM) in the
    x86_ext_save_areas[] are initialized by host_cpuid().

    I think this is a confusion. QEMU KVM doesn't need to use
    host_cpuid() to get xstates information. In fact, it should get this
    from KVM's get_cpuid ioctl (although KVM also use host info directly),
    just like HVF does.

    So this is what patch 6 does - avoid using host_cpuid as much as
    possible. For AMX states that require dynamic permission acquisition,
    since memory allocation is involved, I think it also makes sense to
    continue using host_cpuid().

The changes in patches 6~8 are rather trivial, mainly to address the
comments from the previous version... such as whether such replacements
(in patch 8) would change functionality, etc. So, in this version I'm
doing it step by step.

> Can you move the call to kvm_cpu_xsave_init() after
> x86_cpu_enable_xsave_components()?  Is it used anywhere before the CPU is
> running?

Yes, this is an "ugly" palce. I did not fully defer the initialization
of the xstate array earlier also because I observed that both HVF and
TCG have similar xsave initialization interfaces within their accelerator's
cpu_instance_init() function.

I think it might be better to do the same thing for HVF & TCG as well
(i.e., defer xstate initialization). Otherwise, if we only modify QEMU
KVM logic, it looks a bit fragmented... What do you think?

Thanks for your patience,
Zhao

> > Can you move the call to kvm_cpu_xsave_init() after
> > x86_cpu_enable_xsave_components()?  Is it used anywhere before the CPU is
> > running?
> 
> Yes, this is an "ugly" palce. I did not fully defer the initialization
> of the xstate array earlier also because I observed that both HVF and
> TCG have similar xsave initialization interfaces within their accelerator's
> cpu_instance_init() function.
> 
> I think it might be better to do the same thing for HVF & TCG as well
> (i.e., defer xstate initialization). Otherwise, if we only modify QEMU
> KVM logic, it looks a bit fragmented... What do you think?

Ah, kvm_arch_get_supported_cpuid() currently caches the obtained CPUID.
Delaying xstate initialization would require refreshing the previously
cached old CPUID... This makes the cost of delaying xstate
initialization higher, and I think this way becames not appropriate.

Either keep the current different ways, or back to use host_cpuid for
everything :( . Which do you think is better?

Regards,
Zhao