Re: [PATCH] target/loongarch: Fix the CSRRD CPUID instruction on big endian hosts

[Song Gao]

在 2023/7/21 上午1:53, Thomas Huth 写道:
> The test in tests/avocado/machine_loongarch.py is currently failing
> on big endian hosts like s390x. By comparing the traces between running
> the QEMU_EFI.fd bios on a s390x and on a x86 host, it's quickly obvious
> that the CSRRD instruction for the CPUID is behaving differently. And
> indeed: The code currently does a long read (i.e. 64 bit) from the
> address that points to the CPUState->cpu_index field (with tcg_gen_ld_tl()
> in the trans_csrrd() function). But this cpu_index field is only an "int"
> (i.e. 32 bit). While this dirty pointer magic works on little endian hosts,
> it of course fails on big endian hosts. Fix it by using a proper helper
> function instead.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   Note: There is another bug preventing tests/avocado/machine_loongarch.py
>   from working correctly on big endian hosts (Linux fails to run the shell)
>   but that problem is harder to debug since it happens way later in the boot
>   process...
>
>   target/loongarch/cpu.h                             | 1 +
>   target/loongarch/helper.h                          | 1 +
>   target/loongarch/csr_helper.c                      | 9 +++++++++
>   target/loongarch/insn_trans/trans_privileged.c.inc | 8 +-------
>   4 files changed, 12 insertions(+), 7 deletions(-)

Reviewed-by: Song Gao <gaosong@loongson.cn>

Thanks.
Song Gao

> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
> index ed04027af1..fa371ca8ba 100644
> --- a/target/loongarch/cpu.h
> +++ b/target/loongarch/cpu.h
> @@ -342,6 +342,7 @@ typedef struct CPUArchState {
>       uint64_t CSR_DBG;
>       uint64_t CSR_DERA;
>       uint64_t CSR_DSAVE;
> +    uint64_t CSR_CPUID;
>   
>   #ifndef CONFIG_USER_ONLY
>       LoongArchTLB  tlb[LOONGARCH_TLB_MAX];
> diff --git a/target/loongarch/helper.h b/target/loongarch/helper.h
> index b9de77d926..ffb1e0b0bf 100644
> --- a/target/loongarch/helper.h
> +++ b/target/loongarch/helper.h
> @@ -98,6 +98,7 @@ DEF_HELPER_1(rdtime_d, i64, env)
>   #ifndef CONFIG_USER_ONLY
>   /* CSRs helper */
>   DEF_HELPER_1(csrrd_pgd, i64, env)
> +DEF_HELPER_1(csrrd_cpuid, i64, env)
>   DEF_HELPER_1(csrrd_tval, i64, env)
>   DEF_HELPER_2(csrwr_estat, i64, env, tl)
>   DEF_HELPER_2(csrwr_asid, i64, env, tl)
> diff --git a/target/loongarch/csr_helper.c b/target/loongarch/csr_helper.c
> index 6526367946..55341551a5 100644
> --- a/target/loongarch/csr_helper.c
> +++ b/target/loongarch/csr_helper.c
> @@ -35,6 +35,15 @@ target_ulong helper_csrrd_pgd(CPULoongArchState *env)
>       return v;
>   }
>   
> +target_ulong helper_csrrd_cpuid(CPULoongArchState *env)
> +{
> +    LoongArchCPU *lac = env_archcpu(env);
> +
> +    env->CSR_CPUID = CPU(lac)->cpu_index;
> +
> +    return env->CSR_CPUID;
> +}
> +
>   target_ulong helper_csrrd_tval(CPULoongArchState *env)
>   {
>       LoongArchCPU *cpu = env_archcpu(env);
> diff --git a/target/loongarch/insn_trans/trans_privileged.c.inc 
> b/target/loongarch/insn_trans/trans_privileged.c.inc
> index 02bca7ca23..9c9de090f0 100644
> --- a/target/loongarch/insn_trans/trans_privileged.c.inc
> +++ b/target/loongarch/insn_trans/trans_privileged.c.inc
> @@ -99,13 +99,7 @@ static const CSRInfo csr_info[] = {
>       CSR_OFF(PWCH),
>       CSR_OFF(STLBPS),
>       CSR_OFF(RVACFG),
> -    [LOONGARCH_CSR_CPUID] = {
> -        .offset = (int)offsetof(CPUState, cpu_index)
> -                  - (int)offsetof(LoongArchCPU, env),
> -        .flags = CSRFL_READONLY,
> -        .readfn = NULL,
> -        .writefn = NULL
> -    },
> +    CSR_OFF_FUNCS(CPUID, CSRFL_READONLY, gen_helper_csrrd_cpuid, NULL),
>       CSR_OFF_FLAGS(PRCFG1, CSRFL_READONLY),
>       CSR_OFF_FLAGS(PRCFG2, CSRFL_READONLY),
>       CSR_OFF_FLAGS(PRCFG3, CSRFL_READONLY),