Re: [Qemu-devel] [PATCH for-1.4 03/12] pc: Reverse pc_init_pci() compatibility logic

[Michael S. Tsirkin]

On Thu, Jan 17, 2013 at 06:59:29PM -0200, Eduardo Habkost wrote:
> Currently, the pc-1.4 machine init function enables PV EOI and then
> calls the pc-1.2 machine init function. The problem with this approach
> is that now we can't enable any additional compatibility code inside the
> pc-1.2 init function because it would end up enabling the compatibility
> behavior on pc-1.3 and pc-1.4 as well.
> 
> This reverses the logic so that the pc-1.2 machine init function will
> disable PV EOI, and then call the pc-1.4 machine init function.
> 
> This way we can change older machine-types to enable compatibility
> behavior, and the newer machine-types (pc-1.3, pc-q35-1.4 and
> pc-i440fx-1.4) would just use the default behavior.
> 
> (This means that one nice side-effect of this change is that pc-q35-1.4
> will get PV EOI enabled by default, too)
> 
> It would be interesting to eventually change pc_init_pci_no_kvmclock()
> and pc_init_isa() to reuse pc_init_pci_1_2() as well (so we don't need
> to duplicate compatibility code on those two functions). But this will
> be probably much easier to do after we create a PCInitArgs struct for
> the PC initialization arguments, and/or after we use global-properties
> to implement the compatibility modes present in pc_init_pci_1_2().
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> Cc: address@hidden
> Cc: Michael S. Tsirkin <address@hidden>
> Cc: Gleb Natapov <address@hidden>
> Cc: Marcelo Tosatti <address@hidden>
> 
> Signed-off-by: Eduardo Habkost <address@hidden>

I wasn't actually Cc'd :).
I don't see anything wrong with this patch.

Acked-by: Michael S. Tsirkin <address@hidden>

> ---
>  hw/pc_piix.c      | 22 +++++++++++++---------
>  target-i386/cpu.c |  5 +++--
>  target-i386/cpu.h |  2 +-
>  3 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 0a6923d..f9cfe78 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -233,12 +233,14 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
>               initrd_filename, cpu_model, 1, 1);
>  }
>  
> -static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
> +/* PC machine init function for pc-0.14 to pc-1.2 */
> +static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
>  {
> -    enable_kvm_pv_eoi();
> +    disable_kvm_pv_eoi();
>      pc_init_pci(args);
>  }
>  
> +/* PC init function for pc-0.10 to pc-0.13, and reused by xenfv */
>  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  {
>      ram_addr_t ram_size = args->ram_size;
> @@ -247,6 +249,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs 
> *args)
>      const char *kernel_cmdline = args->kernel_cmdline;
>      const char *initrd_filename = args->initrd_filename;
>      const char *boot_device = args->boot_device;
> +    disable_kvm_pv_eoi();
>      pc_init1(get_system_memory(),
>               get_system_io(),
>               ram_size, boot_device,
> @@ -264,6 +267,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
>      const char *boot_device = args->boot_device;
>      if (cpu_model == NULL)
>          cpu_model = "486";
> +    disable_kvm_pv_eoi();
>      pc_init1(get_system_memory(),
>               get_system_io(),
>               ram_size, boot_device,
> @@ -286,7 +290,7 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
>      .name = "pc-i440fx-1.4",
>      .alias = "pc",
>      .desc = "Standard PC (i440FX + PIIX, 1996)",
> -    .init = pc_init_pci_1_3,
> +    .init = pc_init_pci,
>      .max_cpus = 255,
>      .is_default = 1,
>      DEFAULT_MACHINE_OPTIONS,
> @@ -302,7 +306,7 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
>  static QEMUMachine pc_machine_v1_3 = {
>      .name = "pc-1.3",
>      .desc = "Standard PC",
> -    .init = pc_init_pci_1_3,
> +    .init = pc_init_pci,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_3,
> @@ -342,7 +346,7 @@ static QEMUMachine pc_machine_v1_3 = {
>  static QEMUMachine pc_machine_v1_2 = {
>      .name = "pc-1.2",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_2,
> @@ -386,7 +390,7 @@ static QEMUMachine pc_machine_v1_2 = {
>  static QEMUMachine pc_machine_v1_1 = {
>      .name = "pc-1.1",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_1,
> @@ -422,7 +426,7 @@ static QEMUMachine pc_machine_v1_1 = {
>  static QEMUMachine pc_machine_v1_0 = {
>      .name = "pc-1.0",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_0,
> @@ -438,7 +442,7 @@ static QEMUMachine pc_machine_v1_0 = {
>  static QEMUMachine pc_machine_v0_15 = {
>      .name = "pc-0.15",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_0_15,
> @@ -471,7 +475,7 @@ static QEMUMachine pc_machine_v0_15 = {
>  static QEMUMachine pc_machine_v0_14 = {
>      .name = "pc-0.14",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_0_14, 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 754eb6f..d1a14d5 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -211,11 +211,12 @@ static uint32_t kvm_default_features = (1 << 
> KVM_FEATURE_CLOCKSOURCE) |
>          (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);
>  
> -void enable_kvm_pv_eoi(void)
> +void disable_kvm_pv_eoi(void)
>  {
> -    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> +    kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI);
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 4e091cd..9d4fcf9 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1250,7 +1250,7 @@ void do_smm_enter(CPUX86State *env1);
>  
>  void cpu_report_tpr_access(CPUX86State *env, TPRAccess access);
>  
> -void enable_kvm_pv_eoi(void);
> +void disable_kvm_pv_eoi(void);
>  
>  /* Return name of 32-bit register, from a R_* constant */
>  const char *get_register_name_32(unsigned int reg);
> -- 
> 1.7.11.7
>