[Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[Eduardo Habkost]

I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
I sent last week, though.

Igor argued that APIC ID should be set by the board and not by the CPU itself,
but I am not doing that because:
 - I want to keep the bug fix simple and isolated as we are past soft freeze
 - I believe the creator of the CPU object shouldn't be forced to provide the
   APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
   device_add interface in the future
 - The APIC ID _is_ set by the CPU itself (because each CPU package may have
   multiple core/threads, and each core/thread has a different APIC ID). What
   needs to be provided by the board to the CPU package in the future is the
   package ID and the bit width of the core/thread IDs.

Git tree for reference:
  git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
  https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5

Eduardo Habkost (12):
  kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
    KVM
  target-i386: Don't set any KVM flag by default if KVM is disabled
  pc: Reverse pc_init_pci() compatibility logic
  kvm: Create kvm_arch_vcpu_id() function
  target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
  fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
  target-i386/cpu: Introduce apic_id_for_cpu() function
  cpus.h: Make constant smp_cores/smp_threads available on *-user
  pc: Set fw_cfg data based on APIC ID calculation
  tests: Support target-specific unit tests
  target-i386: Topology & APIC ID utility functions
  pc: Generate APIC IDs according to CPU topology

 hw/fw_cfg.c            |   1 -
 hw/pc.c                |  44 +++++++++++++---
 hw/pc_piix.c           |  26 +++++++---
 hw/ppc_newworld.c      |   1 +
 hw/ppc_oldworld.c      |   1 +
 hw/sun4m.c             |   3 ++
 hw/sun4u.c             |   1 +
 include/sysemu/cpus.h  |   7 +++
 include/sysemu/kvm.h   |   4 ++
 kvm-all.c              |   2 +-
 target-i386/cpu.c      |  52 +++++++++++++++----
 target-i386/cpu.h      |   5 +-
 target-i386/kvm.c      |   6 +++
 target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
 target-ppc/kvm.c       |   5 ++
 target-s390x/kvm.c     |   5 ++
 tests/.gitignore       |   1 +
 tests/Makefile         |  21 +++++++-
 tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
 19 files changed, 391 insertions(+), 28 deletions(-)
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c

-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
---
 include/sysemu/kvm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 6bdd513..22acf91 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -36,6 +36,7 @@
 #define KVM_FEATURE_ASYNC_PF     0
 #define KVM_FEATURE_STEAL_TIME   0
 #define KVM_FEATURE_PV_EOI       0
+#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
 #endif
 
 extern int kvm_allowed;
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 02/12] target-i386: Don't set any KVM flag by default if KVM is disabled

[Eduardo Habkost]

This is a cleanup that tries to solve two small issues:

 - We don't need a separate kvm_pv_eoi_features variable just to keep a
   constant calculated at compile-time, and this style would require
   adding a separate variable (that's declared twice because of the
   CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
   by machine-type compat code.
 - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
   even when KVM is disabled at runtime. This small incosistency in
   the cpuid_kvm_features field isn't a problem today because
   cpuid_kvm_features is ignored by the TCG code, but it may cause
   unexpected problems later when refactoring the CPUID handling code.

This patch eliminates the kvm_pv_eoi_features variable and simply uses
kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
behavior of enable_kvm_pv_eoi() clearer and easier to understand.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>

Changes v2:
 - Coding style fix

Changes v3:
 - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define

Changes v4:
 - Check kvm_enabled() when actually using kvm_default_features
 - Eliminate Yet Another #ifdef by using the fake KVM_FEATURE_*
   #defines on kvm_default_features initialization

Changes v5:
 - Rebase and move the kvm_enabled() check to cpu_x86_register()
---
 target-i386/cpu.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 333745b..754eb6f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -206,22 +206,16 @@ typedef struct model_features_t {
 int check_cpuid = 0;
 int enforce_cpuid = 0;
 
-#if defined(CONFIG_KVM)
 static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
         (1 << KVM_FEATURE_NOP_IO_DELAY) |
         (1 << KVM_FEATURE_CLOCKSOURCE2) |
         (1 << KVM_FEATURE_ASYNC_PF) |
         (1 << KVM_FEATURE_STEAL_TIME) |
         (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
-static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
-#else
-static uint32_t kvm_default_features = 0;
-static const uint32_t kvm_pv_eoi_features = 0;
-#endif
 
 void enable_kvm_pv_eoi(void)
 {
-    kvm_default_features |= kvm_pv_eoi_features;
+    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
 }
 
 void host_cpuid(uint32_t function, uint32_t count,
@@ -1602,7 +1596,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         goto out;
     }
 
-    def->kvm_features |= kvm_default_features;
+    if (kvm_enabled()) {
+        def->kvm_features |= kvm_default_features;
+    }
     def->ext_features |= CPUID_EXT_HYPERVISOR;
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-- 
1.7.11.7

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

[Eduardo Habkost]

Currently, the pc-1.4 machine init function enables PV EOI and then
calls the pc-1.2 machine init function. The problem with this approach
is that now we can't enable any additional compatibility code inside the
pc-1.2 init function because it would end up enabling the compatibility
behavior on pc-1.3 and pc-1.4 as well.

This reverses the logic so that the pc-1.2 machine init function will
disable PV EOI, and then call the pc-1.4 machine init function.

This way we can change older machine-types to enable compatibility
behavior, and the newer machine-types (pc-1.3, pc-q35-1.4 and
pc-i440fx-1.4) would just use the default behavior.

(This means that one nice side-effect of this change is that pc-q35-1.4
will get PV EOI enabled by default, too)

It would be interesting to eventually change pc_init_pci_no_kvmclock()
and pc_init_isa() to reuse pc_init_pci_1_2() as well (so we don't need
to duplicate compatibility code on those two functions). But this will
be probably much easier to do after we create a PCInitArgs struct for
the PC initialization arguments, and/or after we use global-properties
to implement the compatibility modes present in pc_init_pci_1_2().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc_piix.c      | 22 +++++++++++++---------
 target-i386/cpu.c |  5 +++--
 target-i386/cpu.h |  2 +-
 3 files changed, 17 insertions(+), 12 deletions(-)

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

[Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

This will allow each architecture to define how the VCPU ID is set on
the KVM_CREATE_VCPU ioctl call.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>

Changes v2:
 - Get CPUState as argument instead of CPUArchState
---
 include/sysemu/kvm.h | 3 +++
 kvm-all.c            | 2 +-
 target-i386/kvm.c    | 5 +++++
 target-ppc/kvm.c     | 5 +++++
 target-s390x/kvm.c   | 5 +++++
 5 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 22acf91..384ee66 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -196,6 +196,9 @@ int kvm_arch_init(KVMState *s);
 
 int kvm_arch_init_vcpu(CPUState *cpu);
 
+/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
+unsigned long kvm_arch_vcpu_id(CPUState *cpu);
+
 void kvm_arch_reset_vcpu(CPUState *cpu);
 
 int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
diff --git a/kvm-all.c b/kvm-all.c
index 6278d61..995220d 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
 
     DPRINTF("kvm_init_vcpu\n");
 
-    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
+    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
     if (ret < 0) {
         DPRINTF("kvm_create_vcpu failed\n");
         goto err;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 3acff40..5f3f789 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
     }
 }
 
+unsigned long kvm_arch_vcpu_id(CPUState *cpu)
+{
+    return cpu->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     struct {
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 19e9f25..1e544ae 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -384,6 +384,11 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
 
 #endif /* !defined (TARGET_PPC64) */
 
+unsigned long kvm_arch_vcpu_id(CPUState *cpu)
+{
+    return cpu->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 6ec5e6d..bd9864c 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
     return 0;
 }
 
+unsigned long kvm_arch_vcpu_id(CPUState *cpu)
+{
+    return cpu->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUState *cpu)
 {
     int ret = 0;
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 05/12] target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index

[Eduardo Habkost]

The CPU ID in KVM is supposed to be the APIC ID, so change the
KVM_CREATE_VCPU call to match it. The current behavior didn't break
anything yet because today the APIC ID is assumed to be equal to the CPU
index, but this won't be true in the future.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: kvm@vger.kernel.org
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Gleb Natapov <gleb@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>

Changes v2:
 - Change only i386 code (kvm_arch_vcpu_id())

Changes v3:
 - Get CPUState as argument instead of CPUArchState
---
 target-i386/kvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5f3f789..c440809 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -411,9 +411,10 @@ static void cpu_update_state(void *opaque, int running, RunState state)
     }
 }
 
-unsigned long kvm_arch_vcpu_id(CPUState *cpu)
+unsigned long kvm_arch_vcpu_id(CPUState *cs)
 {
-    return cpu->cpu_index;
+    X86CPU *cpu = X86_CPU(cs);
+    return cpu->env.cpuid_apic_id;
 }
 
 int kvm_arch_init_vcpu(CPUState *cs)
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 06/12] fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()

[Eduardo Habkost]

PC will not use max_cpus for that field, so move it outside the common
code so it can use a different value on PC.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/fw_cfg.c       | 1 -
 hw/pc.c           | 2 +-
 hw/ppc_newworld.c | 1 +
 hw/ppc_oldworld.c | 1 +
 hw/sun4m.c        | 3 +++
 hw/sun4u.c        | 1 +
 6 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 7c9480c..2a31339 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -519,7 +519,6 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
-    fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
     fw_cfg_bootsplash(s);
     fw_cfg_reboot(s);
diff --git a/hw/pc.c b/hw/pc.c
index ba1f19d..076398b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -560,7 +560,7 @@ static void *bochs_bios_init(void)
     int i, j;
 
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
-
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, (uint8_t *)acpi_tables,
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index 7a465a7..85ba1c4 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -397,6 +397,7 @@ static void ppc_core99_init(QEMUMachineInitArgs *args)
     /* No PCI init: the BIOS will do it */
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
index de34e75..e4eb444 100644
--- a/hw/ppc_oldworld.c
+++ b/hw/ppc_oldworld.c
@@ -296,6 +296,7 @@ static void ppc_heathrow_init(QEMUMachineInitArgs *args)
     /* No PCI init: the BIOS will do it */
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
diff --git a/hw/sun4m.c b/hw/sun4m.c
index 6f5de44..7474059 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -1021,6 +1021,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
                  hwdef->ecc_version);
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
@@ -1667,6 +1668,7 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
                "Sun4d");
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
@@ -1869,6 +1871,7 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
                "Sun4c");
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
diff --git a/hw/sun4u.c b/hw/sun4u.c
index cb75d03..95850d3 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -878,6 +878,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
                            (uint8_t *)&nd_table[0].macaddr);
 
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 07/12] target-i386/cpu: Introduce apic_id_for_cpu() function

[Eduardo Habkost]

This function will be used by both the CPU initialization code and the
fw_cfg table initialization code.

Later this function will be updated to generate APIC IDs according to
the CPU topology.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 17 ++++++++++++++++-
 target-i386/cpu.h |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d1a14d5..d90789d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2194,6 +2194,21 @@ void x86_cpu_realize(Object *obj, Error **errp)
     cpu_reset(CPU(cpu));
 }
 
+/* Calculates initial APIC ID for a specific CPU index
+ *
+ * Currently we need to be able to calculate the APIC ID from the CPU index
+ * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
+ * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
+ * all CPUs up to max_cpus.
+ */
+uint32_t apic_id_for_cpu(unsigned int cpu_index)
+{
+    /* right now APIC ID == CPU index. this will eventually change to use
+     * the CPU topology configuration properly
+     */
+    return cpu_index;
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -2228,7 +2243,7 @@ static void x86_cpu_initfn(Object *obj)
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
 
-    env->cpuid_apic_id = cs->cpu_index;
+    env->cpuid_apic_id = apic_id_for_cpu(cs->cpu_index);
 
     /* init various static tables used in TCG mode */
     if (tcg_enabled() && !inited) {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 9d4fcf9..d86c0af 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1255,4 +1255,6 @@ void disable_kvm_pv_eoi(void);
 /* Return name of 32-bit register, from a R_* constant */
 const char *get_register_name_32(unsigned int reg);
 
+uint32_t apic_id_for_cpu(unsigned int cpu_index);
+
 #endif /* CPU_I386_H */
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 08/12] cpus.h: Make constant smp_cores/smp_threads available on *-user

[Eduardo Habkost]

The code that calculates the APIC ID will use smp_cores/smp_threads, so
just define them as 1 on *-user to avoid #ifdefs in the code.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/sysemu/cpus.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index 81bd817..f7f6854 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -13,9 +13,16 @@ void cpu_synchronize_all_post_init(void);
 
 void qtest_clock_warp(int64_t dest);
 
+#ifndef CONFIG_USER_ONLY
 /* vl.c */
 extern int smp_cores;
 extern int smp_threads;
+#else
+/* *-user doesn't have configurable SMP topology */
+#define smp_cores   1
+#define smp_threads 1
+#endif
+
 void set_numa_modes(void);
 void set_cpu_log(const char *optarg);
 void set_cpu_log_filename(const char *optarg);
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 09/12] pc: Set fw_cfg data based on APIC ID calculation

[Eduardo Habkost]

This changes FW_CFG_MAX_CPUS and FW_CFG_NUMA to use apic_id_for_cpu(),
so the NUMA table can be based on the APIC IDs, instead of CPU index
(SeaBIOS knows nothing about CPU indexes, just APIC IDs).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 - Get PC object as argument
 - Add more detailed comments explaining the reason for FW_CFG_MAX_CPUS
   not being simply 'max_cpus'

Changes v3:
 - Use PCInitArgs instead of PC object

Changes v4:
 - Don't use PCInitArgs, just add the necessary data for apic_id_limit()
   as argument
 - Rename function to pc_apic_id_limit()
 - Rename max_apic_id to apic_id_limit
---
 hw/pc.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 076398b..655bbd3 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -551,6 +551,18 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
+/* Calculates the limit to CPU APIC ID values
+ *
+ * This function returns the limit for the APIC ID value, so that all
+ * CPU APIC IDs are < pc_apic_id_limit().
+ *
+ * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
+ */
+static unsigned int pc_apic_id_limit(unsigned int max_cpus)
+{
+    return apic_id_for_cpu(max_cpus - 1) + 1;
+}
+
 static void *bochs_bios_init(void)
 {
     void *fw_cfg;
@@ -558,9 +570,24 @@ static void *bochs_bios_init(void)
     size_t smbios_len;
     uint64_t *numa_fw_cfg;
     int i, j;
+    unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
-    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
+    /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
+     *
+     * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug
+     * QEMU<->SeaBIOS interface is not based on the "CPU index", but on the APIC
+     * ID of hotplugged CPUs[1]. This means that FW_CFG_MAX_CPUS is not the
+     * "maximum number of CPUs", but the "limit to the APIC ID values SeaBIOS
+     * may see".
+     *
+     * So, this means we must not use max_cpus, here, but the maximum possible
+     * APIC ID value, plus one.
+     *
+     * [1] The only kind of "CPU identifier" used between SeaBIOS and QEMU is
+     *     the APIC ID, not the "CPU index"
+     */
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)apic_id_limit);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, (uint8_t *)acpi_tables,
@@ -580,21 +607,24 @@ static void *bochs_bios_init(void)
      * of nodes, one word for each VCPU->node and one word for each node to
      * hold the amount of memory.
      */
-    numa_fw_cfg = g_malloc0((1 + max_cpus + nb_numa_nodes) * 8);
+    numa_fw_cfg = g_malloc0((1 + apic_id_limit + nb_numa_nodes) * 8);
     numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
