| Message ID | 20121018150127.GA29551@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 18.10.2012 17:01, schrieb Michael S. Tsirkin: > Enable KVM PV EOI by default. You can still disable it with > -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration, > enable only for qemu 1.3 (or in the future, newer) machine type. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Thanks for updating, Reviewed-by: Andreas Färber <afaerber@suse.de> The commit message is not so telling whether there are any downsides (security implications?) to enabling this by default if supported, so I'll leave it to Anthony to ack/apply this. Regards, Andreas
On Mon, Oct 22, 2012 at 03:12:00PM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 22, 2012 at 12:06:42PM +0200, Andreas Färber wrote: > > Am 18.10.2012 17:01, schrieb Michael S. Tsirkin: > > > Enable KVM PV EOI by default. You can still disable it with > > > -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration, > > > enable only for qemu 1.3 (or in the future, newer) machine type. > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > Thanks for updating, > > > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > > > The commit message is not so telling whether there are any downsides > > (security implications?) to enabling this by default if supported, > > I don't think there could be security downsides because all this does > is tell guest about the feature in a convenient way. > A well behaved guest doesn't use a feature unless it's listed but > that's irrelevant for security. True. It could be relevant if the host kernel did check the CPUID bits before letting the guest read or write the PV EOI MSR, but that's not the case. > > > so > > I'll leave it to Anthony to ack/apply this. > > > > Regards, > > Andreas > > It used to be enabled. It was turned off in > ef8621b1a3b199c348606c0a11a77d8e8bf135f1 > because it affected migration format and doing that > just before the release seemed too risky. > > > > -- > > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
On Thu, Oct 18, 2012 at 05:01:27PM +0200, Michael S. Tsirkin wrote: > Enable KVM PV EOI by default. You can still disable it with > -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration, > enable only for qemu 1.3 (or in the future, newer) machine type. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > --- > > Changes from v2: > Address comments by Andreas: > whitespace fixes and moving function around > Changes from v1: > Address comments by Eduardo: > use include instead of duplicate definition > reduce ifdef spagetti in code using features mask > rename init from _pv_eoi to _1_3 to enable adding > more stuff in this version > > hw/pc_piix.c | 15 ++++++++++++++- > target-i386/cpu.c | 33 ++++++++++++++++++++------------- > target-i386/cpu.h | 2 ++ > 3 files changed, 36 insertions(+), 14 deletions(-) > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 82364ab..be69dbd 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -43,6 +43,7 @@ > #include "xen.h" > #include "memory.h" > #include "exec-memory.h" > +#include "cpu.h" > #ifdef CONFIG_XEN > # include <xen/hvm/hvm_info_table.h> > #endif > @@ -301,6 +302,18 @@ static void pc_init_pci(ram_addr_t ram_size, > initrd_filename, cpu_model, 1, 1); > } > > +static void pc_init_pci_1_3(ram_addr_t ram_size, > + const char *boot_device, > + const char *kernel_filename, > + const char *kernel_cmdline, > + const char *initrd_filename, > + const char *cpu_model) > +{ > + enable_kvm_pv_eoi(); > + pc_init_pci(ram_size, boot_device, kernel_filename, > + kernel_cmdline, initrd_filename, cpu_model); > +} > + > static void pc_init_pci_no_kvmclock(ram_addr_t ram_size, > const char *boot_device, > const char *kernel_filename, > @@ -353,7 +366,7 @@ static QEMUMachine pc_machine_v1_3 = { > .name = "pc-1.3", > .alias = "pc", > .desc = "Standard PC", > - .init = pc_init_pci, > + .init = pc_init_pci_1_3, > .max_cpus = 255, > .is_default = 1, > }; > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index f3708e6..0f77449 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -124,6 +124,25 @@ typedef struct model_features_t { > int check_cpuid = 0; > int enforce_cpuid = 0; > > +#if defined(CONFIG_KVM) > +static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > + (1 << KVM_FEATURE_NOP_IO_DELAY) | > + (1 << KVM_FEATURE_MMU_OP) | > + (1 << KVM_FEATURE_CLOCKSOURCE2) | > + (1 << KVM_FEATURE_ASYNC_PF) | > + (1 << KVM_FEATURE_STEAL_TIME) | > + (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > +static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI); > +#else > +static uint32_t kvm_default_features = 0; > +static const uint32_t kvm_pv_eoi_features = 0; > +#endif > + > +void enable_kvm_pv_eoi(void) > +{ > + kvm_default_features |= kvm_pv_eoi_features; > +} > + > void host_cpuid(uint32_t function, uint32_t count, > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > { > @@ -1107,7 +1126,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > /* Features to be added*/ > uint32_t plus_features = 0, plus_ext_features = 0; > uint32_t plus_ext2_features = 0, plus_ext3_features = 0; > - uint32_t plus_kvm_features = 0, plus_svm_features = 0; > + uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0; > uint32_t plus_7_0_ebx_features = 0; > /* Features to be removed */ > uint32_t minus_features = 0, minus_ext_features = 0; > @@ -1127,18 +1146,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > memcpy(x86_cpu_def, def, sizeof(*def)); > } > > -#if defined(CONFIG_KVM) > - plus_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > - (1 << KVM_FEATURE_NOP_IO_DELAY) | > - (1 << KVM_FEATURE_MMU_OP) | > - (1 << KVM_FEATURE_CLOCKSOURCE2) | > - (1 << KVM_FEATURE_ASYNC_PF) | > - (1 << KVM_FEATURE_STEAL_TIME) | > - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > -#else > - plus_kvm_features = 0; > -#endif > - > add_flagname_to_bitmaps("hypervisor", &plus_features, > &plus_ext_features, &plus_ext2_features, &plus_ext3_features, > &plus_kvm_features, &plus_svm_features, &plus_7_0_ebx_features); > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 871c270..de33303 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1188,4 +1188,6 @@ void do_smm_enter(CPUX86State *env1); > > void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); > > +void enable_kvm_pv_eoi(void); > + > #endif /* CPU_I386_H */ > -- > MST
On Mon, Oct 22, 2012 at 12:06:42PM +0200, Andreas Färber wrote: > Am 18.10.2012 17:01, schrieb Michael S. Tsirkin: > > Enable KVM PV EOI by default. You can still disable it with > > -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration, > > enable only for qemu 1.3 (or in the future, newer) machine type. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Thanks for updating, > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > The commit message is not so telling whether there are any downsides > (security implications?) to enabling this by default if supported, I don't think there could be security downsides because all this does is tell guest about the feature in a convenient way. A well behaved guest doesn't use a feature unless it's listed but that's irrelevant for security. > so > I'll leave it to Anthony to ack/apply this. > > Regards, > Andreas It used to be enabled. It was turned off in ef8621b1a3b199c348606c0a11a77d8e8bf135f1 because it affected migration format and doing that just before the release seemed too risky. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 82364ab..be69dbd 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -43,6 +43,7 @@ #include "xen.h" #include "memory.h" #include "exec-memory.h" +#include "cpu.h" #ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif @@ -301,6 +302,18 @@ static void pc_init_pci(ram_addr_t ram_size, initrd_filename, cpu_model, 1, 1); } +static void pc_init_pci_1_3(ram_addr_t ram_size, + const char *boot_device, + const char *kernel_filename, + const char *kernel_cmdline, + const char *initrd_filename, + const char *cpu_model) +{ + enable_kvm_pv_eoi(); + pc_init_pci(ram_size, boot_device, kernel_filename, + kernel_cmdline, initrd_filename, cpu_model); +} + static void pc_init_pci_no_kvmclock(ram_addr_t ram_size, const char *boot_device, const char *kernel_filename, @@ -353,7 +366,7 @@ static QEMUMachine pc_machine_v1_3 = { .name = "pc-1.3", .alias = "pc", .desc = "Standard PC", - .init = pc_init_pci, + .init = pc_init_pci_1_3, .max_cpus = 255, .is_default = 1, }; diff --git a/target-i386/cpu.c b/target-i386/cpu.c index f3708e6..0f77449 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -124,6 +124,25 @@ typedef struct model_features_t { int check_cpuid = 0; int enforce_cpuid = 0; +#if defined(CONFIG_KVM) +static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | + (1 << KVM_FEATURE_NOP_IO_DELAY) | + (1 << KVM_FEATURE_MMU_OP) | + (1 << KVM_FEATURE_CLOCKSOURCE2) | + (1 << KVM_FEATURE_ASYNC_PF) | + (1 << KVM_FEATURE_STEAL_TIME) | + (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); +static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI); +#else +static uint32_t kvm_default_features = 0; +static const uint32_t kvm_pv_eoi_features = 0; +#endif + +void enable_kvm_pv_eoi(void) +{ + kvm_default_features |= kvm_pv_eoi_features; +} + void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) { @@ -1107,7 +1126,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) /* Features to be added*/ uint32_t plus_features = 0, plus_ext_features = 0; uint32_t plus_ext2_features = 0, plus_ext3_features = 0; - uint32_t plus_kvm_features = 0, plus_svm_features = 0; + uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0; uint32_t plus_7_0_ebx_features = 0; /* Features to be removed */ uint32_t minus_features = 0, minus_ext_features = 0; @@ -1127,18 +1146,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) memcpy(x86_cpu_def, def, sizeof(*def)); } -#if defined(CONFIG_KVM) - plus_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) | - (1 << KVM_FEATURE_NOP_IO_DELAY) | - (1 << KVM_FEATURE_MMU_OP) | - (1 << KVM_FEATURE_CLOCKSOURCE2) | - (1 << KVM_FEATURE_ASYNC_PF) | - (1 << KVM_FEATURE_STEAL_TIME) | - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); -#else - plus_kvm_features = 0; -#endif - add_flagname_to_bitmaps("hypervisor", &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features, &plus_7_0_ebx_features); diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 871c270..de33303 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1188,4 +1188,6 @@ void do_smm_enter(CPUX86State *env1); void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); +void enable_kvm_pv_eoi(void); + #endif /* CPU_I386_H */
Enable KVM PV EOI by default. You can still disable it with -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration, enable only for qemu 1.3 (or in the future, newer) machine type. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Changes from v2: Address comments by Andreas: whitespace fixes and moving function around Changes from v1: Address comments by Eduardo: use include instead of duplicate definition reduce ifdef spagetti in code using features mask rename init from _pv_eoi to _1_3 to enable adding more stuff in this version hw/pc_piix.c | 15 ++++++++++++++- target-i386/cpu.c | 33 ++++++++++++++++++++------------- target-i386/cpu.h | 2 ++ 3 files changed, 36 insertions(+), 14 deletions(-)