On Wed, 12 Jul 2017 13:20:58 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> When commit 0bacd8b3046f ('i386: Don't set CPUClass::cpu_def on
> "max" model') removed the CPUClass::cpu_def field, we kept using
> the x86_cpu_load_def() helper directly in max_x86_cpu_initfn(),
> emulating the previous behavior when CPUClass::cpu_def was set.
>
> However, x86_cpu_load_def() is intended to help initialization of
> CPU models from the builtin_x86_defs table, and does lots of
> other steps that are not necessary for "max".
>
> One of the things x86_cpu_load_def() do is to set the properties
> listed at tcg_default_props/kvm_default_props. We must not do
> that on the "max" CPU model, otherwise under KVM we will
> incorrectly report all KVM features as always available, and the
> "svm" feature as always unavailable. The latter caused the bug
> reported at:
Maybe add that all available features for 'max' are set later at realize() time
to ones actually supported by host.
Also while looking at it, I've noticed that:
x86_cpu_load_def()
...
if (kvm_enabled()) {
if (!kvm_irqchip_in_kernel()) {
x86_cpu_change_kvm_default("x2apic", "off");
}
and
kvm_arch_get_supported_cpuid() also having
if (!kvm_irqchip_in_kernel()) {
ret &= ~CPUID_EXT_X2APIC;
}
so do we really need this duplication in x86_cpu_load_def()?
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1467599
> ("Unable to start domain: the CPU is incompatible with host CPU:
> Host CPU does not provide required features: svm")
>
> Replace x86_cpu_load_def() with simple object_property_set*()
> calls. In addition to fixing the above bug, this makes the KVM
> branch in max_x86_cpu_initfn() very similar to the existing TCG
> branch.
>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> ---
> target/i386/cpu.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index e2cd157..62d8021 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1596,15 +1596,21 @@ static void max_x86_cpu_initfn(Object *obj)
> cpu->max_features = true;
>
> if (kvm_enabled()) {
> - X86CPUDefinition host_cpudef = { };
> - uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> + char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
> + char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
> + int family, model, stepping;
>
> - host_vendor_fms(host_cpudef.vendor, &host_cpudef.family,
> - &host_cpudef.model, &host_cpudef.stepping);
> + host_vendor_fms(vendor, &family, &model, &stepping);
>
> - cpu_x86_fill_model_id(host_cpudef.model_id);
> + cpu_x86_fill_model_id(model_id);
>
> - x86_cpu_load_def(cpu, &host_cpudef, &error_abort);
this looses
env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
but it looks like kvm_arch_get_supported_cpuid() will take care of it later
at realize() time.
> + object_property_set_str(OBJECT(cpu), vendor, "vendor", &error_abort);
> + object_property_set_int(OBJECT(cpu), family, "family", &error_abort);
> + object_property_set_int(OBJECT(cpu), model, "model", &error_abort);
> + object_property_set_int(OBJECT(cpu), stepping, "stepping",
> + &error_abort);
> + object_property_set_str(OBJECT(cpu), model_id, "model-id",
> + &error_abort);
>
> env->cpuid_min_level =
> kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
