diff mbox series

target/loongarch: Use physical cpu id about CSR CPUID for sysemu

Message ID 20241022124247.873232-1-maobibo@loongson.cn (mailing list archive)
State New
Headers show
Series target/loongarch: Use physical cpu id about CSR CPUID for sysemu | expand

Commit Message

Bibo Mao Oct. 22, 2024, 12:42 p.m. UTC 
For user tcg, there is no physical cpu id provided and logic cpuid
is used. For system emulation, physical cpu id is provided, initial
value of register CSR CPUID can be set from physical cpu id.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/intc/loongarch_ipi.c           | 3 ++-
 target/loongarch/cpu.c            | 7 ++++++-
 target/loongarch/tcg/csr_helper.c | 4 ----
 3 files changed, 8 insertions(+), 6 deletions(-)


base-commit: cc5adbbd50d81555b8eb73602ec16fde40b55be4

Comments

Richard Henderson Oct. 22, 2024, 6:54 p.m. UTC | #1 
On 10/22/24 05:42, Bibo Mao wrote:
> For user tcg, there is no physical cpu id provided and logic cpuid
> is used. For system emulation, physical cpu id is provided, initial
> value of register CSR CPUID can be set from physical cpu id.
> 
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>   hw/intc/loongarch_ipi.c           | 3 ++-
>   target/loongarch/cpu.c            | 7 ++++++-
>   target/loongarch/tcg/csr_helper.c | 4 ----
>   3 files changed, 8 insertions(+), 6 deletions(-)

Since cpu_index is arbitrary and assigned by hw/loongarch/virt.c anyway, why do these two 
values differ?  Surely arch_id is already unique per cpu?


r~


