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.
>> +
>> + if (vdef->version == version) {
>> + break;
>> + }
>> + }
>> +
>> + assert(vdef->version == version);
>> + return cache_info;
>> +}
>> +
--
Thanks
Babu Moger
