[Qemu-devel] [RFC 00/15] attempt to fix CPU topology info on CPU APIC IDs

[Qemu-devel] [RFC 00/15] attempt to fix CPU topology info on CPU APIC IDs

[Eduardo Habkost]

First, the bug description:

The CPU APIC IDs generated by QEMU are broken if the number of cores-per-socket
or threads-per-core are not powers of 2, as the bits on the APIC ID do not
correspond to what's expected to reflect the CPU sockets/cores/threads
topology[1].

[1] http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/


This is an attempt to fix it, but there were some obstacles on the way:

1) The NUMA fw_cfg interface requires the full topology information to be
available even for hotplug CPUs that don't exist yet, so we need to be able to
calculate the APIC ID solely from the CPU index.

2) CPU objects are not qdev objects (yet).

3) We don't have a "list CPUs" object (yet), so we don't have an object where
a compatibility global property could be set, and that would be responsible for
calculating the APIC IDs.

That said, the patches are organized as follows:

 - Patches 1 to 3 are just code movements or smaller fixes to prepare for the
   actual fix. They should be safe to be applied right now.
 - Patches 4 to 6 addresses issue #1 above.
 - Patches 7 to 12 change the global property handling code to make it at least
   possible to use a global property without requiring QOM or qdev (due to
   issues #2 and #3 above). I expect it to be controversial and I would like to
   get some feedback.
 - Patches 13 and 14 are the actual fix, that changes the APIC ID calculation to
   match the requested CPU topology, and use a "SMP.contiguous_apic_ids" global
   property to keep compatibility on pc-1.1 and older machine-types.

The fix depends on commit 008c1fc5bd4f1c545c38e07242ad676830ea7785 of SeaBIOS,
that allows CPU APIC IDs to be non-contiguous. However, older SeaBIOS verions
should still work, as long as cores/threads are powers of 2 or you use the
"pc-1.1" machine-type (or older).

I am aware of the coding style warnings from checkpatch.pl (mainly "line over 80
characters" warnings), and I plan to fix them in the final version of the fix.


Eduardo Habkost (15):
  cpus.h: include cpu-common.h
  hw/apic.c: rename bit functions to not conflict with bitops.h (v2)
  kvm: set vcpu_id to APIC ID instead of CPU index
  i386: create apic_id_for_cpu() function (v2)
  remove FW_CFG_MAX_CPUS from fw_cfg_init()
  pc: set FW_CFG data based on APIC ID calculation
  qdev: allow qdev_prop_parse() to report errors
  move global properties code to global-properties.c
  isolate qdev-independent parts of qdev_prop_set_globals()
  create object_prop_set_globals()
  rename qdev_prop_register_global_list to qemu_globals_register_list
  create qemu_global_get() function
  tests: support target-specific unit tests
  i386: topology & APIC ID utility functions (v2)
  generate APIC IDs according to CPU topology (v2)

 Makefile.objs          |   1 +
 cpus.h                 |   2 +
 global-properties.c    |  88 ++++++++++++++++++++++++++++++
 hw/apic.c              |  35 ++++++------
 hw/fw_cfg.c            |   1 -
 hw/pc.c                |  23 +++++---
 hw/pc_piix.c           |   4 ++
 hw/ppc_newworld.c      |   1 +
 hw/ppc_oldworld.c      |   1 +
 hw/qdev-monitor.c      |   6 ++-
 hw/qdev-properties.c   |  67 ++---------------------
 hw/qdev.h              |  18 +++++--
 hw/sun4m.c             |   3 ++
 hw/sun4u.c             |   1 +
 kvm-all.c              |   2 +-
 target-i386/cpu.c      |  40 +++++++++++++-
 target-i386/cpu.h      |  17 ++++++
 target-i386/topology.h | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/.gitignore       |   1 +
 tests/Makefile         |  19 +++++--
 tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++
 vl.c                   |   6 +--
 22 files changed, 488 insertions(+), 100 deletions(-)
 create mode 100644 global-properties.c
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c

-- 
1.7.11.2

[Qemu-devel] [RFC 01/15] cpus.h: include cpu-common.h

[Eduardo Habkost]

Needed for the definition of fprint_function.

This is not necessary right now, but it will be necessary if code that
doesn't include cpu-common.h includes cpus.h.

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

diff --git a/cpus.h b/cpus.h
index 81bd817..061ff7f 100644
--- a/cpus.h
+++ b/cpus.h
@@ -1,6 +1,8 @@
 #ifndef QEMU_CPUS_H
 #define QEMU_CPUS_H
 
+#include "qemu-common.h"
+
 /* cpus.c */
 void qemu_init_cpu_loop(void);
 void resume_all_vcpus(void);
-- 
1.7.11.2

Re: [Qemu-devel] [RFC 01/15] cpus.h: include cpu-common.h

[Igor Mammedov]

On 08/07/2012 09:56 PM, Eduardo Habkost wrote:
> Needed for the definition of fprint_function.
>
> This is not necessary right now, but it will be necessary if code that
> doesn't include cpu-common.h includes cpus.h.
could fprint_function declaration be moved somewhere else?
A lot of headers include cpu-common.h just for the sake of these 
simple/independent declarations and forward declared structures 
pointers. Maybe these trivial cases could be moved in separate
header qemu-common-trivial.h. It could replace cpu-common.h in most 
headers and would help to untangle circular deps between cpu-common.h 
and cpu.h and reduce complications when one tries to embed in cpu some 
Device/SysBusDevice as a child.

>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   cpus.h | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/cpus.h b/cpus.h
> index 81bd817..061ff7f 100644
> --- a/cpus.h
> +++ b/cpus.h
> @@ -1,6 +1,8 @@
>   #ifndef QEMU_CPUS_H
>   #define QEMU_CPUS_H
>
> +#include "qemu-common.h"
> +
>   /* cpus.c */
>   void qemu_init_cpu_loop(void);
>   void resume_all_vcpus(void);
>


-- 
Regards,
   Igor

[Qemu-devel] [RFC 02/15] hw/apic.c: rename bit functions to not conflict with bitops.h (v2)

[Eduardo Habkost]

Changes v1 -> v2:
 - Coding style change: break too-long line

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/apic.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 385555e..e1f633a 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -51,7 +51,7 @@ static int ffs_bit(uint32_t value)
     return ctz32(value);
 }
 
-static inline void set_bit(uint32_t *tab, int index)
+static inline void apic_set_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -59,7 +59,7 @@ static inline void set_bit(uint32_t *tab, int index)
     tab[i] |= mask;
 }
 
-static inline void reset_bit(uint32_t *tab, int index)
+static inline void apic_reset_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -67,7 +67,7 @@ static inline void reset_bit(uint32_t *tab, int index)
     tab[i] &= ~mask;
 }
 
-static inline int get_bit(uint32_t *tab, int index)
+static inline int apic_get_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -184,7 +184,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
         case APIC_DM_FIXED:
             if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
                 break;
-            reset_bit(s->irr, lvt & 0xff);
+            apic_reset_bit(s->irr, lvt & 0xff);
             /* fall through */
         case APIC_DM_EXTINT:
             cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