-    for (i = 0; i < max_cpus; i++) {
+    unsigned int cpu_idx;
+    for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) {
+        unsigned int apic_id = apic_id_for_cpu(cpu_idx);
+        assert(apic_id < apic_id_limit);
         for (j = 0; j < nb_numa_nodes; j++) {
-            if (test_bit(i, node_cpumask[j])) {
-                numa_fw_cfg[i + 1] = cpu_to_le64(j);
+            if (test_bit(cpu_idx, node_cpumask[j])) {
+                numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
                 break;
             }
         }
     }
     for (i = 0; i < nb_numa_nodes; i++) {
-        numa_fw_cfg[max_cpus + 1 + i] = cpu_to_le64(node_mem[i]);
+        numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(node_mem[i]);
     }
     fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, (uint8_t *)numa_fw_cfg,
-                     (1 + max_cpus + nb_numa_nodes) * 8);
+                     (1 + apic_id_limit + nb_numa_nodes) * 8);
 
     return fw_cfg;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 10/12] tests: Support target-specific unit tests

[Eduardo Habkost]

To make unit tests that depend on target-specific files, use
check-unit-<arch>-y and test-obj-<arch>-y.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/Makefile | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile b/tests/Makefile
index d86e95a..41172d6 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -112,9 +112,21 @@ tests/fdc-test$(EXESUF): tests/fdc-test.o
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o
 
-# QTest rules
+# unit test rules:
 
 TARGETS=$(patsubst %-softmmu,%, $(filter %-softmmu,$(TARGET_DIRS)))
+
+# target-specific tests/objs:
+
+test-obj-y += $(foreach TARGET,$(TARGETS), $(test-obj-$(TARGET)-y))
+check-unit-y += $(foreach TARGET,$(TARGETS), $(check-unit-$(TARGET)-y))
+
+$(foreach TARGET,$(TARGETS),$(eval $(test-obj-$(TARGET)-y): QEMU_INCLUDES += -Itarget-$(TARGET)))
+
+$(test-obj-y): QEMU_INCLUDES += -Itests
+
+# QTest rules
+
 QTEST_TARGETS=$(foreach TARGET,$(TARGETS), $(if $(check-qtest-$(TARGET)-y), $(TARGET),))
 check-qtest-$(CONFIG_POSIX)=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y))
 
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 11/12] target-i386: Topology & APIC ID utility functions

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 - Support 32-bit APIC IDs (in case x2APIC is going to be used)
 - Coding style changes
 - Use TARGET_I386_TOPOLOGY_H instead of __QEMU_X86_TOPOLOGY_H__
 - Rename topo_make_apic_id() to topo_apicid_for_cpu()
 - Rename __make_apicid() to topo_make_apicid()
 - Spaces around operators on test-x86-cpuid.c, as requested by
   Blue Swirl
 - Make test-x86-cpuid a target-specific test

Changes v2 -> v3:
 - Add documentation pointers to the code
 - Rename bits_for_count() to bitwidth_for_count()
 - Remove unused apicid_*_id() functions

Changes v3 -> v4:
 - Remove now-obsolete FIXME comment from test-x86-cpuid.c
 - Change bitops.h include to qemu/bitops.h
 - Add gcov file list to test-x86-cpuid
---
 target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/.gitignore       |   1 +
 tests/Makefile         |   7 +++
 tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
 4 files changed, 242 insertions(+)
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c

diff --git a/target-i386/topology.h b/target-i386/topology.h
new file mode 100644
index 0000000..833ab47
--- /dev/null
+++ b/target-i386/topology.h
@@ -0,0 +1,133 @@
+/*
+ *  x86 CPU topology data structures and functions
+ *
+ *  Copyright (c) 2012 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef TARGET_I386_TOPOLOGY_H
+#define TARGET_I386_TOPOLOGY_H
+
+/* This file implements the APIC-ID-based CPU topology enumeration logic,
+ * documented at the following document:
+ *   Intel® 64 Architecture Processor Topology Enumeration
+ *   http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
+ *
+ * This code should be compatible with AMD's "Extended Method" described at:
+ *   AMD CPUID Specification (Publication #25481)
+ *   Section 3: Multiple Core Calcuation
+ * as long as:
+ *  nr_threads is set to 1;
+ *  OFFSET_IDX is assumed to be 0;
+ *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
+ */
+
+#include <stdint.h>
+#include <string.h>
+
+#include "qemu/bitops.h"
+
+/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
+ */
+typedef uint32_t apic_id_t;
+
+/* Return the bit width needed for 'count' IDs
+ */
+static unsigned bitwidth_for_count(unsigned count)
+{
+    g_assert(count >= 1);
+    if (count == 1) {
+        return 0;
+    }
+    return bitops_flsl(count - 1) + 1;
+}
+
+/* Bit width of the SMT_ID (thread ID) field on the APIC ID
+ */
+static inline unsigned apicid_smt_width(unsigned nr_cores, unsigned nr_threads)
+{
+    return bitwidth_for_count(nr_threads);
+}
+
+/* Bit width of the Core_ID field
+ */
+static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
+{
+    return bitwidth_for_count(nr_cores);
+}
+
+/* Bit offset of the Core_ID field
+ */
+static inline unsigned apicid_core_offset(unsigned nr_cores,
+                                          unsigned nr_threads)
+{
+    return apicid_smt_width(nr_cores, nr_threads);
+}
+
+/* Bit offset of the Pkg_ID (socket ID) field
+ */
+static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
+{
+    return apicid_core_offset(nr_cores, nr_threads) + \
+           apicid_core_width(nr_cores, nr_threads);
+}
+
+/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
+ *
+ * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
+ */
+static inline apic_id_t topo_make_apicid(unsigned nr_cores,
+                                         unsigned nr_threads,
+                                         unsigned pkg_id, unsigned core_id,
+                                         unsigned smt_id)
+{
+    return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) | \
+           (core_id << apicid_core_offset(nr_cores, nr_threads)) | \
+           smt_id;
+}
+
+/* Calculate thread/core/package IDs for a specific topology,
+ * based on (contiguous) CPU index
+ */
+static inline void topo_ids_from_idx(unsigned nr_cores, unsigned nr_threads,
+                                     unsigned cpu_index,
+                                     unsigned *pkg_id, unsigned *core_id,
+                                     unsigned *smt_id)
+{
+    unsigned core_index = cpu_index / nr_threads;
+    *smt_id = cpu_index % nr_threads;
+    *core_id = core_index % nr_cores;
+    *pkg_id = core_index / nr_cores;
+}
+
+/* Make APIC ID for the CPU 'cpu_index'
+ *
+ * 'cpu_index' is a sequential, contiguous ID for the CPU.
+ */
+static inline apic_id_t topo_apicid_for_cpu(unsigned nr_cores,
+                                            unsigned nr_threads,
+                                            unsigned cpu_index)
+{
+    unsigned pkg_id, core_id, smt_id;
+    topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
+                      &pkg_id, &core_id, &smt_id);
+    return topo_make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id);
+}
+
+#endif /* TARGET_I386_TOPOLOGY_H */
diff --git a/tests/.gitignore b/tests/.gitignore
index f9041f3..38c94ef 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -10,4 +10,5 @@ test-qmp-commands.h
 test-qmp-commands
 test-qmp-input-strict
 test-qmp-marshal.c
+test-x86-cpuid
 *-test
diff --git a/tests/Makefile b/tests/Makefile
index 41172d6..d46e64c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -46,6 +46,11 @@ gcov-files-test-aio-$(CONFIG_POSIX) = aio-posix.c
 check-unit-y += tests/test-thread-pool$(EXESUF)
 gcov-files-test-thread-pool-y = thread-pool.c
 
+check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
+# all code tested by test-x86-cpuid is inside topology.h,
+# so add the test file itself to the gcov list
+gcov-files-test-x86-cpuid-y = tests/test-x86-cpuid.c
+
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
 # All QTests for now are POSIX-only, but the dependencies are
@@ -71,6 +76,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
 	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
 	tests/test-qmp-commands.o tests/test-visitor-serialization.o
+test-obj-i386-y = tests/test-x86-cpuid.o
 
 test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o
 
@@ -86,6 +92,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil
 tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-iov$(EXESUF): tests/test-iov.o libqemuutil.a
+tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
 
 tests/test-qapi-types.c tests/test-qapi-types.h :\
 $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
new file mode 100644
index 0000000..1fe9f30
--- /dev/null
+++ b/tests/test-x86-cpuid.c
@@ -0,0 +1,101 @@
+/*
+ *  Test code for x86 CPUID and Topology functions
+ *
+ *  Copyright (c) 2012 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <glib.h>
+
+#include "topology.h"
+
+static void test_topo_bits(void)
+{
+    /* simple tests for 1 thread per core, 1 core per socket */
+    g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0);
+    g_assert_cmpuint(apicid_core_width(1, 1), ==, 0);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 0), ==, 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 1), ==, 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 2), ==, 2);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 3), ==, 3);
+
+
+    /* Test field width calculation for multiple values
+     */
+    g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1);
+    g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2);
+    g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2);
+
+    g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5);
+
+
+    g_assert_cmpuint(apicid_core_width(30, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(31, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(32, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(33, 2), ==, 6);
+
+
+    /* build a weird topology and see if IDs are calculated correctly
+     */
+
+    /* This will use 2 bits for thread ID and 3 bits for core ID
+     */
+    g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2);
+    g_assert_cmpuint(apicid_core_width(6, 3), ==, 3);
+    g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 0), ==, 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1), ==, 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2), ==, 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 0), ==, (1 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 1), ==, (1 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 2), ==, (1 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 0), ==, (2 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 1), ==, (2 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 2), ==, (2 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 0 * 3 + 0), ==,
+                                         (1 << 5));
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 1 * 3 + 1), ==,
+                                         (1 << 5) | (1 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 3 * 6 * 3 + 5 * 3 + 2), ==,
+                                         (3 << 5) | (5 << 2) | 2);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/cpuid/topology/basic", test_topo_bits);
+
+    g_test_run();
+
+    return 0;
+}
-- 
1.7.11.7

[Qemu-devel] [PATCH for-1.4 12/12] pc: Generate APIC IDs according to CPU topology

[Eduardo Habkost]

This keeps compatibility on machine-types pc-1.2 and older, and prints a
warning in case the requested configuration won't get the correct
topology.

I couldn't think of a better way to warn about broken topology when in
compat mode other than using error_report(). The warning message will be
probably be buried in a log file somewhere, but it's better than
nothing.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 - Move code to cpu.c
 - keep using cpu_index on *-user
 - Use SMP.contiguous_apic_ids global property
 - Prints warning in case the compatibility mode will expose incorrect
   topology

Changes v2 -> v3:
 - Now all code is inside hw/pc.c
 - Use a real "PC" class and a "contiguous_apic_ids" property

Changes v3 -> v4:
 - Instead of using a global property, use a separate machine init
   function and a PCInitArgs field, to implement compatibility mode
 - Use error_report() instead of fprintf(stderr) for the warning
 - Use a field on PCInitArgs instead of a static variable to check
   if warning was already printed

Changes v4 -> v5:
 - Don't use PCInitArgs: simply add a enable_compat_apic_id_mode()
   function and a static compat_apic_id_mode variable, to enable the
   compatibility mode
 - Move APIC ID calculation code to cpu.c
---
 hw/pc_piix.c      | 12 ++++++++++--
 target-i386/cpu.c | 28 ++++++++++++++++++++++++----
 target-i386/cpu.h |  1 +
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index f9cfe78..b9a9b2e 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -233,11 +233,17 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
              initrd_filename, cpu_model, 1, 1);
 }
 
+static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
+{
+    enable_compat_apic_id_mode();
+    pc_init_pci(args);
+}
+
 /* PC machine init function for pc-0.14 to pc-1.2 */
 static void pc_init_pci_1_2(QEMUMachineInitArgs *args)
 {
     disable_kvm_pv_eoi();
-    pc_init_pci(args);
+    pc_init_pci_1_3(args);
 }
 
 /* PC init function for pc-0.10 to pc-0.13, and reused by xenfv */
@@ -250,6 +256,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
     const char *initrd_filename = args->initrd_filename;
     const char *boot_device = args->boot_device;
     disable_kvm_pv_eoi();
