[Qemu-devel] [PATCHv2 0/4] migrate PV EOI MSR

[Qemu-devel] [PATCHv2 0/4] migrate PV EOI MSR

[Michael S. Tsirkin]

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

-- 
MST

[Qemu-devel] [PATCHv2 1/4] linux-headers: update to 3.6-rc3

[Michael S. Tsirkin]

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.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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(-)

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index bdcbe0f..d25da59 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -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
  *
diff --git a/linux-headers/asm-s390/kvm_para.h b/linux-headers/asm-s390/kvm_para.h
index 8e2dd67..870051f 100644
--- a/linux-headers/asm-s390/kvm_para.h
+++ b/linux-headers/asm-s390/kvm_para.h
@@ -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
  *
diff --git a/linux-headers/asm-x86/kvm.h b/linux-headers/asm-x86/kvm.h
index e7d1c19..246617e 100644
--- a/linux-headers/asm-x86/kvm.h
+++ b/linux-headers/asm-x86/kvm.h
@@ -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
diff --git a/linux-headers/asm-x86/kvm_para.h b/linux-headers/asm-x86/kvm_para.h
index f2ac46a..a1c3d72 100644
--- a/linux-headers/asm-x86/kvm_para.h
+++ b/linux-headers/asm-x86/kvm_para.h
@@ -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      0x4b564d04
 
 struct 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 */
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 5a9d4e3..4b9e575 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -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
-- 
MST

[Qemu-devel] [PATCHv2 2/4] pc: refactor compat code

[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 <mst@redhat.com>
---
 hw/pc_piix.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index a771d79..008d42f 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -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[]) {
-- 
MST

[Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[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_eoi

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 850b87b..3f2532a 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -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
diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
new file mode 100644
index 0000000..2422d20
--- /dev/null
+++ b/hw/cpu_flags.c
@@ -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;
+}
diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
new file mode 100644
index 0000000..05777b6
--- /dev/null
+++ b/hw/cpu_flags.h
@@ -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
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 008d42f..bdbceda 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -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,
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 120a2e3..0d02fd1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -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

[Qemu-devel] [PATCHv2 4/4] kvm: get/set PV EOI MSR

[Michael S. Tsirkin]

Support 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).

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 target-i386/cpu.h     |  1 +
 target-i386/kvm.c     | 13 +++++++++++++
 target-i386/machine.c | 21 +++++++++++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index aabf993..3c57d8b 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -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;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5e2d4f5..6790180 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -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;
         }
     }
 
