Re: [PATCH v4 06/23] i386/kvm: Initialize x86_ext_save_areas[] based on KVM support

[Paolo Bonzini <pbonzini@redhat.com>]

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;
>       }
>   }
>