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

[Zhao Liu <zhao1.liu@intel.com>]

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