Re: [PATCH v3 1/7] target/i386: allow versioned CPUs to specify new 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 于2023年4月25日周二 00:42写道：
>>>
>>> From: Michael Roth
>>>
>>> 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
>>> Signed-off-by: Wei Huang
>>> Signed-off-by: Michael Roth
>>> Signed-off-by: Babu Moger
>>> Acked-by: Michael S. Tsirkin
>>> ---
>>> 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
Re: [PATCH v3 1/7] target/i386: allow versioned CPUs to specify new cache_info
Hi Robert,
On 4/25/23 00:42, Robert Hoo wrote:
> Babu Moger 于2023年4月25日周二 00:42写道：
>>
>> From: Michael Roth
>>
>> 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
>> Signed-off-by: Wei Huang
>> Signed-off-by: Michael Roth
>> Signed-off-by: Babu Moger
>> Acked-by: Michael S. Tsirkin
>> ---
>> 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
Re: [PATCH v3 1/7] target/i386: allow versioned CPUs to specify new cache_info
Babu Moger 于2023年4月25日周二 00:42写道：
>
> From: Michael Roth
>
> 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
> Signed-off-by: Wei Huang
> Signed-off-by: Michael Roth
> Signed-off-by: Babu Moger
> Acked-by: Michael S. Tsirkin
> ---
> 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;
> +}
> +
[PATCH v3 1/7] target/i386: allow versioned CPUs to specify new cache_info
From: Michael Roth
New EPYC CPUs versions require small changes to their cache_info's.
Because current QEMU x86 CPU definition does not support cache
versions, we would have to declare a new CPU type for each such case.
To avoid this duplication, the patch allows new cache_info pointers
to be specified for a new CPU version.
Co-developed-by: Wei Huang
Signed-off-by: Wei Huang
Signed-off-by: Michael Roth
Signed-off-by: Babu Moger
Acked-by: Michael S. Tsirkin
---
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 */
+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 = _l1d_cache;
--
2.34.1
