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 <ehabk...@redhat.com>
> ---
> Cc: k...@vger.kernel.org
> Cc: Michael S. Tsirkin <m...@redhat.com>
> Cc: Gleb Natapov <g...@redhat.com>
> Cc: Marcelo Tosatti <mtosa...@redhat.com>
>
> Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
I wasn't actually Cc'd :).
I don't see anything wrong with this patch.
Acked-by: Michael S. Tsirkin <m...@redhat.com>
> ---
> 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
>