@@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
 
 static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
 {
-    apic_report_irq_delivered(!get_bit(s->irr, vector_num));
+    apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
 
-    set_bit(s->irr, vector_num);
+    apic_set_bit(s->irr, vector_num);
     if (trigger_mode)
-        set_bit(s->tmr, vector_num);
+        apic_set_bit(s->tmr, vector_num);
     else
-        reset_bit(s->tmr, vector_num);
+        apic_reset_bit(s->tmr, vector_num);
     if (s->vapic_paddr) {
         apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
         /*
@@ -405,8 +405,9 @@ static void apic_eoi(APICCommonState *s)
     isrv = get_highest_priority_int(s->isr);
     if (isrv < 0)
         return;
-    reset_bit(s->isr, isrv);
-    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
+    apic_reset_bit(s->isr, isrv);
+    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) &&
+            apic_get_bit(s->tmr, isrv)) {
         ioapic_eoi_broadcast(isrv);
     }
     apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
@@ -445,7 +446,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
             int idx = apic_find_dest(dest);
             memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
             if (idx >= 0)
-                set_bit(deliver_bitmask, idx);
+                apic_set_bit(deliver_bitmask, idx);
         }
     } else {
         /* XXX: cluster mode */
@@ -455,11 +456,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
             if (apic_iter) {
                 if (apic_iter->dest_mode == 0xf) {
                     if (dest & apic_iter->log_dest)
-                        set_bit(deliver_bitmask, i);
+                        apic_set_bit(deliver_bitmask, i);
                 } else if (apic_iter->dest_mode == 0x0) {
                     if ((dest & 0xf0) == (apic_iter->log_dest & 0xf0) &&
                         (dest & apic_iter->log_dest & 0x0f)) {
-                        set_bit(deliver_bitmask, i);
+                        apic_set_bit(deliver_bitmask, i);
                     }
                 }
             } else {
@@ -502,14 +503,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
         break;
     case 1:
         memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
-        set_bit(deliver_bitmask, s->idx);
+        apic_set_bit(deliver_bitmask, s->idx);
         break;
     case 2:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
         break;
     case 3:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
-        reset_bit(deliver_bitmask, s->idx);
+        apic_reset_bit(deliver_bitmask, s->idx);
         break;
     }
 
@@ -566,8 +567,8 @@ int apic_get_interrupt(DeviceState *d)
         apic_sync_vapic(s, SYNC_TO_VAPIC);
         return s->spurious_vec & 0xff;
     }
-    reset_bit(s->irr, intno);
-    set_bit(s->isr, intno);
+    apic_reset_bit(s->irr, intno);
+    apic_set_bit(s->isr, intno);
     apic_sync_vapic(s, SYNC_TO_VAPIC);
 
     /* re-inject if there is still a pending PIC interrupt */
-- 
1.7.11.2

Re: [Qemu-devel] [RFC 02/15] hw/apic.c: rename bit functions to not conflict with bitops.h (v2)

[Igor Mammedov]

On 08/07/2012 09:56 PM, Eduardo Habkost wrote:
> Changes v1 -> v2:
>   - Coding style change: break too-long line
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/apic.c | 35 ++++++++++++++++++-----------------
>   1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index 385555e..e1f633a 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -51,7 +51,7 @@ static int ffs_bit(uint32_t value)
>       return ctz32(value);
>   }
>
> -static inline void set_bit(uint32_t *tab, int index)
> +static inline void apic_set_bit(uint32_t *tab, int index)
>   {
>       int i, mask;
>       i = index >> 5;
> @@ -59,7 +59,7 @@ static inline void set_bit(uint32_t *tab, int index)
>       tab[i] |= mask;
>   }
>
> -static inline void reset_bit(uint32_t *tab, int index)
> +static inline void apic_reset_bit(uint32_t *tab, int index)
>   {
>       int i, mask;
>       i = index >> 5;
> @@ -67,7 +67,7 @@ static inline void reset_bit(uint32_t *tab, int index)
>       tab[i] &= ~mask;
>   }
>
> -static inline int get_bit(uint32_t *tab, int index)
> +static inline int apic_get_bit(uint32_t *tab, int index)
>   {
>       int i, mask;
>       i = index >> 5;
> @@ -184,7 +184,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
>           case APIC_DM_FIXED:
>               if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
>                   break;
> -            reset_bit(s->irr, lvt & 0xff);
> +            apic_reset_bit(s->irr, lvt & 0xff);
>               /* fall through */
>           case APIC_DM_EXTINT:
>               cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
> @@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
>
>   static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
>   {
> -    apic_report_irq_delivered(!get_bit(s->irr, vector_num));
> +    apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
>
> -    set_bit(s->irr, vector_num);
> +    apic_set_bit(s->irr, vector_num);
>       if (trigger_mode)
> -        set_bit(s->tmr, vector_num);
> +        apic_set_bit(s->tmr, vector_num);
>       else
> -        reset_bit(s->tmr, vector_num);
> +        apic_reset_bit(s->tmr, vector_num);
>       if (s->vapic_paddr) {
>           apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
>           /*
> @@ -405,8 +405,9 @@ static void apic_eoi(APICCommonState *s)
>       isrv = get_highest_priority_int(s->isr);
>       if (isrv < 0)
>           return;
> -    reset_bit(s->isr, isrv);
> -    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
> +    apic_reset_bit(s->isr, isrv);
> +    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) &&
> +            apic_get_bit(s->tmr, isrv)) {
>           ioapic_eoi_broadcast(isrv);
>       }
>       apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
> @@ -445,7 +446,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
>               int idx = apic_find_dest(dest);
>               memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
>               if (idx >= 0)
> -                set_bit(deliver_bitmask, idx);
> +                apic_set_bit(deliver_bitmask, idx);
>           }
>       } else {
>           /* XXX: cluster mode */
> @@ -455,11 +456,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
>               if (apic_iter) {
>                   if (apic_iter->dest_mode == 0xf) {
>                       if (dest & apic_iter->log_dest)
> -                        set_bit(deliver_bitmask, i);
> +                        apic_set_bit(deliver_bitmask, i);
>                   } else if (apic_iter->dest_mode == 0x0) {
>                       if ((dest & 0xf0) == (apic_iter->log_dest & 0xf0) &&
>                           (dest & apic_iter->log_dest & 0x0f)) {
> -                        set_bit(deliver_bitmask, i);
> +                        apic_set_bit(deliver_bitmask, i);
>                       }
>                   }
>               } else {
> @@ -502,14 +503,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
>           break;
>       case 1:
>           memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
> -        set_bit(deliver_bitmask, s->idx);
> +        apic_set_bit(deliver_bitmask, s->idx);
>           break;
>       case 2:
>           memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
>           break;
>       case 3:
>           memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
> -        reset_bit(deliver_bitmask, s->idx);
> +        apic_reset_bit(deliver_bitmask, s->idx);
>           break;
>       }
>
> @@ -566,8 +567,8 @@ int apic_get_interrupt(DeviceState *d)
>           apic_sync_vapic(s, SYNC_TO_VAPIC);
>           return s->spurious_vec & 0xff;
>       }
> -    reset_bit(s->irr, intno);
> -    set_bit(s->isr, intno);
> +    apic_reset_bit(s->irr, intno);
> +    apic_set_bit(s->isr, intno);
>       apic_sync_vapic(s, SYNC_TO_VAPIC);
>
>       /* re-inject if there is still a pending PIC interrupt */
>
Looks good to me.

-- 
Regards,
   Igor

[Qemu-devel] [RFC 03/15] 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. It didn't break anything yet because
today the APIC ID is assumed to be == the CPU index, but this won't be
true in the future.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 kvm-all.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kvm-all.c b/kvm-all.c
index 2148b20..38de992 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -213,7 +213,7 @@ int kvm_init_vcpu(CPUArchState *env)
 
     DPRINTF("kvm_init_vcpu\n");
 
-    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
+    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpuid_apic_id);
     if (ret < 0) {
         DPRINTF("kvm_create_vcpu failed\n");
         goto err;
-- 
1.7.11.2

Re: [Qemu-devel] [RFC 03/15] kvm: set vcpu_id to APIC ID instead of CPU index

[Igor Mammedov]

On 08/07/2012 09:56 PM, Eduardo Habkost wrote:
> The CPU ID in KVM is supposed to be the APIC ID, so change the
> KVM_CREATE_VCPU call to match it. It didn't break anything yet because
> today the APIC ID is assumed to be == the CPU index, but this won't be
> true in the future.
What it would break if APIC ID != CPU index ?

>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   kvm-all.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 2148b20..38de992 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -213,7 +213,7 @@ int kvm_init_vcpu(CPUArchState *env)
>
>       DPRINTF("kvm_init_vcpu\n");
>
> -    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
> +    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpuid_apic_id);
>       if (ret < 0) {
>           DPRINTF("kvm_create_vcpu failed\n");
>           goto err;
>


-- 
Regards,
   Igor

Re: [Qemu-devel] [RFC 03/15] kvm: set vcpu_id to APIC ID instead of CPU index

[Eduardo Habkost]

On Mon, Aug 13, 2012 at 09:16:52PM +0200, Igor Mammedov wrote:
> On 08/07/2012 09:56 PM, Eduardo Habkost wrote:
> >The CPU ID in KVM is supposed to be the APIC ID, so change the
> >KVM_CREATE_VCPU call to match it. It didn't break anything yet because
> >today the APIC ID is assumed to be == the CPU index, but this won't be
> >true in the future.
> What it would break if APIC ID != CPU index ?

It is not supposed to break anything, now that SeaBIOS was changed to
accept non-contiguous APIC IDs (and after applying patches 3-6 from this
series). If there is other code that expect APIC ID == CPU index, that
code also needs to be fixed.

> 
> >
> >Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >---
> >  kvm-all.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/kvm-all.c b/kvm-all.c
> >index 2148b20..38de992 100644
> >--- a/kvm-all.c
> >+++ b/kvm-all.c
> >@@ -213,7 +213,7 @@ int kvm_init_vcpu(CPUArchState *env)
> >
> >      DPRINTF("kvm_init_vcpu\n");
> >
> >-    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
> >+    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpuid_apic_id);
> >      if (ret < 0) {
> >          DPRINTF("kvm_create_vcpu failed\n");
> >          goto err;
> >
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

[Qemu-devel] [RFC 04/15] i386: create apic_id_for_cpu() function (v2)

[Eduardo Habkost]

Currently we need a way to calculate the Initial APIC ID using only the
CPU index (without needing a CPU object), as the NUMA fw_cfg data is
APIC-ID-based, and may include data for hotplug CPUs (that don't exist
yet), up to max_cpus.

Changes v1 -> v2:
  - make function return value 'unsigned int' (it's not specific for the 8-bit
    xAPIC ID)
  - move implementation to cpu.c

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 857b94e..1703373 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -24,6 +24,10 @@
 #include "cpu.h"
 #include "kvm.h"
 
+#ifndef CONFIG_USER_ONLY
+#include "sysemu.h"
+#endif
+
 #include "qemu-option.h"
 #include "qemu-config.h"
 
@@ -488,6 +492,14 @@ static x86_def_t builtin_x86_defs[] = {
     },
 };
 
+unsigned int apic_id_for_cpu(int cpu_index)
+{
+    /* right now APIC ID == CPU index. this will eventually change to use
+     * the CPU topology configuration properly
+     */
+    return cpu_index;
+}
+
 static int cpu_x86_fill_model_id(char *str)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
@@ -1774,7 +1786,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 = env->cpu_index;
+    env->cpuid_apic_id = apic_id_for_cpu(env->cpu_index);
 }
 
 static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 2a61c81..39ea005 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -910,6 +910,16 @@ void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
 
