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

[Moger, Babu]

Hi Robert,

On 4/25/23 10:22, Moger, Babu wrote:
> 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.

Forgot to respond to this comment.
Yes. That is correct. Idea is to get the valid cache_info from the
previous version if the latest does not have one.
Also tested it to verify the case. Good question.
Thanks
Babu Moger