+    enable_compat_apic_id_mode();
     pc_init1(get_system_memory(),
              get_system_io(),
              ram_size, boot_device,
@@ -268,6 +275,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
     if (cpu_model == NULL)
         cpu_model = "486";
     disable_kvm_pv_eoi();
+    enable_compat_apic_id_mode();
     pc_init1(get_system_memory(),
              get_system_io(),
              ram_size, boot_device,
@@ -306,7 +314,7 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
 static QEMUMachine pc_machine_v1_3 = {
     .name = "pc-1.3",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_1_3,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_1_3,
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d90789d..4588f08 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -23,6 +23,8 @@
 
 #include "cpu.h"
 #include "sysemu/kvm.h"
+#include "sysemu/cpus.h"
+#include "topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"
@@ -2194,6 +2196,14 @@ void x86_cpu_realize(Object *obj, Error **errp)
     cpu_reset(CPU(cpu));
 }
 
+/* Enables contiguous-apic-ID mode, for compatibility */
+static bool compat_apic_id_mode;
+
+void enable_compat_apic_id_mode(void)
+{
+    compat_apic_id_mode = true;
+}
+
 /* Calculates initial APIC ID for a specific CPU index
  *
  * Currently we need to be able to calculate the APIC ID from the CPU index
@@ -2203,10 +2213,20 @@ void x86_cpu_realize(Object *obj, Error **errp)
  */
 uint32_t apic_id_for_cpu(unsigned int cpu_index)
 {
-    /* right now APIC ID == CPU index. this will eventually change to use
-     * the CPU topology configuration properly
-     */
-    return cpu_index;
+    uint32_t correct_id;
+    static bool warned;
+
+    correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index);
+    if (compat_apic_id_mode) {
+        if (cpu_index != correct_id && !warned) {
+            error_report("APIC IDs set in compatibility mode, "
+                         "CPU topology won't match the configuration");
+            warned = true;
+        }
+        return cpu_index;
+    } else {
+        return correct_id;
+    }
 }
 
 static void x86_cpu_initfn(Object *obj)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index d86c0af..cf42d76 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1256,5 +1256,6 @@ void disable_kvm_pv_eoi(void);
 const char *get_register_name_32(unsigned int reg);
 
 uint32_t apic_id_for_cpu(unsigned int cpu_index);
+void enable_compat_apic_id_mode(void);
 
 #endif /* CPU_I386_H */
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[li guang]

在 2013-01-17四的 18:59 -0200，Eduardo Habkost写道：
> I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
> I sent last week, though.
> 
> Igor argued that APIC ID should be set by the board and not by the CPU itself,

per Intel's SPEC, seems APIC ID really based on design of board.
(refer to Intel® 64 and IA-32 Architectures
    Software Developer’s Manual
                Volume 3 (3A, 3B & 3C):
             System Programming Guide
     chapter 10.4.6)
but, actually, it maybe meaningless for emulation.
after go though your patches,
I can't capture the purpose you do a topology map between 
APIC ID and cpu_index, (sorry for that)
can you help to clear that?

> but I am not doing that because:
>  - I want to keep the bug fix simple and isolated as we are past soft freeze
>  - I believe the creator of the CPU object shouldn't be forced to provide the
>    APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
>    device_add interface in the future
>  - The APIC ID _is_ set by the CPU itself (because each CPU package may have
>    multiple core/threads, and each core/thread has a different APIC ID). What
>    needs to be provided by the board to the CPU package in the future is the
>    package ID and the bit width of the core/thread IDs.
> 
> Git tree for reference:
>   git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
>   https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5
> 
> Eduardo Habkost (12):
>   kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
>     KVM
>   target-i386: Don't set any KVM flag by default if KVM is disabled
>   pc: Reverse pc_init_pci() compatibility logic
>   kvm: Create kvm_arch_vcpu_id() function
>   target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
>   fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
>   target-i386/cpu: Introduce apic_id_for_cpu() function
>   cpus.h: Make constant smp_cores/smp_threads available on *-user
>   pc: Set fw_cfg data based on APIC ID calculation
>   tests: Support target-specific unit tests
>   target-i386: Topology & APIC ID utility functions
>   pc: Generate APIC IDs according to CPU topology
> 
>  hw/fw_cfg.c            |   1 -
>  hw/pc.c                |  44 +++++++++++++---
>  hw/pc_piix.c           |  26 +++++++---
>  hw/ppc_newworld.c      |   1 +
>  hw/ppc_oldworld.c      |   1 +
>  hw/sun4m.c             |   3 ++
>  hw/sun4u.c             |   1 +
>  include/sysemu/cpus.h  |   7 +++
>  include/sysemu/kvm.h   |   4 ++
>  kvm-all.c              |   2 +-
>  target-i386/cpu.c      |  52 +++++++++++++++----
>  target-i386/cpu.h      |   5 +-
>  target-i386/kvm.c      |   6 +++
>  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-ppc/kvm.c       |   5 ++
>  target-s390x/kvm.c     |   5 ++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |  21 +++++++-
>  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
>  19 files changed, 391 insertions(+), 28 deletions(-)
>  create mode 100644 target-i386/topology.h
>  create mode 100644 tests/test-x86-cpuid.c
> 

-- 
regards!
li guang

Re: [Qemu-devel] [PATCH for-1.4 02/12] target-i386: Don't set any KVM flag by default if KVM is disabled

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> This is a cleanup that tries to solve two small issues:
> 
>  - We don't need a separate kvm_pv_eoi_features variable just to keep a
>    constant calculated at compile-time, and this style would require
>    adding a separate variable (that's declared twice because of the
>    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
>    by machine-type compat code.
>  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
>    even when KVM is disabled at runtime. This small incosistency in
>    the cpuid_kvm_features field isn't a problem today because
>    cpuid_kvm_features is ignored by the TCG code, but it may cause
>    unexpected problems later when refactoring the CPUID handling code.
> 
> This patch eliminates the kvm_pv_eoi_features variable and simply uses
> kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
> enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
> behavior of enable_kvm_pv_eoi() clearer and easier to understand.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Fine with me if KVM folks agree.

Andreas

> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Changes v2:
>  - Coding style fix
> 
> Changes v3:
>  - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
> 
> Changes v4:
>  - Check kvm_enabled() when actually using kvm_default_features
>  - Eliminate Yet Another #ifdef by using the fake KVM_FEATURE_*
>    #defines on kvm_default_features initialization
> 
> Changes v5:
>  - Rebase and move the kvm_enabled() check to cpu_x86_register()
> ---
>  target-i386/cpu.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 333745b..754eb6f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -206,22 +206,16 @@ typedef struct model_features_t {
>  int check_cpuid = 0;
>  int enforce_cpuid = 0;
>  
> -#if defined(CONFIG_KVM)
>  static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
>          (1 << KVM_FEATURE_NOP_IO_DELAY) |
>          (1 << KVM_FEATURE_CLOCKSOURCE2) |
>          (1 << KVM_FEATURE_ASYNC_PF) |
>          (1 << KVM_FEATURE_STEAL_TIME) |
>          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
> -#else
> -static uint32_t kvm_default_features = 0;
> -static const uint32_t kvm_pv_eoi_features = 0;
> -#endif
>  
>  void enable_kvm_pv_eoi(void)
>  {
> -    kvm_default_features |= kvm_pv_eoi_features;
> +    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,
> @@ -1602,7 +1596,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          goto out;
>      }
>  
> -    def->kvm_features |= kvm_default_features;
> +    if (kvm_enabled()) {
> +        def->kvm_features |= kvm_default_features;
> +    }
>      def->ext_features |= CPUID_EXT_HYPERVISOR;
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 02/12] target-i386: Don't set any KVM flag by default if KVM is disabled

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> This is a cleanup that tries to solve two small issues:
> 
>  - We don't need a separate kvm_pv_eoi_features variable just to keep a
>    constant calculated at compile-time, and this style would require
>    adding a separate variable (that's declared twice because of the
>    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
>    by machine-type compat code.
>  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
>    even when KVM is disabled at runtime. This small incosistency in
>    the cpuid_kvm_features field isn't a problem today because
>    cpuid_kvm_features is ignored by the TCG code, but it may cause
>    unexpected problems later when refactoring the CPUID handling code.
> 
> This patch eliminates the kvm_pv_eoi_features variable and simply uses
> kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
> enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
> behavior of enable_kvm_pv_eoi() clearer and easier to understand.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Fine with me if KVM folks agree.

Andreas

> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Changes v2:
>  - Coding style fix
> 
> Changes v3:
>  - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
> 
> Changes v4:
>  - Check kvm_enabled() when actually using kvm_default_features
>  - Eliminate Yet Another #ifdef by using the fake KVM_FEATURE_*
>    #defines on kvm_default_features initialization
> 
> Changes v5:
>  - Rebase and move the kvm_enabled() check to cpu_x86_register()
> ---
>  target-i386/cpu.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 333745b..754eb6f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -206,22 +206,16 @@ typedef struct model_features_t {
>  int check_cpuid = 0;
>  int enforce_cpuid = 0;
>  
> -#if defined(CONFIG_KVM)
>  static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
>          (1 << KVM_FEATURE_NOP_IO_DELAY) |
>          (1 << KVM_FEATURE_CLOCKSOURCE2) |
>          (1 << KVM_FEATURE_ASYNC_PF) |
>          (1 << KVM_FEATURE_STEAL_TIME) |
>          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
> -#else
> -static uint32_t kvm_default_features = 0;
> -static const uint32_t kvm_pv_eoi_features = 0;
> -#endif
>  
>  void enable_kvm_pv_eoi(void)
>  {
> -    kvm_default_features |= kvm_pv_eoi_features;
> +    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
>  }
>  
>  void host_cpuid(uint32_t function, uint32_t count,
> @@ -1602,7 +1596,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          goto out;
>      }
>  
> -    def->kvm_features |= kvm_default_features;
> +    if (kvm_enabled()) {
> +        def->kvm_features |= kvm_default_features;
> +    }
>      def->ext_features |= CPUID_EXT_HYPERVISOR;
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH for-1.4 02/12] target-i386: Don't set any KVM flag by default if KVM is disabled

[Gleb Natapov]

On Fri, Jan 18, 2013 at 11:58:34AM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > This is a cleanup that tries to solve two small issues:
> > 
> >  - We don't need a separate kvm_pv_eoi_features variable just to keep a
> >    constant calculated at compile-time, and this style would require
> >    adding a separate variable (that's declared twice because of the
> >    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
> >    by machine-type compat code.
> >  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
> >    even when KVM is disabled at runtime. This small incosistency in
> >    the cpuid_kvm_features field isn't a problem today because
> >    cpuid_kvm_features is ignored by the TCG code, but it may cause
> >    unexpected problems later when refactoring the CPUID handling code.
> > 
> > This patch eliminates the kvm_pv_eoi_features variable and simply uses
> > kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
> > enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
> > behavior of enable_kvm_pv_eoi() clearer and easier to understand.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Fine with me if KVM folks agree.
> 
Acked-by: Gleb Natapov <gleb@redhat.com>

> Andreas
> 
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Changes v2:
> >  - Coding style fix
> > 
> > Changes v3:
> >  - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
> > 
> > Changes v4:
> >  - Check kvm_enabled() when actually using kvm_default_features
> >  - Eliminate Yet Another #ifdef by using the fake KVM_FEATURE_*
> >    #defines on kvm_default_features initialization
> > 
> > Changes v5:
> >  - Rebase and move the kvm_enabled() check to cpu_x86_register()
> > ---
> >  target-i386/cpu.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 333745b..754eb6f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -206,22 +206,16 @@ typedef struct model_features_t {
> >  int check_cpuid = 0;
> >  int enforce_cpuid = 0;
> >  
> > -#if defined(CONFIG_KVM)
> >  static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
> >          (1 << KVM_FEATURE_NOP_IO_DELAY) |
> >          (1 << KVM_FEATURE_CLOCKSOURCE2) |
> >          (1 << KVM_FEATURE_ASYNC_PF) |
> >          (1 << KVM_FEATURE_STEAL_TIME) |
> >          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> > -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
> > -#else
> > -static uint32_t kvm_default_features = 0;
> > -static const uint32_t kvm_pv_eoi_features = 0;
> > -#endif
> >  
> >  void enable_kvm_pv_eoi(void)
> >  {
> > -    kvm_default_features |= kvm_pv_eoi_features;
> > +    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> >  }
> >  
> >  void host_cpuid(uint32_t function, uint32_t count,
> > @@ -1602,7 +1596,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >          goto out;
> >      }
> >  
> > -    def->kvm_features |= kvm_default_features;
> > +    if (kvm_enabled()) {
> > +        def->kvm_features |= kvm_default_features;
> > +    }
> >      def->ext_features |= CPUID_EXT_HYPERVISOR;
> >  
> >      if (cpu_x86_parse_featurestr(def, features) < 0) {
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

--
			Gleb.

Re: [Qemu-devel] [PATCH for-1.4 02/12] target-i386: Don't set any KVM flag by default if KVM is disabled

[Gleb Natapov]

On Fri, Jan 18, 2013 at 11:58:34AM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > This is a cleanup that tries to solve two small issues:
> > 
> >  - We don't need a separate kvm_pv_eoi_features variable just to keep a
> >    constant calculated at compile-time, and this style would require
> >    adding a separate variable (that's declared twice because of the
> >    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
> >    by machine-type compat code.
> >  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
> >    even when KVM is disabled at runtime. This small incosistency in
> >    the cpuid_kvm_features field isn't a problem today because
> >    cpuid_kvm_features is ignored by the TCG code, but it may cause
> >    unexpected problems later when refactoring the CPUID handling code.
> > 
> > This patch eliminates the kvm_pv_eoi_features variable and simply uses
> > kvm_enabled() inside the enable_kvm_pv_eoi() compat function, so it
> > enables kvm_pv_eoi only if KVM is enabled. I believe this makes the
> > behavior of enable_kvm_pv_eoi() clearer and easier to understand.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Fine with me if KVM folks agree.
> 
Acked-by: Gleb Natapov <gleb@redhat.com>

> Andreas
> 
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Changes v2:
> >  - Coding style fix
> > 
> > Changes v3:
> >  - Eliminate #ifdef by using the fake KVM_FEATURE_PV_EOI #define
> > 
> > Changes v4:
> >  - Check kvm_enabled() when actually using kvm_default_features
> >  - Eliminate Yet Another #ifdef by using the fake KVM_FEATURE_*
> >    #defines on kvm_default_features initialization
> > 
> > Changes v5:
> >  - Rebase and move the kvm_enabled() check to cpu_x86_register()
> > ---
> >  target-i386/cpu.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 333745b..754eb6f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -206,22 +206,16 @@ typedef struct model_features_t {
> >  int check_cpuid = 0;
> >  int enforce_cpuid = 0;
> >  
> > -#if defined(CONFIG_KVM)
> >  static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
> >          (1 << KVM_FEATURE_NOP_IO_DELAY) |
> >          (1 << KVM_FEATURE_CLOCKSOURCE2) |
> >          (1 << KVM_FEATURE_ASYNC_PF) |
> >          (1 << KVM_FEATURE_STEAL_TIME) |
> >          (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT);
> > -static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI);
> > -#else
> > -static uint32_t kvm_default_features = 0;
> > -static const uint32_t kvm_pv_eoi_features = 0;
> > -#endif
> >  
> >  void enable_kvm_pv_eoi(void)
> >  {
> > -    kvm_default_features |= kvm_pv_eoi_features;
> > +    kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> >  }
> >  
> >  void host_cpuid(uint32_t function, uint32_t count,
> > @@ -1602,7 +1596,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >          goto out;
> >      }
> >  
> > -    def->kvm_features |= kvm_default_features;
> > +    if (kvm_enabled()) {
> > +        def->kvm_features |= kvm_default_features;
> > +    }
> >      def->ext_features |= CPUID_EXT_HYPERVISOR;
> >  
> >      if (cpu_x86_parse_featurestr(def, features) < 0) {
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

--
			Gleb.

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> This will allow each architecture to define how the VCPU ID is set on
> the KVM_CREATE_VCPU ioctl call.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Changes v2:
>  - Get CPUState as argument instead of CPUArchState
> ---
>  include/sysemu/kvm.h | 3 +++
>  kvm-all.c            | 2 +-
>  target-i386/kvm.c    | 5 +++++
>  target-ppc/kvm.c     | 5 +++++
>  target-s390x/kvm.c   | 5 +++++
>  5 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 22acf91..384ee66 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -196,6 +196,9 @@ int kvm_arch_init(KVMState *s);
>  
>  int kvm_arch_init_vcpu(CPUState *cpu);
>  
> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> +
>  void kvm_arch_reset_vcpu(CPUState *cpu);
>  
>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> diff --git a/kvm-all.c b/kvm-all.c
> index 6278d61..995220d 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
>  
>      DPRINTF("kvm_init_vcpu\n");
>  
> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
>      if (ret < 0) {
>          DPRINTF("kvm_create_vcpu failed\n");
>          goto err;

This is changing the vararg from int to unsigned long. I have no
insights yet on how this is handled and whether that is okay; I would at
least expect this change to be mentioned in the commit message.

> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3acff40..5f3f789 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>      }
>  }
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      struct {

Minor nit: If you change this to CPUState *cs you spare the renaming in
05/12. Alternatively use x86_cpu there (not much code affected so you
can just ignore this, no need to respin just for that).

Otherwise looks okay to me.

Andreas

> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 19e9f25..1e544ae 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -384,6 +384,11 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>  
>  #endif /* !defined (TARGET_PPC64) */
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 6ec5e6d..bd9864c 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
>      return 0;
>  }
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cpu)
>  {
>      int ret = 0;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> This will allow each architecture to define how the VCPU ID is set on
> the KVM_CREATE_VCPU ioctl call.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Changes v2:
>  - Get CPUState as argument instead of CPUArchState
> ---
>  include/sysemu/kvm.h | 3 +++
>  kvm-all.c            | 2 +-
>  target-i386/kvm.c    | 5 +++++
>  target-ppc/kvm.c     | 5 +++++
>  target-s390x/kvm.c   | 5 +++++
>  5 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 22acf91..384ee66 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -196,6 +196,9 @@ int kvm_arch_init(KVMState *s);
>  
>  int kvm_arch_init_vcpu(CPUState *cpu);
>  
> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> +
>  void kvm_arch_reset_vcpu(CPUState *cpu);
>  
>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> diff --git a/kvm-all.c b/kvm-all.c
> index 6278d61..995220d 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
>  
>      DPRINTF("kvm_init_vcpu\n");
>  
> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
>      if (ret < 0) {
>          DPRINTF("kvm_create_vcpu failed\n");
>          goto err;

This is changing the vararg from int to unsigned long. I have no
insights yet on how this is handled and whether that is okay; I would at
least expect this change to be mentioned in the commit message.

> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 3acff40..5f3f789 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>      }
>  }
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      struct {

Minor nit: If you change this to CPUState *cs you spare the renaming in
05/12. Alternatively use x86_cpu there (not much code affected so you
can just ignore this, no need to respin just for that).

Otherwise looks okay to me.

Andreas

> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 19e9f25..1e544ae 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -384,6 +384,11 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>  
>  #endif /* !defined (TARGET_PPC64) */
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 6ec5e6d..bd9864c 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
>      return 0;
>  }
>  
> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> +{
> +    return cpu->cpu_index;
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cpu)
>  {
>      int ret = 0;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: kvm@vger.kernel.org
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Gleb Natapov <gleb@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>

It appears as if Cc: below --- gets ignored? Please check and ping those
people if necessary.

Personally, I adopted the convention of having to-be-persisted CCs above
Sob and stripping trailing CCs when applying.

Andreas

> ---
>  include/sysemu/kvm.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 6bdd513..22acf91 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -36,6 +36,7 @@
>  #define KVM_FEATURE_ASYNC_PF     0
>  #define KVM_FEATURE_STEAL_TIME   0
>  #define KVM_FEATURE_PV_EOI       0
> +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
>  #endif
>  
>  extern int kvm_allowed;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> It appears as if Cc: below --- gets ignored? Please check and ping those
> people if necessary.

I believe it depends on git-send-email suppress-cc option. At least I
remember this format have worked for me before, I need to check what
changed on my config.

> 
> Personally, I adopted the convention of having to-be-persisted CCs above
> Sob and stripping trailing CCs when applying.

I considered the CC lines not part of the commit message (just like the
changelog isn't), so I decided to put them after "---".

> 
> Andreas
> 
> > ---
> >  include/sysemu/kvm.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 6bdd513..22acf91 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -36,6 +36,7 @@
> >  #define KVM_FEATURE_ASYNC_PF     0
> >  #define KVM_FEATURE_STEAL_TIME   0
> >  #define KVM_FEATURE_PV_EOI       0
> > +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> >  #endif
> >  
> >  extern int kvm_allowed;
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> It appears as if Cc: below --- gets ignored? Please check and ping those
> people if necessary.

I believe it depends on git-send-email suppress-cc option. At least I
remember this format have worked for me before, I need to check what
changed on my config.

> 
> Personally, I adopted the convention of having to-be-persisted CCs above
> Sob and stripping trailing CCs when applying.

I considered the CC lines not part of the commit message (just like the
changelog isn't), so I decided to put them after "---".

> 
> Andreas
> 
> > ---
> >  include/sysemu/kvm.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 6bdd513..22acf91 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -36,6 +36,7 @@
> >  #define KVM_FEATURE_ASYNC_PF     0
> >  #define KVM_FEATURE_STEAL_TIME   0
> >  #define KVM_FEATURE_PV_EOI       0
> > +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> >  #endif
> >  
> >  extern int kvm_allowed;
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Gleb Natapov]

On Fri, Jan 18, 2013 at 09:36:33AM -0200, Eduardo Habkost wrote:
> On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> > Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Cc: kvm@vger.kernel.org
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Gleb Natapov <gleb@redhat.com>
> > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > It appears as if Cc: below --- gets ignored? Please check and ping those
> > people if necessary.
> 
> I believe it depends on git-send-email suppress-cc option. At least I
> remember this format have worked for me before, I need to check what
> changed on my config.
> 
> > 
> > Personally, I adopted the convention of having to-be-persisted CCs above
> > Sob and stripping trailing CCs when applying.
> 
> I considered the CC lines not part of the commit message (just like the
> changelog isn't), so I decided to put them after "---".
> 
This may be the reason git-send-email ignored it.

> > 
> > Andreas
> > 
> > > ---
> > >  include/sysemu/kvm.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > > index 6bdd513..22acf91 100644
> > > --- a/include/sysemu/kvm.h
> > > +++ b/include/sysemu/kvm.h
> > > @@ -36,6 +36,7 @@
> > >  #define KVM_FEATURE_ASYNC_PF     0
> > >  #define KVM_FEATURE_STEAL_TIME   0
> > >  #define KVM_FEATURE_PV_EOI       0
> > > +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> > >  #endif
> > >  
> > >  extern int kvm_allowed;
> > 
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 
> -- 
> Eduardo

--
			Gleb.

Re: [Qemu-devel] [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Gleb Natapov]

On Fri, Jan 18, 2013 at 09:36:33AM -0200, Eduardo Habkost wrote:
> On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> > Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > Cc: kvm@vger.kernel.org
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Gleb Natapov <gleb@redhat.com>
> > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > It appears as if Cc: below --- gets ignored? Please check and ping those
> > people if necessary.
> 
> I believe it depends on git-send-email suppress-cc option. At least I
> remember this format have worked for me before, I need to check what
> changed on my config.
> 
> > 
> > Personally, I adopted the convention of having to-be-persisted CCs above
> > Sob and stripping trailing CCs when applying.
> 
> I considered the CC lines not part of the commit message (just like the
> changelog isn't), so I decided to put them after "---".
> 
This may be the reason git-send-email ignored it.

> > 
> > Andreas
> > 
> > > ---
> > >  include/sysemu/kvm.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > > index 6bdd513..22acf91 100644
> > > --- a/include/sysemu/kvm.h
> > > +++ b/include/sysemu/kvm.h
> > > @@ -36,6 +36,7 @@
> > >  #define KVM_FEATURE_ASYNC_PF     0
> > >  #define KVM_FEATURE_STEAL_TIME   0
> > >  #define KVM_FEATURE_PV_EOI       0
> > > +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> > >  #endif
> > >  
> > >  extern int kvm_allowed;
> > 
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 
> -- 
> Eduardo

--
			Gleb.

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 01:48:38PM +0200, Gleb Natapov wrote:
> On Fri, Jan 18, 2013 at 09:36:33AM -0200, Eduardo Habkost wrote:
> > On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> > > Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > > Cc: kvm@vger.kernel.org
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Gleb Natapov <gleb@redhat.com>
> > > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > 
> > > It appears as if Cc: below --- gets ignored? Please check and ping those
> > > people if necessary.
> > 
> > I believe it depends on git-send-email suppress-cc option. At least I
> > remember this format have worked for me before, I need to check what
> > changed on my config.
> > 
> > > 
> > > Personally, I adopted the convention of having to-be-persisted CCs above
> > > Sob and stripping trailing CCs when applying.
> > 
> > I considered the CC lines not part of the commit message (just like the
> > changelog isn't), so I decided to put them after "---".
> > 
> This may be the reason git-send-email ignored it.

I just tested it here, and it ignored it because I have suppress-cc=all enabled
by default and (by mistake) I didn't use my script that explicitly disabled it.
If I use --suppress-cc=author, git-send-email uses the CC lines before _and_
after "---".

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 01:48:38PM +0200, Gleb Natapov wrote:
> On Fri, Jan 18, 2013 at 09:36:33AM -0200, Eduardo Habkost wrote:
> > On Fri, Jan 18, 2013 at 12:17:56PM +0100, Andreas Färber wrote:
> > > Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > > Cc: kvm@vger.kernel.org
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Gleb Natapov <gleb@redhat.com>
> > > > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > > 
> > > It appears as if Cc: below --- gets ignored? Please check and ping those
> > > people if necessary.
> > 
> > I believe it depends on git-send-email suppress-cc option. At least I
> > remember this format have worked for me before, I need to check what
> > changed on my config.
> > 
> > > 
> > > Personally, I adopted the convention of having to-be-persisted CCs above
> > > Sob and stripping trailing CCs when applying.
> > 
> > I considered the CC lines not part of the commit message (just like the
> > changelog isn't), so I decided to put them after "---".
> > 
> This may be the reason git-send-email ignored it.

I just tested it here, and it ignored it because I have suppress-cc=all enabled
by default and (by mistake) I didn't use my script that explicitly disabled it.
If I use --suppress-cc=author, git-send-email uses the CC lines before _and_
after "---".

-- 
Eduardo

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
[...]
> > +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> > +
> >  void kvm_arch_reset_vcpu(CPUState *cpu);
> >  
> >  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 6278d61..995220d 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
> >  
> >      DPRINTF("kvm_init_vcpu\n");
> >  
> > -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> > +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
> >      if (ret < 0) {
> >          DPRINTF("kvm_create_vcpu failed\n");
> >          goto err;
> 
> This is changing the vararg from int to unsigned long. I have no
> insights yet on how this is handled and whether that is okay; I would at
> least expect this change to be mentioned in the commit message.

It was an unexpected change (I didn't notice that cpu_index was int),
but strictly speaking the previous code was incorrect (as ioctl() gets
an unsigned long argument, not int). I doubt there are cases where it
would really break, but it is a good thing to fix it.

I agree this should be mentioned in the commit message, though. Will you
add it before committing, or should I resubmit?

> 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 3acff40..5f3f789 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
> >      }
> >  }
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cs)
> >  {
> >      struct {
> 
> Minor nit: If you change this to CPUState *cs you spare the renaming in
> 05/12. Alternatively use x86_cpu there (not much code affected so you
> can just ignore this, no need to respin just for that).
> 
> Otherwise looks okay to me.

I actually wanted to rename the variable only when necessary, otherwise
this patch would be confusing if all architectures used 'cpu' and i386
used 'cs'.

(And I like using "cpu" for the more specific CPU type in the function
[e.g.  CPUState or X86CPUState depending on the case] and abbreviations
[like 'cs'] for the more generic types. I believe I have seen this style
used in other parts of the code.)

> 
> Andreas
> 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 19e9f25..1e544ae 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -384,6 +384,11 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
> >  
> >  #endif /* !defined (TARGET_PPC64) */
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cs)
> >  {
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 6ec5e6d..bd9864c 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
> >      return 0;
> >  }
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cpu)
> >  {
> >      int ret = 0;
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
[...]
> > +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> > +
> >  void kvm_arch_reset_vcpu(CPUState *cpu);
> >  
> >  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 6278d61..995220d 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
> >  
> >      DPRINTF("kvm_init_vcpu\n");
> >  
> > -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> > +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
> >      if (ret < 0) {
> >          DPRINTF("kvm_create_vcpu failed\n");
> >          goto err;
> 
> This is changing the vararg from int to unsigned long. I have no
> insights yet on how this is handled and whether that is okay; I would at
> least expect this change to be mentioned in the commit message.

It was an unexpected change (I didn't notice that cpu_index was int),
but strictly speaking the previous code was incorrect (as ioctl() gets
an unsigned long argument, not int). I doubt there are cases where it
would really break, but it is a good thing to fix it.

I agree this should be mentioned in the commit message, though. Will you
add it before committing, or should I resubmit?

> 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 3acff40..5f3f789 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
> >      }
> >  }
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cs)
> >  {
> >      struct {
> 
> Minor nit: If you change this to CPUState *cs you spare the renaming in
> 05/12. Alternatively use x86_cpu there (not much code affected so you
> can just ignore this, no need to respin just for that).
> 
> Otherwise looks okay to me.

I actually wanted to rename the variable only when necessary, otherwise
this patch would be confusing if all architectures used 'cpu' and i386
used 'cs'.

(And I like using "cpu" for the more specific CPU type in the function
[e.g.  CPUState or X86CPUState depending on the case] and abbreviations
[like 'cs'] for the more generic types. I believe I have seen this style
used in other parts of the code.)

> 
> Andreas
> 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 19e9f25..1e544ae 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -384,6 +384,11 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
> >  
> >  #endif /* !defined (TARGET_PPC64) */
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cs)
> >  {
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 6ec5e6d..bd9864c 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
> >      return 0;
> >  }
> >  
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > +    return cpu->cpu_index;
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cpu)
> >  {
> >      int ret = 0;
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

Am 18.01.2013 13:53, schrieb Eduardo Habkost:
> On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
> [...]
>>> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
>>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
>>> +
>>>  void kvm_arch_reset_vcpu(CPUState *cpu);
>>>  
>>>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 6278d61..995220d 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
>>>  
>>>      DPRINTF("kvm_init_vcpu\n");
>>>  
>>> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
>>> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
>>>      if (ret < 0) {
>>>          DPRINTF("kvm_create_vcpu failed\n");
>>>          goto err;
>>
>> This is changing the vararg from int to unsigned long. I have no
>> insights yet on how this is handled and whether that is okay; I would at
>> least expect this change to be mentioned in the commit message.
> 
> It was an unexpected change (I didn't notice that cpu_index was int),
> but strictly speaking the previous code was incorrect (as ioctl() gets
> an unsigned long argument, not int). I doubt there are cases where it
> would really break, but it is a good thing to fix it.
> 
> I agree this should be mentioned in the commit message, though. Will you
> add it before committing, or should I resubmit?

Could you suggest a text for me to add please?

>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>> index 3acff40..5f3f789 100644
>>> --- a/target-i386/kvm.c
>>> +++ b/target-i386/kvm.c
>>> @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>>>      }
>>>  }
>>>  
>>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
>>> +{
>>> +    return cpu->cpu_index;
>>> +}
>>> +
>>>  int kvm_arch_init_vcpu(CPUState *cs)
>>>  {
>>>      struct {
>>
>> Minor nit: If you change this to CPUState *cs you spare the renaming in
>> 05/12. Alternatively use x86_cpu there (not much code affected so you
>> can just ignore this, no need to respin just for that).
>>
>> Otherwise looks okay to me.
> 
> I actually wanted to rename the variable only when necessary, otherwise
> this patch would be confusing if all architectures used 'cpu' and i386
> used 'cs'.

It's inconsistent anyway, 'cs' is relatively new and I see no reason to
use it in the prototype.
But OK, once 03/12 gets an ack I'll start applying.

> 
> (And I like using "cpu" for the more specific CPU type in the function
> [e.g.  CPUState or X86CPUState depending on the case] and abbreviations
> [like 'cs'] for the more generic types. I believe I have seen this style
> used in other parts of the code.)

Yes, I chose to use "cpu" for the more frequently used type. I don't
really like "cs" but it seemed better than "base_cpu" or so. When
changing something over from "env" something with three letters looks
nicer though, easier to review. Can't have everything. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

Am 18.01.2013 13:53, schrieb Eduardo Habkost:
> On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
> [...]
>>> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
>>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
>>> +
>>>  void kvm_arch_reset_vcpu(CPUState *cpu);
>>>  
>>>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 6278d61..995220d 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
>>>  
>>>      DPRINTF("kvm_init_vcpu\n");
>>>  
>>> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
>>> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
>>>      if (ret < 0) {
>>>          DPRINTF("kvm_create_vcpu failed\n");
>>>          goto err;
>>
>> This is changing the vararg from int to unsigned long. I have no
>> insights yet on how this is handled and whether that is okay; I would at
>> least expect this change to be mentioned in the commit message.
> 
> It was an unexpected change (I didn't notice that cpu_index was int),
> but strictly speaking the previous code was incorrect (as ioctl() gets
> an unsigned long argument, not int). I doubt there are cases where it
> would really break, but it is a good thing to fix it.
> 
> I agree this should be mentioned in the commit message, though. Will you
> add it before committing, or should I resubmit?

Could you suggest a text for me to add please?

>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>> index 3acff40..5f3f789 100644
>>> --- a/target-i386/kvm.c
>>> +++ b/target-i386/kvm.c
>>> @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
>>>      }
>>>  }
>>>  
>>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
>>> +{
>>> +    return cpu->cpu_index;
>>> +}
>>> +
>>>  int kvm_arch_init_vcpu(CPUState *cs)
>>>  {
>>>      struct {
>>
>> Minor nit: If you change this to CPUState *cs you spare the renaming in
>> 05/12. Alternatively use x86_cpu there (not much code affected so you
>> can just ignore this, no need to respin just for that).
>>
>> Otherwise looks okay to me.
> 
> I actually wanted to rename the variable only when necessary, otherwise
> this patch would be confusing if all architectures used 'cpu' and i386
> used 'cs'.

It's inconsistent anyway, 'cs' is relatively new and I see no reason to
use it in the prototype.
But OK, once 03/12 gets an ack I'll start applying.

> 
> (And I like using "cpu" for the more specific CPU type in the function
> [e.g.  CPUState or X86CPUState depending on the case] and abbreviations
> [like 'cs'] for the more generic types. I believe I have seen this style
> used in other parts of the code.)

Yes, I chose to use "cpu" for the more frequently used type. I don't
really like "cs" but it seemed better than "base_cpu" or so. When
changing something over from "env" something with three letters looks
nicer though, easier to review. Can't have everything. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 02:03:09PM +0100, Andreas Färber wrote:
> Am 18.01.2013 13:53, schrieb Eduardo Habkost:
> > On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
> > [...]
> >>> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> >>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> >>> +
> >>>  void kvm_arch_reset_vcpu(CPUState *cpu);
> >>>  
> >>>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> >>> diff --git a/kvm-all.c b/kvm-all.c
> >>> index 6278d61..995220d 100644
> >>> --- a/kvm-all.c
> >>> +++ b/kvm-all.c
> >>> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
> >>>  
> >>>      DPRINTF("kvm_init_vcpu\n");
> >>>  
> >>> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> >>> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
> >>>      if (ret < 0) {
> >>>          DPRINTF("kvm_create_vcpu failed\n");
> >>>          goto err;
> >>
> >> This is changing the vararg from int to unsigned long. I have no
> >> insights yet on how this is handled and whether that is okay; I would at
> >> least expect this change to be mentioned in the commit message.
> > 
> > It was an unexpected change (I didn't notice that cpu_index was int),
> > but strictly speaking the previous code was incorrect (as ioctl() gets
> > an unsigned long argument, not int). I doubt there are cases where it
> > would really break, but it is a good thing to fix it.
> > 
> > I agree this should be mentioned in the commit message, though. Will you
> > add it before committing, or should I resubmit?
> 
> Could you suggest a text for me to add please?

"The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
works on most or all architectures supporting KVM, but it is safer to
use an appropriate 'unsigned long' parameter."

To find out if 'int' breaks on any architecture, I would need to check
the ABI specification for each architecture. I didn't do that, but I am
sure we should pass an unsigned long instead, if that's the type
expected by the kernel.

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 02:03:09PM +0100, Andreas Färber wrote:
> Am 18.01.2013 13:53, schrieb Eduardo Habkost:
> > On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
> > [...]
> >>> +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> >>> +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> >>> +
> >>>  void kvm_arch_reset_vcpu(CPUState *cpu);
> >>>  
> >>>  int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> >>> diff --git a/kvm-all.c b/kvm-all.c
> >>> index 6278d61..995220d 100644
> >>> --- a/kvm-all.c
> >>> +++ b/kvm-all.c
> >>> @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
> >>>  
> >>>      DPRINTF("kvm_init_vcpu\n");
> >>>  
> >>> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> >>> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
> >>>      if (ret < 0) {
> >>>          DPRINTF("kvm_create_vcpu failed\n");
> >>>          goto err;
> >>
> >> This is changing the vararg from int to unsigned long. I have no
> >> insights yet on how this is handled and whether that is okay; I would at
> >> least expect this change to be mentioned in the commit message.
> > 
> > It was an unexpected change (I didn't notice that cpu_index was int),
> > but strictly speaking the previous code was incorrect (as ioctl() gets
> > an unsigned long argument, not int). I doubt there are cases where it
> > would really break, but it is a good thing to fix it.
> > 
> > I agree this should be mentioned in the commit message, though. Will you
> > add it before committing, or should I resubmit?
> 
> Could you suggest a text for me to add please?

"The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
works on most or all architectures supporting KVM, but it is safer to
use an appropriate 'unsigned long' parameter."

To find out if 'int' breaks on any architecture, I would need to check
the ABI specification for each architecture. I didn't do that, but I am
sure we should pass an unsigned long instead, if that's the type
expected by the kernel.

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 02:54:41PM +0800, li guang wrote:
> 在 2013-01-17四的 18:59 -0200，Eduardo Habkost写道：
> > I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
> > I sent last week, though.
> > 
> > Igor argued that APIC ID should be set by the board and not by the CPU itself,
> 
> per Intel's SPEC, seems APIC ID really based on design of board.
> (refer to Intel® 64 and IA-32 Architectures
>     Software Developer’s Manual
>                 Volume 3 (3A, 3B & 3C):
>              System Programming Guide
>      chapter 10.4.6)
> but, actually, it maybe meaningless for emulation.
> after go though your patches,
> I can't capture the purpose you do a topology map between 
> APIC ID and cpu_index, (sorry for that)
> can you help to clear that?

See the documents mentioned on PATCH 11/12:

+/* This file implements the APIC-ID-based CPU topology enumeration logic,
+ * documented at the following document:
+ *   Intel® 64 Architecture Processor Topology Enumeration
+ *   http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
+ *
+ * This code should be compatible with AMD's "Extended Method" described at:
+ *   AMD CPUID Specification (Publication #25481)
+ *   Section 3: Multiple Core Calcuation
+ * as long as:
+ *  nr_threads is set to 1;
+ *  OFFSET_IDX is assumed to be 0;
+ *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
+ */

If we don't generate the APIC IDs properly, identification of CPU
sockets/cores/threads is broken.

e.g. today -smp 12,cores=3,threads=2 currently ends up exposing 4 cores on the
first socket, and 2 cores in the second one, because the APIC IDs are generated
sequentially instead of being based on package/core/thread IDs.

> 
> > but I am not doing that because:
> >  - I want to keep the bug fix simple and isolated as we are past soft freeze
> >  - I believe the creator of the CPU object shouldn't be forced to provide the
> >    APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
> >    device_add interface in the future
> >  - The APIC ID _is_ set by the CPU itself (because each CPU package may have
> >    multiple core/threads, and each core/thread has a different APIC ID). What
> >    needs to be provided by the board to the CPU package in the future is the
> >    package ID and the bit width of the core/thread IDs.
> > 
> > Git tree for reference:
> >   git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
> >   https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5
> > 
> > Eduardo Habkost (12):
> >   kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
> >     KVM
> >   target-i386: Don't set any KVM flag by default if KVM is disabled
> >   pc: Reverse pc_init_pci() compatibility logic
> >   kvm: Create kvm_arch_vcpu_id() function
> >   target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
> >   fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
> >   target-i386/cpu: Introduce apic_id_for_cpu() function
> >   cpus.h: Make constant smp_cores/smp_threads available on *-user
> >   pc: Set fw_cfg data based on APIC ID calculation
> >   tests: Support target-specific unit tests
> >   target-i386: Topology & APIC ID utility functions
> >   pc: Generate APIC IDs according to CPU topology
> > 
> >  hw/fw_cfg.c            |   1 -
> >  hw/pc.c                |  44 +++++++++++++---
> >  hw/pc_piix.c           |  26 +++++++---
> >  hw/ppc_newworld.c      |   1 +
> >  hw/ppc_oldworld.c      |   1 +
> >  hw/sun4m.c             |   3 ++
> >  hw/sun4u.c             |   1 +
> >  include/sysemu/cpus.h  |   7 +++
> >  include/sysemu/kvm.h   |   4 ++
> >  kvm-all.c              |   2 +-
> >  target-i386/cpu.c      |  52 +++++++++++++++----
> >  target-i386/cpu.h      |   5 +-
> >  target-i386/kvm.c      |   6 +++
> >  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  target-ppc/kvm.c       |   5 ++
> >  target-s390x/kvm.c     |   5 ++
> >  tests/.gitignore       |   1 +
> >  tests/Makefile         |  21 +++++++-
> >  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
> >  19 files changed, 391 insertions(+), 28 deletions(-)
> >  create mode 100644 target-i386/topology.h
> >  create mode 100644 tests/test-x86-cpuid.c
> > 
> 
> -- 
> regards!
> li guang
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[Eduardo Habkost]

Adding kvm@vger, Gleb, Michael, Marcelo to Cc, as I forgot to disable
suppress-cc when sending the patches.

On Thu, Jan 17, 2013 at 06:59:26PM -0200, Eduardo Habkost wrote:
> I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
> I sent last week, though.
> 
> Igor argued that APIC ID should be set by the board and not by the CPU itself,
> but I am not doing that because:
>  - I want to keep the bug fix simple and isolated as we are past soft freeze
>  - I believe the creator of the CPU object shouldn't be forced to provide the
>    APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
>    device_add interface in the future
>  - The APIC ID _is_ set by the CPU itself (because each CPU package may have
>    multiple core/threads, and each core/thread has a different APIC ID). What
>    needs to be provided by the board to the CPU package in the future is the
>    package ID and the bit width of the core/thread IDs.
> 
> Git tree for reference:
>   git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
>   https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5
> 
> Eduardo Habkost (12):
>   kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
>     KVM
>   target-i386: Don't set any KVM flag by default if KVM is disabled
>   pc: Reverse pc_init_pci() compatibility logic
>   kvm: Create kvm_arch_vcpu_id() function
>   target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
>   fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
>   target-i386/cpu: Introduce apic_id_for_cpu() function
>   cpus.h: Make constant smp_cores/smp_threads available on *-user
>   pc: Set fw_cfg data based on APIC ID calculation
>   tests: Support target-specific unit tests
>   target-i386: Topology & APIC ID utility functions
>   pc: Generate APIC IDs according to CPU topology
> 
>  hw/fw_cfg.c            |   1 -
>  hw/pc.c                |  44 +++++++++++++---
>  hw/pc_piix.c           |  26 +++++++---
>  hw/ppc_newworld.c      |   1 +
>  hw/ppc_oldworld.c      |   1 +
>  hw/sun4m.c             |   3 ++
>  hw/sun4u.c             |   1 +
>  include/sysemu/cpus.h  |   7 +++
>  include/sysemu/kvm.h   |   4 ++
>  kvm-all.c              |   2 +-
>  target-i386/cpu.c      |  52 +++++++++++++++----
>  target-i386/cpu.h      |   5 +-
>  target-i386/kvm.c      |   6 +++
>  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-ppc/kvm.c       |   5 ++
>  target-s390x/kvm.c     |   5 ++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |  21 +++++++-
>  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
>  19 files changed, 391 insertions(+), 28 deletions(-)
>  create mode 100644 target-i386/topology.h
>  create mode 100644 tests/test-x86-cpuid.c
> 
> -- 
> 1.7.11.7
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[Eduardo Habkost]

Adding kvm@vger, Gleb, Michael, Marcelo to Cc, as I forgot to disable
suppress-cc when sending the patches.

On Thu, Jan 17, 2013 at 06:59:26PM -0200, Eduardo Habkost wrote:
> I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
> I sent last week, though.
> 
> Igor argued that APIC ID should be set by the board and not by the CPU itself,
> but I am not doing that because:
>  - I want to keep the bug fix simple and isolated as we are past soft freeze
>  - I believe the creator of the CPU object shouldn't be forced to provide the
>    APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
>    device_add interface in the future
>  - The APIC ID _is_ set by the CPU itself (because each CPU package may have
>    multiple core/threads, and each core/thread has a different APIC ID). What
>    needs to be provided by the board to the CPU package in the future is the
>    package ID and the bit width of the core/thread IDs.
> 
> Git tree for reference:
>   git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
>   https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5
> 
> Eduardo Habkost (12):
>   kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
>     KVM
>   target-i386: Don't set any KVM flag by default if KVM is disabled
>   pc: Reverse pc_init_pci() compatibility logic
>   kvm: Create kvm_arch_vcpu_id() function
>   target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
>   fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
>   target-i386/cpu: Introduce apic_id_for_cpu() function
>   cpus.h: Make constant smp_cores/smp_threads available on *-user
>   pc: Set fw_cfg data based on APIC ID calculation
>   tests: Support target-specific unit tests
>   target-i386: Topology & APIC ID utility functions
>   pc: Generate APIC IDs according to CPU topology
> 
>  hw/fw_cfg.c            |   1 -
>  hw/pc.c                |  44 +++++++++++++---
>  hw/pc_piix.c           |  26 +++++++---
>  hw/ppc_newworld.c      |   1 +
>  hw/ppc_oldworld.c      |   1 +
>  hw/sun4m.c             |   3 ++
>  hw/sun4u.c             |   1 +
>  include/sysemu/cpus.h  |   7 +++
>  include/sysemu/kvm.h   |   4 ++
>  kvm-all.c              |   2 +-
>  target-i386/cpu.c      |  52 +++++++++++++++----
>  target-i386/cpu.h      |   5 +-
>  target-i386/kvm.c      |   6 +++
>  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-ppc/kvm.c       |   5 ++
>  target-s390x/kvm.c     |   5 ++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |  21 +++++++-
>  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
>  19 files changed, 391 insertions(+), 28 deletions(-)
>  create mode 100644 target-i386/topology.h
>  create mode 100644 tests/test-x86-cpuid.c
> 
> -- 
> 1.7.11.7
> 
> 

-- 
Eduardo

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>> Could you suggest a text for me to add please?
> 
> "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
> instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
> works on most or all architectures supporting KVM, but it is safer to
> use an appropriate 'unsigned long' parameter."

Interestingly enough, while the Linux syscall uses 'unsigned long', the
POSIX definition of ioctl() uses 'int'; so the Linux kernel is already
constrained to never use an ioctl value that doesn't fit within 'int',
and glibc is already responsible for ensuring that argument promotion of
an int doesn't change the behavior of ioctl() in libc when converting it
over to the unsigned long syscall semantics expected by the kernel.

> 
> To find out if 'int' breaks on any architecture, I would need to check
> the ABI specification for each architecture. I didn't do that, but I am
> sure we should pass an unsigned long instead, if that's the type
> expected by the kernel.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>> Could you suggest a text for me to add please?
> 
> "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
> instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
> works on most or all architectures supporting KVM, but it is safer to
> use an appropriate 'unsigned long' parameter."

Interestingly enough, while the Linux syscall uses 'unsigned long', the
POSIX definition of ioctl() uses 'int'; so the Linux kernel is already
constrained to never use an ioctl value that doesn't fit within 'int',
and glibc is already responsible for ensuring that argument promotion of
an int doesn't change the behavior of ioctl() in libc when converting it
over to the unsigned long syscall semantics expected by the kernel.

> 
> To find out if 'int' breaks on any architecture, I would need to check
> the ABI specification for each architecture. I didn't do that, but I am
> sure we should pass an unsigned long instead, if that's the type
> expected by the kernel.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
> >> Could you suggest a text for me to add please?
> > 
> > "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
> > instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
> > works on most or all architectures supporting KVM, but it is safer to
> > use an appropriate 'unsigned long' parameter."
> 
> Interestingly enough, while the Linux syscall uses 'unsigned long', the
> POSIX definition of ioctl() uses 'int'; so the Linux kernel is already
> constrained to never use an ioctl value that doesn't fit within 'int',

Really? What about the ioctl()s that get a pointer as argument on
architectures where pointers don't fit in an int?

Do you have a pointer to the POSIX definition you are talking about?

Note that I'm talking about the the extra ioctl() argument, not the
ioctl() number (that is an unsigned int in the kernel code).


> and glibc is already responsible for ensuring that argument promotion of
> an int doesn't change the behavior of ioctl() in libc when converting it
> over to the unsigned long syscall semantics expected by the kernel.
> 
> > 
> > To find out if 'int' breaks on any architecture, I would need to check
> > the ABI specification for each architecture. I didn't do that, but I am
> > sure we should pass an unsigned long instead, if that's the type
> > expected by the kernel.
> > 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

-- 
Eduardo

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3170 bytes --]

On 01/18/2013 09:40 AM, Eduardo Habkost wrote:
> On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
>> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>>>> Could you suggest a text for me to add please?
>>>
>>> "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
>>> instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
>>> works on most or all architectures supporting KVM, but it is safer to
>>> use an appropriate 'unsigned long' parameter."
>>
>> Interestingly enough, while the Linux syscall uses 'unsigned long', the
>> POSIX definition of ioctl() uses 'int'; so the Linux kernel is already
>> constrained to never use an ioctl value that doesn't fit within 'int',
> 
> Really? What about the ioctl()s that get a pointer as argument on
> architectures where pointers don't fit in an int?
> 
> Do you have a pointer to the POSIX definition you are talking about?
> 
> Note that I'm talking about the the extra ioctl() argument, not the
> ioctl() number (that is an unsigned int in the kernel code).

Okay, now you made me go back and check sources.

POSIX 2008 says:
#include <stropts.h>
int ioctl(int fildes, int request, ... /* arg */);

Gnulib says this about a bug that it works around:
@item
On glibc platforms, the second parameter is of type @code{unsigned long}
rather than @code{int}.

But gnulib also suggests using <sys/ioctl.h> instead of the POSIX header
<stropts.h> for getting ioctl(), because <stropts.h> was declared
obsolete in POSIX 2008 and was never implemented in glibc.

Sure enough, looking at Fedora 18 /usr/include/sys/ioctl.h, I still see:
extern int ioctl (int __fd, unsigned long int __request, ...) __THROW;

Meanwhile, you are correct that the kernel defines request as 32 bits:
linux.git:include/uapi/asm-generic/ioctl.h
/* ioctl command encoding: 32 bits total, command in lower 16 bits,
 * size of the parameter structure in the lower 14 bits of the
 * upper 16 bits.
 * Encoding the size of the parameter structure in the ioctl request
 * is useful for catching programs compiled with old versions
 * and to avoid overwriting user space outside the user buffer area.
 * The highest 2 bits are reserved for indicating the ``access mode''.
 * NOTE: This limits the max parameter size to 16kB -1 !
 */

> 
>> and glibc is already responsible for ensuring that argument promotion of
>> an int doesn't change the behavior of ioctl() in libc when converting it
>> over to the unsigned long syscall semantics expected by the kernel.

So a more precise wording of this is:

glibc is already responsible from converting the 'unsigned long int' of
the user declaration back into the 'unsigned int' that the kernel
expects for the second argument.  The third argument (when present), is
generally treated as a pointer (of size appropriate for the
architecture).  Although there _might_ be an ioctl that uses it directly
as an integer instead of dereferencing it as a pointer, those would be
the exceptions to the rule.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3170 bytes --]

On 01/18/2013 09:40 AM, Eduardo Habkost wrote:
> On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
>> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>>>> Could you suggest a text for me to add please?
>>>
>>> "The argument passed to KVM_CREATE_VCPU now has 'unsigned long' type
>>> instead of 'int', as expected by the Linux ioctl() syscall. Maybe an int
>>> works on most or all architectures supporting KVM, but it is safer to
>>> use an appropriate 'unsigned long' parameter."
>>
>> Interestingly enough, while the Linux syscall uses 'unsigned long', the
>> POSIX definition of ioctl() uses 'int'; so the Linux kernel is already
>> constrained to never use an ioctl value that doesn't fit within 'int',
> 
> Really? What about the ioctl()s that get a pointer as argument on
> architectures where pointers don't fit in an int?
> 
> Do you have a pointer to the POSIX definition you are talking about?
> 
> Note that I'm talking about the the extra ioctl() argument, not the
> ioctl() number (that is an unsigned int in the kernel code).

Okay, now you made me go back and check sources.

POSIX 2008 says:
#include <stropts.h>
int ioctl(int fildes, int request, ... /* arg */);

Gnulib says this about a bug that it works around:
@item
On glibc platforms, the second parameter is of type @code{unsigned long}
rather than @code{int}.

But gnulib also suggests using <sys/ioctl.h> instead of the POSIX header
<stropts.h> for getting ioctl(), because <stropts.h> was declared
obsolete in POSIX 2008 and was never implemented in glibc.

Sure enough, looking at Fedora 18 /usr/include/sys/ioctl.h, I still see:
extern int ioctl (int __fd, unsigned long int __request, ...) __THROW;

Meanwhile, you are correct that the kernel defines request as 32 bits:
linux.git:include/uapi/asm-generic/ioctl.h
/* ioctl command encoding: 32 bits total, command in lower 16 bits,
 * size of the parameter structure in the lower 14 bits of the
 * upper 16 bits.
 * Encoding the size of the parameter structure in the ioctl request
 * is useful for catching programs compiled with old versions
 * and to avoid overwriting user space outside the user buffer area.
 * The highest 2 bits are reserved for indicating the ``access mode''.
 * NOTE: This limits the max parameter size to 16kB -1 !
 */

> 
>> and glibc is already responsible for ensuring that argument promotion of
>> an int doesn't change the behavior of ioctl() in libc when converting it
>> over to the unsigned long syscall semantics expected by the kernel.

So a more precise wording of this is:

glibc is already responsible from converting the 'unsigned long int' of
the user declaration back into the 'unsigned int' that the kernel
expects for the second argument.  The third argument (when present), is
generally treated as a pointer (of size appropriate for the
architecture).  Although there _might_ be an ioctl that uses it directly
as an integer instead of dereferencing it as a pointer, those would be
the exceptions to the rule.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[li guang]

在 2013-01-18五的 12:49 -0200，Eduardo Habkost写道：
> On Fri, Jan 18, 2013 at 02:54:41PM +0800, li guang wrote:
> > 在 2013-01-17四的 18:59 -0200，Eduardo Habkost写道：
> > > I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
> > > I sent last week, though.
> > > 
> > > Igor argued that APIC ID should be set by the board and not by the CPU itself,
> > 
> > per Intel's SPEC, seems APIC ID really based on design of board.
> > (refer to Intel® 64 and IA-32 Architectures
> >     Software Developer’s Manual
> >                 Volume 3 (3A, 3B & 3C):
> >              System Programming Guide
> >      chapter 10.4.6)
> > but, actually, it maybe meaningless for emulation.
> > after go though your patches,
> > I can't capture the purpose you do a topology map between 
> > APIC ID and cpu_index, (sorry for that)
> > can you help to clear that?
> 
> See the documents mentioned on PATCH 11/12:
> 
> +/* This file implements the APIC-ID-based CPU topology enumeration logic,
> + * documented at the following document:
> + *   Intel® 64 Architecture Processor Topology Enumeration
> + *   http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
> + *
> + * This code should be compatible with AMD's "Extended Method" described at:
> + *   AMD CPUID Specification (Publication #25481)
> + *   Section 3: Multiple Core Calcuation
> + * as long as:
> + *  nr_threads is set to 1;
> + *  OFFSET_IDX is assumed to be 0;
> + *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
> + */
> 
> If we don't generate the APIC IDs properly, identification of CPU
> sockets/cores/threads is broken.
> 
> e.g. today -smp 12,cores=3,threads=2 currently ends up exposing 4 cores on the
> first socket, and 2 cores in the second one, because the APIC IDs are generated
> sequentially instead of being based on package/core/thread IDs.

okay, Thanks!

> 
> > 
> > > but I am not doing that because:
> > >  - I want to keep the bug fix simple and isolated as we are past soft freeze
> > >  - I believe the creator of the CPU object shouldn't be forced to provide the
> > >    APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
> > >    device_add interface in the future
> > >  - The APIC ID _is_ set by the CPU itself (because each CPU package may have
> > >    multiple core/threads, and each core/thread has a different APIC ID). What
> > >    needs to be provided by the board to the CPU package in the future is the
> > >    package ID and the bit width of the core/thread IDs.
> > > 
> > > Git tree for reference:
> > >   git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
> > >   https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5
> > > 
> > > Eduardo Habkost (12):
> > >   kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
> > >     KVM
> > >   target-i386: Don't set any KVM flag by default if KVM is disabled
> > >   pc: Reverse pc_init_pci() compatibility logic
> > >   kvm: Create kvm_arch_vcpu_id() function
> > >   target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
> > >   fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
> > >   target-i386/cpu: Introduce apic_id_for_cpu() function
> > >   cpus.h: Make constant smp_cores/smp_threads available on *-user
> > >   pc: Set fw_cfg data based on APIC ID calculation
> > >   tests: Support target-specific unit tests
> > >   target-i386: Topology & APIC ID utility functions
> > >   pc: Generate APIC IDs according to CPU topology
> > > 
> > >  hw/fw_cfg.c            |   1 -
> > >  hw/pc.c                |  44 +++++++++++++---
> > >  hw/pc_piix.c           |  26 +++++++---
> > >  hw/ppc_newworld.c      |   1 +
> > >  hw/ppc_oldworld.c      |   1 +
> > >  hw/sun4m.c             |   3 ++
> > >  hw/sun4u.c             |   1 +
> > >  include/sysemu/cpus.h  |   7 +++
> > >  include/sysemu/kvm.h   |   4 ++
> > >  kvm-all.c              |   2 +-
> > >  target-i386/cpu.c      |  52 +++++++++++++++----
> > >  target-i386/cpu.h      |   5 +-
> > >  target-i386/kvm.c      |   6 +++
> > >  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  target-ppc/kvm.c       |   5 ++
> > >  target-s390x/kvm.c     |   5 ++
> > >  tests/.gitignore       |   1 +
> > >  tests/Makefile         |  21 +++++++-
> > >  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
> > >  19 files changed, 391 insertions(+), 28 deletions(-)
> > >  create mode 100644 target-i386/topology.h
> > >  create mode 100644 tests/test-x86-cpuid.c
> > > 
> > 
> > -- 
> > regards!
> > li guang
> > 
> > 
> 

-- 
regards!
li guang

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

[Andreas Färber]

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

Ping! mst, you handled a previous PC machine compatibility patch - can
you ack or nack?

Eduardo, which of the following patches depend on this one? Only 12/12?

Andreas

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


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Andreas Färber]

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

Ping! mst, you handled a previous PC machine compatibility patch - can
you ack or nack?

Eduardo, which of the following patches depend on this one? Only 12/12?

Andreas

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


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Michael S. Tsirkin]

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

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

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

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

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

[Eduardo Habkost]

On Mon, Jan 21, 2013 at 04:39:24AM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > Currently, the pc-1.4 machine init function enables PV EOI and then
> > calls the pc-1.2 machine init function. The problem with this approach
> > is that now we can't enable any additional compatibility code inside the
> > pc-1.2 init function because it would end up enabling the compatibility
> > behavior on pc-1.3 and pc-1.4 as well.
> > 
> > This reverses the logic so that the pc-1.2 machine init function will
> > disable PV EOI, and then call the pc-1.4 machine init function.
> > 
> > This way we can change older machine-types to enable compatibility
> > behavior, and the newer machine-types (pc-1.3, pc-q35-1.4 and
> > pc-i440fx-1.4) would just use the default behavior.
> > 
> > (This means that one nice side-effect of this change is that pc-q35-1.4
> > will get PV EOI enabled by default, too)
> > 
> > It would be interesting to eventually change pc_init_pci_no_kvmclock()
> > and pc_init_isa() to reuse pc_init_pci_1_2() as well (so we don't need
> > to duplicate compatibility code on those two functions). But this will
> > be probably much easier to do after we create a PCInitArgs struct for
> > the PC initialization arguments, and/or after we use global-properties
> > to implement the compatibility modes present in pc_init_pci_1_2().
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Ping! mst, you handled a previous PC machine compatibility patch - can
> you ack or nack?
> 
> Eduardo, which of the following patches depend on this one? Only 12/12?

Yes, only 12/12 depend on it (it is the patch that finally introduces
the fix, on pc-1.4 only).

-- 
Eduardo

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

[Eduardo Habkost]

On Mon, Jan 21, 2013 at 04:39:24AM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > Currently, the pc-1.4 machine init function enables PV EOI and then
> > calls the pc-1.2 machine init function. The problem with this approach
> > is that now we can't enable any additional compatibility code inside the
> > pc-1.2 init function because it would end up enabling the compatibility
> > behavior on pc-1.3 and pc-1.4 as well.
> > 
> > This reverses the logic so that the pc-1.2 machine init function will
> > disable PV EOI, and then call the pc-1.4 machine init function.
> > 
> > This way we can change older machine-types to enable compatibility
> > behavior, and the newer machine-types (pc-1.3, pc-q35-1.4 and
> > pc-i440fx-1.4) would just use the default behavior.
> > 
> > (This means that one nice side-effect of this change is that pc-q35-1.4
> > will get PV EOI enabled by default, too)
> > 
> > It would be interesting to eventually change pc_init_pci_no_kvmclock()
> > and pc_init_isa() to reuse pc_init_pci_1_2() as well (so we don't need
> > to duplicate compatibility code on those two functions). But this will
> > be probably much easier to do after we create a PCInitArgs struct for
> > the PC initialization arguments, and/or after we use global-properties
> > to implement the compatibility modes present in pc_init_pci_1_2().
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: kvm@vger.kernel.org
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Gleb Natapov <gleb@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Ping! mst, you handled a previous PC machine compatibility patch - can
> you ack or nack?
> 
> Eduardo, which of the following patches depend on this one? Only 12/12?

Yes, only 12/12 depend on it (it is the patch that finally introduces
the fix, on pc-1.4 only).

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 07/12] target-i386/cpu: Introduce apic_id_for_cpu() function

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> This function will be used by both the CPU initialization code and the
> fw_cfg table initialization code.
> 
> Later this function will be updated to generate APIC IDs according to
> the CPU topology.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 17 ++++++++++++++++-
>  target-i386/cpu.h |  2 ++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index d1a14d5..d90789d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2194,6 +2194,21 @@ void x86_cpu_realize(Object *obj, Error **errp)
>      cpu_reset(CPU(cpu));
>  }
>  
> +/* Calculates initial APIC ID for a specific CPU index
> + *
> + * Currently we need to be able to calculate the APIC ID from the CPU index
> + * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
> + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
> + * all CPUs up to max_cpus.
> + */
> +uint32_t apic_id_for_cpu(unsigned int cpu_index)

Can we rather make this x86_cpu_apic_id(X86CPU *cpu) to account for
future changes to topology modelling?

Andreas

> +{
> +    /* right now APIC ID == CPU index. this will eventually change to use
> +     * the CPU topology configuration properly
> +     */
> +    return cpu_index;
> +}
> +
>  static void x86_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
> @@ -2228,7 +2243,7 @@ static void x86_cpu_initfn(Object *obj)
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
>  
> -    env->cpuid_apic_id = cs->cpu_index;
> +    env->cpuid_apic_id = apic_id_for_cpu(cs->cpu_index);
>  
>      /* init various static tables used in TCG mode */
>      if (tcg_enabled() && !inited) {
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 9d4fcf9..d86c0af 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1255,4 +1255,6 @@ void disable_kvm_pv_eoi(void);
>  /* Return name of 32-bit register, from a R_* constant */
>  const char *get_register_name_32(unsigned int reg);
>  
> +uint32_t apic_id_for_cpu(unsigned int cpu_index);
> +
>  #endif /* CPU_I386_H */

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 11/12] target-i386: Topology & APIC ID utility functions

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
>  - Support 32-bit APIC IDs (in case x2APIC is going to be used)
>  - Coding style changes
>  - Use TARGET_I386_TOPOLOGY_H instead of __QEMU_X86_TOPOLOGY_H__
>  - Rename topo_make_apic_id() to topo_apicid_for_cpu()
>  - Rename __make_apicid() to topo_make_apicid()
>  - Spaces around operators on test-x86-cpuid.c, as requested by
>    Blue Swirl
>  - Make test-x86-cpuid a target-specific test
> 
> Changes v2 -> v3:
>  - Add documentation pointers to the code
>  - Rename bits_for_count() to bitwidth_for_count()
>  - Remove unused apicid_*_id() functions
> 
> Changes v3 -> v4:
>  - Remove now-obsolete FIXME comment from test-x86-cpuid.c
>  - Change bitops.h include to qemu/bitops.h
>  - Add gcov file list to test-x86-cpuid
> ---
>  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |   7 +++
>  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 242 insertions(+)
>  create mode 100644 target-i386/topology.h
>  create mode 100644 tests/test-x86-cpuid.c
> 
> diff --git a/target-i386/topology.h b/target-i386/topology.h
> new file mode 100644
> index 0000000..833ab47
> --- /dev/null
> +++ b/target-i386/topology.h
> @@ -0,0 +1,133 @@
> +/*
> + *  x86 CPU topology data structures and functions
> + *
> + *  Copyright (c) 2012 Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#ifndef TARGET_I386_TOPOLOGY_H
> +#define TARGET_I386_TOPOLOGY_H
> +
> +/* This file implements the APIC-ID-based CPU topology enumeration logic,
> + * documented at the following document:
> + *   Intel® 64 Architecture Processor Topology Enumeration
> + *   http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
> + *
> + * This code should be compatible with AMD's "Extended Method" described at:
> + *   AMD CPUID Specification (Publication #25481)
> + *   Section 3: Multiple Core Calcuation
> + * as long as:
> + *  nr_threads is set to 1;
> + *  OFFSET_IDX is assumed to be 0;
> + *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
> + */
> +
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "qemu/bitops.h"
> +
> +/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
> + */
> +typedef uint32_t apic_id_t;

Is this file imported from somewhere? There was a discussion some time
ago about not using _t since reserved by POSIX...

> +
> +/* Return the bit width needed for 'count' IDs
> + */
> +static unsigned bitwidth_for_count(unsigned count)
> +{
> +    g_assert(count >= 1);
> +    if (count == 1) {
> +        return 0;
> +    }
> +    return bitops_flsl(count - 1) + 1;
> +}
> +
> +/* Bit width of the SMT_ID (thread ID) field on the APIC ID
> + */
> +static inline unsigned apicid_smt_width(unsigned nr_cores, unsigned nr_threads)
> +{
> +    return bitwidth_for_count(nr_threads);
> +}
> +
> +/* Bit width of the Core_ID field
> + */
> +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
> +{
> +    return bitwidth_for_count(nr_cores);
> +}
> +
> +/* Bit offset of the Core_ID field
> + */
> +static inline unsigned apicid_core_offset(unsigned nr_cores,
> +                                          unsigned nr_threads)
> +{
> +    return apicid_smt_width(nr_cores, nr_threads);
> +}
> +
> +/* Bit offset of the Pkg_ID (socket ID) field
> + */
> +static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
> +{
> +    return apicid_core_offset(nr_cores, nr_threads) + \

Not a macro. :)

> +           apicid_core_width(nr_cores, nr_threads);
> +}
> +
> +/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
> + *
> + * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
> + */
> +static inline apic_id_t topo_make_apicid(unsigned nr_cores,
> +                                         unsigned nr_threads,
> +                                         unsigned pkg_id, unsigned core_id,
> +                                         unsigned smt_id)
> +{
> +    return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) | \
> +           (core_id << apicid_core_offset(nr_cores, nr_threads)) | \

Ditto.

> +           smt_id;
> +}
> +
> +/* Calculate thread/core/package IDs for a specific topology,
> + * based on (contiguous) CPU index
> + */
> +static inline void topo_ids_from_idx(unsigned nr_cores, unsigned nr_threads,
> +                                     unsigned cpu_index,
> +                                     unsigned *pkg_id, unsigned *core_id,
> +                                     unsigned *smt_id)
> +{
> +    unsigned core_index = cpu_index / nr_threads;
> +    *smt_id = cpu_index % nr_threads;
> +    *core_id = core_index % nr_cores;
> +    *pkg_id = core_index / nr_cores;
> +}
> +
> +/* Make APIC ID for the CPU 'cpu_index'
> + *
> + * 'cpu_index' is a sequential, contiguous ID for the CPU.
> + */
> +static inline apic_id_t topo_apicid_for_cpu(unsigned nr_cores,
> +                                            unsigned nr_threads,
> +                                            unsigned cpu_index)
> +{
> +    unsigned pkg_id, core_id, smt_id;
> +    topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
> +                      &pkg_id, &core_id, &smt_id);
> +    return topo_make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id);
> +}
> +
> +#endif /* TARGET_I386_TOPOLOGY_H */

As a follow-up it would be nice to clean up the documentation gtk-doc
style, e.g. @cpu_index: bla bla, and first function name, then
parameters, then description.

> diff --git a/tests/.gitignore b/tests/.gitignore
> index f9041f3..38c94ef 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -10,4 +10,5 @@ test-qmp-commands.h
>  test-qmp-commands
>  test-qmp-input-strict
>  test-qmp-marshal.c
> +test-x86-cpuid
>  *-test
> diff --git a/tests/Makefile b/tests/Makefile
> index 41172d6..d46e64c 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -46,6 +46,11 @@ gcov-files-test-aio-$(CONFIG_POSIX) = aio-posix.c
>  check-unit-y += tests/test-thread-pool$(EXESUF)
>  gcov-files-test-thread-pool-y = thread-pool.c
>  
> +check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
> +# all code tested by test-x86-cpuid is inside topology.h,
> +# so add the test file itself to the gcov list
> +gcov-files-test-x86-cpuid-y = tests/test-x86-cpuid.c
> +
>  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>  
>  # All QTests for now are POSIX-only, but the dependencies are
> @@ -71,6 +76,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>  	tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
>  	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
>  	tests/test-qmp-commands.o tests/test-visitor-serialization.o
> +test-obj-i386-y = tests/test-x86-cpuid.o
>  
>  test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o
>  
> @@ -86,6 +92,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil
>  tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
>  tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(block-obj-y) libqemuutil.a libqemustub.a
>  tests/test-iov$(EXESUF): tests/test-iov.o libqemuutil.a
> +tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
>  
>  tests/test-qapi-types.c tests/test-qapi-types.h :\
>  $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
> diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
> new file mode 100644
> index 0000000..1fe9f30
> --- /dev/null
> +++ b/tests/test-x86-cpuid.c
> @@ -0,0 +1,101 @@
> +/*
> + *  Test code for x86 CPUID and Topology functions
> + *
> + *  Copyright (c) 2012 Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include <glib.h>
> +
> +#include "topology.h"
> +
> +static void test_topo_bits(void)
> +{
> +    /* simple tests for 1 thread per core, 1 core per socket */
> +    g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0);
> +    g_assert_cmpuint(apicid_core_width(1, 1), ==, 0);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 0), ==, 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 1), ==, 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 2), ==, 2);
> +    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 3), ==, 3);
> +
> +
> +    /* Test field width calculation for multiple values
> +     */
> +    g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1);
> +    g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2);
> +    g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2);
> +
> +    g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4);
> +    g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4);
> +    g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4);
> +    g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5);
> +
> +
> +    g_assert_cmpuint(apicid_core_width(30, 2), ==, 5);
> +    g_assert_cmpuint(apicid_core_width(31, 2), ==, 5);
> +    g_assert_cmpuint(apicid_core_width(32, 2), ==, 5);
> +    g_assert_cmpuint(apicid_core_width(33, 2), ==, 6);
> +
> +
> +    /* build a weird topology and see if IDs are calculated correctly
> +     */
> +
> +    /* This will use 2 bits for thread ID and 3 bits for core ID
> +     */
> +    g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2);
> +    g_assert_cmpuint(apicid_core_width(6, 3), ==, 3);
> +    g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 0), ==, 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1), ==, 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2), ==, 2);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 0), ==, (1 << 2) | 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 1), ==, (1 << 2) | 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 2), ==, (1 << 2) | 2);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 0), ==, (2 << 2) | 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 1), ==, (2 << 2) | 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 2), ==, (2 << 2) | 2);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2);
> +
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 0 * 3 + 0), ==,
> +                                         (1 << 5));
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 1 * 3 + 1), ==,
> +                                         (1 << 5) | (1 << 2) | 1);
> +    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 3 * 6 * 3 + 5 * 3 + 2), ==,
> +                                         (3 << 5) | (5 << 2) | 2);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    g_test_add_func("/cpuid/topology/basic", test_topo_bits);
> +
> +    g_test_run();
> +
> +    return 0;
> +}

