Re: [PATCH] target/i386: Fix cpuid level for AMD

[Babu Moger]

On 6/29/21 9:06 AM, Dr. David Alan Gilbert wrote:
> * zhenwei pi (pizhenwei@bytedance.com) wrote:
>> A AMD server typically has cpuid level 0x10(test on Rome/Milan), it
>> should not be changed to 0x1f in multi-dies case.
>>
>> Fixes: a94e1428991 (target/i386: Add CPUID.1F generation support
>> for multi-dies PCMachine)
>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> 
> (Copying in Babu)
> 
> Hmm I think you're right.  I've cc'd in Babu and Wei.
> 
> Eduardo: What do we need to do about compatibility, do we need to wire
> this to machine type or CPU version?
> 
> Dave
> 
>> ---
>>  target/i386/cpu.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index a9fe1662d3..3934c559e4 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -5961,8 +5961,12 @@ void x86_cpu_expand_features(X86CPU *cpu, Error 
>> **errp)
>>              }
>>          }
>>  
>> -        /* CPU topology with multi-dies support requires CPUID[0x1F] */
>> -        if (env->nr_dies > 1) {
>> +        /*
>> +         * Intel CPU topology with multi-dies support requires CPUID[0x1F].
>> +         * For AMD Rome/Milan, cpuid level is 0x10, and guest OS should 
>> detect
>> +         * extended toplogy by leaf 0xB. Only adjust it for Intel CPU.

The patch appears correct to me. AMD should use leaf 0xB to detect
extended topology. What is the problem here? Or is it just correcting the
cpuid based on the SPECS?

AMD uses nr_dies to simulate some topology. I dont know if it could become
a problem after this patch.


>> +         */
>> +        if ((env->nr_dies > 1) && IS_INTEL_CPU(env)) {
>>              x86_cpu_adjust_level(cpu, &env->cpuid_min_level, 0x1F);
>>          }
>>  
>> -- 
>> 2.25.1
>>
>>