Re: [PATCH 1/6] target/i386: Add AVX512 state when AVX10 is supported

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/28/24 10:25, Tao Su wrote:
> On Mon, Oct 28, 2024 at 09:41:14AM +0100, Paolo Bonzini wrote:
>> On 10/28/24 03:45, Tao Su wrote:
>>> AVX10 state enumeration in CPUID leaf D and enabling in XCR0 register
>>> are identical to AVX512 state regardless of the supported vector lengths.
>>>
>>> Given that some E-cores will support AVX10 but not support AVX512, add
>>> AVX512 state components to guest when AVX10 is enabled.
>>>
>>> Tested-by: Xuelian Guo <xuelian.guo@intel.com>
>>> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
>>> ---
>>>    target/i386/cpu.c | 14 ++++++++++++++
>>>    target/i386/cpu.h |  2 ++
>>>    2 files changed, 16 insertions(+)
>>>
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index 1ff1af032e..d845ff5e4e 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -7177,6 +7177,13 @@ static void x86_cpu_reset_hold(Object *obj, ResetType type)
>>>            }
>>>            if (env->features[esa->feature] & esa->bits) {
>>>                xcr0 |= 1ull << i;
>>> +            continue;
>>> +        }
>>> +        if (i == XSTATE_OPMASK_BIT || i == XSTATE_ZMM_Hi256_BIT ||
>>> +            i == XSTATE_Hi16_ZMM_BIT) {
>>
>> Can you confirm that XSTATE_ZMM_Hi256_BIT depends on AVX10 and not
>> AVX10-512?
>>
> 
> Sorry, I should attach AVX10.2 spec [*].
> 
> In 3.1.3, spec said Intel AVX10 state enumeration in CPUID leaf 0xD and
> enabling in XCR0 register are identical to Intel AVX-512 regardless of the
> maximum vector length supported.
> 
> So XSTATE_ZMM_Hi256_BIT doesn't depend on AVX10-512.
> 
> [*] https://cdrdv2.intel.com/v1/dl/getContent/828965

Ok, thanks.

Another related issue is that kvm_cpu_xsave_init() is using esa->feature 
and esa->bits, which misses these three features.

I think we need to change the code to not look at esa->feature at all. 
I'll send a v2 of your series.

Paolo