Otherwise looks good, thanks for adding tests!

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 07/12] target-i386/cpu: Introduce apic_id_for_cpu() function

[Eduardo Habkost]

On Mon, Jan 21, 2013 at 12:18:55PM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > This function will be used by both the CPU initialization code and the
> > fw_cfg table initialization code.
> > 
> > Later this function will be updated to generate APIC IDs according to
> > the CPU topology.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 17 ++++++++++++++++-
> >  target-i386/cpu.h |  2 ++
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index d1a14d5..d90789d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2194,6 +2194,21 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >      cpu_reset(CPU(cpu));
> >  }
> >  
> > +/* Calculates initial APIC ID for a specific CPU index
> > + *
> > + * Currently we need to be able to calculate the APIC ID from the CPU index
> > + * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
> > + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
> > + * all CPUs up to max_cpus.
> > + */
> > +uint32_t apic_id_for_cpu(unsigned int cpu_index)
> 
> Can we rather make this x86_cpu_apic_id(X86CPU *cpu) to account for
> future changes to topology modelling?

We can't make it get a X86CPU as parameter, because the ACPI tables have
to be built up to max_cpus, before the CPUs get actually created. But it
can be renamed, yes.

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-1.4 00/12] target-i386: Fix APIC-ID-based topology (v4)

