migrate PV EOI MSR
v2 cover migrate PV EOI MSR
It turns out PV EOI gets disabled after migration - until next guest reset.
This is because we are missing code to actually migrate it. This patch fixes it up: it applies cleanly to qemu.git as well as qemu-kvm.git, so I think it's cleaner to apply it in qemu.git to keep diff to minimum.
Note: there's talk about adding infrastructure for CPUID whitelisting which thinkably could be used for migration compat support. I am guessing this won't be 1.2 material - when it's ready we can easily replace a simple flag that this patchset adds with something else.
So this just adds minimal code to avoid regressing cross-version migration.
Note: there's a kernel bug in linux 3.6-rc3 - apply my patch 'kvm: fix KVM_GET_MSR for PV EOI' in order to use this patchset on it.
Needed for 1.2.
Changes from v1:
Update all headers from 3.6-rc3 to keep them in sync (Jan) Disable cpuid flag for qemu 1.2 and older (Orit)
Michael S. Tsirkin (4):
linux-headers: update to 3.6-rc3
pc: refactor compat code
cpuid: disable pv eoi for 1.1 and older compat types
kvm: get/set PV EOI MSR
hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 +++++++++++++++++++++++++++ hw/cpu_flags.h | 9 ++++++++ hw/pc_piix.c | 46 ++++++++++++++++++++++++++++++++------- linux-headers/asm-s390/kvm.h | 2 +- linux-headers/asm-s390/kvm_para.h | 2 +- linux-headers/asm-x86/kvm.h | 1 + linux-headers/asm-x86/kvm_para.h | 7 ++++++ linux-headers/linux/kvm.h | 3 +++ target-i386/cpu.c | 8 +++++++ target-i386/cpu.h | 1 + target-i386/kvm.c | 13 +++++++++++ target-i386/machine.c | 21 ++++++++++++++++++ 13 files changed, 136 insertions(+), 11 deletions(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.h
Signature · 1 line
MST
PATCH v2 1/4 linux-headers: update to 3.6-rc3
Update linux-headers to version present in Linux 3.6-rc3. Header asm-x96_64/kvm_para.h update is needed for the new PV EOI feature.
--- linux-headers/asm-s390/kvm.h | 2 +- linux-headers/asm-s390/kvm_para.h | 2 +- linux-headers/asm-x86/kvm.h | 1 + linux-headers/asm-x86/kvm_para.h | 7 +++++++ linux-headers/linux/kvm.h | 3 +++ 5 files changed, 13 insertions(+), 2 deletions(-)
linux-headers/asm-s390/kvm.h +1 -1
@@ -1,7 +1,7 @@#ifndef __LINUX_KVM_S390_H#define __LINUX_KVM_S390_H/* -* asm-s390/kvm.h - KVM s390 specific structures and definitions +* KVM s390 specific structures and definitions * * Copyright IBM Corp. 2008 *
linux-headers/asm-s390/kvm_para.h +1 -1
@@ -1,5 +1,5 @@/* -* asm-s390/kvm_para.h - definition for paravirtual devices on s390 +* definition for paravirtual devices on s390 * * Copyright IBM Corp. 2008 *
linux-headers/asm-x86/kvm.h +1 -0
@@ -12,6 +12,7 @@/* Select x86 specific features in <linux/kvm.h> */ #define __KVM_HAVE_PIT#define __KVM_HAVE_IOAPIC +#define __KVM_HAVE_IRQ_LINE#define __KVM_HAVE_DEVICE_ASSIGNMENT#define __KVM_HAVE_MSI#define __KVM_HAVE_USER_NMI
linux-headers/asm-x86/kvm_para.h +7 -0
@@ -22,6 +22,7 @@#define KVM_FEATURE_CLOCKSOURCE2 3#define KVM_FEATURE_ASYNC_PF 4#define KVM_FEATURE_STEAL_TIME 5 +#define KVM_FEATURE_PV_EOI 6/* The last 8 bits are used to indicate how to interpret the flags field * in pvclock structure. If no bits are set, all flags are ignored.
@@ -37,6 +38,7 @@#define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01#define MSR_KVM_ASYNC_PF_EN 0x4b564d02#define MSR_KVM_STEAL_TIME 0x4b564d03 +#define MSR_KVM_PV_EOI_EN 0x4b564d04struct kvm_steal_time {__u64 steal;
@@ -89,5 +91,10 @@ struct kvm_vcpu_pv_apf_data { __u32 enabled; }; +#define KVM_PV_EOI_BIT 0 +#define KVM_PV_EOI_MASK (0x1 << KVM_PV_EOI_BIT) +#define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK +#define KVM_PV_EOI_DISABLED 0x0 +#endif /* _ASM_X86_KVM_PARA_H */
linux-headers/linux/kvm.h +3 -0
@@ -617,6 +617,7 @@ struct kvm_ppc_smmu_info {#define KVM_CAP_SIGNAL_MSI 77#define KVM_CAP_PPC_GET_SMMU_INFO 78#define KVM_CAP_S390_COW 79 +#define KVM_CAP_PPC_ALLOC_HTAB 80#ifdef KVM_CAP_IRQ_ROUTING
@@ -828,6 +829,8 @@ struct kvm_s390_ucas_mapping {#define KVM_SIGNAL_MSI _IOW(KVMIO, 0xa5, struct kvm_msi)/* Available with KVM_CAP_PPC_GET_SMMU_INFO */ #define KVM_PPC_GET_SMMU_INFO _IOR(KVMIO, 0xa6, struct kvm_ppc_smmu_info) +/* Available with KVM_CAP_PPC_ALLOC_HTAB */ +#define KVM_PPC_ALLOCATE_HTAB _IOWR(KVMIO, 0xa7, __u32)/* * ioctls for vcpu fds
Signature · 1 line
MST
Re: [Qemu-devel] [PATCHv2 1/4] linux-headers: update to 3.6-rc3
Quoted 4 lines · Michael S. Tsirkin
Update linux-headers to version present in Linux 3.6-rc3. Header
asm-x96_64/kvm_para.hupdate is needed for the new PV EOI feature.
Quoted 8 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- linux-headers/asm-s390/kvm.h | 2 +- linux-headers/asm-s390/kvm_para.h | 2 +- linux-headers/asm-x86/kvm.h | 1 + linux-headers/asm-x86/kvm_para.h | 7 +++++++ linux-headers/linux/kvm.h | 3 +++ 5 files changed, 13 insertions(+), 2 deletions(-)
The latest version of update-linux-headers.sh should have caused this update to include asm-generic/kvm_para.h, I think. Did the script not pull that header in, or were you maybe using an old version of the script or forgot to git add the new file?
thanks
-- PMM
Re: [Qemu-devel] [PATCHv2 1/4] linux-headers: update to 3.6-rc3
Quoted 14 lines · Peter Maydell
Quoted 4 lines · Michael S. Tsirkin
Update linux-headers to version present in Linux 3.6-rc3. Header
asm-x96_64/kvm_para.hupdate is needed for the new PV EOI feature.Quoted 8 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- linux-headers/asm-s390/kvm.h | 2 +- linux-headers/asm-s390/kvm_para.h | 2 +- linux-headers/asm-x86/kvm.h | 1 + linux-headers/asm-x86/kvm_para.h | 7 +++++++ linux-headers/linux/kvm.h | 3 +++ 5 files changed, 13 insertions(+), 2 deletions(-)
Quoted 4 lines
The latest version of
update-linux-headers.shshould have caused this update to includeasm-generic/kvm_para.h, I think. Did the script not pull that header in, or were you maybe using an old version of the script or forgot to git add the new file?
To be fair, that is hard to guess. We should add some magic to the update script to detect new files and maybe suggest them for addition.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCHv2 1/4] linux-headers: update to 3.6-rc3
Quoted 20 lines · Jan Kiszka
Quoted 14 lines · Peter Maydell
Quoted 4 lines · Michael S. Tsirkin
Update linux-headers to version present in Linux 3.6-rc3. Header
asm-x96_64/kvm_para.hupdate is needed for the new PV EOI feature.Quoted 8 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- linux-headers/asm-s390/kvm.h | 2 +- linux-headers/asm-s390/kvm_para.h | 2 +- linux-headers/asm-x86/kvm.h | 1 + linux-headers/asm-x86/kvm_para.h | 7 +++++++ linux-headers/linux/kvm.h | 3 +++ 5 files changed, 13 insertions(+), 2 deletions(-)Quoted 4 lines
The latest version of
update-linux-headers.shshould have caused this update to includeasm-generic/kvm_para.h, I think. Did the script not pull that header in, or were you maybe using an old version of the script or forgot to git add the new file?
Quoted 4 lines
To be fair, that is hard to guess. We should add some magic to the update script to detect new files and maybe suggest them for addition.
Jan
But why did you add a header to qemu without adding it to git? That's a cleaner solution and needs no magic scripting.
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [PATCHv2 1/4] linux-headers: update to 3.6-rc3
Quoted 21 lines · Michael S. Tsirkin
Quoted 20 lines · Jan Kiszka
Quoted 18 lines · Peter Maydell
Quoted 4 lines · Michael S. Tsirkin
Update linux-headers to version present in Linux 3.6-rc3. Header
asm-x96_64/kvm_para.hupdate is needed for the new PV EOI feature.Quoted 8 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- linux-headers/asm-s390/kvm.h | 2 +- linux-headers/asm-s390/kvm_para.h | 2 +- linux-headers/asm-x86/kvm.h | 1 + linux-headers/asm-x86/kvm_para.h | 7 +++++++ linux-headers/linux/kvm.h | 3 +++ 5 files changed, 13 insertions(+), 2 deletions(-)The latest version of
update-linux-headers.shshould have caused this update to includeasm-generic/kvm_para.h, I think. Did the script not pull that header in, or were you maybe using an old version of the script or forgot to git add the new file?
Quoted 7 lines
Quoted 4 lines
To be fair, that is hard to guess. We should add some magic to the update script to detect new files and maybe suggest them for addition.
Jan
But why did you add a header to qemu without adding it to git? That's a cleaner solution and needs no magic scripting.
Yes, this would have been appropriate. Still, a simple "git status -s linux-headers" run at the end of the update script can help reminding people in the future.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCHv2 1/4] linux-headers: update to 3.6-rc3
Quoted 27 lines · Jan Kiszka
Quoted 26 lines · Michael S. Tsirkin
Quoted 24 lines · Jan Kiszka
Quoted 14 lines · Peter Maydell
Quoted 4 lines · Michael S. Tsirkin
Update linux-headers to version present in Linux 3.6-rc3. Header
asm-x96_64/kvm_para.hupdate is needed for the new PV EOI feature.Quoted 8 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- linux-headers/asm-s390/kvm.h | 2 +- linux-headers/asm-s390/kvm_para.h | 2 +- linux-headers/asm-x86/kvm.h | 1 + linux-headers/asm-x86/kvm_para.h | 7 +++++++ linux-headers/linux/kvm.h | 3 +++ 5 files changed, 13 insertions(+), 2 deletions(-)Quoted 4 lines
The latest version of
update-linux-headers.shshould have caused this update to includeasm-generic/kvm_para.h, I think. Did the script not pull that header in, or were you maybe using an old version of the script or forgot to git add the new file?To be fair, that is hard to guess. We should add some magic to the update script to detect new files and maybe suggest them for addition.
Jan
Quoted 8 lines
Quoted 2 lines
But why did you add a header to qemu without adding it to git? That's a cleaner solution and needs no magic scripting.
Yes, this would have been appropriate. Still, a simple "git status -s linux-headers" run at the end of the update script can help reminding people in the future.
Jan
Yes. But it would be better if instead of duplicating a list of files/directories, update-linux-headers.sh would just look at what is under linux-headers and update exactly that.
This removes any chance of error, and avoids the need to tweak shell scripts each time we add a header.
As a bonus we do not blow away random stuff
developer might have under linux-headers.
Thoughts?
WFM
--->
scripts: better update headers
Be more careful when updating headers: only
update files we already have in git.
Also remove need to list files in this script.
--
scripts/update-linux-headers.sh +26 -16
@@ -28,23 +28,33 @@ if [ -z "$output" ]; then output="$PWD" fi -for arch in x86 powerpc s390; do - make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch headers_install - - rm -rf "$output/linux-headers/asm-$arch" - mkdir -p "$output/linux-headers/asm-$arch" - for header in kvm.h kvm_para.h; do - cp "$tmpdir/include/asm/$header" "$output/linux-headers/asm-$arch" - done - if [ $arch = x86 ]; then - cp "$tmpdir/include/asm/hyperv.h" "$output/linux-headers/asm-x86" - fi -done +IFS=$'\n' + +#get list of files +dirs=`git ls-tree HEAD -- linux-headers/|grep tree|cut -f 2` +if [ -z "$dirs" ]; then + echo "Unable to get list of directories under linux-headers/ to update" +fi -rm -rf "$output/linux-headers/linux" -mkdir -p "$output/linux-headers/linux" -for header in kvm.h kvm_para.h vhost.h virtio_config.h virtio_ring.h; do - cp "$tmpdir/include/linux/$header" "$output/linux-headers/linux" +for d in $dirs; do + a=${d/#linux-headers\//} + case "$a" in + asm-*) + arch=${a/asm-/} + make -C "$linux" INSTALL_HDR_PATH="$tmpdir" SRCARCH=$arch headers_install + files=`git ls-tree -r HEAD -- "$d" |cut -f 2` + for dst in $files; do + src=include/asm/${dst/linux-headers\/asm-$arch\//} + cp -f "$tmpdir/$src" "$output/$dst" || exit 2 + done ;; + *) + make -C "$linux" INSTALL_HDR_PATH="$tmpdir" headers_install + files=`git ls-tree -r HEAD -- "$d" |cut -f 2` + for dst in $files; do + src=include/${dst/linux-headers\//} + cp -f "$tmpdir/$src" "$output/$dst" || exit 2 + done ;; + esac done if [ -L "$linux/source" ]; then cp "$linux/source/COPYING" "$output/linux-headers"
Signature · 1 line
MST
Re: [Qemu-devel] [PATCHv2 1/4] linux-headers: update to 3.6-rc3
In reply to Peter MaydellQuoted 14 lines · Peter Maydell
Quoted 4 lines · Michael S. Tsirkin
Update linux-headers to version present in Linux 3.6-rc3. Header
asm-x96_64/kvm_para.hupdate is needed for the new PV EOI feature.Quoted 8 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- linux-headers/asm-s390/kvm.h | 2 +- linux-headers/asm-s390/kvm_para.h | 2 +- linux-headers/asm-x86/kvm.h | 1 + linux-headers/asm-x86/kvm_para.h | 7 +++++++ linux-headers/linux/kvm.h | 3 +++ 5 files changed, 13 insertions(+), 2 deletions(-)
Quoted 7 lines
The latest version of
update-linux-headers.shshould have caused this update to includeasm-generic/kvm_para.h, I think. Did the script not pull that header in, or were you maybe using an old version of the script or forgot to git add the new file?thanks
-- PMM
I have no idea but adding new files is not the same as updating existing ones.
Why don't you add it when you update headers to a version that actually uses it?
Signature · 1 line
MST
PATCH v2 2/4 pc: refactor compat code
In reply to Michael S. TsirkinIn preparation to adding PV EOI migration for 1.2, trivially refactor some some compat code to make it easier to add version specific cpuid tweaks.
--- hw/pc_piix.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-)
hw/pc_piix.c +36 -8
@@ -369,6 +369,22 @@ static QEMUMachine pc_machine_v1_2 = { .default_machine_opts = KVM_MACHINE_OPTIONS, }; +static void pc_machine_v1_1_compat (void ) +{ +} + +static void pc_init_pci_v1_1 (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) +{ +pc_machine_v1_1_compat (); +pc_init_pci (ram_size, boot_device, kernel_filename, + kernel_cmdline, initrd_filename, cpu_model); +} +#define PC_COMPAT_1_1 \ {\ .driver = "virtio-scsi-pci",\
@@ -403,7 +419,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_v1_1, .max_cpus =255 , .default_machine_opts =KVM_MACHINE_OPTIONS , .compat_props = (GlobalProperty []) {
@@ -439,7 +455,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_v1_1, .max_cpus =255 , .default_machine_opts =KVM_MACHINE_OPTIONS , .compat_props = (GlobalProperty []) {
@@ -455,7 +471,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_v1_1, .max_cpus =255 , .default_machine_opts =KVM_MACHINE_OPTIONS , .compat_props = (GlobalProperty []) {
@@ -488,7 +504,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_v1_1, .max_cpus =255 , .default_machine_opts =KVM_MACHINE_OPTIONS , .compat_props = (GlobalProperty []) {
@@ -519,10 +535,22 @@ static QEMUMachine pc_machine_v0_14 = { .value = stringify(1),\ } +static void pc_init_pci_v0_13 (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) +{ +pc_machine_v1_1_compat (); +pc_init_pci_no_kvmclock (ram_size, boot_device, kernel_filename, + kernel_cmdline, initrd_filename, cpu_model); +} +static QEMUMachine pc_machine_v0_13 = { .name ="pc-0.13" , .desc ="Standard PC" , - .init = pc_init_pci_no_kvmclock, + .init = pc_init_pci_v0_13, .max_cpus =255 , .default_machine_opts =KVM_MACHINE_OPTIONS , .compat_props = (GlobalProperty []) {
@@ -560,7 +588,7 @@ static QEMUMachine pc_machine_v0_13 = {static QEMUMachine pc_machine_v0_12 = { .name ="pc-0.12" , .desc ="Standard PC" , - .init = pc_init_pci_no_kvmclock, + .init = pc_init_pci_v0_13, .max_cpus =255 , .default_machine_opts =KVM_MACHINE_OPTIONS , .compat_props = (GlobalProperty []) {
@@ -594,7 +622,7 @@ static QEMUMachine pc_machine_v0_12 = {static QEMUMachine pc_machine_v0_11 = { .name ="pc-0.11" , .desc ="Standard PC, qemu 0.11" , - .init = pc_init_pci_no_kvmclock, + .init = pc_init_pci_v0_13, .max_cpus =255 , .default_machine_opts =KVM_MACHINE_OPTIONS , .compat_props = (GlobalProperty []) {
@@ -616,7 +644,7 @@ static QEMUMachine pc_machine_v0_11 = {static QEMUMachine pc_machine_v0_10 = { .name ="pc-0.10" , .desc ="Standard PC, qemu 0.10" , - .init = pc_init_pci_no_kvmclock, + .init = pc_init_pci_v0_13, .max_cpus =255 , .default_machine_opts =KVM_MACHINE_OPTIONS , .compat_props = (GlobalProperty []) {
Signature · 1 line
MST
Re: [PATCHv2 2/4] pc: refactor compat code
Quoted 9 lines · Michael S. Tsirkin
In preparation to adding PV EOI migration for 1.2, trivially refactor some some compat code to make it easier to add version specific cpuid tweaks.
Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/pc_piix.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-)
Why can't you clear the cpuid bit in kvm paravirt leaf at hw/pc_piix.c, leaving compat code isolated there?
Re: [PATCHv2 2/4] pc: refactor compat code
Quoted 6 lines · Marcelo Tosatti
Quoted 5 lines · Michael S. Tsirkin
In preparation to adding PV EOI migration for 1.2, trivially refactor some some compat code to make it easier to add version specific cpuid tweaks.
Quoted 7 lines
Quoted 4 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/pc_piix.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-)Why can't you clear the cpuid bit in kvm paravirt leaf at
hw/pc_piix.c, leaving compat code isolated there?
This is not how we handle it for compat properties:
there we set flag in pc_piix and make devices look at
that flag.
And it makes sense because what you suggest does not scale: we can not teach pc_piix about quirks of all hardware.
It will also scale better if we ever get interested about compatibility and migration for non pc machines.
Signature · 1 line
MST
Re: [PATCHv2 2/4] pc: refactor compat code
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Marcelo Tosatti
Quoted 9 lines · Michael S. Tsirkin
In preparation to adding PV EOI migration for 1.2, trivially refactor some some compat code to make it easier to add version specific cpuid tweaks.
Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/pc_piix.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-)
Quoted 8 lines
Quoted 2 lines
Why can't you clear the cpuid bit in kvm paravirt leaf at
hw/pc_piix.c, leaving compat code isolated there?This is not how we handle it for compat properties:
there we set flag inpc_piixand make devices look at
that flag.
And it makes sense because what you suggest does not scale: we can not teachpc_piixabout quirks of all hardware.
What is there to scale? Old machines are static, they don't change. This way you are putting the burden of compat support in the generic driver code, which has better things to worry about than old machine types.
IMO it is better to have all of the compat mess localized.
Quoted 4 lines
It will also scale better if we ever get interested about compatibility and migration for non pc machines.
Signature · 1 line
MST
Alright, will wait a couple of days before merging.
Re: [PATCHv2 2/4] pc: refactor compat code
Quoted 16 lines · Marcelo Tosatti
Quoted 15 lines · Michael S. Tsirkin
Quoted 13 lines · Marcelo Tosatti
Quoted 9 lines · Michael S. Tsirkin
In preparation to adding PV EOI migration for 1.2, trivially refactor some some compat code to make it easier to add version specific cpuid tweaks.
Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/pc_piix.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-)Why can't you clear the cpuid bit in kvm paravirt leaf at
hw/pc_piix.c, leaving compat code isolated there?
Quoted 7 lines
Quoted 5 lines
This is not how we handle it for compat properties:
there we set flag inpc_piixand make devices look at
that flag.
And it makes sense because what you suggest does not scale: we can not teachpc_piixabout quirks of all hardware.What is there to scale? Old machines are static, they don't change.
But we keep adding new features in each version and each of them adds new device specific stuff. If we stick all logic in pc_piix.c it will quickly have to know about internals of virtio,cdrom,ide,cpu ...
Quoted 12 lines
This
way you are putting the burden of compat support in the generic driver code, which has better things to worry about than old machine types.IMO it is better to have all of the compat mess localized.
Quoted 4 lines
It will also scale better if we ever get interested about compatibility and migration for non pc machines.
Signature · 1 line
MSTAlright, will wait a couple of days before merging.
PATCH v2 3/4 cpuid: disable pv eoi for 1.1 and older compat types
In reply to Michael S. TsirkinIn preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoi
--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.h
hw/Makefile.objs +1 -1
@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.o
hw/cpu_flags.c new file +32 -0
@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled; + +void disable_kvm_pv_eoi (void ) +{ + __kvm_pv_eoi_disabled = true; +} + +bool kvm_pv_eoi_disabled (void ) +{ +return __kvm_pv_eoi_disabled; +}
hw/cpu_flags.h new file +9 -0
@@ -0,0 +1,9 @@ +#ifndef HW_CPU_FLAGS_H +#define HW_CPU_FLAGS_H + +#include <stdbool.h> + +void disable_kvm_pv_eoi (void ); +bool kvm_pv_eoi_disabled (void ); + +#endif
hw/pc_piix.c +2 -0
@@ -46,6 +46,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "cpu_flags.h" #define MAX_IDE_BUS 2
@@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {static void pc_machine_v1_1_compat (void ) { +disable_kvm_pv_eoi (); } static void pc_init_pci_v1_1(ram_addr_t ram_size,
target-i386/cpu.c +8 -0
@@ -23,6 +23,7 @@#include "cpu.h" #include "kvm.h" +#include "asm/kvm_para.h" #include "qemu-option.h" #include "qemu-config.h"
@@ -33,6 +34,7 @@#include "hyperv.h" #include "hw/hw.h" +#include "hw/cpu_flags.h" /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement
@@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) plus_kvm_features = ~0 ;/* not supported bits will be filtered out later */ +/* Disable PV EOI for old machine types. +* Feature flags can still override. */ +if (kvm_pv_eoi_disabled ()) { + plus_kvm_features &= ~(0x1 <<KVM_FEATURE_PV_EOI ); + } +add_flagname_to_bitmaps ("hypervisor" , &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features);
Signature · 1 line
MST
Re: [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 49 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +12
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful,
hw/cpu_flags.c line 22
Quoted 10 lines
hw/cpu_flags.c new file +10
-0+* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled;
Don't use identifiers with leading underscores.
Quoted 82 lines
hw/cpu_flags.c new file +10
-0+ +void disable_kvm_pv_eoi (void ) +{ + __kvm_pv_eoi_disabled = true; +} + +bool kvm_pv_eoi_disabled (void ) +{ +return __kvm_pv_eoi_disabled; +}hw/cpu_flags.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef HW_CPU_FLAGS_H +#define HW_CPU_FLAGS_H + +#include <stdbool.h> + +void disable_kvm_pv_eoi (void ); +bool kvm_pv_eoi_disabled (void ); + +#endif hw/pc_piix.c +2
-0@@ -46,6 +46,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "cpu_flags.h" #define MAX_IDE_BUS 2@@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {static void pc_machine_v1_1_compat (void ) { +disable_kvm_pv_eoi (); } static void pc_init_pci_v1_1(ram_addr_t ram_size,target-i386/cpu.c +8
-1@@ -23,6 +23,7 @@#include "cpu.h" #include "kvm.h" +#include "asm/kvm_para.h" #include "qemu-option.h" #include "qemu-config.h" @@ -33,6 +34,7 @@#include "hyperv.h" #include "hw/hw.h" +#include "hw/cpu_flags.h" /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement @@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) plus_kvm_features = ~0 ;/* not supported bits will be filtered out later */ +/* Disable PV EOI for old machine types. +* Feature flags can still override. */ +if (kvm_pv_eoi_disabled ()) { + plus_kvm_features &= ~(0x1 <<KVM_FEATURE_PV_EOI ); + } +add_flagname_to_bitmaps ("hypervisor" , &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features); --MST
Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 54 lines · Blue Swirl
Quoted 53 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +16
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*
Quoted 8 lines
hw/cpu_flags.c line 22
Quoted 6 lines
hw/cpu_flags.c new file +6
-0+* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled;Don't use identifiers with leading underscores.
C99 spec says "
Any other predefined macro names
shall begin with a leading underscore followed by an uppercase letter or a second underscore.
"
what are chances of compiler predefining macro __kvm_pv_eoi_disabled?
But OK, will rename _kvm_pv_eoi_disabled.
_ + lower case is guaranteed OK.
Quoted 82 lines
Quoted 82 lines
hw/cpu_flags.c new file +10
-0+ +void disable_kvm_pv_eoi (void ) +{ + __kvm_pv_eoi_disabled = true; +} + +bool kvm_pv_eoi_disabled (void ) +{ +return __kvm_pv_eoi_disabled; +}hw/cpu_flags.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef HW_CPU_FLAGS_H +#define HW_CPU_FLAGS_H + +#include <stdbool.h> + +void disable_kvm_pv_eoi (void ); +bool kvm_pv_eoi_disabled (void ); + +#endif hw/pc_piix.c +2
-0@@ -46,6 +46,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "cpu_flags.h" #define MAX_IDE_BUS 2@@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {static void pc_machine_v1_1_compat (void ) { +disable_kvm_pv_eoi (); } static void pc_init_pci_v1_1(ram_addr_t ram_size,target-i386/cpu.c +8
-1@@ -23,6 +23,7 @@#include "cpu.h" #include "kvm.h" +#include "asm/kvm_para.h" #include "qemu-option.h" #include "qemu-config.h" @@ -33,6 +34,7 @@#include "hyperv.h" #include "hw/hw.h" +#include "hw/cpu_flags.h" /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement @@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) plus_kvm_features = ~0 ;/* not supported bits will be filtered out later */ +/* Disable PV EOI for old machine types. +* Feature flags can still override. */ +if (kvm_pv_eoi_disabled ()) { + plus_kvm_features &= ~(0x1 <<KVM_FEATURE_PV_EOI ); + } +add_flagname_to_bitmaps ("hypervisor" , &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features); --MST
Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 64 lines · Michael S. Tsirkin
Quoted 54 lines · Blue Swirl
Quoted 53 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +16
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* Quoted 8 lines
hw/cpu_flags.c line 22
Quoted 6 lines
hw/cpu_flags.c new file +6
-0+* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled;Don't use identifiers with leading underscores.
Quoted 7 lines
C99 spec says "
Any other predefined macro names
shall begin with a leading underscore followed by an uppercase letter or a second underscore.
"what are chances of compiler predefining macro
__kvm_pv_eoi_disabled?
Why do you even consider that since it's trivially easy to use something else? If a standard (and HACKING in our case) specifies something, why do you want to fight it?
Quoted 3 lines
But OK, will rename
_kvm_pv_eoi_disabled.
_ + lower case is guaranteed OK.
No, just use kvm_pv_eoi_disabled, the underscore is useless.
Quoted 84 lines
Quoted 84 lines
Quoted 84 lines
hw/cpu_flags.c new file +10
-0+ +void disable_kvm_pv_eoi (void ) +{ + __kvm_pv_eoi_disabled = true; +} + +bool kvm_pv_eoi_disabled (void ) +{ +return __kvm_pv_eoi_disabled; +}hw/cpu_flags.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef HW_CPU_FLAGS_H +#define HW_CPU_FLAGS_H + +#include <stdbool.h> + +void disable_kvm_pv_eoi (void ); +bool kvm_pv_eoi_disabled (void ); + +#endif hw/pc_piix.c +2
-0@@ -46,6 +46,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "cpu_flags.h" #define MAX_IDE_BUS 2@@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {static void pc_machine_v1_1_compat (void ) { +disable_kvm_pv_eoi (); } static void pc_init_pci_v1_1(ram_addr_t ram_size,target-i386/cpu.c +8
-1@@ -23,6 +23,7 @@#include "cpu.h" #include "kvm.h" +#include "asm/kvm_para.h" #include "qemu-option.h" #include "qemu-config.h" @@ -33,6 +34,7 @@#include "hyperv.h" #include "hw/hw.h" +#include "hw/cpu_flags.h" /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement @@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) plus_kvm_features = ~0 ;/* not supported bits will be filtered out later */ +/* Disable PV EOI for old machine types. +* Feature flags can still override. */ +if (kvm_pv_eoi_disabled ()) { + plus_kvm_features &= ~(0x1 <<KVM_FEATURE_PV_EOI ); + } +add_flagname_to_bitmaps ("hypervisor" , &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features); --MST
Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 71 lines · Blue Swirl
Quoted 70 lines · Michael S. Tsirkin
Quoted 54 lines · Blue Swirl
Quoted 53 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +16
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* Quoted 8 lines
hw/cpu_flags.c line 22
Quoted 6 lines
hw/cpu_flags.c new file +6
-0+* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled;Don't use identifiers with leading underscores.
C99 spec says "
Any other predefined macro names
shall begin with a leading underscore followed by an uppercase letter or a second underscore.
"
Quoted 5 lines
Quoted 1 line
what are chances of compiler predefining macro
__kvm_pv_eoi_disabled?Why do you even consider that since it's trivially easy to use something else? If a standard (and HACKING in our case) specifies something, why do you want to fight it?
I missed this in HACKING, you are right:
2.4. Reserved namespaces in C and POSIX
Underscore capital, double underscore, and underscore 't' suffixes should be avoided.
so _kvm_pv_eoi_disabled is ok __kvm_pv_eoi_disabled is not. Will fix.
Quoted 5 lines
Quoted 3 lines
But OK, will rename
_kvm_pv_eoi_disabled.
_ + lower case is guaranteed OK.No, just use
kvm_pv_eoi_disabled, the underscore is useless.
It isn't useless, this avoids conflict with function name. _ says it's an internal variable used to implement kvm_pv_eoi_disabled in a very clear way.
Signature · 1 line
MST
Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 78 lines · Michael S. Tsirkin
Quoted 71 lines · Blue Swirl
Quoted 70 lines · Michael S. Tsirkin
Quoted 54 lines · Blue Swirl
Quoted 53 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +16
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* Quoted 8 lines
hw/cpu_flags.c line 22
Quoted 6 lines
hw/cpu_flags.c new file +6
-0+* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled;Don't use identifiers with leading underscores.
C99 spec says "
Any other predefined macro names
shall begin with a leading underscore followed by an uppercase letter or a second underscore.
"Quoted 5 lines
Quoted 1 line
what are chances of compiler predefining macro
__kvm_pv_eoi_disabled?Why do you even consider that since it's trivially easy to use something else? If a standard (and HACKING in our case) specifies something, why do you want to fight it?
Quoted 8 lines
I missed this in HACKING, you are right:
2.4. Reserved namespaces in C and POSIX
Underscore capital, double underscore, and underscore 't' suffixes should be avoided.so
_kvm_pv_eoi_disabledis ok__kvm_pv_eoi_disabledis not. Will fix.
No leading underscores. They are not used in QEMU.
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
But OK, will rename
_kvm_pv_eoi_disabled.
_ + lower case is guaranteed OK.No, just use
kvm_pv_eoi_disabled, the underscore is useless.It isn't useless, this avoids conflict with function name. _ says it's an internal variable used to implement
kvm_pv_eoi_disabledin a very clear way.
Sure, but there are infinite number of ways of making the identifiers unique. Using leading underscores is a way to ever conflict with compiler, linker, libc, POSIX etc. Don't do it.
Where's your imagination, can't you invent any other prefix or suffix?
Quoted 3 lines
--
MST
Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 81 lines · Blue Swirl
Quoted 80 lines · Michael S. Tsirkin
Quoted 71 lines · Blue Swirl
Quoted 70 lines · Michael S. Tsirkin
Quoted 54 lines · Blue Swirl
Quoted 53 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +16
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* Quoted 8 lines
hw/cpu_flags.c line 22
Quoted 6 lines
hw/cpu_flags.c new file +6
-0+* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled;Don't use identifiers with leading underscores.
C99 spec says "
Any other predefined macro names
shall begin with a leading underscore followed by an uppercase letter or a second underscore.
"Quoted 5 lines
Quoted 1 line
what are chances of compiler predefining macro
__kvm_pv_eoi_disabled?Why do you even consider that since it's trivially easy to use something else? If a standard (and HACKING in our case) specifies something, why do you want to fight it?
I missed this in HACKING, you are right:
Quoted 8 lines
Quoted 6 lines
2.4. Reserved namespaces in C and POSIX
Underscore capital, double underscore, and underscore 't' suffixes should be avoided.so
_kvm_pv_eoi_disabledis ok__kvm_pv_eoi_disabledis not. Will fix.No leading underscores. They are not used in QEMU.
They are *widely* used in QEMU to mark internal
stuff. E.g. parameters in many macros.
In reality __ is also widely used. I'm still mulling removing 2.4 from HACKING - it appears too draconian, the chances of a conflict with preprocessor are remote and if it triggers, it's trivial to catch.
We also have lots of existing code violating this rule.
And the rule about _t suffix is just silly.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 4 lines
But OK, will rename
_kvm_pv_eoi_disabled.
_ + lower case is guaranteed OK.No, just use
kvm_pv_eoi_disabled, the underscore is useless.
Quoted 7 lines
Quoted 3 lines
It isn't useless, this avoids conflict with function name. _ says it's an internal variable used to implement
kvm_pv_eoi_disabledin a very clear way.Sure, but there are infinite number of ways of making the identifiers unique. Using leading underscores is a way to ever conflict with compiler, linker, libc, POSIX etc.
I think you are mistaken. Anything to support this claim?
Quoted 2 lines
Don't do it.
Where's your imagination, can't you invent any other prefix or suffix?
I like _, I think it's a standard C way to mark internal stuff and there is no need to invent anything.
Quoted 3 lines
Quoted 3 lines
--
MST
Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 21 lines · Michael S. Tsirkin
Quoted 20 lines · Blue Swirl
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Blue Swirl
Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Blue Swirl
Quoted 15 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++
[..snip..]
Quoted 6 lines
Quoted 2 lines
No leading underscores. They are not used in QEMU.
They are *widely* used in QEMU to mark internal stuff. E.g. parameters in many macros.
ISO/IEC 9899:TC3 7.1.3#1
- All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.
IOW no __ or _[A-Z] at all.
- All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.
IOW _ as the name of an argument to a macro is (probably) okay.
Quoted 7 lines
In reality __ is also widely used. I'm still mulling removing 2.4 from HACKING - it appears too draconian, the chances of a conflict with preprocessor are remote and if it triggers, it's trivial to catch.
We also have lots of existing code violating this rule.And the rule about _t suffix is just silly.
http://pubs.opengroup.org/onlinepubs/7908799/xns/namespace.html
[..snip..]
Signature · 1 line
mailto:[email protected]
Re: [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 52 lines · malc
Quoted 21 lines · Michael S. Tsirkin
Quoted 20 lines · Blue Swirl
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Blue Swirl
Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Blue Swirl
Quoted 15 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++[..snip..]
Quoted 6 lines
Quoted 2 lines
No leading underscores. They are not used in QEMU.
They are *widely* used in QEMU to mark internal stuff. E.g. parameters in many macros.
ISO/IEC 9899:TC3 7.1.3#1
- All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.
IOW no __ or _[A-Z] at all.
- All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.
IOW _ as the name of an argument to a macro is (probably) okay.
Quoted 6 lines
In reality __ is also widely used. I'm still mulling removing 2.4 from HACKING - it appears too draconian, the chances of a conflict with preprocessor are remote and if it triggers, it's trivial to catch.
We also have lots of existing code violating this rule.
Quoted 5 lines
Quoted 1 line
And the rule about _t suffix is just silly.
http://pubs.opengroup.org/onlinepubs/7908799/xns/namespace.html
[..snip..]
It's still silly :)
Quoted 2 lines
Signature · 1 line
mailto:[email protected]
Re: [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
In reply to Michael S. TsirkinQuoted 59 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +22
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled;
hw/cpu_flags.c line 32
Quoted 10 lines
hw/cpu_flags.c new file +10
-0+ +void disable_kvm_pv_eoi (void ) +{ + __kvm_pv_eoi_disabled = true; +} + +bool kvm_pv_eoi_disabled (void ) +{ +return __kvm_pv_eoi_disabled; +}
Would this make more sense as a CPU flag or at least the KVM PV LAPIC?
We really ought to embed the LAPIC in the CPU objects. Then it would all make a bit more sense conceptionally. But I definitely thing a CPU feature flag makes the most sense here.
Then we can handle it via globals once we make CPU's qdev devices.
Regards,
Anthony Liguori
Quoted 70 lines
hw/cpu_flags.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef HW_CPU_FLAGS_H +#define HW_CPU_FLAGS_H + +#include <stdbool.h> + +void disable_kvm_pv_eoi (void ); +bool kvm_pv_eoi_disabled (void ); + +#endif hw/pc_piix.c +2
-0@@ -46,6 +46,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "cpu_flags.h" #define MAX_IDE_BUS 2@@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {static void pc_machine_v1_1_compat (void ) { +disable_kvm_pv_eoi (); } static void pc_init_pci_v1_1(ram_addr_t ram_size,target-i386/cpu.c +8
-0@@ -23,6 +23,7 @@#include "cpu.h" #include "kvm.h" +#include "asm/kvm_para.h" #include "qemu-option.h" #include "qemu-config.h" @@ -33,6 +34,7 @@#include "hyperv.h" #include "hw/hw.h" +#include "hw/cpu_flags.h" /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement @@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) plus_kvm_features = ~0 ;/* not supported bits will be filtered out later */ +/* Disable PV EOI for old machine types. +* Feature flags can still override. */ +if (kvm_pv_eoi_disabled ()) { + plus_kvm_features &= ~(0x1 <<KVM_FEATURE_PV_EOI ); + } +add_flagname_to_bitmaps ("hypervisor" , &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features);Signature · 1 line
MST
Re: [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 65 lines · Anthony Liguori
Quoted 63 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +26
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled; + +void disable_kvm_pv_eoi (void ) +{ + __kvm_pv_eoi_disabled = true;
Quoted 8 lines
hw/cpu_flags.c line 32
Quoted 6 lines
hw/cpu_flags.c new file +6
-0+} + +bool kvm_pv_eoi_disabled (void ) +{ +return __kvm_pv_eoi_disabled; +}Would this make more sense as a CPU flag or at least the KVM PV LAPIC?
It is a CPU flag, already. The problem is that QEMU defaults to enabling blindly everything reported by the host kernel, potentially breaking migration (and this still has to be fixed).
Quoted 6 lines
We really ought to embed the LAPIC in the CPU objects. Then it would all make a bit more sense conceptionally. But I definitely thing a CPU feature flag makes the most sense here.
Then we can handle it via globals once we make CPU's qdev devices.
Yes, this is a perfect candidate to use globals/qdev for machine-type compatibility.
Quoted 76 lines
Regards,
Anthony Liguori
Quoted 70 lines
hw/cpu_flags.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef HW_CPU_FLAGS_H +#define HW_CPU_FLAGS_H + +#include <stdbool.h> + +void disable_kvm_pv_eoi (void ); +bool kvm_pv_eoi_disabled (void ); + +#endif hw/pc_piix.c +2
-0@@ -46,6 +46,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "cpu_flags.h" #define MAX_IDE_BUS 2@@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {static void pc_machine_v1_1_compat (void ) { +disable_kvm_pv_eoi (); } static void pc_init_pci_v1_1(ram_addr_t ram_size,target-i386/cpu.c +8
-0@@ -23,6 +23,7 @@#include "cpu.h" #include "kvm.h" +#include "asm/kvm_para.h" #include "qemu-option.h" #include "qemu-config.h" @@ -33,6 +34,7 @@#include "hyperv.h" #include "hw/hw.h" +#include "hw/cpu_flags.h" /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement @@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) plus_kvm_features = ~0 ;/* not supported bits will be filtered out later */ +/* Disable PV EOI for old machine types. +* Feature flags can still override. */ +if (kvm_pv_eoi_disabled ()) { + plus_kvm_features &= ~(0x1 <<KVM_FEATURE_PV_EOI ); + } +add_flagname_to_bitmaps ("hypervisor" , &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features);Signature · 1 line
MST
Signature · 1 line
Eduardo
Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
Quoted 79 lines · Eduardo Habkost
Quoted 65 lines · Anthony Liguori
Quoted 63 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +26
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled; + +void disable_kvm_pv_eoi (void ) +{ + __kvm_pv_eoi_disabled = true;Quoted 8 lines
hw/cpu_flags.c line 32
Quoted 6 lines
hw/cpu_flags.c new file +6
-0+} + +bool kvm_pv_eoi_disabled (void ) +{ +return __kvm_pv_eoi_disabled; +}Would this make more sense as a CPU flag or at least the KVM PV LAPIC?
It is a CPU flag, already. The problem is that QEMU defaults to enabling blindly everything reported by the host kernel, potentially breaking migration (and this still has to be fixed).
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
We really ought to embed the LAPIC in the CPU objects. Then it would all make a bit more sense conceptionally. But I definitely thing a CPU feature flag makes the most sense here.
Then we can handle it via globals once we make CPU's qdev devices.
Yes, this is a perfect candidate to use globals/qdev for machine-type compatibility.
OK so ... ACK?
Quoted 79 lines
Quoted 76 lines
Regards,
Anthony Liguori
Quoted 70 lines
hw/cpu_flags.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef HW_CPU_FLAGS_H +#define HW_CPU_FLAGS_H + +#include <stdbool.h> + +void disable_kvm_pv_eoi (void ); +bool kvm_pv_eoi_disabled (void ); + +#endif hw/pc_piix.c +2
-0@@ -46,6 +46,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "cpu_flags.h" #define MAX_IDE_BUS 2@@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {static void pc_machine_v1_1_compat (void ) { +disable_kvm_pv_eoi (); } static void pc_init_pci_v1_1(ram_addr_t ram_size,target-i386/cpu.c +8
-0@@ -23,6 +23,7 @@#include "cpu.h" #include "kvm.h" +#include "asm/kvm_para.h" #include "qemu-option.h" #include "qemu-config.h" @@ -33,6 +34,7 @@#include "hyperv.h" #include "hw/hw.h" +#include "hw/cpu_flags.h" /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement @@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) plus_kvm_features = ~0 ;/* not supported bits will be filtered out later */ +/* Disable PV EOI for old machine types. +* Feature flags can still override. */ +if (kvm_pv_eoi_disabled ()) { + plus_kvm_features &= ~(0x1 <<KVM_FEATURE_PV_EOI ); + } +add_flagname_to_bitmaps ("hypervisor" , &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features);Signature · 1 line
MSTSignature · 1 line
Eduardo
Re: [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types
In reply to Anthony LiguoriQuoted 78 lines · Anthony Liguori
Quoted 59 lines · Michael S. Tsirkin
In preparation for adding PV EOI support, disable PV EOI by default for 1.1 and older machine types, to avoid CPUID changing during migration.
PV EOI can still be enabled/disabled by specifying it explicitly.
Enable for 1.1
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi
Disable for 1.2
-M pc-1.2 -cpu kvm64,-kvm_pv_eoiSigned-off-by Michael S. Tsirkin <[email protected]>--- hw/Makefile.objs | 2 +- hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ hw/cpu_flags.h | 9 +++++++++ hw/pc_piix.c | 2 ++ target-i386/cpu.c | 8 ++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 hw/cpu_flags.c create mode 100644 hw/cpu_flags.hhw/Makefile.objs +1
-1@@ -1,5 +1,5 @@ hw-obj-y = usb/ ide/ -hw-obj-y += loader.o +hw-obj-y += loader.o cpu_flags.o hw-obj-$(CONFIG_VIRTIO) += virtio-console.o hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o hw-obj-y += fw_cfg.ohw/cpu_flags.c new file +22
-0@@ -0,0 +1,32 @@ +/* +* CPU compatibility flags. +* +* Copyright (c) 2012 Red Hat Inc. +* Author: Michael S. Tsirkin. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License along +* with this program; if not, see <http://www.gnu.org/licenses/>. +*/ +#include "hw/cpu_flags.h" + +static bool __kvm_pv_eoi_disabled;hw/cpu_flags.c line 32
Quoted 10 lines
hw/cpu_flags.c new file +10
-0+ +void disable_kvm_pv_eoi (void ) +{ + __kvm_pv_eoi_disabled = true; +} + +bool kvm_pv_eoi_disabled (void ) +{ +return __kvm_pv_eoi_disabled; +}Would this make more sense as a CPU flag or at least the KVM PV LAPIC?
We really ought to embed the LAPIC in the CPU objects. Then it would all make a bit more sense conceptionally. But I definitely thing a CPU feature flag makes the most sense here.
Quoted 5 lines
Then we can handle it via globals once we make CPU's qdev devices.
Regards,
Anthony Liguori
Sorry I do not understand what you suggest.
This is a trivial bugfix patch it can be written in a million ways, I am confused by all the improvement suggestions.
Could you please simply send me a patch? I can even test it.
All I care about is that we get the functionality above:
-M pc-1.1 --- disabled
-M pc-1.2 --- enabled
-M pc-1.1 -cpu kvm64,+kvm_pv_eoi -- enabled
-M pc-1.2 -cpu kvm64,-kvm_pv_eoi -- disabledQuoted 70 lines
Quoted 70 lines
hw/cpu_flags.h new file +9
-0@@ -0,0 +1,9 @@ +#ifndef HW_CPU_FLAGS_H +#define HW_CPU_FLAGS_H + +#include <stdbool.h> + +void disable_kvm_pv_eoi (void ); +bool kvm_pv_eoi_disabled (void ); + +#endif hw/pc_piix.c +2
-0@@ -46,6 +46,7 @@#ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif +#include "cpu_flags.h" #define MAX_IDE_BUS 2@@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {static void pc_machine_v1_1_compat (void ) { +disable_kvm_pv_eoi (); } static void pc_init_pci_v1_1(ram_addr_t ram_size,target-i386/cpu.c +8
-0@@ -23,6 +23,7 @@#include "cpu.h" #include "kvm.h" +#include "asm/kvm_para.h" #include "qemu-option.h" #include "qemu-config.h" @@ -33,6 +34,7 @@#include "hyperv.h" #include "hw/hw.h" +#include "hw/cpu_flags.h" /* feature flags taken from "Intel Processor Identification and the CPUID * Instruction" and AMD's "CPUID Specification". In cases of disagreement @@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) plus_kvm_features = ~0 ;/* not supported bits will be filtered out later */ +/* Disable PV EOI for old machine types. +* Feature flags can still override. */ +if (kvm_pv_eoi_disabled ()) { + plus_kvm_features &= ~(0x1 <<KVM_FEATURE_PV_EOI ); + } +add_flagname_to_bitmaps ("hypervisor" , &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features);Signature · 1 line
MST
PATCH v2 4/4 kvm: get/set PV EOI MSR
In reply to Michael S. TsirkinSupport get/set of new PV EOI MSR, for migration. Add an optional section for MSR value - send it out in case MSR was changed from the default value (0).
--- target-i386/cpu.h | 1 + target-i386/kvm.c | 13 +++++++++++++ target-i386/machine.c | 21 +++++++++++++++++++++ 3 files changed, 35 insertions(+)
target-i386/cpu.h +1 -0
@@ -699,6 +699,7 @@ typedef struct CPUX86State {uint64_t system_time_msr;uint64_t wall_clock_msr;uint64_t async_pf_en_msr; +uint64_t pv_eoi_en_msr;uint64_t tsc;uint64_t tsc_deadline;
target-i386/kvm.c +13 -0
@@ -64,6 +64,7 @@ static bool has_msr_star;static bool has_msr_hsave_pa;static bool has_msr_tsc_deadline;static bool has_msr_async_pf_en; +static bool has_msr_pv_eoi_en;static bool has_msr_misc_enable;static int lm_capable_kernel;
@@ -456,6 +457,8 @@ int kvm_arch_init_vcpu(CPUX86State *env) has_msr_async_pf_en = c->eax & (1 <<KVM_FEATURE_ASYNC_PF ); + has_msr_pv_eoi_en = c->eax & (1 <<KVM_FEATURE_PV_EOI ); +cpu_x86_cpuid (env,0 ,0 , &limit, &unused, &unused, &unused);for (i =0 ; i <= limit; i++) {
@@ -1018,6 +1021,10 @@ static int kvm_put_msrs(CPUX86State *env, int level)kvm_msr_entry_set (&msrs[n++],MSR_KVM_ASYNC_PF_EN , env->async_pf_en_msr); } +if (has_msr_pv_eoi_en) { +kvm_msr_entry_set (&msrs[n++],MSR_KVM_PV_EOI_EN , + env->pv_eoi_en_msr); + }if (hyperv_hypercall_available ()) {kvm_msr_entry_set (&msrs[n++],HV_X64_MSR_GUEST_OS_ID ,0 );kvm_msr_entry_set (&msrs[n++],HV_X64_MSR_HYPERCALL ,0 );
@@ -1260,6 +1267,9 @@ static int kvm_get_msrs(CPUX86State *env)if (has_msr_async_pf_en) { msrs[n++].index =MSR_KVM_ASYNC_PF_EN ; } +if (has_msr_pv_eoi_en) { + msrs[n++].index =MSR_KVM_PV_EOI_EN ; + }if (env->mcg_cap) { msrs[n++].index =MSR_MCG_STATUS ;
@@ -1339,6 +1349,9 @@ static int kvm_get_msrs(CPUX86State *env)case MSR_KVM_ASYNC_PF_EN : env->async_pf_en_msr = msrs[i].data;break ; +case MSR_KVM_PV_EOI_EN : + env->pv_eoi_en_msr = msrs[i].data; +break ; } }
target-i386/machine.c +21 -0
@@ -279,6 +279,13 @@ static bool async_pf_msr_needed(void *opaque)return cpu->async_pf_en_msr !=0 ; } +static bool pv_eoi_msr_needed (void *opaque) +{ +CPUX86State *cpu = opaque; + +return cpu->pv_eoi_en_msr !=0 ; +} +static const VMStateDescription vmstate_async_pf_msr = { .name ="cpu/async_pf_msr" , .version_id =1 ,
@@ -290,6 +297,17 @@ static const VMStateDescription vmstate_async_pf_msr = { } }; +static const VMStateDescription vmstate_pv_eoi_msr = { + .name ="cpu/async_pv_eoi_msr" , + .version_id =1 , + .minimum_version_id =1 , + .minimum_version_id_old =1 , + .fields = (VMStateField []) { +VMSTATE_UINT64 (pv_eoi_en_msr, CPUX86State), +VMSTATE_END_OF_LIST () + } +}; +static bool fpop_ip_dp_needed (void *opaque) {CPUX86State *env = opaque;
@@ -454,6 +472,9 @@ static const VMStateDescription vmstate_cpu = { .vmsd = &vmstate_async_pf_msr, .needed = async_pf_msr_needed, } , { + .vmsd = &vmstate_pv_eoi_msr, + .needed = pv_eoi_msr_needed, + } , { .vmsd = &vmstate_fpop_ip_dp, .needed = fpop_ip_dp_needed, }, {
Signature · 1 line
MST