diff mbox

cpu: introduce CpuTopoInfo structure for argument simplification

Message ID 1440149685-14057-1-git-send-email-zhugh.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Zhu Guihua Aug. 21, 2015, 9:34 a.m. UTC 
From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>

In order to simplify arguments of function, introduce a new struct
named X86CPUTopoInfo.

Originally, this patch was in the patchset "cpu: add device_add
foo-x86_64-cpu support". Now put it into a separate patch so that
it can be merged ASAP.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
---
 include/hw/i386/topology.h | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

Comments

Eduardo Habkost Aug. 21, 2015, 4:25 p.m. UTC | #1 
On Fri, Aug 21, 2015 at 05:34:45PM +0800, Zhu Guihua wrote:
> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> 
> In order to simplify arguments of function, introduce a new struct
> named X86CPUTopoInfo.
> 
> Originally, this patch was in the patchset "cpu: add device_add
> foo-x86_64-cpu support". Now put it into a separate patch so that
> it can be merged ASAP.
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>

Applied to the x86 tree. Thanks!
Paolo Bonzini Sept. 7, 2015, 11:29 a.m. UTC | #2 
On 21/08/2015 11:34, Zhu Guihua wrote:
> @@ -107,14 +111,12 @@ static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
>  static inline void x86_topo_ids_from_idx(unsigned nr_cores,
>                                           unsigned nr_threads,
>                                           unsigned cpu_index,
> -                                         unsigned *pkg_id,
> -                                         unsigned *core_id,
> -                                         unsigned *smt_id)
> +                                         X86CPUTopoInfo *topo)
>  {

Isn't this function used in hw/i386/pc.c as well?

Paolo
Andreas Färber Sept. 7, 2015, 2:22 p.m. UTC | #3 
Am 07.09.2015 um 13:29 schrieb Paolo Bonzini:
> On 21/08/2015 11:34, Zhu Guihua wrote:
>> @@ -107,14 +111,12 @@ static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
>>  static inline void x86_topo_ids_from_idx(unsigned nr_cores,
>>                                           unsigned nr_threads,
>>                                           unsigned cpu_index,
>> -                                         unsigned *pkg_id,
>> -                                         unsigned *core_id,
>> -                                         unsigned *smt_id)
>> +                                         X86CPUTopoInfo *topo)
>>  {
> 
> Isn't this function used in hw/i386/pc.c as well?

In case it gets respun now, in Seattle I had asked Eduardo to update the
subject with s/CpuTopoInfo/X86CPUTopoInfo/.

Regards,
Andreas
Eduardo Habkost Sept. 9, 2015, 4:11 p.m. UTC | #4 
On Mon, Sep 07, 2015 at 04:22:10PM +0200, Andreas Färber wrote:
> Am 07.09.2015 um 13:29 schrieb Paolo Bonzini:
> > On 21/08/2015 11:34, Zhu Guihua wrote:
> >> @@ -107,14 +111,12 @@ static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
> >>  static inline void x86_topo_ids_from_idx(unsigned nr_cores,
> >>                                           unsigned nr_threads,
> >>                                           unsigned cpu_index,
> >> -                                         unsigned *pkg_id,
> >> -                                         unsigned *core_id,
> >> -                                         unsigned *smt_id)
> >> +                                         X86CPUTopoInfo *topo)
> >>  {
> > 
> > Isn't this function used in hw/i386/pc.c as well?
> 
> In case it gets respun now, in Seattle I had asked Eduardo to update the
> subject with s/CpuTopoInfo/X86CPUTopoInfo/.

I have fixed the subject line when applying to x86, and now added the
following fix to the patch to avoid a respin:

  diff --git a/hw/i386/pc.c b/hw/i386/pc.c
  index 9f2924e..c515fca 100644
  --- a/hw/i386/pc.c
  +++ b/hw/i386/pc.c
  @@ -1938,10 +1938,10 @@ static void pc_machine_initfn(Object *obj)
   
   static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
   {
  -    unsigned pkg_id, core_id, smt_id;
  +    X86CPUTopoInfo topo;
       x86_topo_ids_from_idx(smp_cores, smp_threads, cpu_index,
  -                          &pkg_id, &core_id, &smt_id);
  -    return pkg_id;
  +                          &topo);
  +    return topo.pkg_id;
   }
   
   static void pc_machine_class_init(ObjectClass *oc, void *data)