[Andreas Färber]

Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> I am hoping to get this bug fixed in 1.4. I didn't get much feedback on the RFC
> I sent last week, though.
> 
> Igor argued that APIC ID should be set by the board and not by the CPU itself,
> but I am not doing that because:
>  - I want to keep the bug fix simple and isolated as we are past soft freeze
>  - I believe the creator of the CPU object shouldn't be forced to provide the
>    APIC ID, so the APIC ID is not unnecessarily exposed on the CPU hotplug
>    device_add interface in the future
>  - The APIC ID _is_ set by the CPU itself (because each CPU package may have
>    multiple core/threads, and each core/thread has a different APIC ID). What
>    needs to be provided by the board to the CPU package in the future is the
>    package ID and the bit width of the core/thread IDs.
> 
> Git tree for reference:
>   git://github.com/ehabkost/qemu-hacks.git apicid-topology.v5
>   https://github.com/ehabkost/qemu-hacks/tree/apicid-topology.v5
> 
> Eduardo Habkost (12):
>   kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou
>     KVM
>   target-i386: Don't set any KVM flag by default if KVM is disabled
>   pc: Reverse pc_init_pci() compatibility logic

Applied these to qom-cpu (for 1.4, with some typo fixes):
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

For the rest there's either open questions or still ongoing review.

