Re: [Qemu-devel] [PATCH v4 31/33] pc: Add default KVM features fields to PCMachineClass

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Aug 14, 2014 at 04:26:00PM -0300, Eduardo Habkost wrote:
> With the new fields, the x86_cpu_compat_disable_kvm_features() calls on
> pc_compat_*() functions can be replace 

be replaced

> by simple field initialization on
> class_init functions. This gets us one step closer to eliminating all
> pc_compat_*() functions.
> 
> Those new fields may eventually become simple compat_props properties,
> if we QOMify the accelerator code, and move the KVM-specific CPUID code
> to a x86-kvm-accel class.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/pc.c         | 15 +++++++++++++++
>  hw/i386/pc_piix.c    |  7 ++++---
>  hw/i386/pc_q35.c     |  2 +-
>  include/hw/i386/pc.h |  3 +++
>  target-i386/cpu.c    | 25 ++++++++++++-------------
>  target-i386/cpu.h    |  2 +-
>  6 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index dba7f62..a0eaaf6 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -24,6 +24,7 @@
>  #include "hw/hw.h"
>  #include "hw/i386/pc.h"
>  #include "hw/char/serial.h"
> +#include "hw/i386/cpu.h"
>  #include "hw/i386/apic.h"
>  #include "hw/block/fdc.h"
>  #include "hw/ide.h"
> @@ -1020,6 +1021,10 @@ void pc_cpus_init(PCMachineState *pcms, DeviceState *icc_bridge)
>      Error *error = NULL;
>      unsigned long apic_id_limit;
>      MachineState *machine = MACHINE(pcms);
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +
> +    x86_cpu_set_kvm_defaults(pcmc->kvm_default_features,
> +                             pcmc->kvm_default_unset_features);
>  
>      /* init CPUs */
>      current_cpu_model = machine->cpu_model;
> @@ -1687,6 +1692,16 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
>      pcmc->gigabyte_align = true;
>      pcmc->has_reserved_memory = true;
>      pcmc->kvmclock_enabled = true;
> +    pcmc->kvm_default_features[FEAT_KVM] =
> +        (1 << KVM_FEATURE_CLOCKSOURCE) |
> +        (1 << KVM_FEATURE_NOP_IO_DELAY) |
> +        (1 << KVM_FEATURE_CLOCKSOURCE2) |
> +        (1 << KVM_FEATURE_ASYNC_PF) |
> +        (1 << KVM_FEATURE_STEAL_TIME) |
> +        (1 << KVM_FEATURE_PV_EOI) |
> +        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT),
> +    pcmc->kvm_default_features[FEAT_1_ECX] = CPUID_EXT_X2APIC,
> +    pcmc->kvm_default_unset_features[FEAT_1_ECX] = CPUID_EXT_MONITOR,
>  
>      mc->get_hotplug_handler = pc_get_hotpug_handler;
>      mc->default_boot_order = "cad";
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 988583c..9ec63d5 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -298,7 +298,6 @@ static void pc_compat_2_0(MachineState *machine)
>  static void pc_compat_1_7(MachineState *machine)
>  {
>      pc_compat_2_0(machine);
> -    x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
>  }
>  
>  static void pc_compat_1_6(MachineState *machine)
> @@ -325,7 +324,6 @@ static void pc_compat_1_3(MachineState *machine)
>  static void pc_compat_1_2(MachineState *machine)
>  {
>      pc_compat_1_3(machine);
> -    x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
>  }
>  
>  /* PC compat function for pc-0.10 to pc-0.13 */
> @@ -386,7 +384,6 @@ static void pc_init_pci_no_kvmclock(MachineState *machine)
>  
>  static void pc_init_isa(MachineState *machine)
>  {
> -    x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
>      pc_init1(machine);
>  }
>  
> @@ -494,6 +491,7 @@ static void pc_i440fx_machine_v1_7_class_init(ObjectClass *oc, void *data)
>      pcmc->smbios_defaults = false;
>      pcmc->gigabyte_align = false;
>      pcmc->legacy_acpi_table_size = 6414;
> +    pcmc->kvm_default_features[FEAT_1_ECX] &= ~CPUID_EXT_X2APIC;
>  }
>  
>  static const TypeInfo pc_i440fx_machine_v1_7_type_info = {
> @@ -633,6 +631,7 @@ static const TypeInfo pc_machine_v1_3_type_info = {
>  static void pc_machine_v1_2_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
>      static GlobalProperty compat_props[] = {
>          PC_COMPAT_1_2,
>          { /* end of list */ }
> @@ -641,6 +640,7 @@ static void pc_machine_v1_2_class_init(ObjectClass *oc, void *data)
>      mc->init = pc_init_pci_1_2;
>      mc->name = "pc-1.2";
>      machine_class_add_compat_props(mc, compat_props);
> +    pcmc->kvm_default_features[FEAT_KVM] &= ~KVM_FEATURE_PV_EOI;
>  }
>  
>  static const TypeInfo pc_machine_v1_2_type_info = {
> @@ -984,6 +984,7 @@ static void isapc_machine_class_init(ObjectClass *oc, void *data)
>      pcmc->smbios_legacy_mode = true;
>      pcmc->has_reserved_memory = false;
>      pcmc->compat_apic_id_mode = true;
> +    pcmc->kvm_default_features[FEAT_KVM] &= ~KVM_FEATURE_PV_EOI;
>  }
>  
>  static const TypeInfo isapc_machine_type_info = {
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 2ebd598..5878964 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -271,7 +271,6 @@ static void pc_compat_2_0(MachineState *machine)
>  static void pc_compat_1_7(MachineState *machine)
>  {
>      pc_compat_2_0(machine);
> -    x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
>  }
>  
>  static void pc_compat_1_6(MachineState *machine)
> @@ -389,6 +388,7 @@ static void pc_q35_machine_v1_7_class_init(ObjectClass *oc, void *data)
>      mc->name = "pc-q35-1.7";
>      pcmc->smbios_defaults = false;
>      pcmc->gigabyte_align = false;
> +    pcmc->kvm_default_features[FEAT_1_ECX] &= ~CPUID_EXT_X2APIC;
>  }
>  
>  static TypeInfo pc_q35_machine_v1_7_type_info = {
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 460ff21..b7f36d6 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -8,6 +8,7 @@
>  #include "hw/block/fdc.h"
>  #include "net/net.h"
>  #include "hw/i386/ioapic.h"
> +#include "hw/i386/cpu.h"
>  #include "hw/boards.h"
>  
>  #include "qemu/range.h"
> @@ -66,6 +67,8 @@ struct PCMachineClass {
>      bool has_reserved_memory;
>      bool kvmclock_enabled;
>      bool compat_apic_id_mode;
> +    uint32_t kvm_default_features[FEATURE_WORDS];
> +    uint32_t kvm_default_unset_features[FEATURE_WORDS];
>  };
>  
>  typedef struct PCMachineState PCMachineState;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e86f99e..e02b0d9 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -446,27 +446,26 @@ typedef struct model_features_t {
>  
>  /* KVM-specific features that are automatically added to all CPU models
>   * when KVM is enabled.
> + *
> + * The default is to enable all KVM features, but machine-types may override it
> + * to keep a stable ABI. See x86_cpu_set_kvm_defaults().
>   */
>  static uint32_t kvm_default_features[FEATURE_WORDS] = {
> -    [FEAT_KVM] = (1 << KVM_FEATURE_CLOCKSOURCE) |
> -        (1 << KVM_FEATURE_NOP_IO_DELAY) |
> -        (1 << KVM_FEATURE_CLOCKSOURCE2) |
> -        (1 << KVM_FEATURE_ASYNC_PF) |
> -        (1 << KVM_FEATURE_STEAL_TIME) |
> -        (1 << KVM_FEATURE_PV_EOI) |
> -        (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT),
> -    [FEAT_1_ECX] = CPUID_EXT_X2APIC,
> +    [FEAT_KVM] = ~0,
>  };
>  
>  /* Features that are not added by default to any CPU model when KVM is enabled.
>   */
> -static uint32_t kvm_default_unset_features[FEATURE_WORDS] = {
> -    [FEAT_1_ECX] = CPUID_EXT_MONITOR,
> -};
> +static uint32_t kvm_default_unset_features[FEATURE_WORDS];
>  
> -void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features)
> +/* Override default-enabled and default-disabled KVM features. Used by
> + * machine-type code to ensure a stable ABI.
> + */
> +void x86_cpu_set_kvm_defaults(uint32_t *default_set, uint32_t *default_unset)
>  {
> -    kvm_default_features[w] &= ~features;
> +    memcpy(kvm_default_features, default_set, sizeof(kvm_default_features));
> +    memcpy(kvm_default_unset_features, default_unset,
> +           sizeof(kvm_default_unset_features));
>  }
>  
>  /*
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 3f299d7..2138996 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -741,7 +741,7 @@ void do_smm_enter(X86CPU *cpu);
>  
>  void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
>  
> -void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features);
> +void x86_cpu_set_kvm_defaults(uint32_t *default_set, uint32_t *default_unset);
>  
>  
>  /* Return name of 32-bit register, from a R_* constant */
> -- 
> 1.9.3