diff --git a/target-i386/machine.c b/target-i386/machine.c
index a8be058..4771508 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -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,
         }, {
-- 
MST

Re: [Qemu-devel] [PATCHv2 1/4] linux-headers: update to 3.6-rc3

[Peter Maydell]

On 27 August 2012 13:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  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

[Jan Kiszka]

On 2012-08-27 14:42, Peter Maydell wrote:
> On 27 August 2012 13:20, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 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.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  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?

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

-- 
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

[Michael S. Tsirkin]

On Mon, Aug 27, 2012 at 01:42:03PM +0100, Peter Maydell wrote:
> On 27 August 2012 13:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  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

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?

-- 
MST

Re: [Qemu-devel] [PATCHv2 1/4] linux-headers: update to 3.6-rc3

[Michael S. Tsirkin]

On Mon, Aug 27, 2012 at 02:48:57PM +0200, Jan Kiszka wrote:
> On 2012-08-27 14:42, Peter Maydell wrote:
> > On 27 August 2012 13:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> 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.
> >>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >>  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?
> 
> 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.

> -- 
> 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

[Jan Kiszka]

On 2012-08-27 16:53, Michael S. Tsirkin wrote:
> On Mon, Aug 27, 2012 at 02:48:57PM +0200, Jan Kiszka wrote:
>> On 2012-08-27 14:42, Peter Maydell wrote:
>>> On 27 August 2012 13:20, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>  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?
>>
>> 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

-- 
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

[Michael S. Tsirkin]

On Mon, Aug 27, 2012 at 04:59:40PM +0200, Jan Kiszka wrote:
> On 2012-08-27 16:53, Michael S. Tsirkin wrote:
> > On Mon, Aug 27, 2012 at 02:48:57PM +0200, Jan Kiszka wrote:
> >> On 2012-08-27 14:42, Peter Maydell wrote:
> >>> On 27 August 2012 13:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> 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.
> >>>>
> >>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>> ---
> >>>>  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?
> >>
> >> 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


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.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index 9d2a4bc..6607e56 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -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"
-- 
MST

Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[Blue Swirl]

On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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_eoi
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  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
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 850b87b..3f2532a 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -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
> diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
> new file mode 100644
> index 0000000..2422d20
> --- /dev/null
> +++ b/hw/cpu_flags.c
> @@ -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;

Don't use identifiers with leading underscores.

> +
> +void disable_kvm_pv_eoi(void)
> +{
> +       __kvm_pv_eoi_disabled = true;
> +}
> +
> +bool kvm_pv_eoi_disabled(void)
> +{
> +       return __kvm_pv_eoi_disabled;
> +}
> diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
> new file mode 100644
> index 0000000..05777b6
> --- /dev/null
> +++ b/hw/cpu_flags.h
> @@ -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
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 008d42f..bdbceda 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -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,
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 120a2e3..0d02fd1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -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

[Michael S. Tsirkin]

On Mon, Aug 27, 2012 at 06:58:29PM +0000, Blue Swirl wrote:
> On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > 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_eoi
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  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
> >
> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > index 850b87b..3f2532a 100644
> > --- a/hw/Makefile.objs
> > +++ b/hw/Makefile.objs
> > @@ -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
> > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
> > new file mode 100644
> > index 0000000..2422d20
> > --- /dev/null
> > +++ b/hw/cpu_flags.c
> > @@ -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;
> 
> 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.


> > +
> > +void disable_kvm_pv_eoi(void)
> > +{
> > +       __kvm_pv_eoi_disabled = true;
> > +}
> > +
> > +bool kvm_pv_eoi_disabled(void)
> > +{
> > +       return __kvm_pv_eoi_disabled;
> > +}
> > diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
> > new file mode 100644
> > index 0000000..05777b6
> > --- /dev/null
> > +++ b/hw/cpu_flags.h
> > @@ -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
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 008d42f..bdbceda 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -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,
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 120a2e3..0d02fd1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -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

[Blue Swirl]

On Mon, Aug 27, 2012 at 7:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Aug 27, 2012 at 06:58:29PM +0000, Blue Swirl wrote:
>> On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > 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_eoi
>> >
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> >  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
>> >
>> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> > index 850b87b..3f2532a 100644
>> > --- a/hw/Makefile.objs
>> > +++ b/hw/Makefile.objs
>> > @@ -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
>> > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
>> > new file mode 100644
>> > index 0000000..2422d20
>> > --- /dev/null
>> > +++ b/hw/cpu_flags.c
>> > @@ -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;
>>
>> 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?

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?

>
> But OK, will rename _kvm_pv_eoi_disabled.
> _ + lower case is guaranteed OK.

No, just use kvm_pv_eoi_disabled, the underscore is useless.

>
>
>> > +
>> > +void disable_kvm_pv_eoi(void)
>> > +{
>> > +       __kvm_pv_eoi_disabled = true;
>> > +}
>> > +
>> > +bool kvm_pv_eoi_disabled(void)
>> > +{
>> > +       return __kvm_pv_eoi_disabled;
>> > +}
>> > diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
>> > new file mode 100644
>> > index 0000000..05777b6
>> > --- /dev/null
>> > +++ b/hw/cpu_flags.h
>> > @@ -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
>> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> > index 008d42f..bdbceda 100644
>> > --- a/hw/pc_piix.c
>> > +++ b/hw/pc_piix.c
>> > @@ -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,
>> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> > index 120a2e3..0d02fd1 100644
>> > --- a/target-i386/cpu.c
>> > +++ b/target-i386/cpu.c
>> > @@ -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

[Michael S. Tsirkin]

On Mon, Aug 27, 2012 at 07:12:27PM +0000, Blue Swirl wrote:
> On Mon, Aug 27, 2012 at 7:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Aug 27, 2012 at 06:58:29PM +0000, Blue Swirl wrote:
> >> On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > 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_eoi
> >> >
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> > ---
> >> >  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
> >> >
> >> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> >> > index 850b87b..3f2532a 100644
> >> > --- a/hw/Makefile.objs
> >> > +++ b/hw/Makefile.objs
> >> > @@ -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
> >> > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
> >> > new file mode 100644
> >> > index 0000000..2422d20
> >> > --- /dev/null
> >> > +++ b/hw/cpu_flags.c
> >> > @@ -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;
> >>
> >> 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?
> 
> 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.

> >
> > 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.

-- 
MST

Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[Blue Swirl]

On Mon, Aug 27, 2012 at 7:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Aug 27, 2012 at 07:12:27PM +0000, Blue Swirl wrote:
>> On Mon, Aug 27, 2012 at 7:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Mon, Aug 27, 2012 at 06:58:29PM +0000, Blue Swirl wrote:
>> >> On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> > 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_eoi
>> >> >
>> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> >> > ---
>> >> >  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
>> >> >
>> >> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> >> > index 850b87b..3f2532a 100644
>> >> > --- a/hw/Makefile.objs
>> >> > +++ b/hw/Makefile.objs
>> >> > @@ -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
>> >> > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
>> >> > new file mode 100644
>> >> > index 0000000..2422d20
>> >> > --- /dev/null
>> >> > +++ b/hw/cpu_flags.c
>> >> > @@ -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;
>> >>
>> >> 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?
>>
>> 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.

No leading underscores. They are not used in QEMU.

>
>> >
>> > 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.

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?

>
> --
> MST

Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[Michael S. Tsirkin]

On Mon, Aug 27, 2012 at 07:40:56PM +0000, Blue Swirl wrote:
> On Mon, Aug 27, 2012 at 7:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Aug 27, 2012 at 07:12:27PM +0000, Blue Swirl wrote:
> >> On Mon, Aug 27, 2012 at 7:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Mon, Aug 27, 2012 at 06:58:29PM +0000, Blue Swirl wrote:
> >> >> On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> >> > 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_eoi
> >> >> >
> >> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> >> > ---
> >> >> >  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
> >> >> >
> >> >> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> >> >> > index 850b87b..3f2532a 100644
> >> >> > --- a/hw/Makefile.objs
> >> >> > +++ b/hw/Makefile.objs
> >> >> > @@ -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
> >> >> > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
> >> >> > new file mode 100644
> >> >> > index 0000000..2422d20
> >> >> > --- /dev/null
> >> >> > +++ b/hw/cpu_flags.c
> >> >> > @@ -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;
> >> >>
> >> >> 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?
> >>
> >> 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.
> 
> 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.

> >
> >> >
> >> > 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.
> 
> 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?

> 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.

> >
> > --
> > MST

Re: [Qemu-devel] [PATCHv2 2/4] pc: refactor compat code

[Marcelo Tosatti]

On Mon, Aug 27, 2012 at 03:20:13PM +0300, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
>  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: [Qemu-devel] [PATCHv2 2/4] pc: refactor compat code

[Michael S. Tsirkin]

On Tue, Aug 28, 2012 at 01:23:18PM -0300, Marcelo Tosatti wrote:
> On Mon, Aug 27, 2012 at 03:20:13PM +0300, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> > ---
> >  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.
-- 
MST

Re: [Qemu-devel] [PATCHv2 2/4] pc: refactor compat code

[Marcelo Tosatti]

On Tue, Aug 28, 2012 at 07:31:24PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 28, 2012 at 01:23:18PM -0300, Marcelo Tosatti wrote:
> > On Mon, Aug 27, 2012 at 03:20:13PM +0300, Michael S. Tsirkin wrote:
> > > 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 <mst@redhat.com>
> > > ---
> > >  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.

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.

> It will also scale better if we ever get interested about
> compatibility and migration for non pc machines.
> -- 
> MST

Alright, will wait a couple of days before merging.

Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[malc]

On Tue, 28 Aug 2012, Michael S. Tsirkin wrote:

> On Mon, Aug 27, 2012 at 07:40:56PM +0000, Blue Swirl wrote:
> > On Mon, Aug 27, 2012 at 7:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Mon, Aug 27, 2012 at 07:12:27PM +0000, Blue Swirl wrote:
> > >> On Mon, Aug 27, 2012 at 7:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> > On Mon, Aug 27, 2012 at 06:58:29PM +0000, Blue Swirl wrote:
> > >> >> On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> >> > 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_eoi
> > >> >> >
> > >> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >> >> > ---
> > >> >> >  hw/Makefile.objs  |  2 +-
> > >> >> >  hw/cpu_flags.c    | 32 ++++++++++++++++++++++++++++++++
> > >> >> >  hw/cpu_flags.h    |  9 +++++++++
> > >> >> >  hw/pc_piix.c      |  2 ++

[..snip..]

> > 
> > 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.

> 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..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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_eoi
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  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
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 850b87b..3f2532a 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -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
> diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
> new file mode 100644
> index 0000000..2422d20
> --- /dev/null
> +++ b/hw/cpu_flags.c
> @@ -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;
> +}

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

> diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
> new file mode 100644
> index 0000000..05777b6
> --- /dev/null
> +++ b/hw/cpu_flags.h
> @@ -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
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 008d42f..bdbceda 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -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,
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 120a2e3..0d02fd1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -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

[Eduardo Habkost]

On Tue, Aug 28, 2012 at 02:13:18PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > 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_eoi
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  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
> >
> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > index 850b87b..3f2532a 100644
> > --- a/hw/Makefile.objs
> > +++ b/hw/Makefile.objs
> > @@ -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
> > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
> > new file mode 100644
> > index 0000000..2422d20
> > --- /dev/null
> > +++ b/hw/cpu_flags.c
> > @@ -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;
> > +}
> 
> 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).

> 
> 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.

> 
> Regards,
> 
> Anthony Liguori
> 
> > diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
> > new file mode 100644
> > index 0000000..05777b6
> > --- /dev/null
> > +++ b/hw/cpu_flags.h
> > @@ -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
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 008d42f..bdbceda 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -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,
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 120a2e3..0d02fd1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -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
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCHv2 2/4] pc: refactor compat code

[Michael S. Tsirkin]

On Tue, Aug 28, 2012 at 01:37:18PM -0300, Marcelo Tosatti wrote:
> On Tue, Aug 28, 2012 at 07:31:24PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 28, 2012 at 01:23:18PM -0300, Marcelo Tosatti wrote:
> > > On Mon, Aug 27, 2012 at 03:20:13PM +0300, Michael S. Tsirkin wrote:
> > > > 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 <mst@redhat.com>
> > > > ---
> > > >  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.
> 
> 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 ...