Andreas

>   kvm: Create kvm_arch_vcpu_id() function
>   target-i386: kvm: Set vcpu_id to APIC ID instead of CPU index
>   fw_cfg: Remove FW_CFG_MAX_CPUS from fw_cfg_init()
>   target-i386/cpu: Introduce apic_id_for_cpu() function
>   cpus.h: Make constant smp_cores/smp_threads available on *-user
>   pc: Set fw_cfg data based on APIC ID calculation
>   tests: Support target-specific unit tests
>   target-i386: Topology & APIC ID utility functions
>   pc: Generate APIC IDs according to CPU topology
> 
>  hw/fw_cfg.c            |   1 -
>  hw/pc.c                |  44 +++++++++++++---
>  hw/pc_piix.c           |  26 +++++++---
>  hw/ppc_newworld.c      |   1 +
>  hw/ppc_oldworld.c      |   1 +
>  hw/sun4m.c             |   3 ++
>  hw/sun4u.c             |   1 +
>  include/sysemu/cpus.h  |   7 +++
>  include/sysemu/kvm.h   |   4 ++
>  kvm-all.c              |   2 +-
>  target-i386/cpu.c      |  52 +++++++++++++++----
>  target-i386/cpu.h      |   5 +-
>  target-i386/kvm.c      |   6 +++
>  target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target-ppc/kvm.c       |   5 ++
>  target-s390x/kvm.c     |   5 ++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |  21 +++++++-
>  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
>  19 files changed, 391 insertions(+), 28 deletions(-)
>  create mode 100644 target-i386/topology.h
>  create mode 100644 tests/test-x86-cpuid.c
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 18.01.2013 18:46, schrieb Eric Blake:
> On 01/18/2013 09:40 AM, Eduardo Habkost wrote:
>> On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
>>> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>>>>> Could you suggest a text for me to add please?
>>>> 
>>>> "The argument passed to KVM_CREATE_VCPU now has 'unsigned
>>>> long' type instead of 'int', as expected by the Linux ioctl()
>>>> syscall. Maybe an int works on most or all architectures
>>>> supporting KVM, but it is safer to use an appropriate
>>>> 'unsigned long' parameter."
>>> 
>>> Interestingly enough, while the Linux syscall uses 'unsigned
>>> long', the POSIX definition of ioctl() uses 'int'; so the Linux
>>> kernel is already constrained to never use an ioctl value that
>>> doesn't fit within 'int',
>> 
>> Really? What about the ioctl()s that get a pointer as argument
>> on architectures where pointers don't fit in an int?
>> 
>> Do you have a pointer to the POSIX definition you are talking
>> about?
>> 
>> Note that I'm talking about the the extra ioctl() argument, not
>> the ioctl() number (that is an unsigned int in the kernel code).
> 
> Okay, now you made me go back and check sources.
> 
> POSIX 2008 says: #include <stropts.h> int ioctl(int fildes, int
> request, ... /* arg */);
> 
> Gnulib says this about a bug that it works around: @item On glibc
> platforms, the second parameter is of type @code{unsigned long} 
> rather than @code{int}.
> 
> But gnulib also suggests using <sys/ioctl.h> instead of the POSIX
> header <stropts.h> for getting ioctl(), because <stropts.h> was
> declared obsolete in POSIX 2008 and was never implemented in
> glibc.
> 
> Sure enough, looking at Fedora 18 /usr/include/sys/ioctl.h, I still
> see: extern int ioctl (int __fd, unsigned long int __request, ...)
> __THROW;
> 
> Meanwhile, you are correct that the kernel defines request as 32
> bits: linux.git:include/uapi/asm-generic/ioctl.h /* ioctl command
> encoding: 32 bits total, command in lower 16 bits, * size of the
> parameter structure in the lower 14 bits of the * upper 16 bits. *
> Encoding the size of the parameter structure in the ioctl request *
> is useful for catching programs compiled with old versions * and to
> avoid overwriting user space outside the user buffer area. * The
> highest 2 bits are reserved for indicating the ``access mode''. *
> NOTE: This limits the max parameter size to 16kB -1 ! */
> 
>> 
>>> and glibc is already responsible for ensuring that argument
>>> promotion of an int doesn't change the behavior of ioctl() in
>>> libc when converting it over to the unsigned long syscall
>>> semantics expected by the kernel.
> 
> So a more precise wording of this is:
> 
> glibc is already responsible from converting the 'unsigned long
> int' of the user declaration back into the 'unsigned int' that the
> kernel expects for the second argument.  The third argument (when
> present), is generally treated as a pointer (of size appropriate
> for the architecture).  Although there _might_ be an ioctl that
> uses it directly as an integer instead of dereferencing it as a
> pointer, those would be the exceptions to the rule.

