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

[Robert Hoo]

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;
> +}
> +