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

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

[Eduardo Habkost]

This series uses a much simpler approach than the previous versions:

 - The APIC ID calculation code is now inside cpu.c
 - It doesn't require touching the PC CPU creation code at all
 - It simply uses a static variable to enable the compat behavior on pc-1.3
   and older
   - I considered making the compat-apic-id setting a global property for the
     X86CPU objects, the only problem is that the fw_cfg initialization code
     on pc.c also depends on the compat behavior

I am sending this as RFC because it depends on two not-included-yet series:
 - Andreas' qom-cpu-7 branch
 - My cpu-enforce fixes series

I hope to be able to get this buf fix into QEMU 1.4. I don't know if we should
try to get this before soft freeze, or we can include it after that.

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

Eduardo Habkost (12):
  kvm: add KVM_FEATURE_CLOCKSOURCE_STABLE_BIT fake #define
  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           |  27 +++++++---
 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      |  56 ++++++++++++++++-----
 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         |  19 +++++--
 tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
 19 files changed, 390 insertions(+), 32 deletions(-)
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c

-- 
1.7.11.7

[Qemu-devel] [RFC 01/12] kvm: add KVM_FEATURE_CLOCKSOURCE_STABLE_BIT fake #define

[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

Re: [Qemu-devel] [RFC 01/12] kvm: add KVM_FEATURE_CLOCKSOURCE_STABLE_BIT fake #define

[Igor Mammedov]

On Wed,  9 Jan 2013 16:53:41 -0200
Eduardo Habkost <ehabkost@redhat.com> 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
> 
> 
Perhaps "kvm: add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for TCG only
configurations" would be better

-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 01/12] kvm: add KVM_FEATURE_CLOCKSOURCE_STABLE_BIT fake #define

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 12:15:16AM +0100, Igor Mammedov wrote:
[...]
> > 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
<snip>
> >  
> Perhaps "kvm: add fake KVM_FEATURE_CLOCKSOURCE_STABLE_BIT for TCG only
> configurations" would be better

I'll change it in the next version. Thanks.

-- 
Eduardo

[Qemu-devel] [RFC 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
---
 target-i386/cpu.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e4dc370..57a22b7 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,
@@ -1343,13 +1337,15 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     unsigned int i;
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
-    FeatureWordArray plus_features = {
-        [FEAT_KVM] = kvm_default_features,
-    };
+    FeatureWordArray plus_features = { 0 };
     /* Features to be removed */
     FeatureWordArray minus_features = { 0 };
     uint32_t numvalue;
 
+    if (kvm_enabled()) {
+        plus_features[FEAT_KVM] = kvm_default_features;
+    }
+
     add_flagname_to_bitmaps("hypervisor", plus_features);
 
     featurestr = features ? strtok(features, ",") : NULL;
-- 
1.7.11.7

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

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 04:53:42PM -0200, Eduardo Habkost wrote:
> 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>

Acked-by: Michael S. Tsirkin <mst@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
> ---
>  target-i386/cpu.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e4dc370..57a22b7 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,
> @@ -1343,13 +1337,15 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      unsigned int i;
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
> -    FeatureWordArray plus_features = {
> -        [FEAT_KVM] = kvm_default_features,
> -    };
> +    FeatureWordArray plus_features = { 0 };
>      /* Features to be removed */
>      FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
>  
> +    if (kvm_enabled()) {
> +        plus_features[FEAT_KVM] = kvm_default_features;
> +    }
> +
>      add_flagname_to_bitmaps("hypervisor", plus_features);
>  
>      featurestr = features ? strtok(features, ",") : NULL;
> -- 
> 1.7.11.7

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

[Igor Mammedov]

On Wed,  9 Jan 2013 16:53:42 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> 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>
> 
[snip]
>  void host_cpuid(uint32_t function, uint32_t count,
> @@ -1343,13 +1337,15 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      unsigned int i;
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
> -    FeatureWordArray plus_features = {
> -        [FEAT_KVM] = kvm_default_features,
> -    };
> +    FeatureWordArray plus_features = { 0 };
>      /* Features to be removed */
>      FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
>  
> +    if (kvm_enabled()) {
> +        plus_features[FEAT_KVM] = kvm_default_features;
> +    }
While touching it please move setting defaults to cpu_x86_register() or
cpu_x86_find_by_name() to so that cpu_x86_parse_featurestr() would deal only
with custom settings.

[snip]

-- 
Regards,
  Igor

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 12:07:40AM +0100, Igor Mammedov wrote:
[...]
> > @@ -1343,13 +1337,15 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >      unsigned int i;
> >      char *featurestr; /* Single 'key=value" string being parsed */
> >      /* Features to be added */
> > -    FeatureWordArray plus_features = {
> > -        [FEAT_KVM] = kvm_default_features,
> > -    };
> > +    FeatureWordArray plus_features = { 0 };
> >      /* Features to be removed */
> >      FeatureWordArray minus_features = { 0 };
> >      uint32_t numvalue;
> >  
> > +    if (kvm_enabled()) {
> > +        plus_features[FEAT_KVM] = kvm_default_features;
> > +    }
> While touching it please move setting defaults to cpu_x86_register() or
> cpu_x86_find_by_name() to so that cpu_x86_parse_featurestr() would deal only
> with custom settings.

OK, I will do it in the next version.

-- 
Eduardo

[Qemu-devel] [RFC 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 compatibility code inside the pc-1.2
init function because it would enable 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 the pc-1.2 init function to enable compatibility behavior,
and the pc-1.4 init function would just use the default behavior.

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.

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>
---
 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 2b3d58b..13d7cc8 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_machine_v1_4 = {
     .name = "pc-1.4",
     .alias = "pc",
     .desc = "Standard PC",
-    .init = pc_init_pci_1_3,
+    .init = pc_init_pci,
     .max_cpus = 255,
     .is_default = 1,
 };
@@ -301,7 +305,7 @@ static QEMUMachine pc_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,
@@ -340,7 +344,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,
@@ -383,7 +387,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,
@@ -418,7 +422,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,
@@ -433,7 +437,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,
@@ -465,7 +469,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 57a22b7..492656c 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 e4a7c50..f3c9df5 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1233,7 +1233,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] [RFC 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 6e2164b..d37359f 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 e4a387f..bdcd133 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] [RFC 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] [RFC 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 26f7125..0f13437 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 df0c48e..beb816b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -550,7 +550,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 fabcc08..fdecfcf 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 fff5129..7406c10 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 0d84b37..316e94c 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);
@@ -1658,6 +1659,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);
@@ -1858,6 +1860,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 cbfd217..d114af3 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] [RFC 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 492656c..33787dc 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2192,6 +2192,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);
@@ -2226,7 +2241,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 f3c9df5..dbd9899 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1238,4 +1238,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

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