> 
> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
> index 2ae1a42c46..78b6fce81b 100644
> --- a/hw/intc/loongarch_ipi.c
> +++ b/hw/intc/loongarch_ipi.c
> @@ -42,7 +42,8 @@ static CPUState *loongarch_cpu_by_arch_id(int64_t arch_id)
>       CPUArchId *archid;
>   
>       archid = find_cpu_by_archid(machine, arch_id);
> -    if (archid) {
> +    /* For offlined cpus, archid->cpu may be NULL */
> +    if (archid && archid->cpu) {
>           return CPU(archid->cpu);
>       }
>   
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 7212fb5f8f..d4659e8d45 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -510,8 +510,10 @@ static void loongarch_max_initfn(Object *obj)
>   static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
>   {
>       CPUState *cs = CPU(obj);
> +    LoongArchCPU *cpu = LOONGARCH_CPU(obj);
>       LoongArchCPUClass *lacc = LOONGARCH_CPU_GET_CLASS(obj);
>       CPULoongArchState *env = cpu_env(cs);
> +    int n;
>   
>       if (lacc->parent_phases.hold) {
>           lacc->parent_phases.hold(obj, type);
> @@ -522,7 +524,6 @@ static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
>   #endif
>       env->fcsr0 = 0x0;
>   
> -    int n;
>       /* Set csr registers value after reset, see the manual 6.4. */
>       env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, PLV, 0);
>       env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, IE, 0);
> @@ -543,7 +544,11 @@ static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
>   
>       env->CSR_ESTAT = env->CSR_ESTAT & (~MAKE_64BIT_MASK(0, 2));
>       env->CSR_RVACFG = FIELD_DP64(env->CSR_RVACFG, CSR_RVACFG, RBITS, 0);
> +#ifndef CONFIG_USER_ONLY
> +    env->CSR_CPUID = cpu->phy_id;
> +#else
>       env->CSR_CPUID = cs->cpu_index;
> +#endif
>       env->CSR_TCFG = FIELD_DP64(env->CSR_TCFG, CSR_TCFG, EN, 0);
>       env->CSR_LLBCTL = FIELD_DP64(env->CSR_LLBCTL, CSR_LLBCTL, KLO, 0);
>       env->CSR_TLBRERA = FIELD_DP64(env->CSR_TLBRERA, CSR_TLBRERA, ISTLBR, 0);
> diff --git a/target/loongarch/tcg/csr_helper.c b/target/loongarch/tcg/csr_helper.c
> index 15f94caefa..2aeca2343d 100644
> --- a/target/loongarch/tcg/csr_helper.c
> +++ b/target/loongarch/tcg/csr_helper.c
> @@ -37,10 +37,6 @@ target_ulong helper_csrrd_pgd(CPULoongArchState *env)
>   
>   target_ulong helper_csrrd_cpuid(CPULoongArchState *env)
>   {
> -    LoongArchCPU *lac = env_archcpu(env);
> -
> -    env->CSR_CPUID = CPU(lac)->cpu_index;
> -
>       return env->CSR_CPUID;
>   }
>   
> 
> base-commit: cc5adbbd50d81555b8eb73602ec16fde40b55be4
Bibo Mao Oct. 23, 2024, 1:22 a.m. UTC | #2 
On 2024/10/23 上午2:54, Richard Henderson wrote:
> On 10/22/24 05:42, Bibo Mao wrote:
>> For user tcg, there is no physical cpu id provided and logic cpuid
>> is used. For system emulation, physical cpu id is provided, initial
>> value of register CSR CPUID can be set from physical cpu id.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/intc/loongarch_ipi.c           | 3 ++-
>>   target/loongarch/cpu.c            | 7 ++++++-
>>   target/loongarch/tcg/csr_helper.c | 4 ----
>>   3 files changed, 8 insertions(+), 6 deletions(-)
> 
> Since cpu_index is arbitrary and assigned by hw/loongarch/virt.c anyway, 
> why do these two values differ?  Surely arch_id is already unique per cpu?
arch_id comes from cpu topo and cpu_index comes from vcpu object created 
order. There are two places where cpu_index may differ from arch_id, 
such as command "-smp 2,maxcpus=16,sockets=4,cores=4,threads=1".

1. If one cpu is hot-added with command "device_add 
la464-loongarch-cpu,socket-id=1,core-id=0,thread-id=0,id=cpu-4". The 
value of cpu_index is 2, however its arch_id is 4.

2. The other condition is not decided now how to generate arch_id, for
the command "-smp 2,maxcpus=36,sockets=6,cores=6,threads=1". It is not 
decided what it max number about one socket, it may be 6, also it may be 
8 which is component of 2.

arch_id comes from cpu topo, it should be unique per cpu.

Regards
Bibo Mao

> 
> 
> r~
> 
> 
>>
>> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
>> index 2ae1a42c46..78b6fce81b 100644
>> --- a/hw/intc/loongarch_ipi.c
>> +++ b/hw/intc/loongarch_ipi.c
>> @@ -42,7 +42,8 @@ static CPUState *loongarch_cpu_by_arch_id(int64_t 
>> arch_id)
>>       CPUArchId *archid;
>>       archid = find_cpu_by_archid(machine, arch_id);
>> -    if (archid) {
>> +    /* For offlined cpus, archid->cpu may be NULL */
>> +    if (archid && archid->cpu) {
>>           return CPU(archid->cpu);
>>       }
>> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
>> index 7212fb5f8f..d4659e8d45 100644
>> --- a/target/loongarch/cpu.c
>> +++ b/target/loongarch/cpu.c
>> @@ -510,8 +510,10 @@ static void loongarch_max_initfn(Object *obj)
>>   static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
>>   {
>>       CPUState *cs = CPU(obj);
>> +    LoongArchCPU *cpu = LOONGARCH_CPU(obj);
>>       LoongArchCPUClass *lacc = LOONGARCH_CPU_GET_CLASS(obj);
>>       CPULoongArchState *env = cpu_env(cs);
>> +    int n;
>>       if (lacc->parent_phases.hold) {
>>           lacc->parent_phases.hold(obj, type);
>> @@ -522,7 +524,6 @@ static void loongarch_cpu_reset_hold(Object *obj, 
>> ResetType type)
>>   #endif
>>       env->fcsr0 = 0x0;
>> -    int n;
>>       /* Set csr registers value after reset, see the manual 6.4. */
>>       env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, PLV, 0);
>>       env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, IE, 0);
>> @@ -543,7 +544,11 @@ static void loongarch_cpu_reset_hold(Object *obj, 
>> ResetType type)
>>       env->CSR_ESTAT = env->CSR_ESTAT & (~MAKE_64BIT_MASK(0, 2));
>>       env->CSR_RVACFG = FIELD_DP64(env->CSR_RVACFG, CSR_RVACFG, RBITS, 
>> 0);
>> +#ifndef CONFIG_USER_ONLY
>> +    env->CSR_CPUID = cpu->phy_id;
>> +#else
>>       env->CSR_CPUID = cs->cpu_index;
>> +#endif
>>       env->CSR_TCFG = FIELD_DP64(env->CSR_TCFG, CSR_TCFG, EN, 0);
>>       env->CSR_LLBCTL = FIELD_DP64(env->CSR_LLBCTL, CSR_LLBCTL, KLO, 0);
>>       env->CSR_TLBRERA = FIELD_DP64(env->CSR_TLBRERA, CSR_TLBRERA, 
>> ISTLBR, 0);
>> diff --git a/target/loongarch/tcg/csr_helper.c 
>> b/target/loongarch/tcg/csr_helper.c
>> index 15f94caefa..2aeca2343d 100644
>> --- a/target/loongarch/tcg/csr_helper.c
>> +++ b/target/loongarch/tcg/csr_helper.c
>> @@ -37,10 +37,6 @@ target_ulong helper_csrrd_pgd(CPULoongArchState *env)
>>   target_ulong helper_csrrd_cpuid(CPULoongArchState *env)
>>   {
>> -    LoongArchCPU *lac = env_archcpu(env);
>> -
>> -    env->CSR_CPUID = CPU(lac)->cpu_index;
>> -
>>       return env->CSR_CPUID;
>>   }
>>
>> base-commit: cc5adbbd50d81555b8eb73602ec16fde40b55be4
Bibo Mao Oct. 24, 2024, 12:06 p.m. UTC | #3 
On 2024/10/23 上午9:22, maobibo wrote:
> 
> 
> On 2024/10/23 上午2:54, Richard Henderson wrote:
>> On 10/22/24 05:42, Bibo Mao wrote:
>>> For user tcg, there is no physical cpu id provided and logic cpuid
>>> is used. For system emulation, physical cpu id is provided, initial
>>> value of register CSR CPUID can be set from physical cpu id.
>>>
>>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>> ---
>>>   hw/intc/loongarch_ipi.c           | 3 ++-
>>>   target/loongarch/cpu.c            | 7 ++++++-
>>>   target/loongarch/tcg/csr_helper.c | 4 ----
>>>   3 files changed, 8 insertions(+), 6 deletions(-)
>>
>> Since cpu_index is arbitrary and assigned by hw/loongarch/virt.c 
>> anyway, why do these two values differ?  Surely arch_id is already 
>> unique per cpu?
> arch_id comes from cpu topo and cpu_index comes from vcpu object created 
> order. There are two places where cpu_index may differ from arch_id, 
> such as command "-smp 2,maxcpus=16,sockets=4,cores=4,threads=1".
> 
> 1. If one cpu is hot-added with command "device_add 
> la464-loongarch-cpu,socket-id=1,core-id=0,thread-id=0,id=cpu-4". The 
> value of cpu_index is 2, however its arch_id is 4.
> 
> 2. The other condition is not decided now how to generate arch_id, for
> the command "-smp 2,maxcpus=36,sockets=6,cores=6,threads=1". It is not 
> decided what it max number about one socket, it may be 6, also it may be 
> 8 which is component of 2.
After second thought, cpu_index can be set again by machines, it can be 
cpuslot index of possible_cpus coming from possible_cpu_arch_ids(). For 
the general arch_id calculation method:
    archid = socket_id * ms->smp.threads * ms->smp.cores  +
             core_id * ms->smp.threads + thread_id
Then arch_id will be the same with cpu_index.

Generic arch_id calculation method is enough, will modify it in furture 
if HW platform defines separate method.

Will refresh the patch in next round.

Regards
Bibo Mao
> 
> arch_id comes from cpu topo, it should be unique per cpu.
> 
> Regards
> Bibo Mao
> 
>>
>>
>> r~
>>
>>
>>>
>>> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
>>> index 2ae1a42c46..78b6fce81b 100644
>>> --- a/hw/intc/loongarch_ipi.c
>>> +++ b/hw/intc/loongarch_ipi.c
>>> @@ -42,7 +42,8 @@ static CPUState *loongarch_cpu_by_arch_id(int64_t 
>>> arch_id)
>>>       CPUArchId *archid;
>>>       archid = find_cpu_by_archid(machine, arch_id);
>>> -    if (archid) {
>>> +    /* For offlined cpus, archid->cpu may be NULL */
>>> +    if (archid && archid->cpu) {
>>>           return CPU(archid->cpu);
>>>       }
>>> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
>>> index 7212fb5f8f..d4659e8d45 100644
>>> --- a/target/loongarch/cpu.c
>>> +++ b/target/loongarch/cpu.c
>>> @@ -510,8 +510,10 @@ static void loongarch_max_initfn(Object *obj)
>>>   static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
>>>   {
>>>       CPUState *cs = CPU(obj);
>>> +    LoongArchCPU *cpu = LOONGARCH_CPU(obj);
>>>       LoongArchCPUClass *lacc = LOONGARCH_CPU_GET_CLASS(obj);
>>>       CPULoongArchState *env = cpu_env(cs);
>>> +    int n;
>>>       if (lacc->parent_phases.hold) {
>>>           lacc->parent_phases.hold(obj, type);
>>> @@ -522,7 +524,6 @@ static void loongarch_cpu_reset_hold(Object *obj, 
>>> ResetType type)
>>>   #endif
>>>       env->fcsr0 = 0x0;
>>> -    int n;
>>>       /* Set csr registers value after reset, see the manual 6.4. */
>>>       env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, PLV, 0);
>>>       env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, IE, 0);
>>> @@ -543,7 +544,11 @@ static void loongarch_cpu_reset_hold(Object 
>>> *obj, ResetType type)
>>>       env->CSR_ESTAT = env->CSR_ESTAT & (~MAKE_64BIT_MASK(0, 2));
>>>       env->CSR_RVACFG = FIELD_DP64(env->CSR_RVACFG, CSR_RVACFG, 
>>> RBITS, 0);
>>> +#ifndef CONFIG_USER_ONLY
>>> +    env->CSR_CPUID = cpu->phy_id;
>>> +#else
>>>       env->CSR_CPUID = cs->cpu_index;
>>> +#endif
>>>       env->CSR_TCFG = FIELD_DP64(env->CSR_TCFG, CSR_TCFG, EN, 0);
>>>       env->CSR_LLBCTL = FIELD_DP64(env->CSR_LLBCTL, CSR_LLBCTL, KLO, 0);
>>>       env->CSR_TLBRERA = FIELD_DP64(env->CSR_TLBRERA, CSR_TLBRERA, 
>>> ISTLBR, 0);
>>> diff --git a/target/loongarch/tcg/csr_helper.c 
>>> b/target/loongarch/tcg/csr_helper.c
>>> index 15f94caefa..2aeca2343d 100644
>>> --- a/target/loongarch/tcg/csr_helper.c
>>> +++ b/target/loongarch/tcg/csr_helper.c
>>> @@ -37,10 +37,6 @@ target_ulong helper_csrrd_pgd(CPULoongArchState *env)
>>>   target_ulong helper_csrrd_cpuid(CPULoongArchState *env)
>>>   {
>>> -    LoongArchCPU *lac = env_archcpu(env);
>>> -
>>> -    env->CSR_CPUID = CPU(lac)->cpu_index;
>>> -
>>>       return env->CSR_CPUID;
>>>   }
>>>
>>> base-commit: cc5adbbd50d81555b8eb73602ec16fde40b55be4
>
diff mbox series

Patch

diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
index 2ae1a42c46..78b6fce81b 100644
--- a/hw/intc/loongarch_ipi.c
+++ b/hw/intc/loongarch_ipi.c
@@ -42,7 +42,8 @@  static CPUState *loongarch_cpu_by_arch_id(int64_t arch_id)
     CPUArchId *archid;
 
     archid = find_cpu_by_archid(machine, arch_id);
-    if (archid) {
+    /* For offlined cpus, archid->cpu may be NULL */
+    if (archid && archid->cpu) {
         return CPU(archid->cpu);
     }
 
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 7212fb5f8f..d4659e8d45 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -510,8 +510,10 @@  static void loongarch_max_initfn(Object *obj)
 static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
 {
     CPUState *cs = CPU(obj);
+    LoongArchCPU *cpu = LOONGARCH_CPU(obj);
     LoongArchCPUClass *lacc = LOONGARCH_CPU_GET_CLASS(obj);
     CPULoongArchState *env = cpu_env(cs);
+    int n;
 
     if (lacc->parent_phases.hold) {
         lacc->parent_phases.hold(obj, type);
@@ -522,7 +524,6 @@  static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
 #endif
     env->fcsr0 = 0x0;
 
-    int n;
     /* Set csr registers value after reset, see the manual 6.4. */
     env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, PLV, 0);
     env->CSR_CRMD = FIELD_DP64(env->CSR_CRMD, CSR_CRMD, IE, 0);
@@ -543,7 +544,11 @@  static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
 
     env->CSR_ESTAT = env->CSR_ESTAT & (~MAKE_64BIT_MASK(0, 2));
     env->CSR_RVACFG = FIELD_DP64(env->CSR_RVACFG, CSR_RVACFG, RBITS, 0);
+#ifndef CONFIG_USER_ONLY
+    env->CSR_CPUID = cpu->phy_id;
+#else
     env->CSR_CPUID = cs->cpu_index;
+#endif
     env->CSR_TCFG = FIELD_DP64(env->CSR_TCFG, CSR_TCFG, EN, 0);
     env->CSR_LLBCTL = FIELD_DP64(env->CSR_LLBCTL, CSR_LLBCTL, KLO, 0);
     env->CSR_TLBRERA = FIELD_DP64(env->CSR_TLBRERA, CSR_TLBRERA, ISTLBR, 0);
diff --git a/target/loongarch/tcg/csr_helper.c b/target/loongarch/tcg/csr_helper.c
index 15f94caefa..2aeca2343d 100644
--- a/target/loongarch/tcg/csr_helper.c
+++ b/target/loongarch/tcg/csr_helper.c
@@ -37,10 +37,6 @@  target_ulong helper_csrrd_pgd(CPULoongArchState *env)
 
 target_ulong helper_csrrd_cpuid(CPULoongArchState *env)
 {
-    LoongArchCPU *lac = env_archcpu(env);
-
-    env->CSR_CPUID = CPU(lac)->cpu_index;
-
     return env->CSR_CPUID;
 }