[v5,7/8] Revert "hw/386: Add EPYC mode topology decoding functions"

Commit Message

Babu Moger Aug. 21, 2020, 10:13 p.m. UTC

Remove the EPYC specific apicid decoding and use the generic
default decoding.

This reverts commit 7568b205555a6405042f62c64af3268f4330aed5.

Signed-off-by: Babu Moger <[e-mail]>
---
 include/hw/i386/topology.h |   79 --------------------------------------------
 target/i386/cpu.c          |    2 +
 2 files changed, 1 insertion(+), 80 deletions(-)

Comments

Eduardo Habkost Aug. 28, 2020, 5:27 p.m. UTC | #1

On Fri, Aug 21, 2020 at 05:13:03PM -0500, Babu Moger wrote:
> Remove the EPYC specific apicid decoding and use the generic
> default decoding.
> 
> This reverts commit 7568b205555a6405042f62c64af3268f4330aed5.
> 
> Signed-off-by: Babu Moger <[e-mail]>
> ---
[...]
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 19198e3e7f..b29686220e 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -388,7 +388,7 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>      unsigned long dies = topo_info->dies_per_pkg;
>      int shift;
>  
> -    x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
> +    x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);

This was not part of commit 7568b205555a6405042f62c64af3268f4330aed5.
I suggest doing this change as a separate patch, to make review easier.

That line was addd by commit dd08ef0318e2
("target/i386: Cleanup and use the EPYC mode topology functions").
Wouldn't it be simpler to revert that commit?  If there are parts
of commit dd08ef0318e2 we want to keep, they can be re-added
in a separate patch.

Babu Moger Aug. 28, 2020, 6:03 p.m. UTC | #2

On 8/28/20 12:27 PM, Eduardo Habkost wrote:
> On Fri, Aug 21, 2020 at 05:13:03PM -0500, Babu Moger wrote:
>> Remove the EPYC specific apicid decoding and use the generic
>> default decoding.
>>
>> This reverts commit 7568b205555a6405042f62c64af3268f4330aed5.
>>
>> Signed-off-by: Babu Moger <[e-mail]>
>> ---
> [...]
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 19198e3e7f..b29686220e 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -388,7 +388,7 @@ static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
>>      unsigned long dies = topo_info->dies_per_pkg;
>>      int shift;
>>  
>> -    x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
>> +    x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);
> 
> This was not part of commit 7568b205555a6405042f62c64af3268f4330aed5.
> I suggest doing this change as a separate patch, to make review easier.
> 
> That line was addd by commit dd08ef0318e2
> ("target/i386: Cleanup and use the EPYC mode topology functions").
> Wouldn't it be simpler to revert that commit?  If there are parts
> of commit dd08ef0318e2 we want to keep, they can be re-added
> in a separate patch.
> 

Sure. Will take care of it in next revision. Thanks.

Patch

diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index 05ddde7ba0..81573f6cfd 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -107,85 +107,6 @@  static inline unsigned apicid_pkg_offset(X86CPUTopoInfo *topo_info)
     return apicid_die_offset(topo_info) + apicid_die_width(topo_info);
 }
 
-#define EPYC_DIE_OFFSET 3 /* Minimum die_id offset if numa configured */
-
-/*
- * Bit offset of the die_id field
- */
-static inline unsigned apicid_die_offset_epyc(X86CPUTopoInfo *topo_info)
-{
-    unsigned offset = apicid_core_offset(topo_info) +
-                      apicid_core_width(topo_info);
-
-    return MAX(EPYC_DIE_OFFSET, offset);
-}
-
-/* Bit offset of the Pkg_ID (socket ID) field */
-static inline unsigned apicid_pkg_offset_epyc(X86CPUTopoInfo *topo_info)
-{
-    return apicid_die_offset_epyc(topo_info) + apicid_die_width(topo_info);
-}
-
-/*
- * 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
-x86_apicid_from_topo_ids_epyc(X86CPUTopoInfo *topo_info,
-                              const X86CPUTopoIDs *topo_ids)
-{
-    return (topo_ids->pkg_id  << apicid_pkg_offset_epyc(topo_info)) |
-           (topo_ids->die_id  << apicid_die_offset_epyc(topo_info)) |
-           (topo_ids->core_id << apicid_core_offset(topo_info)) |
-           topo_ids->smt_id;
-}
-
-static inline void x86_topo_ids_from_idx_epyc(X86CPUTopoInfo *topo_info,
-                                              unsigned cpu_index,
-                                              X86CPUTopoIDs *topo_ids)
-{
-    unsigned nr_dies = topo_info->dies_per_pkg;
-    unsigned nr_cores = topo_info->cores_per_die;
-    unsigned nr_threads = topo_info->threads_per_core;
-
-    topo_ids->pkg_id = cpu_index / (nr_dies * nr_cores * nr_threads);
-    topo_ids->die_id = cpu_index / (nr_cores * nr_threads) % nr_dies;
-    topo_ids->core_id = cpu_index / nr_threads % nr_cores;
-    topo_ids->smt_id = cpu_index % nr_threads;
-}
-
-/*
- * Calculate thread/core/package IDs for a specific topology,
- * based on APIC ID
- */
-static inline void x86_topo_ids_from_apicid_epyc(apic_id_t apicid,
-                                            X86CPUTopoInfo *topo_info,
-                                            X86CPUTopoIDs *topo_ids)
-{
-    topo_ids->smt_id = apicid &
-            ~(0xFFFFFFFFUL << apicid_smt_width(topo_info));
-    topo_ids->core_id =
-            (apicid >> apicid_core_offset(topo_info)) &
-            ~(0xFFFFFFFFUL << apicid_core_width(topo_info));
-    topo_ids->die_id =
-            (apicid >> apicid_die_offset_epyc(topo_info)) &
-            ~(0xFFFFFFFFUL << apicid_die_width(topo_info));
-    topo_ids->pkg_id = apicid >> apicid_pkg_offset_epyc(topo_info);
-}
-
-/*
- * Make APIC ID for the CPU 'cpu_index'
- *
- * 'cpu_index' is a sequential, contiguous ID for the CPU.
- */
-static inline apic_id_t x86_apicid_from_cpu_idx_epyc(X86CPUTopoInfo *topo_info,
-                                                     unsigned cpu_index)
-{
-    X86CPUTopoIDs topo_ids;
-    x86_topo_ids_from_idx_epyc(topo_info, cpu_index, &topo_ids);
-    return x86_apicid_from_topo_ids_epyc(topo_info, &topo_ids);
-}
 /* 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.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 19198e3e7f..b29686220e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -388,7 +388,7 @@  static void encode_topo_cpuid8000001e(X86CPUTopoInfo *topo_info, X86CPU *cpu,
     unsigned long dies = topo_info->dies_per_pkg;
     int shift;
 
-    x86_topo_ids_from_apicid_epyc(cpu->apic_id, topo_info, &topo_ids);
+    x86_topo_ids_from_apicid(cpu->apic_id, topo_info, &topo_ids);
 
     *eax = cpu->apic_id;
     /*