Zhu Guihua Sept. 10, 2015, 1:09 a.m. UTC | #5 
On 09/10/2015 12:11 AM, Eduardo Habkost wrote:
> On Mon, Sep 07, 2015 at 04:22:10PM +0200, Andreas Färber wrote:
>> Am 07.09.2015 um 13:29 schrieb Paolo Bonzini:
>>> On 21/08/2015 11:34, Zhu Guihua wrote:
>>>> @@ -107,14 +111,12 @@ static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
>>>>   static inline void x86_topo_ids_from_idx(unsigned nr_cores,
>>>>                                            unsigned nr_threads,
>>>>                                            unsigned cpu_index,
>>>> -                                         unsigned *pkg_id,
>>>> -                                         unsigned *core_id,
>>>> -                                         unsigned *smt_id)
>>>> +                                         X86CPUTopoInfo *topo)
>>>>   {
>>> Isn't this function used in hw/i386/pc.c as well?
>> In case it gets respun now, in Seattle I had asked Eduardo to update the
>> subject with s/CpuTopoInfo/X86CPUTopoInfo/.
> I have fixed the subject line when applying to x86, and now added the
> following fix to the patch to avoid a respin:
>
>    diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>    index 9f2924e..c515fca 100644
>    --- a/hw/i386/pc.c
>    +++ b/hw/i386/pc.c
>    @@ -1938,10 +1938,10 @@ static void pc_machine_initfn(Object *obj)
>     
>     static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
>     {
>    -    unsigned pkg_id, core_id, smt_id;
>    +    X86CPUTopoInfo topo;
>         x86_topo_ids_from_idx(smp_cores, smp_threads, cpu_index,
>    -                          &pkg_id, &core_id, &smt_id);
>    -    return pkg_id;
>    +                          &topo);
>    +    return topo.pkg_id;
>     }
>     
>     static void pc_machine_class_init(ObjectClass *oc, void *data)
>

I am sorry for my carelessness, thanks for your work.

Regards,
Zhu
diff mbox

Patch

diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index 9c6f3a9..148cc1b 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -47,6 +47,12 @@ 
  */
 typedef uint32_t apic_id_t;
 
+typedef struct X86CPUTopoInfo {
+    unsigned pkg_id;
+    unsigned core_id;
+    unsigned smt_id;
+} X86CPUTopoInfo;
+
 /* Return the bit width needed for 'count' IDs
  */
 static unsigned apicid_bitwidth_for_count(unsigned count)
@@ -92,13 +98,11 @@  static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
  */
 static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
                                              unsigned nr_threads,
-                                             unsigned pkg_id,
-                                             unsigned core_id,
-                                             unsigned smt_id)
+                                             const X86CPUTopoInfo *topo)
 {
-    return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) |
-           (core_id << apicid_core_offset(nr_cores, nr_threads)) |
-           smt_id;
+    return (topo->pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) |
+           (topo->core_id << apicid_core_offset(nr_cores, nr_threads)) |
+           topo->smt_id;
 }
 
 /* Calculate thread/core/package IDs for a specific topology,
@@ -107,14 +111,12 @@  static inline apic_id_t apicid_from_topo_ids(unsigned nr_cores,
 static inline void x86_topo_ids_from_idx(unsigned nr_cores,
                                          unsigned nr_threads,
                                          unsigned cpu_index,
-                                         unsigned *pkg_id,
-                                         unsigned *core_id,
-                                         unsigned *smt_id)
+                                         X86CPUTopoInfo *topo)
 {
     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;
+    topo->smt_id = cpu_index % nr_threads;
+    topo->core_id = core_index % nr_cores;
+    topo->pkg_id = core_index / nr_cores;
 }
 
 /* Make APIC ID for the CPU 'cpu_index'
@@ -125,10 +127,9 @@  static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
                                                 unsigned nr_threads,
                                                 unsigned cpu_index)
 {
-    unsigned pkg_id, core_id, smt_id;
-    x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
-                          &pkg_id, &core_id, &smt_id);
-    return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, smt_id);
+    X86CPUTopoInfo topo;
+    x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index, &topo);
+    return apicid_from_topo_ids(nr_cores, nr_threads, &topo);
 }
 
 #endif /* HW_I386_TOPOLOGY_H */