+
+/* 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, as the QEMU<->Seabios interfaces have no concept of "CPU index",
+ * and the NUMA tables need the APIC ID of all CPUs up to max_cpus.
+ */
+unsigned int apic_id_for_cpu(int cpu_index);
+
+
 /* helper.c */
 int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
                              int is_write, int mmu_idx);
-- 
1.7.11.2

[Qemu-devel] [RFC 05/15] 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 7b3b576..ce3da2e 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -494,7 +494,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);
 
diff --git a/hw/pc.c b/hw/pc.c
index 81c391c..10449bd 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -602,7 +602,7 @@ static void *bochs_bios_init(void)
     register_ioport_write(0x503, 1, 1, bochs_bios_write, NULL);
 
     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 4e2a6e6..13c597c 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -381,6 +381,7 @@ static void ppc_core99_init (ram_addr_t ram_size,
     /* 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 f2c6908..7b6af68 100644
--- a/hw/ppc_oldworld.c
+++ b/hw/ppc_oldworld.c
@@ -296,6 +296,7 @@ static void ppc_heathrow_init (ram_addr_t ram_size,
     /* 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 a959261..edaaeaa 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -1000,6 +1000,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);
@@ -1611,6 +1612,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);
@@ -1803,6 +1805,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 137a7c6..2291270 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -873,6 +873,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.2

[Qemu-devel] [RFC 06/15] 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>
---
 hw/pc.c           | 23 ++++++++++++++++-------
 target-i386/cpu.h |  7 +++++++
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 10449bd..9afb838 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -581,6 +581,11 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
+unsigned int apic_id_limit(void)
+{
+    return apic_id_for_cpu(max_cpus - 1) + 1;
+}
+
 static void *bochs_bios_init(void)
 {
     void *fw_cfg;
@@ -588,6 +593,7 @@ static void *bochs_bios_init(void)
     size_t smbios_len;
     uint64_t *numa_fw_cfg;
     int i, j;
+    unsigned int max_apic_id = apic_id_limit();
 
     register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
     register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
@@ -602,7 +608,7 @@ static void *bochs_bios_init(void)
     register_ioport_write(0x503, 1, 1, bochs_bios_write, NULL);
 
     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_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_apic_id);
     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,
@@ -622,21 +628,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 + max_apic_id + 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 < max_apic_id);
         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[max_apic_id + 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 + max_apic_id + nb_numa_nodes) * 8);
 
     return fw_cfg;
 }
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 39ea005..257d6c7 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -919,6 +919,13 @@ void host_cpuid(uint32_t function, uint32_t count,
  */
 unsigned int apic_id_for_cpu(int cpu_index);
 
+/* Calculate limit for the APIC ID value, based on max_cpus
+ *
+ * On PC, FW_CFG_MAX_CPUS is not max_cpus, but the limit for the APIC IDs
+ * of all CPUs (so that of all CPUs APIC ID < MAX_CPUS).
+ */
+unsigned int apic_id_limit(void);
+
 
 /* helper.c */
 int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
-- 
1.7.11.2

Re: [Qemu-devel] [RFC 06/15] pc: set FW_CFG data based on APIC ID calculation

[Igor Mammedov]

On 08/07/2012 09:56 PM, Eduardo Habkost wrote:
> 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>
> ---
>   hw/pc.c           | 23 ++++++++++++++++-------
>   target-i386/cpu.h |  7 +++++++
>   2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 10449bd..9afb838 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -581,6 +581,11 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>       return index;
>   }
>
> +unsigned int apic_id_limit(void)
> +{
> +    return apic_id_for_cpu(max_cpus - 1) + 1;
> +}
> +
>   static void *bochs_bios_init(void)
>   {
>       void *fw_cfg;
> @@ -588,6 +593,7 @@ static void *bochs_bios_init(void)
>       size_t smbios_len;
>       uint64_t *numa_fw_cfg;
>       int i, j;
> +    unsigned int max_apic_id = apic_id_limit();
>
>       register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
>       register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
> @@ -602,7 +608,7 @@ static void *bochs_bios_init(void)
>       register_ioport_write(0x503, 1, 1, bochs_bios_write, NULL);
>
>       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_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_apic_id);
FW_CFG_MAX_CPUS becoming not MAX_CPUS sounds a bit confusing, perhaps 
short comment should be here to document this and why it's not? So code 
reader won't make false assumptions?

