| Message ID | 20230424163401.23018-2-babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add EPYC-Genoa model and update previous EPYC Models | expand |
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? > Because current QEMU x86 CPU definition does not support cache > versions, cache version --> versioned cache info > 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. > 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" > > 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. > +static const CPUCaches *x86_cpu_get_version_cache_info(X86CPU *cpu, > + X86CPUModel *model) Will "version" --> "versioned" be better? > +{ > + 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; > +} > +
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; >> +} >> +
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
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 */ +static const CPUCaches *x86_cpu_get_version_cache_info(X86CPU *cpu, + X86CPUModel *model) +{ + 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; + } + + if (vdef->version == version) { + break; + } + } + + assert(vdef->version == version); + return cache_info; +} + /* * Load data from X86CPUDefinition into a X86CPU object. * Only for builtin_x86_defs models initialized with x86_register_cpudef_types. @@ -5224,7 +5251,7 @@ static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model) } /* legacy-cache defaults to 'off' if CPU model provides cache info */ - cpu->legacy_cache = !def->cache_info; + cpu->legacy_cache = !x86_cpu_get_version_cache_info(cpu, model); env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR; @@ -6703,14 +6730,17 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) /* Cache information initialization */ if (!cpu->legacy_cache) { - if (!xcc->model || !xcc->model->cpudef->cache_info) { + const CPUCaches *cache_info = + x86_cpu_get_version_cache_info(cpu, xcc->model); + + if (!xcc->model || !cache_info) { g_autofree char *name = x86_cpu_class_get_model_name(xcc); error_setg(errp, "CPU model '%s' doesn't support legacy-cache=off", name); return; } env->cache_info_cpuid2 = env->cache_info_cpuid4 = env->cache_info_amd = - *xcc->model->cpudef->cache_info; + *cache_info; } else { /* Build legacy cache information */ env->cache_info_cpuid2.l1d_cache = &legacy_l1d_cache;