So ... do we have a conclusion what to put into the commit message? :)

It looks to me as if kvm-all.c:kvm_vm_ioctl() is using void*. I like
unsigned long but maybe uintptr_t would be more correct then?

Or should kvm_vm_ioctl() be fixed to use something else instead?
Eric's int would be a semantic change for the 64-bit platforms, no?

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIcBAEBAgAGBQJQ/T8lAAoJEPou0S0+fgE/gl8QALesZwG5q07W21mp2j4ikL8N
jrBHjG2VZ9Kda+AIGMClVsWntGZSOzHdtriJ4gjxp90D71S/LQfsYAy6bj45FIwS
kPbIQblLlL5Xc6ZiTS5yTzkwyEd7gUpDVouXyv3XxeyUxqhQKwgWxpiP4RftbBRI
Z8wLbVFNpIoIsHfhKoNkT4M/Ucm1iZbChV6y4zqltAfdQhcl6Gq0jzhtkAfmN41t
p3tCJYldRwayiKLsO2Y0BMNrKmCJisKCEGmkCQzye/3cuFoat/WUmpjV/65hLNtm
ruzfn6pCqMTEGPC4YeDdUsxAhVzVX+Sd4mBKHBGItmvhhJMFYUtwTosRwX5bOrAJ
mpVLAj5/XDYTm2/jQUEOJAqpxUr5oAVMQL3sNeWJPmXkk1kNaNWTNVHHDW1iJnRj
ty0YIWOnuNabkwiDEjPCz6ghjfA3wOBWy8Gk3+F21MYgRQwDTFw4JZuroOIzy3iD
6Vs4MmiBUGnoLobSqw2dUZFmjL7a1500AxZG0MwBd+EqnbLHGqD33kvLrbUYT8+F
eW+cqKV+ZXo3ux343rTxD6EFgmN7GmHSkknxJN5m6ldlw5wfFQ8KhdCiKjwSq3EP
X0bVGmryEdIh+6w/RbhL75Vfb/Je0mr/GzhtijtXo+FORkF8ip2mlpVSl46r0AfI
KvsZ0HZqZHsfoaSBC1js
=/cL3
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 18.01.2013 18:46, schrieb Eric Blake:
> On 01/18/2013 09:40 AM, Eduardo Habkost wrote:
>> On Fri, Jan 18, 2013 at 09:11:42AM -0700, Eric Blake wrote:
>>> On 01/18/2013 07:20 AM, Eduardo Habkost wrote:
>>>>> Could you suggest a text for me to add please?
>>>> 
>>>> "The argument passed to KVM_CREATE_VCPU now has 'unsigned
>>>> long' type instead of 'int', as expected by the Linux ioctl()
>>>> syscall. Maybe an int works on most or all architectures
>>>> supporting KVM, but it is safer to use an appropriate
>>>> 'unsigned long' parameter."
>>> 
>>> Interestingly enough, while the Linux syscall uses 'unsigned
>>> long', the POSIX definition of ioctl() uses 'int'; so the Linux
>>> kernel is already constrained to never use an ioctl value that
>>> doesn't fit within 'int',
>> 
>> Really? What about the ioctl()s that get a pointer as argument
>> on architectures where pointers don't fit in an int?
>> 
>> Do you have a pointer to the POSIX definition you are talking
>> about?
>> 
>> Note that I'm talking about the the extra ioctl() argument, not
>> the ioctl() number (that is an unsigned int in the kernel code).
> 
> Okay, now you made me go back and check sources.
> 
> POSIX 2008 says: #include <stropts.h> int ioctl(int fildes, int
> request, ... /* arg */);
> 
> Gnulib says this about a bug that it works around: @item On glibc
> platforms, the second parameter is of type @code{unsigned long} 
> rather than @code{int}.
> 
> But gnulib also suggests using <sys/ioctl.h> instead of the POSIX
> header <stropts.h> for getting ioctl(), because <stropts.h> was
> declared obsolete in POSIX 2008 and was never implemented in
> glibc.
> 
> Sure enough, looking at Fedora 18 /usr/include/sys/ioctl.h, I still
> see: extern int ioctl (int __fd, unsigned long int __request, ...)
> __THROW;
> 
> Meanwhile, you are correct that the kernel defines request as 32
> bits: linux.git:include/uapi/asm-generic/ioctl.h /* ioctl command
> encoding: 32 bits total, command in lower 16 bits, * size of the
> parameter structure in the lower 14 bits of the * upper 16 bits. *
> Encoding the size of the parameter structure in the ioctl request *
> is useful for catching programs compiled with old versions * and to
> avoid overwriting user space outside the user buffer area. * The
> highest 2 bits are reserved for indicating the ``access mode''. *
> NOTE: This limits the max parameter size to 16kB -1 ! */
> 
>> 
>>> and glibc is already responsible for ensuring that argument
>>> promotion of an int doesn't change the behavior of ioctl() in
>>> libc when converting it over to the unsigned long syscall
>>> semantics expected by the kernel.
> 
> So a more precise wording of this is:
> 
> glibc is already responsible from converting the 'unsigned long
> int' of the user declaration back into the 'unsigned int' that the
> kernel expects for the second argument.  The third argument (when
> present), is generally treated as a pointer (of size appropriate
> for the architecture).  Although there _might_ be an ioctl that
> uses it directly as an integer instead of dereferencing it as a
> pointer, those would be the exceptions to the rule.