>       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,
> @@ -622,21 +628,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 + max_apic_id + 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 < max_apic_id);
>           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[max_apic_id + 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 + max_apic_id + nb_numa_nodes) * 8);
>
>       return fw_cfg;
>   }
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 39ea005..257d6c7 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -919,6 +919,13 @@ void host_cpuid(uint32_t function, uint32_t count,
>    */
>   unsigned int apic_id_for_cpu(int cpu_index);
>
> +/* Calculate limit for the APIC ID value, based on max_cpus
> + *
> + * On PC, FW_CFG_MAX_CPUS is not max_cpus, but the limit for the APIC IDs
> + * of all CPUs (so that of all CPUs APIC ID < MAX_CPUS).
> + */
> +unsigned int apic_id_limit(void);
> +
>
>   /* helper.c */
>   int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
>


-- 
Regards,
   Igor

Re: [Qemu-devel] [RFC 06/15] pc: set FW_CFG data based on APIC ID calculation

[Eduardo Habkost]

On Mon, Aug 13, 2012 at 09:52:43PM +0200, Igor Mammedov wrote:
> On 08/07/2012 09:56 PM, Eduardo Habkost wrote:
> >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>
> >---
> >  hw/pc.c           | 23 ++++++++++++++++-------
> >  target-i386/cpu.h |  7 +++++++
> >  2 files changed, 23 insertions(+), 7 deletions(-)
> >
> >diff --git a/hw/pc.c b/hw/pc.c
> >index 10449bd..9afb838 100644
> >--- a/hw/pc.c
> >+++ b/hw/pc.c
> >@@ -581,6 +581,11 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> >      return index;
> >  }
> >
> >+unsigned int apic_id_limit(void)
> >+{
> >+    return apic_id_for_cpu(max_cpus - 1) + 1;
> >+}
> >+
> >  static void *bochs_bios_init(void)
> >  {
> >      void *fw_cfg;
> >@@ -588,6 +593,7 @@ static void *bochs_bios_init(void)
> >      size_t smbios_len;
> >      uint64_t *numa_fw_cfg;
> >      int i, j;
> >+    unsigned int max_apic_id = apic_id_limit();
> >
> >      register_ioport_write(0x400, 1, 2, bochs_bios_write, NULL);
> >      register_ioport_write(0x401, 1, 2, bochs_bios_write, NULL);
> >@@ -602,7 +608,7 @@ static void *bochs_bios_init(void)
> >      register_ioport_write(0x503, 1, 1, bochs_bios_write, NULL);
> >
> >      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_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_apic_id);
> FW_CFG_MAX_CPUS becoming not MAX_CPUS sounds a bit confusing, perhaps
> short comment should be here to document this and why it's not? So
> code reader won't make false assumptions?

True. I tried to quickly explain the problem in the commit message, but
it sounds confusing enough to deserve a comment in the code.

(It would be even better if we had an official place to document _all_
the fw_cfg fields).

> 
> >      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,
> >@@ -622,21 +628,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 + max_apic_id + 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 < max_apic_id);
> >          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[max_apic_id + 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 + max_apic_id + nb_numa_nodes) * 8);
> >
> >      return fw_cfg;
> >  }
> >diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> >index 39ea005..257d6c7 100644
> >--- a/target-i386/cpu.h
> >+++ b/target-i386/cpu.h
> >@@ -919,6 +919,13 @@ void host_cpuid(uint32_t function, uint32_t count,
> >   */
> >  unsigned int apic_id_for_cpu(int cpu_index);
> >
> >+/* Calculate limit for the APIC ID value, based on max_cpus
> >+ *
> >+ * On PC, FW_CFG_MAX_CPUS is not max_cpus, but the limit for the APIC IDs
> >+ * of all CPUs (so that of all CPUs APIC ID < MAX_CPUS).
> >+ */
> >+unsigned int apic_id_limit(void);
> >+
> >
> >  /* helper.c */
> >  int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
> >
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

[Qemu-devel] [RFC 07/15] qdev: allow qdev_prop_parse() to report errors

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/qdev-monitor.c    |  6 +++++-
 hw/qdev-properties.c | 21 ++++++++-------------
 hw/qdev.h            |  3 ++-
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index b22a37a..99784c1 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -101,13 +101,17 @@ static void qdev_print_devinfo(ObjectClass *klass, void *opaque)
 static int set_property(const char *name, const char *value, void *opaque)
 {
     DeviceState *dev = opaque;
+    Error *err = NULL;
 
     if (strcmp(name, "driver") == 0)
         return 0;
     if (strcmp(name, "bus") == 0)
         return 0;
 
-    if (qdev_prop_parse(dev, name, value) == -1) {
+    qdev_prop_parse(dev, name, value, &err);
+    if (err) {
+        qerror_report_err(err);
+        error_free(err);
         return -1;
     }
     return 0;
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 8aca0d4..a52884f 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1087,25 +1087,17 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
     }
 }
 
-int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
+void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
+                     Error **errp)
 {
     char *legacy_name;
-    Error *err = NULL;
-
     legacy_name = g_strdup_printf("legacy-%s", name);
     if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
-        object_property_parse(OBJECT(dev), value, legacy_name, &err);
+        object_property_parse(OBJECT(dev), value, legacy_name, errp);
     } else {
-        object_property_parse(OBJECT(dev), value, name, &err);
+        object_property_parse(OBJECT(dev), value, name, errp);
     }
     g_free(legacy_name);