> 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.
>
> > It will also scale better if we ever get interested about
> > compatibility and migration for non pc machines.
> > -- 
> > MST
> 
> Alright, will wait a couple of days before merging.

Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[Michael S. Tsirkin]

On Tue, Aug 28, 2012 at 02:13:18PM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > 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_eoi
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  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
> >
> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > index 850b87b..3f2532a 100644
> > --- a/hw/Makefile.objs
> > +++ b/hw/Makefile.objs
> > @@ -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
> > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
> > new file mode 100644
> > index 0000000..2422d20
> > --- /dev/null
> > +++ b/hw/cpu_flags.c
> > @@ -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;
> > +}
> 
> 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

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 -- disabled

> > diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
> > new file mode 100644
> > index 0000000..05777b6
> > --- /dev/null
> > +++ b/hw/cpu_flags.h
> > @@ -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
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 008d42f..bdbceda 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -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,
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 120a2e3..0d02fd1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -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

[Michael S. Tsirkin]

On Tue, Aug 28, 2012 at 04:40:38PM -0300, Eduardo Habkost wrote:
> On Tue, Aug 28, 2012 at 02:13:18PM -0500, Anthony Liguori wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > 
> > > 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_eoi
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  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
> > >
> > > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > > index 850b87b..3f2532a 100644
> > > --- a/hw/Makefile.objs
> > > +++ b/hw/Makefile.objs
> > > @@ -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
> > > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c
> > > new file mode 100644
> > > index 0000000..2422d20
> > > --- /dev/null
> > > +++ b/hw/cpu_flags.c
> > > @@ -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;
> > > +}
> > 
> > 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).
> > 
> > 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?

> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> > > diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
> > > new file mode 100644
> > > index 0000000..05777b6
> > > --- /dev/null
> > > +++ b/hw/cpu_flags.h
> > > @@ -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
> > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > > index 008d42f..bdbceda 100644
> > > --- a/hw/pc_piix.c
> > > +++ b/hw/pc_piix.c
> > > @@ -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,
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 120a2e3..0d02fd1 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -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
> > 
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[Michael S. Tsirkin]

On Tue, Aug 28, 2012 at 09:02:05PM +0400, malc wrote:
> On Tue, 28 Aug 2012, Michael S. Tsirkin wrote:
> 
> > On Mon, Aug 27, 2012 at 07:40:56PM +0000, Blue Swirl wrote:
> > > On Mon, Aug 27, 2012 at 7:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Mon, Aug 27, 2012 at 07:12:27PM +0000, Blue Swirl wrote:
> > > >> On Mon, Aug 27, 2012 at 7:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >> > On Mon, Aug 27, 2012 at 06:58:29PM +0000, Blue Swirl wrote:
> > > >> >> On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >> >> > 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_eoi
> > > >> >> >
> > > >> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >> >> > ---
> > > >> >> >  hw/Makefile.objs  |  2 +-
> > > >> >> >  hw/cpu_flags.c    | 32 ++++++++++++++++++++++++++++++++
> > > >> >> >  hw/cpu_flags.h    |  9 +++++++++
> > > >> >> >  hw/pc_piix.c      |  2 ++
> 
> [..snip..]
> 
> > > 
> > > 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.
> 
> > 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..]

It's still silly :)

> -- 
> mailto:av1474@comtv.ru