So ... do we have a conclusion what to put into the commit message? :)

It looks to me as if kvm-all.c:kvm_vm_ioctl() is using void*. I like
unsigned long but maybe uintptr_t would be more correct then?

Or should kvm_vm_ioctl() be fixed to use something else instead?
Eric's int would be a semantic change for the 64-bit platforms, no?

Andreas

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIcBAEBAgAGBQJQ/T8lAAoJEPou0S0+fgE/gl8QALesZwG5q07W21mp2j4ikL8N
jrBHjG2VZ9Kda+AIGMClVsWntGZSOzHdtriJ4gjxp90D71S/LQfsYAy6bj45FIwS
kPbIQblLlL5Xc6ZiTS5yTzkwyEd7gUpDVouXyv3XxeyUxqhQKwgWxpiP4RftbBRI
Z8wLbVFNpIoIsHfhKoNkT4M/Ucm1iZbChV6y4zqltAfdQhcl6Gq0jzhtkAfmN41t
p3tCJYldRwayiKLsO2Y0BMNrKmCJisKCEGmkCQzye/3cuFoat/WUmpjV/65hLNtm
ruzfn6pCqMTEGPC4YeDdUsxAhVzVX+Sd4mBKHBGItmvhhJMFYUtwTosRwX5bOrAJ
mpVLAj5/XDYTm2/jQUEOJAqpxUr5oAVMQL3sNeWJPmXkk1kNaNWTNVHHDW1iJnRj
ty0YIWOnuNabkwiDEjPCz6ghjfA3wOBWy8Gk3+F21MYgRQwDTFw4JZuroOIzy3iD
6Vs4MmiBUGnoLobSqw2dUZFmjL7a1500AxZG0MwBd+EqnbLHGqD33kvLrbUYT8+F
eW+cqKV+ZXo3ux343rTxD6EFgmN7GmHSkknxJN5m6ldlw5wfFQ8KhdCiKjwSq3EP
X0bVGmryEdIh+6w/RbhL75Vfb/Je0mr/GzhtijtXo+FORkF8ip2mlpVSl46r0AfI
KvsZ0HZqZHsfoaSBC1js
=/cL3
-----END PGP SIGNATURE-----

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

On 01/21/2013 06:14 AM, Andreas Färber wrote:
>> glibc is already responsible from converting the 'unsigned long
>> int' of the user declaration back into the 'unsigned int' that the
>> kernel expects for the second argument.  The third argument (when
>> present), is generally treated as a pointer (of size appropriate
>> for the architecture).  Although there _might_ be an ioctl that
>> uses it directly as an integer instead of dereferencing it as a
>> pointer, those would be the exceptions to the rule.
> 
> So ... do we have a conclusion what to put into the commit message? :)
> 
> It looks to me as if kvm-all.c:kvm_vm_ioctl() is using void*. I like
> unsigned long but maybe uintptr_t would be more correct then?

uintptr_t feels more correct - the 3rd (vararg) argument through the
ioctl() syscall is always retrieved using the same size as void*.

> 
> Or should kvm_vm_ioctl() be fixed to use something else instead?
> Eric's int would be a semantic change for the 64-bit platforms, no?

My discussion about 'int' vs. 'unsigned long' was in regards to the
second argument KVM_CREATE_VCPU, which your patch does not change
(perhaps my fault for jumping in on a conversation mid-thread without
actually reading your original patch, which I have now done).  That is,
KVM_CREATE_VCPU as a constant is always 32 bits (kernel constraint),
widened out to unsigned long when passed to the glibc function (due to
the glibc signature disagreeing with POSIX), then narrowed back down to
32 bits when forwarded to the kernel syscall.

Meanwhile, your patch is fixing the third argument from 'int' to a wider
type, which is necessary for passing that value through varargs when the
receiving end will retrieve the same argument via a void* variable.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Andreas Färber]

Am 22.01.2013 02:43, schrieb Marcelo Tosatti:
> On Thu, Jan 17, 2013 at 06:59:27PM -0200, Eduardo Habkost wrote:
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>> Cc: kvm@vger.kernel.org
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Gleb Natapov <gleb@redhat.com>
>> Cc: Marcelo Tosatti <mtosatti@redhat.com>
>> ---
>>  include/sysemu/kvm.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index 6bdd513..22acf91 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -36,6 +36,7 @@
>>  #define KVM_FEATURE_ASYNC_PF     0
>>  #define KVM_FEATURE_STEAL_TIME   0
>>  #define KVM_FEATURE_PV_EOI       0
>> +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
>>  #endif
>>  
>>  extern int kvm_allowed;
>> -- 
>> 1.7.11.7
>>
> 
> ACK

BTW is it the general strategy to add these as needed for new patches?
Or should we add all current ones and mandate adding such dummy
definitions when new ones get introduced via linux-headers/ update?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Eduardo Habkost]

On Tue, Jan 22, 2013 at 05:59:14AM +0100, Andreas Färber wrote:
> Am 22.01.2013 02:43, schrieb Marcelo Tosatti:
> > On Thu, Jan 17, 2013 at 06:59:27PM -0200, Eduardo Habkost wrote:
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >> Cc: kvm@vger.kernel.org
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Gleb Natapov <gleb@redhat.com>
> >> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> >> ---
> >>  include/sysemu/kvm.h | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >> index 6bdd513..22acf91 100644
> >> --- a/include/sysemu/kvm.h
> >> +++ b/include/sysemu/kvm.h
> >> @@ -36,6 +36,7 @@
> >>  #define KVM_FEATURE_ASYNC_PF     0
> >>  #define KVM_FEATURE_STEAL_TIME   0
> >>  #define KVM_FEATURE_PV_EOI       0
> >> +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> >>  #endif
> >>  
> >>  extern int kvm_allowed;
> >> -- 
> >> 1.7.11.7
> >>
> > 
> > ACK
> 
> BTW is it the general strategy to add these as needed for new patches?
> Or should we add all current ones and mandate adding such dummy
> definitions when new ones get introduced via linux-headers/ update?

I meant to include all existing bits in a single patch previously, but
somehow I missed the KVM_FEATURE_CLOCKSOURCE_STABLE_BIT definition when
looking at the kernel header.

It would be nice to automatically refresh the fake-defines list when
updating linux-headers, but I won't mind if we update the list (pulling
all existing bits again) only when QEMU starts using a define that is
missing.

-- 
Eduardo

Re: [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Eduardo Habkost]

On Mon, Jan 21, 2013 at 07:35:22AM -0700, Eric Blake wrote:
> On 01/21/2013 06:14 AM, Andreas Färber wrote:
> >> glibc is already responsible from converting the 'unsigned long
> >> int' of the user declaration back into the 'unsigned int' that the
> >> kernel expects for the second argument.  The third argument (when
> >> present), is generally treated as a pointer (of size appropriate
> >> for the architecture).  Although there _might_ be an ioctl that
> >> uses it directly as an integer instead of dereferencing it as a
> >> pointer, those would be the exceptions to the rule.
> > 
> > So ... do we have a conclusion what to put into the commit message? :)
> > 
> > It looks to me as if kvm-all.c:kvm_vm_ioctl() is using void*. I like
> > unsigned long but maybe uintptr_t would be more correct then?
> 
> uintptr_t feels more correct - the 3rd (vararg) argument through the
> ioctl() syscall is always retrieved using the same size as void*.

Actually, sys_ioctl() always retrieve it using "unsigned long", but
nothing prevents the arch-specific syscall entry code to from
translating something from a different type to "unsigned long" before
calling sys_ioctl().

So I guess the only guarantee we have is the Linux ioctl(2) man page,
that says: "The third argument is an untyped pointer to memory. It's
traditionally char *argp (from the days before void * was valid C), and
will be so named for this discussion."

That said, I plan to change the code to cast the argument to (void*) in
the next version.

> 
> > 
> > Or should kvm_vm_ioctl() be fixed to use something else instead?
> > Eric's int would be a semantic change for the 64-bit platforms, no?
> 
> My discussion about 'int' vs. 'unsigned long' was in regards to the
> second argument KVM_CREATE_VCPU, which your patch does not change
> (perhaps my fault for jumping in on a conversation mid-thread without
> actually reading your original patch, which I have now done).  That is,
> KVM_CREATE_VCPU as a constant is always 32 bits (kernel constraint),
> widened out to unsigned long when passed to the glibc function (due to
> the glibc signature disagreeing with POSIX), then narrowed back down to
> 32 bits when forwarded to the kernel syscall.
> 
> Meanwhile, your patch is fixing the third argument from 'int' to a wider
> type, which is necessary for passing that value through varargs when the
> receiving end will retrieve the same argument via a void* variable.

I am confident that "unsigned long" will work properly on all
architectures we care about today, but I also don't know if this is
documented and guaranteed to work on all architectures. Passing an
argument of the documented type (void*) sounds like the right thing to
do.

-- 
Eduardo

Re: [PATCH for-1.4 01/12] kvm: Add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for builds withou KVM

[Marcelo Tosatti]

On Tue, Jan 22, 2013 at 05:59:14AM +0100, Andreas Färber wrote:
> Am 22.01.2013 02:43, schrieb Marcelo Tosatti:
> > On Thu, Jan 17, 2013 at 06:59:27PM -0200, Eduardo Habkost wrote:
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >> Cc: kvm@vger.kernel.org
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Gleb Natapov <gleb@redhat.com>
> >> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> >> ---
> >>  include/sysemu/kvm.h | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> >> index 6bdd513..22acf91 100644
> >> --- a/include/sysemu/kvm.h
> >> +++ b/include/sysemu/kvm.h
> >> @@ -36,6 +36,7 @@
> >>  #define KVM_FEATURE_ASYNC_PF     0
> >>  #define KVM_FEATURE_STEAL_TIME   0
> >>  #define KVM_FEATURE_PV_EOI       0
> >> +#define KVM_FEATURE_CLOCKSOURCE_STABLE_BIT 0
> >>  #endif
> >>  
> >>  extern int kvm_allowed;
> >> -- 
> >> 1.7.11.7
> >>
> > 
> > ACK
> 
> BTW is it the general strategy to add these as needed for new patches?
> Or should we add all current ones and mandate adding such dummy
> definitions when new ones get introduced via linux-headers/ update?

Its a good idea to update the sync scripts to automatically create the
dummy ones, i suppose (there is no proactive strategy).