-
-    if (err) {
-        qerror_report_err(err);
-        error_free(err);
-        return -1;
-    }
-    return 0;
 }
 
 void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
@@ -1250,11 +1242,14 @@ void qdev_prop_set_globals(DeviceState *dev)
 
     do {
         GlobalProperty *prop;
+        Error *err = NULL;
         QTAILQ_FOREACH(prop, &global_props, next) {
             if (strcmp(object_class_get_name(class), prop->driver) != 0) {
                 continue;
             }
-            if (qdev_prop_parse(dev, prop->property, prop->value) != 0) {
+            qdev_prop_parse(dev, prop->property, prop->value, &err);
+            if (err) {
+                qerror_report_err(err);
                 exit(1);
             }
         }
diff --git a/hw/qdev.h b/hw/qdev.h
index d699194..41db6c0 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -311,7 +311,8 @@ extern PropertyInfo qdev_prop_pci_host_devaddr;
 
 /* Set properties between creation and init.  */
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
-int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
+void qdev_prop_parse(DeviceState *dev, const char *name, const char *value,
+                     Error **errp);
 void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value);
 void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value);
 void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value);
-- 
1.7.11.2

[Qemu-devel] [RFC 08/15] move global properties code to global-properties.c

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 Makefile.objs        |  1 +
 global-properties.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev-properties.c | 54 --------------------------------------------------
 3 files changed, 57 insertions(+), 54 deletions(-)
 create mode 100644 global-properties.c

diff --git a/Makefile.objs b/Makefile.objs
index 5ebbcfa..5cd4082 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -77,6 +77,7 @@ common-obj-y += qemu-char.o #aio.o
 common-obj-y += block-migration.o iohandler.o
 common-obj-y += pflib.o
 common-obj-y += bitmap.o bitops.o
+common-obj-y += global-properties.o
 
 common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
 common-obj-$(CONFIG_WIN32) += version.o
diff --git a/global-properties.c b/global-properties.c
new file mode 100644
index 0000000..a1c3581
--- /dev/null
+++ b/global-properties.c
@@ -0,0 +1,56 @@
+#include "hw/qdev.h"
+#include "qerror.h"
+
+static QTAILQ_HEAD(, GlobalProperty) global_props = QTAILQ_HEAD_INITIALIZER(global_props);
+
+static void qdev_prop_register_global(GlobalProperty *prop)
+{
+    QTAILQ_INSERT_TAIL(&global_props, prop, next);
+}
+
+void qdev_prop_register_global_list(GlobalProperty *props)
+{
+    int i;
+
+    for (i = 0; props[i].driver != NULL; i++) {
+        qdev_prop_register_global(props+i);
+    }
+}
+
+void qdev_prop_set_globals(DeviceState *dev)
+{
+    ObjectClass *class = object_get_class(OBJECT(dev));
+
+    do {
+        GlobalProperty *prop;
+        Error *err = NULL;
+        QTAILQ_FOREACH(prop, &global_props, next) {
+            if (strcmp(object_class_get_name(class), prop->driver) != 0) {
+                continue;
+            }
+            qdev_prop_parse(dev, prop->property, prop->value, &err);
+            if (err) {
+                qerror_report_err(err);
+                exit(1);
+            }
+        }
+        class = object_class_get_parent(class);
+    } while (class);
+}
+
+static int qdev_add_one_global(QemuOpts *opts, void *opaque)
+{
+    GlobalProperty *g;
+
+    g = g_malloc0(sizeof(*g));
+    g->driver   = qemu_opt_get(opts, "driver");
+    g->property = qemu_opt_get(opts, "property");
+    g->value    = qemu_opt_get(opts, "value");
+    qdev_prop_register_global(g);
+    return 0;
+}
+
+void qemu_add_globals(void)
+{
+    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
+}
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index a52884f..3217490 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1219,57 +1219,3 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
     ptr = qdev_get_prop_ptr(dev, prop);
     *ptr = value;
 }
-
-static QTAILQ_HEAD(, GlobalProperty) global_props = QTAILQ_HEAD_INITIALIZER(global_props);
-
-static void qdev_prop_register_global(GlobalProperty *prop)
-{
-    QTAILQ_INSERT_TAIL(&global_props, prop, next);
-}
-
-void qdev_prop_register_global_list(GlobalProperty *props)
-{
-    int i;
-
-    for (i = 0; props[i].driver != NULL; i++) {
-        qdev_prop_register_global(props+i);
-    }
-}
-
-void qdev_prop_set_globals(DeviceState *dev)
-{
-    ObjectClass *class = object_get_class(OBJECT(dev));
-
-    do {
-        GlobalProperty *prop;
-        Error *err = NULL;
-        QTAILQ_FOREACH(prop, &global_props, next) {
-            if (strcmp(object_class_get_name(class), prop->driver) != 0) {
-                continue;
-            }
-            qdev_prop_parse(dev, prop->property, prop->value, &err);
-            if (err) {
-                qerror_report_err(err);
-                exit(1);
-            }
-        }
-        class = object_class_get_parent(class);
-    } while (class);
-}
-
-static int qdev_add_one_global(QemuOpts *opts, void *opaque)
-{
-    GlobalProperty *g;
-
-    g = g_malloc0(sizeof(*g));
-    g->driver   = qemu_opt_get(opts, "driver");
-    g->property = qemu_opt_get(opts, "property");
-    g->value    = qemu_opt_get(opts, "value");
-    qdev_prop_register_global(g);
-    return 0;
-}
-
-void qemu_add_globals(void)
-{
-    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
-}
-- 
1.7.11.2

[Qemu-devel] [RFC 09/15] isolate qdev-independent parts of qdev_prop_set_globals()

[Eduardo Habkost]

Create a qdev-independent function, and use a callback that calls
qdev_prop_parse().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 global-properties.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/global-properties.c b/global-properties.c
index a1c3581..d99bcee 100644
--- a/global-properties.c
+++ b/global-properties.c
@@ -17,9 +17,20 @@ void qdev_prop_register_global_list(GlobalProperty *props)
     }
 }
 