[Igor Mammedov]

On Wed,  9 Jan 2013 16:53:47 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> 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 492656c..33787dc 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2192,6 +2192,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);
> @@ -2226,7 +2241,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);
APIC ID is set by board so CPU shouldn't do it by itself.

Could you create static property for it and set it from board using property
instead?

>  
>      /* 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 f3c9df5..dbd9899 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1238,4 +1238,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
> 


-- 
Regards,
  Igor

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 12:31:23AM +0100, Igor Mammedov wrote:
> On Wed,  9 Jan 2013 16:53:47 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > 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 492656c..33787dc 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2192,6 +2192,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);
> > @@ -2226,7 +2241,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);
> APIC ID is set by board so CPU shouldn't do it by itself.
> 
> Could you create static property for it and set it from board using property
> instead?

I had a version that had the PC CPU creation code set the apic-id using
a apic-id property, but I am using this method to keep the bug fix
simpler and less intrusive, as I we don't have much time to remodel the
CPU creation code before soft freeze.

The previous version can be seen at:
  https://github.com/ehabkost/qemu-hacks/tree/apic-id-topology-using-apic-id-prop
The relevant apic-id-setting code is at:
  https://github.com/ehabkost/qemu-hacks/commit/a79156d0ad99ee89c0def535ce4cafcc4aa9a486

I believe the final solution for calculating the APIC IDs should instead
involve passing the package ID and bit width of core+thread IDs to a
package/socket object (so we don't need to expose the APIC IDs outside
QEMU). But whatever design we're going to choose to allow CPU hotplug, I
didn't want to introduce complexity that will get into our way when we
remodel the CPU creation/hotplug interfaces later.


> 
> >  
> >      /* 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 f3c9df5..dbd9899 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1238,4 +1238,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
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 12:31:23AM +0100, Igor Mammedov wrote:
> On Wed,  9 Jan 2013 16:53:47 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > 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 492656c..33787dc 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2192,6 +2192,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);
> > @@ -2226,7 +2241,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);
> APIC ID is set by board so CPU shouldn't do it by itself.
> 
> Could you create static property for it and set it from board using property
> instead?

I had a version that had the PC CPU creation code set the apic-id using
a apic-id property, but I am using this method to keep the bug fix
simpler and less intrusive, as I we don't have much time to remodel the
CPU creation code before soft freeze.

The previous version can be seen at:
  https://github.com/ehabkost/qemu-hacks/tree/apic-id-topology-using-apic-id-prop
The relevant apic-id-setting code is at:
  https://github.com/ehabkost/qemu-hacks/commit/a79156d0ad99ee89c0def535ce4cafcc4aa9a486

I believe the final solution for calculating the APIC IDs should instead
involve passing the package ID and bit width of core+thread IDs to a
package/socket object (so we don't need to expose the APIC IDs outside
QEMU). But whatever design we're going to choose to allow CPU hotplug, I
didn't want to introduce complexity that will get into our way when we
remodel the CPU creation/hotplug interfaces later.


> 
> >  
> >      /* 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 f3c9df5..dbd9899 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1238,4 +1238,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
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

[Qemu-devel] [RFC 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] [RFC 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 beb816b..38ae807 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -541,6 +541,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;
@@ -548,9 +560,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,
@@ -570,21 +597,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] [RFC 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 | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index b09a343..fa96d1a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -74,8 +74,6 @@ test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) qemu-tool.o
 test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
 test-qapi-obj-y += module.o
 
-$(test-obj-y): QEMU_INCLUDES += -Itests
-
 tests/check-qint$(EXESUF): tests/check-qint.o qint.o
 tests/check-qstring$(EXESUF): tests/check-qstring.o qstring.o
 tests/check-qdict$(EXESUF): tests/check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qlist.o
@@ -111,9 +109,20 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
 tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
 
-# 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] [RFC 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
---
 target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/.gitignore       |   1 +
 tests/Makefile         |   4 ++
 tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
 4 files changed, 239 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 fa96d1a..004b8df 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -46,6 +46,8 @@ 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)
+
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
 # All QTests for now are POSIX-only, but the dependencies are
@@ -69,6 +71,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 =  $(qobject-obj-y) $(qapi-obj-y) qemu-tool.o
 test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
@@ -84,6 +87,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools
 tests/test-aio$(EXESUF): tests/test-aio.o $(coroutine-obj-y) $(tools-obj-y) $(block-obj-y) libqemustub.a
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(coroutine-obj-y) $(tools-obj-y) $(block-obj-y) libqemustub.a
 tests/test-iov$(EXESUF): tests/test-iov.o iov.o
+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] [RFC 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      | 13 +++++++++++--
 target-i386/cpu.c | 28 ++++++++++++++++++++++++----
 target-i386/cpu.h |  1 +
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 13d7cc8..be4fea1 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -233,11 +233,18 @@ 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 +257,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 +276,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,
@@ -305,7 +314,7 @@ static QEMUMachine pc_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 33787dc..f34192c 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"
@@ -2192,6 +2194,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
@@ -2201,10 +2211,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 dbd9899..d9a9e8f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1239,5 +1239,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] [RFC 12/12] pc: Generate APIC IDs according to CPU topology

[Igor Mammedov]

On Wed,  9 Jan 2013 16:53:52 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> 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      | 13 +++++++++++--
>  target-i386/cpu.c | 28 ++++++++++++++++++++++++----
>  target-i386/cpu.h |  1 +
>  3 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 13d7cc8..be4fea1 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -233,11 +233,18 @@ 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 +257,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 +276,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,
> @@ -305,7 +314,7 @@ static QEMUMachine pc_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 33787dc..f34192c 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"
> @@ -2192,6 +2194,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
> @@ -2201,10 +2211,20 @@ void x86_cpu_realize(Object *obj, Error **errp)
>   */
>  uint32_t apic_id_for_cpu(unsigned int cpu_index)
if you move ^^^ to board, there won't be need in [8/12]
>  {
> -    /* 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 dbd9899..d9a9e8f 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1239,5 +1239,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Regards,
  Igor

Re: [Qemu-devel] [RFC 12/12] pc: Generate APIC IDs according to CPU topology

[Eduardo Habkost]

On Fri, Jan 11, 2013 at 12:36:35AM +0100, Igor Mammedov wrote:
[...]
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 33787dc..f34192c 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"
> > @@ -2192,6 +2194,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
> > @@ -2201,10 +2211,20 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >   */
> >  uint32_t apic_id_for_cpu(unsigned int cpu_index)
> if you move ^^^ to board, there won't be need in [8/12]

True. The previous version I had (apic-id-topology-using-apic-id-prop
branch) didn't need it. But I just wanted to avoid touching the CPU
creation code by now.

BTW, *-user doesn't have a board but does have an APIC ID (because
*-user has CPUID and CPUID exposes the APIC ID). This would mean we
would need APIC ID calculation code present in two separated places
(which I would like to avoid).


> >  {
> > -    /* 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 dbd9899..d9a9e8f 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1239,5 +1239,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
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Jan 09, 2013 at 04:53:40PM -0200, Eduardo Habkost wrote:
> This series uses a much simpler approach than the previous versions:
> 
>  - The APIC ID calculation code is now inside cpu.c
>  - It doesn't require touching the PC CPU creation code at all
>  - It simply uses a static variable to enable the compat behavior on pc-1.3
>    and older
>    - I considered making the compat-apic-id setting a global property for the
>      X86CPU objects, the only problem is that the fw_cfg initialization code
>      on pc.c also depends on the compat behavior
> 
> I am sending this as RFC because it depends on two not-included-yet series:
>  - Andreas' qom-cpu-7 branch
>  - My cpu-enforce fixes series
> 
> I hope to be able to get this buf fix into QEMU 1.4. I don't know if we should
> try to get this before soft freeze, or we can include it after that.
> 

Andreas, Anthony: I still would like to include this fix in 1.4. Do you
think it would be reasonable?

-- 
Eduardo