Message ID | 20210722083851.24068-1-cfontana@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [for-6.1] i386: do not call cpudef-only models functions for max, host, base | expand |
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!
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(); }