-void qdev_prop_set_globals(DeviceState *dev)
+static void qdev_global_parse(Object *obj, const char *property,
+                              const char *value, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    qdev_prop_parse(dev, property, value, errp);
+}
+
+static void object_set_globals(Object *obj,
+                               void (*parse_fn)(Object *obj,
+                                                const char *property,
+                                                const char *value,
+                                                Error **errp))
 {
-    ObjectClass *class = object_get_class(OBJECT(dev));
+    ObjectClass *class = object_get_class(obj);
 
     do {
         GlobalProperty *prop;
@@ -28,7 +39,7 @@ void qdev_prop_set_globals(DeviceState *dev)
             if (strcmp(object_class_get_name(class), prop->driver) != 0) {
                 continue;
             }
-            qdev_prop_parse(dev, prop->property, prop->value, &err);
+            parse_fn(obj, prop->property, prop->value, &err);
             if (err) {
                 qerror_report_err(err);
                 exit(1);
@@ -38,6 +49,11 @@ void qdev_prop_set_globals(DeviceState *dev)
     } while (class);
 }
 
+void qdev_prop_set_globals(DeviceState *dev)
+{
+    return object_set_globals(OBJECT(dev), qdev_global_parse);
+}
+
 static int qdev_add_one_global(QemuOpts *opts, void *opaque)
 {
     GlobalProperty *g;
-- 
1.7.11.2

[Qemu-devel] [RFC 10/15] create object_prop_set_globals()

[Eduardo Habkost]

It's like qdev_prop_set_globals(), but calls object_property_parse().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 global-properties.c | 5 +++++
 hw/qdev.h           | 1 +
 2 files changed, 6 insertions(+)

diff --git a/global-properties.c b/global-properties.c
index d99bcee..d827955 100644
--- a/global-properties.c
+++ b/global-properties.c
@@ -54,6 +54,11 @@ void qdev_prop_set_globals(DeviceState *dev)
     return object_set_globals(OBJECT(dev), qdev_global_parse);
 }
 
+void object_prop_set_globals(Object *obj)
+{
+    return object_set_globals(obj, object_property_parse);
+}
+
 static int qdev_add_one_global(QemuOpts *opts, void *opaque)
 {
     GlobalProperty *g;
diff --git a/hw/qdev.h b/hw/qdev.h
index 41db6c0..b522f11 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -331,6 +331,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev);
+void object_prop_set_globals(Object *obj);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
-- 
1.7.11.2

[Qemu-devel] [RFC 11/15] rename qdev_prop_register_global_list to qemu_globals_register_list

[Eduardo Habkost]

The function is not qdev-specific, it just keeps a global
driver/property/value list.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 global-properties.c | 2 +-
 hw/qdev.h           | 8 +++++---
 vl.c                | 6 +++---
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/global-properties.c b/global-properties.c
index d827955..20a838c 100644
--- a/global-properties.c
+++ b/global-properties.c
@@ -8,7 +8,7 @@ static void qdev_prop_register_global(GlobalProperty *prop)
     QTAILQ_INSERT_TAIL(&global_props, prop, next);
 }
 
-void qdev_prop_register_global_list(GlobalProperty *props)
+void qemu_globals_register_list(GlobalProperty *props)
 {
     int i;
 
diff --git a/hw/qdev.h b/hw/qdev.h
index b522f11..5941cae 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -329,12 +329,14 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
 /* FIXME: Remove opaque pointer properties.  */
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
-void qdev_prop_register_global_list(GlobalProperty *props);
-void qdev_prop_set_globals(DeviceState *dev);
-void object_prop_set_globals(Object *obj);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
                                     Property *prop, const char *value);
 
+/* Handling of global properties: */
+void qemu_globals_register_list(GlobalProperty *props);
+void qdev_prop_set_globals(DeviceState *dev);
+void object_prop_set_globals(Object *obj);
+
 char *qdev_get_fw_dev_path(DeviceState *dev);
 
 /**
diff --git a/vl.c b/vl.c
index e71cb30..d8e409c 100644
--- a/vl.c
+++ b/vl.c
@@ -559,7 +559,7 @@ static void configure_rtc(QemuOpts *opts)
                 { /* end of list */ }
             };
 
-            qdev_prop_register_global_list(slew_lost_ticks);
+            qemu_globals_register_list(slew_lost_ticks);
         } else if (!strcmp(value, "none")) {
             /* discard is default */
         } else {
@@ -2951,7 +2951,7 @@ int main(int argc, char **argv, char **envp)
                     { /* end of list */ }
                 };
 
-                qdev_prop_register_global_list(slew_lost_ticks);
+                qemu_globals_register_list(slew_lost_ticks);
                 break;
             }
             case QEMU_OPTION_acpitable:
@@ -3510,7 +3510,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (machine->compat_props) {
-        qdev_prop_register_global_list(machine->compat_props);
+        qemu_globals_register_list(machine->compat_props);
     }
     qemu_add_globals();
 
-- 
1.7.11.2

[Qemu-devel] [RFC 12/15] create qemu_global_get() function

[Eduardo Habkost]

Useful for cases where code is not converted to QOM and/or QDEV yet, but
needs to check the value of a global property.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 global-properties.c | 11 +++++++++++
 hw/qdev.h           |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/global-properties.c b/global-properties.c
index 20a838c..0c3d1b6 100644
--- a/global-properties.c
+++ b/global-properties.c
@@ -49,6 +49,17 @@ static void object_set_globals(Object *obj,
     } while (class);
 }
 
+const char *qemu_global_get(const char *driver, const char *property)
+{
+    GlobalProperty *prop;
+    QTAILQ_FOREACH(prop, &global_props, next) {
+        if (!strcmp(prop->driver, driver) && !strcmp(prop->property, property)) {
+            return prop->value;
+        }
+    }
+    return NULL;
+}
+
 void qdev_prop_set_globals(DeviceState *dev)
 {
     return object_set_globals(OBJECT(dev), qdev_global_parse);
diff --git a/hw/qdev.h b/hw/qdev.h
index 5941cae..d3210d8 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -337,6 +337,14 @@ void qemu_globals_register_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev);
 void object_prop_set_globals(Object *obj);
 
