target/i386/cpu.c | 19 ++++++- target/i386/host-cpu.c | 13 +++-- target/i386/kvm/kvm-cpu.c | 105 ++++++++++++++++++++------------------ target/i386/tcg/tcg-cpu.c | 11 ++-- 4 files changed, 89 insertions(+), 59 deletions(-)
properties set by function x86_cpu_apply_props, including
kvm_default_props, tcg_default_props,
and the "vendor" property for KVM and HVF,
are actually to be set only for cpu models in builtin_x86_defs,
registered with x86_register_cpu_model_type, and not for
cpu models "base", "max", and the subclass "host".
This has been detected as a bug with Nested on AMD with cpu "host",
as svm was not turned on by default, due to the wrongful setting of
kvm_default_props via x86_cpu_apply_props.
Rectify the bug introduced in commit "i386: split cpu accelerators"
and document the functions that are builtin_x86_defs-only.
Signed-off-by: Claudio Fontana <cfontana@suse.de>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Fixes: f5cc5a5c ("i386: split cpu accelerators from cpu.c,"...)
Buglink: https://gitlab.com/qemu-project/qemu/-/issues/477
---
target/i386/cpu.c | 19 ++++++-
target/i386/host-cpu.c | 13 +++--
target/i386/kvm/kvm-cpu.c | 105 ++++++++++++++++++++------------------
target/i386/tcg/tcg-cpu.c | 11 ++--
4 files changed, 89 insertions(+), 59 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 48b55ebd0a..edb97ebbbe 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4919,6 +4919,9 @@ static uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
return r;
}
+/*
+ * Only for builtin_x86_defs models initialized with x86_register_cpudef_types.
+ */
void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
{
PropValue *pv;
@@ -4931,7 +4934,11 @@ void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
}
}
-/* Apply properties for the CPU model version specified in model */
+/*
+ * Apply properties for the CPU model version specified in model.
+ * Only for builtin_x86_defs models initialized with x86_register_cpudef_types.
+ */
+
static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUModel *model)
{
const X86CPUVersionDefinition *vdef;
@@ -4960,7 +4967,9 @@ static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUModel *model)
assert(vdef->version == version);
}
-/* Load data from X86CPUDefinition into a X86CPU object
+/*
+ * Load data from X86CPUDefinition into a X86CPU object.
+ * Only for builtin_x86_defs models initialized with x86_register_cpudef_types.
*/
static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model)
{
@@ -5051,6 +5060,12 @@ static void x86_register_cpu_model_type(const char *name, X86CPUModel *model)
type_register(&ti);
}
+
+/*
+ * register builtin_x86_defs;
+ * "max", "base" and subclasses ("host") are not registered here.
+ * See x86_cpu_register_types for all model registrations.
+ */
static void x86_register_cpudef_types(const X86CPUDefinition *def)
{
X86CPUModel *m;
diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
index 4ea9e354ea..10f8aba86e 100644
--- a/target/i386/host-cpu.c
+++ b/target/i386/host-cpu.c
@@ -150,13 +150,16 @@ void host_cpu_vendor_fms(char *vendor, int *family, int *model, int *stepping)
void host_cpu_instance_init(X86CPU *cpu)
{
- uint32_t ebx = 0, ecx = 0, edx = 0;
- char vendor[CPUID_VENDOR_SZ + 1];
+ X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
- host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
- x86_cpu_vendor_words2str(vendor, ebx, edx, ecx);
+ if (xcc->model) {
+ uint32_t ebx = 0, ecx = 0, edx = 0;
+ char vendor[CPUID_VENDOR_SZ + 1];
- object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
+ host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
+ x86_cpu_vendor_words2str(vendor, ebx, edx, ecx);
+ object_property_set_str(OBJECT(cpu), "vendor", vendor, &error_abort);
+ }
}
void host_cpu_max_instance_init(X86CPU *cpu)
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index bbe817764d..d95028018e 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -52,47 +52,6 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
return host_cpu_realizefn(cs, errp);
}
-/*
- * KVM-specific features that are automatically added/removed
- * from all CPU models when KVM is enabled.
- *
- * NOTE: features can be enabled by default only if they were
- * already available in the oldest kernel version supported
- * by the KVM accelerator (see "OS requirements" section at
- * docs/system/target-i386.rst)
- */
-static PropValue kvm_default_props[] = {
- { "kvmclock", "on" },
- { "kvm-nopiodelay", "on" },
- { "kvm-asyncpf", "on" },
- { "kvm-steal-time", "on" },
- { "kvm-pv-eoi", "on" },
- { "kvmclock-stable-bit", "on" },
- { "x2apic", "on" },
- { "kvm-msi-ext-dest-id", "off" },
- { "acpi", "off" },
- { "monitor", "off" },
- { "svm", "off" },
- { NULL, NULL },
-};
-
-void x86_cpu_change_kvm_default(const char *prop, const char *value)
-{
- PropValue *pv;
- for (pv = kvm_default_props; pv->prop; pv++) {
- if (!strcmp(pv->prop, prop)) {
- pv->value = value;
- break;
- }
- }
-
- /*
- * It is valid to call this function only for properties that
- * are already present in the kvm_default_props table.
- */
- assert(pv->prop);
-}
-
static bool lmce_supported(void)
{
uint64_t mce_cap = 0;
@@ -150,21 +109,69 @@ static void kvm_cpu_xsave_init(void)
}
}
+/*
+ * KVM-specific features that are automatically added/removed
+ * from cpudef models when KVM is enabled.
+ * Only for builtin_x86_defs models initialized with x86_register_cpudef_types.
+ *
+ * NOTE: features can be enabled by default only if they were
+ * already available in the oldest kernel version supported
+ * by the KVM accelerator (see "OS requirements" section at
+ * docs/system/target-i386.rst)
+ */
+static PropValue kvm_default_props[] = {
+ { "kvmclock", "on" },
+ { "kvm-nopiodelay", "on" },
+ { "kvm-asyncpf", "on" },
+ { "kvm-steal-time", "on" },
+ { "kvm-pv-eoi", "on" },
+ { "kvmclock-stable-bit", "on" },
+ { "x2apic", "on" },
+ { "kvm-msi-ext-dest-id", "off" },
+ { "acpi", "off" },
+ { "monitor", "off" },
+ { "svm", "off" },
+ { NULL, NULL },
+};
+
+/*
+ * Only for builtin_x86_defs models initialized with x86_register_cpudef_types.
+ */
+void x86_cpu_change_kvm_default(const char *prop, const char *value)
+{
+ PropValue *pv;
+ for (pv = kvm_default_props; pv->prop; pv++) {
+ if (!strcmp(pv->prop, prop)) {
+ pv->value = value;
+ break;
+ }
+ }
+
+ /*
+ * It is valid to call this function only for properties that
+ * are already present in the kvm_default_props table.
+ */
+ assert(pv->prop);
+}
+
static void kvm_cpu_instance_init(CPUState *cs)
{
X86CPU *cpu = X86_CPU(cs);
+ X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
host_cpu_instance_init(cpu);
- if (!kvm_irqchip_in_kernel()) {
- x86_cpu_change_kvm_default("x2apic", "off");
- } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
- x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
- }
-
- /* Special cases not set in the X86CPUDefinition structs: */
+ if (xcc->model) {
+ /* only applies to builtin_x86_defs cpus */
+ if (!kvm_irqchip_in_kernel()) {
+ x86_cpu_change_kvm_default("x2apic", "off");
+ } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
+ x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
+ }
- x86_cpu_apply_props(cpu, kvm_default_props);
+ /* Special cases not set in the X86CPUDefinition structs: */
+ x86_cpu_apply_props(cpu, kvm_default_props);
+ }
if (cpu->max_features) {
kvm_cpu_max_instance_init(cpu);
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index e96ec9bbcc..e86bc93384 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -99,7 +99,8 @@ static void tcg_cpu_xsave_init(void)
}
/*
- * TCG-specific defaults that override all CPU models when using TCG
+ * TCG-specific defaults that override cpudef models when using TCG.
+ * Only for builtin_x86_defs models initialized with x86_register_cpudef_types.
*/
static PropValue tcg_default_props[] = {
{ "vme", "off" },
@@ -109,8 +110,12 @@ static PropValue tcg_default_props[] = {
static void tcg_cpu_instance_init(CPUState *cs)
{
X86CPU *cpu = X86_CPU(cs);
- /* Special cases not set in the X86CPUDefinition structs: */
- x86_cpu_apply_props(cpu, tcg_default_props);
+ X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
+
+ if (xcc->model) {
+ /* Special cases not set in the X86CPUDefinition structs: */
+ x86_cpu_apply_props(cpu, tcg_default_props);
+ }
tcg_cpu_xsave_init();
}
--
2.26.2
On 7/22/21 10:38 AM, Claudio Fontana wrote: It seems the subject got dropped and the first line used as subject... But I'm not sure you want to start the description with it. > properties set by function x86_cpu_apply_props, including > kvm_default_props, tcg_default_props, > and the "vendor" property for KVM and HVF, > This newline is what confuses me. > are actually to be set only for cpu models in builtin_x86_defs, > registered with x86_register_cpu_model_type, and not for > cpu models "base", "max", and the subclass "host". > > This has been detected as a bug with Nested on AMD with cpu "host", > as svm was not turned on by default, due to the wrongful setting of > kvm_default_props via x86_cpu_apply_props. > > Rectify the bug introduced in commit "i386: split cpu accelerators" > and document the functions that are builtin_x86_defs-only. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > Tested-by: Alexander Bulekov <alxndr@bu.edu> > Fixes: f5cc5a5c ("i386: split cpu accelerators from cpu.c,"...) > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/477 If you want to have gitlab closes the issue once merged, you'd need to use Resolves:/Fixes: tag instead, see https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern > --- > target/i386/cpu.c | 19 ++++++- > target/i386/host-cpu.c | 13 +++-- > target/i386/kvm/kvm-cpu.c | 105 ++++++++++++++++++++------------------ > target/i386/tcg/tcg-cpu.c | 11 ++-- > 4 files changed, 89 insertions(+), 59 deletions(-)
On 7/22/21 6:13 PM, Philippe Mathieu-Daudé wrote: > On 7/22/21 10:38 AM, Claudio Fontana wrote: > > It seems the subject got dropped and the first line > used as subject... But I'm not sure you want to > start the description with it. hmm the subject got dropped from where? I see it in the mail subject.. > >> properties set by function x86_cpu_apply_props, including >> kvm_default_props, tcg_default_props, >> and the "vendor" property for KVM and HVF, >> > > This newline is what confuses me. hmm maybe better: " Some cpu properties have to be set only for cpu models in builtin_x86_defs, registered with x86_register_cpu_model_type, and not for cpu models "base", "max", and the subclass "host". These properties are the ones set by function x86_cpu_apply_props, (also including kvm_default_props, tcg_default_props), and the "vendor" property for the KVM and HVF accelerators. After recent refactoring of cpu, which also affected these properties, they were instead set unconditionally for all x86 cpus. >> This has been detected as a bug with Nested on AMD with cpu "host", >> as svm was not turned on by default, due to the wrongful setting of >> kvm_default_props via x86_cpu_apply_props. .. which set svm to "off". >> Rectify the bug introduced in commit "i386: split cpu accelerators" >> and document the functions that are builtin_x86_defs-only. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> Tested-by: Alexander Bulekov <alxndr@bu.edu> >> Fixes: f5cc5a5c ("i386: split cpu accelerators from cpu.c,"...) >> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/477 > > If you want to have gitlab closes the issue once merged, you'd > need to use Resolves:/Fixes: tag instead, see > https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern I'll try Resolves: to avoid collision with Fixes: used to mark the commit that introduced the regression. Wdyt about the new text? Thanks, Claudio > >> --- >> target/i386/cpu.c | 19 ++++++- >> target/i386/host-cpu.c | 13 +++-- >> target/i386/kvm/kvm-cpu.c | 105 ++++++++++++++++++++------------------ >> target/i386/tcg/tcg-cpu.c | 11 ++-- >> 4 files changed, 89 insertions(+), 59 deletions(-) >
On 7/23/21 10:19 AM, Claudio Fontana wrote: > On 7/22/21 6:13 PM, Philippe Mathieu-Daudé wrote: >> On 7/22/21 10:38 AM, Claudio Fontana wrote: >> >> It seems the subject got dropped and the first line >> used as subject... But I'm not sure you want to >> start the description with it. > > hmm the subject got dropped from where? I see it in the mail subject.. >> >>> properties set by function x86_cpu_apply_props, including >>> kvm_default_props, tcg_default_props, >>> and the "vendor" property for KVM and HVF, >>> >> >> This newline is what confuses me. > > hmm maybe better: > > " > Some cpu properties have to be set only for cpu models in builtin_x86_defs, > registered with x86_register_cpu_model_type, and not for > cpu models "base", "max", and the subclass "host". > > These properties are the ones set by function x86_cpu_apply_props, > (also including kvm_default_props, tcg_default_props), > and the "vendor" property for the KVM and HVF accelerators. > > After recent refactoring of cpu, which also affected these properties, > they were instead set unconditionally for all x86 cpus. > >>> This has been detected as a bug with Nested on AMD with cpu "host", >>> as svm was not turned on by default, due to the wrongful setting of >>> kvm_default_props via x86_cpu_apply_props. > > .. which set svm to "off". > >>> Rectify the bug introduced in commit "i386: split cpu accelerators" >>> and document the functions that are builtin_x86_defs-only. >>> >>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>> Tested-by: Alexander Bulekov <alxndr@bu.edu> >>> Fixes: f5cc5a5c ("i386: split cpu accelerators from cpu.c,"...) >>> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/477 >> >> If you want to have gitlab closes the issue once merged, you'd >> need to use Resolves:/Fixes: tag instead, see >> https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern > > I'll try Resolves: to avoid collision with Fixes: used to mark the commit that introduced the regression. > > Wdyt about the new text? Clearer, thanks!
© 2016 - 2024 Red Hat, Inc.