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@xxxxxxx> 于2023年4月25日周二 00:42写道：
>>>
>>> From: Michael Roth <michael.roth@xxxxxxx>
>>>
>>> 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@xxxxxxx>
>>> Signed-off-by: Wei Huang <wei.huang2@xxxxxxx>
>>> Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
>>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>>> Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>>> ---
>>> 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