+/* Get the string value of a global property directly
+ *
+ * Using this function is discouraged. Code should be converted to use
+ * QOM and/or qdev instead of using it. It is provided only as a convenience
+ * for code that is not converted yet.
+ */
+const char *qemu_global_get(const char *driver, const char *property);
+
 char *qdev_get_fw_dev_path(DeviceState *dev);
 
 /**
-- 
1.7.11.2

[Qemu-devel] [RFC 13/15] 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 | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index f3f4159..79796aa 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -40,8 +40,6 @@ test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
 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 $(tools-obj-y)
 tests/check-qstring$(EXESUF): tests/check-qstring.o qstring.o $(tools-obj-y)
 tests/check-qdict$(EXESUF): tests/check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qlist.o $(tools-obj-y)
@@ -75,9 +73,21 @@ 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.2

[Qemu-devel] [RFC 14/15] i386: topology & APIC ID utility functions (v2)

[Eduardo Habkost]

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

Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/topology.h | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/.gitignore       |   1 +
 tests/Makefile         |   3 ++
 tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++
 4 files changed, 256 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..35d9817
--- /dev/null
+++ b/target-i386/topology.h
@@ -0,0 +1,144 @@
+/*
+ *  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
+
+#include <stdint.h>
+#include <string.h>
+
+#include "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 bits_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 bits_for_count(nr_threads);
+}
+
+/* Bit width of the Core_ID field
+ */
+static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
+{
+    return bits_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 (continguous) 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;
+}
+
+/* Get package ID from an APIC ID
+ */
+static inline unsigned apicid_pkg_id(unsigned nr_cores, unsigned nr_threads,
+                                     apic_id_t apic_id)
+{
+    return apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
+}
+
+/* Get core ID from an APIC ID
+ */
+static inline unsigned apicid_core_id(unsigned nr_cores, unsigned nr_threads,
+                                      apic_id_t apic_id)
+{
+    return (apic_id >> apicid_core_offset(nr_cores, nr_threads)) & \
+           ((1 << apicid_core_width(nr_cores, nr_threads)) - 1);
+}
+
+/* Get SMT (thread) ID from an APIC ID
+ */
+static inline unsigned apicid_smt_id(unsigned nr_cores, unsigned nr_threads,
+                                     apic_id_t apic_id)
+{
+    return apic_id & ((1 << apicid_smt_width(nr_cores, nr_threads)) - 1);
+}
+
+/* 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 79796aa..e74efdd 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
 check-unit-y += tests/test-coroutine$(EXESUF)
 check-unit-y += tests/test-visitor-serialization$(EXESUF)
 check-unit-y += tests/test-iov$(EXESUF)
+check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -35,6 +36,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) $(tools-obj-y)
 test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
@@ -48,6 +50,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
 tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
 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..d782c9c
--- /dev/null
+++ b/tests/test-x86-cpuid.c
@@ -0,0 +1,108 @@
+/*
+ *  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>
+
+/*FIXME: this should be built inside the i386 target directory instead */
+#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);
+
+
+    /* Check the APIC ID -> {pkg,core,thread} ID functions */
+    g_assert_cmpuint( apicid_pkg_id(6, 3, (3 << 5) | (5 << 2) | 2), ==, 3);
+    g_assert_cmpuint(apicid_core_id(6, 3, (3 << 5) | (5 << 2) | 2), ==, 5);
+    g_assert_cmpuint( apicid_smt_id(6, 3, (3 << 5) | (5 << 2) | 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.2

Re: [Qemu-devel] [RFC 14/15] i386: topology & APIC ID utility functions (v2)

[Blue Swirl]

On Tue, Aug 7, 2012 at 7:56 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> 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
>
> Cc: Blue Swirl <blauwirbel@gmail.com>

Looks OK.

Would it make sense to add a quick fuzzing test (call the functions
with changing random parameters a large number of times, see other
tests) later?

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/topology.h | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |   3 ++
>  tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 256 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..35d9817
> --- /dev/null
> +++ b/target-i386/topology.h
> @@ -0,0 +1,144 @@
> +/*
> + *  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
> +
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "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 bits_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 bits_for_count(nr_threads);
> +}
> +
> +/* Bit width of the Core_ID field
> + */
> +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
> +{
> +    return bits_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 (continguous) 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;
> +}
> +
> +/* Get package ID from an APIC ID
> + */
> +static inline unsigned apicid_pkg_id(unsigned nr_cores, unsigned nr_threads,
> +                                     apic_id_t apic_id)
> +{
> +    return apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
> +}
> +
> +/* Get core ID from an APIC ID
> + */
> +static inline unsigned apicid_core_id(unsigned nr_cores, unsigned nr_threads,
> +                                      apic_id_t apic_id)
> +{
> +    return (apic_id >> apicid_core_offset(nr_cores, nr_threads)) & \
> +           ((1 << apicid_core_width(nr_cores, nr_threads)) - 1);
> +}
> +
> +/* Get SMT (thread) ID from an APIC ID
> + */
> +static inline unsigned apicid_smt_id(unsigned nr_cores, unsigned nr_threads,
> +                                     apic_id_t apic_id)
> +{
> +    return apic_id & ((1 << apicid_smt_width(nr_cores, nr_threads)) - 1);
> +}
> +
> +/* 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 79796aa..e74efdd 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
>  check-unit-y += tests/test-coroutine$(EXESUF)
>  check-unit-y += tests/test-visitor-serialization$(EXESUF)
>  check-unit-y += tests/test-iov$(EXESUF)
> +check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
>
>  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>
> @@ -35,6 +36,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) $(tools-obj-y)
>  test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
> @@ -48,6 +50,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
>  tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
>  tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
>  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..d782c9c
> --- /dev/null
> +++ b/tests/test-x86-cpuid.c
> @@ -0,0 +1,108 @@
> +/*
> + *  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>
> +
> +/*FIXME: this should be built inside the i386 target directory instead */
> +#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);
> +
> +
> +    /* Check the APIC ID -> {pkg,core,thread} ID functions */
> +    g_assert_cmpuint( apicid_pkg_id(6, 3, (3 << 5) | (5 << 2) | 2), ==, 3);
> +    g_assert_cmpuint(apicid_core_id(6, 3, (3 << 5) | (5 << 2) | 2), ==, 5);
> +    g_assert_cmpuint( apicid_smt_id(6, 3, (3 << 5) | (5 << 2) | 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.2
>

Re: [Qemu-devel] [RFC 14/15] i386: topology & APIC ID utility functions (v2)

[Eduardo Habkost]

On Wed, Aug 08, 2012 at 06:57:30PM +0000, Blue Swirl wrote:
> On Tue, Aug 7, 2012 at 7:56 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 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
> >
> > Cc: Blue Swirl <blauwirbel@gmail.com>
> 
> Looks OK.
> 
> Would it make sense to add a quick fuzzing test (call the functions
> with changing random parameters a large number of times, see other
> tests) later?

I think the functions are simple enough to not need that. But a fuzzing
black-box qtest test with lots of weird topology and CPU feature
configurations would be interesting.

> 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/topology.h | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/.gitignore       |   1 +
> >  tests/Makefile         |   3 ++
> >  tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 256 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..35d9817
> > --- /dev/null
> > +++ b/target-i386/topology.h
> > @@ -0,0 +1,144 @@
> > +/*
> > + *  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
> > +
> > +#include <stdint.h>
> > +#include <string.h>
> > +
> > +#include "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 bits_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 bits_for_count(nr_threads);
> > +}
> > +
> > +/* Bit width of the Core_ID field
> > + */
> > +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
> > +{
> > +    return bits_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 (continguous) 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;
> > +}
> > +
> > +/* Get package ID from an APIC ID
> > + */
> > +static inline unsigned apicid_pkg_id(unsigned nr_cores, unsigned nr_threads,
> > +                                     apic_id_t apic_id)
> > +{
> > +    return apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
> > +}
> > +
> > +/* Get core ID from an APIC ID
> > + */
> > +static inline unsigned apicid_core_id(unsigned nr_cores, unsigned nr_threads,
> > +                                      apic_id_t apic_id)
> > +{
> > +    return (apic_id >> apicid_core_offset(nr_cores, nr_threads)) & \
> > +           ((1 << apicid_core_width(nr_cores, nr_threads)) - 1);
> > +}
> > +
> > +/* Get SMT (thread) ID from an APIC ID
> > + */
> > +static inline unsigned apicid_smt_id(unsigned nr_cores, unsigned nr_threads,
> > +                                     apic_id_t apic_id)
> > +{
> > +    return apic_id & ((1 << apicid_smt_width(nr_cores, nr_threads)) - 1);
> > +}
> > +
> > +/* 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 79796aa..e74efdd 100644
> > --- a/tests/Makefile
> > +++ b/tests/Makefile
> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
> >  check-unit-y += tests/test-coroutine$(EXESUF)
> >  check-unit-y += tests/test-visitor-serialization$(EXESUF)
> >  check-unit-y += tests/test-iov$(EXESUF)
> > +check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
> >
> >  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
> >
> > @@ -35,6 +36,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) $(tools-obj-y)
> >  test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
> > @@ -48,6 +50,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
> >  tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
> >  tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
> >  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..d782c9c
> > --- /dev/null
> > +++ b/tests/test-x86-cpuid.c
> > @@ -0,0 +1,108 @@
> > +/*
> > + *  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>
> > +
> > +/*FIXME: this should be built inside the i386 target directory instead */
> > +#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);
> > +
> > +
> > +    /* Check the APIC ID -> {pkg,core,thread} ID functions */
> > +    g_assert_cmpuint( apicid_pkg_id(6, 3, (3 << 5) | (5 << 2) | 2), ==, 3);
> > +    g_assert_cmpuint(apicid_core_id(6, 3, (3 << 5) | (5 << 2) | 2), ==, 5);
> > +    g_assert_cmpuint( apicid_smt_id(6, 3, (3 << 5) | (5 << 2) | 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.2
> >

-- 
Eduardo

Re: [Qemu-devel] [RFC 14/15] i386: topology & APIC ID utility functions (v2)

[Igor Mammedov]

On Tue,  7 Aug 2012 16:56:52 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> 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

adding to commit description ref to intel(& maybe amd) spec or/and short
description how apicid is structured/constructed would be nice

> Cc: Blue Swirl <blauwirbel@gmail.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/topology.h | 144 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/.gitignore       |   1 +
>  tests/Makefile         |   3 ++
>  tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 256 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..35d9817
> --- /dev/null
> +++ b/target-i386/topology.h
> @@ -0,0 +1,144 @@
> +/*
> + *  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
> +
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include "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 bits_for_count(unsigned count)
bitwidth_for_count(...) perhaps would be clearer?
and you could drop apicid_*_width/apicid_*_offset () funcs and use
it directly in topo_make_apicid() to make code more concise

> +{
> +    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 bits_for_count(nr_threads);
> +}
> +
> +/* Bit width of the Core_ID field
> + */
> +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
> +{
> +    return bits_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;
> +}
Looks like it considers only Intel spec, would it be correct for AMD case?
see support.amd.com/us/Embedded_TechDocs/25481.pdf "3.2 Extended Method"

AMD doesn't have SMT, perhaps nr_threads should be enforced to 1 when VCPU is
AMD?

> +
> +/* Calculate thread/core/package IDs for a specific topology,
> + * based on (continguous) 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;
> +}
> +
> +/* Get package ID from an APIC ID
> + */
> +static inline unsigned apicid_pkg_id(unsigned nr_cores, unsigned nr_threads,
> +                                     apic_id_t apic_id)
> +{
> +    return apic_id >> apicid_pkg_offset(nr_cores, nr_threads);
> +}
> +
> +/* Get core ID from an APIC ID
> + */
> +static inline unsigned apicid_core_id(unsigned nr_cores, unsigned nr_threads,
> +                                      apic_id_t apic_id)
> +{
> +    return (apic_id >> apicid_core_offset(nr_cores, nr_threads)) & \
> +           ((1 << apicid_core_width(nr_cores, nr_threads)) - 1);
> +}
> +
> +/* Get SMT (thread) ID from an APIC ID
> + */
> +static inline unsigned apicid_smt_id(unsigned nr_cores, unsigned nr_threads,
> +                                     apic_id_t apic_id)
> +{
> +    return apic_id & ((1 << apicid_smt_width(nr_cores, nr_threads)) - 1);
> +}
Are above getters necessary for fix to work or are there plans to use them?

> +
> +/* 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 79796aa..e74efdd 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
>  check-unit-y += tests/test-coroutine$(EXESUF)
>  check-unit-y += tests/test-visitor-serialization$(EXESUF)
>  check-unit-y += tests/test-iov$(EXESUF)
> +check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
>  
>  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>  
> @@ -35,6 +36,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) $(tools-obj-y)
>  test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
> @@ -48,6 +50,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
>  tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
>  tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
>  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..d782c9c
> --- /dev/null
> +++ b/tests/test-x86-cpuid.c
> @@ -0,0 +1,108 @@
> +/*
> + *  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>
> +
> +/*FIXME: this should be built inside the i386 target directory instead */
> +#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);
> +
> +
> +    /* Check the APIC ID -> {pkg,core,thread} ID functions */
> +    g_assert_cmpuint( apicid_pkg_id(6, 3, (3 << 5) | (5 << 2) | 2), ==, 3);
> +    g_assert_cmpuint(apicid_core_id(6, 3, (3 << 5) | (5 << 2) | 2), ==, 5);
> +    g_assert_cmpuint( apicid_smt_id(6, 3, (3 << 5) | (5 << 2) | 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.2
> 
> 


-- 
Regards,
  Igor

[Qemu-devel] [RFC 15/15] generate APIC IDs according to CPU topology (v2)

[Eduardo Habkost]

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

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

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc_piix.c      |  4 ++++
 target-i386/cpu.c | 34 ++++++++++++++++++++++++++++++----
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 0c0096f..f073916 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -375,6 +375,10 @@ static QEMUMachine pc_machine_v1_2 = {
             .driver   = "qxl",\
             .property = "vgamem_mb",\
             .value    = stringify(8),\
+        },{\
+            .driver   = "SMP",\
+            .property = "contiguous_apic_ids",\
+            .value    = "true",\
         }
 
 static QEMUMachine pc_machine_v1_1 = {
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1703373..ea6c7a7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -23,9 +23,11 @@
 
 #include "cpu.h"
 #include "kvm.h"
+#include "topology.h"
 
 #ifndef CONFIG_USER_ONLY
-#include "sysemu.h"
+#include "hw/qdev.h"
+#include "cpus.h"
 #endif
 
 #include "qemu-option.h"
@@ -492,13 +494,37 @@ static x86_def_t builtin_x86_defs[] = {
     },
 };
 
+#ifdef CONFIG_USER_ONLY
 unsigned int apic_id_for_cpu(int cpu_index)
 {
-    /* right now APIC ID == CPU index. this will eventually change to use
-     * the CPU topology configuration properly
-     */
+    /* *-user doesn't have any CPU topology settings, just use the CPU index */
     return cpu_index;
 }
+#else
+unsigned int apic_id_for_cpu(int cpu_index)
+{
+    const char *contig;
+    bool is_contiguous;
+    unsigned int correct_id;
+    static bool warned = false;
+
+    /* Global property SMP.contiguous_apic_ids=true will keep compatibility
+     * with the old (broken) behavior when calculating APIC IDs
+     */
+    contig = qemu_global_get("SMP", "contiguous_apic_ids");
+    is_contiguous = contig && !strcmp(contig, "true");
+    correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index);
+    if (is_contiguous) {
+        if (cpu_index != correct_id && !warned) {
+            fprintf(stderr, "warning: CPU topology in compatibility mode, it will not match the requested topology\n");
+            warned = true;
+        }
+        return cpu_index;
+    } else {
+        return correct_id;
+    }
+}
+#endif
 
 static int cpu_x86_fill_model_id(char *str)
 {
-- 
1.7.11.2