Re: [PATCH v3 1/7] target/i386: allow versioned CPUs to specify new cache_info

[Moger, Babu]

Hi Robert,

On 4/25/23 00:42, Robert Hoo wrote:
> Babu Moger <babu.moger@amd.com> 于2023年4月25日周二 00:42写道：
>>
>> From: Michael Roth <michael.roth@amd.com>
>>
>> New EPYC CPUs versions require small changes to their cache_info's.
> 
> Do you mean, for the real HW of EPYC CPU, each given model, e.g. Rome,
> has HW version updates periodically?

Yes. Real hardware can change slightly changing the cache properties, but
everything else exactly same as the base HW. But this is not a common
thing. We don't see the need for adding new EPYC model for these cases.
That is the reason we added cache_info here.
> 
>> Because current QEMU x86 CPU definition does not support cache
>> versions,
> 
> cache version --> versioned cache info

Sure.
> 
>> we would have to declare a new CPU type for each such case.
> 
> My understanding was, for new HW CPU model, we should define a new
> vCPU model mapping it. But if answer to my above question is yes, i.e.
> new HW version of same CPU model, looks like it makes sense to some
> extent.

Please see my response above.

> 
>> To avoid this duplication, the patch allows new cache_info pointers
>> to be specified for a new CPU version.
> 
> "To avoid the dup work, the patch adds "cache_info" in 
> X86CPUVersionDefinition"

Sure

>>
>> Co-developed-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Wei Huang <wei.huang2@amd.com>
>> Signed-off-by: Michael Roth <michael.roth@amd.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  target/i386/cpu.c | 36 +++++++++++++++++++++++++++++++++---
>>  1 file changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 6576287e5b..e3d9eaa307 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -1598,6 +1598,7 @@ typedef struct X86CPUVersionDefinition {
>>      const char *alias;
>>      const char *note;
>>      PropValue *props;
>> +    const CPUCaches *const cache_info;
>>  } X86CPUVersionDefinition;
>>
>>  /* Base definition for a CPU model */
>> @@ -5192,6 +5193,32 @@ static void x86_cpu_apply_version_props(X86CPU *cpu, 
>> X86CPUModel *model)
>>      assert(vdef->version == version);
>>  }
>>
>> +/* Apply properties for the CPU model version specified in model */
> 
> I don't think this comment matches below function.

Ok. Will remove it.

> 
>> +static const CPUCaches *x86_cpu_get_version_cache_info(X86CPU *cpu,
>> +                                                       X86CPUModel *model)
> 
> Will "version" --> "versioned" be better?

Sure.

> 
>> +{
>> +    const X86CPUVersionDefinition *vdef;
>> +    X86CPUVersion version = x86_cpu_model_resolve_version(model);
>> +    const CPUCaches *cache_info = model->cpudef->cache_info;
>> +
>> +    if (version == CPU_VERSION_LEGACY) {
>> +        return cache_info;
>> +    }
>> +
>> +    for (vdef = x86_cpu_def_get_versions(model->cpudef); vdef->version; 
>> vdef++) {
>> +        if (vdef->cache_info) {
>> +            cache_info = vdef->cache_info;
>> +        }
> 
> No need to assign "cache_info" when traverse the vdef list, but in
> below version matching block, do the assignment. Or, do you mean to
> have last valid cache info (during the traverse) returned? e.g. v2 has
> valid cache info, but v3 doesn't.
>> +
>> +        if (vdef->version == version) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    assert(vdef->version == version);
>> +    return cache_info;
>> +}
>> +

-- 
Thanks
Babu Moger