[PATCH v3 0/1] Fix endless translation loop of riscv

[PATCH v3 0/1] Fix endless translation loop of riscv

[Ziqiao Kong]

This version fixes the return value `old_pte` not correctly handled in
my previous patch.

This patch refers to common usages of qatomic_cmpxchg like those in
target/i386/tcg/system/excp_helper.c and target/arm/ptw.c. I also add
a brief explanation of the correctness in the commit message.

Thanks Philippe Mathieu-Daudé for offering previous review for my
previous patch! 

Ziqiao Kong (1):
  target/riscv: fix endless translation loop on big endian systems

 target/riscv/cpu_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v3 1/1] target/riscv: fix endless translation loop on big endian systems

[Ziqiao Kong]

On big endian systems, pte and updated_pte hold big endian host data
while pte_pa points to little endian target data. This means the branch
at cpu_helper.c:1669 will be always satisfied and restart translation,
causing an endless translation loop.

The correctness of this patch can be deduced by:

old_pte will hold value either from cpu_to_le32/64(pte) or 
cpu_to_le32/64(updated_pte), both of wich is litte endian. After that, 
an in-place conversion by le32/64_to_cpu(old_pte) ensures that old_pte 
now is in native endian, same with pte. Therefore, the endianness of the
both side of if (old_pte != pte) is correct. 

Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>
---
 target/riscv/cpu_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 6c4391d96b..3233b66e7e 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1662,9 +1662,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
             target_ulong *pte_pa = qemu_map_ram_ptr(mr->ram_block, addr1);
             target_ulong old_pte;
             if (riscv_cpu_sxl(env) == MXL_RV32) {
-                old_pte = qatomic_cmpxchg((uint32_t *)pte_pa, pte, updated_pte);
+                old_pte = qatomic_cmpxchg((uint32_t *)pte_pa, cpu_to_le32(pte), cpu_to_le32(updated_pte));
+                old_pte = le32_to_cpu(old_pte);
             } else {
-                old_pte = qatomic_cmpxchg(pte_pa, pte, updated_pte);
+                old_pte = qatomic_cmpxchg(pte_pa, cpu_to_le64(pte), cpu_to_le64(updated_pte));
+                old_pte = le64_to_cpu(old_pte);
             }
             if (old_pte != pte) {
                 goto restart;
-- 
2.34.1

Re: [PATCH v3 1/1] target/riscv: fix endless translation loop on big endian systems

[Richard Henderson]

On 4/15/25 01:02, Ziqiao Kong wrote:
> On big endian systems, pte and updated_pte hold big endian host data
> while pte_pa points to little endian target data. This means the branch
> at cpu_helper.c:1669 will be always satisfied and restart translation,
> causing an endless translation loop.
> 
> The correctness of this patch can be deduced by:
> 
> old_pte will hold value either from cpu_to_le32/64(pte) or
> cpu_to_le32/64(updated_pte), both of wich is litte endian. After that,
> an in-place conversion by le32/64_to_cpu(old_pte) ensures that old_pte
> now is in native endian, same with pte. Therefore, the endianness of the
> both side of if (old_pte != pte) is correct.
> 
> Signed-off-by: Ziqiao Kong<ziqiaokong@gmail.com>
> ---
>   target/riscv/cpu_helper.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 1/1] target/riscv: fix endless translation loop on big endian systems

[Alistair Francis]

On Tue, Apr 15, 2025 at 6:06 PM Ziqiao Kong <ziqiaokong@gmail.com> wrote:
>
> On big endian systems, pte and updated_pte hold big endian host data
> while pte_pa points to little endian target data. This means the branch
> at cpu_helper.c:1669 will be always satisfied and restart translation,
> causing an endless translation loop.
>
> The correctness of this patch can be deduced by:
>
> old_pte will hold value either from cpu_to_le32/64(pte) or
> cpu_to_le32/64(updated_pte), both of wich is litte endian. After that,
> an in-place conversion by le32/64_to_cpu(old_pte) ensures that old_pte
> now is in native endian, same with pte. Therefore, the endianness of the
> both side of if (old_pte != pte) is correct.
>
> Signed-off-by: Ziqiao Kong <ziqiaokong@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu_helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 6c4391d96b..3233b66e7e 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -1662,9 +1662,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>              target_ulong *pte_pa = qemu_map_ram_ptr(mr->ram_block, addr1);
>              target_ulong old_pte;
>              if (riscv_cpu_sxl(env) == MXL_RV32) {
> -                old_pte = qatomic_cmpxchg((uint32_t *)pte_pa, pte, updated_pte);
> +                old_pte = qatomic_cmpxchg((uint32_t *)pte_pa, cpu_to_le32(pte), cpu_to_le32(updated_pte));
> +                old_pte = le32_to_cpu(old_pte);
>              } else {
> -                old_pte = qatomic_cmpxchg(pte_pa, pte, updated_pte);
> +                old_pte = qatomic_cmpxchg(pte_pa, cpu_to_le64(pte), cpu_to_le64(updated_pte));
> +                old_pte = le64_to_cpu(old_pte);
>              }
>              if (old_pte != pte) {
>                  goto restart;
> --
> 2.34.1
>
>

Re: [PATCH v3 0/1] Fix endless translation loop of riscv

[Alistair Francis]

On Tue, Apr 15, 2025 at 6:05 PM Ziqiao Kong <ziqiaokong@gmail.com> wrote:
>
> This version fixes the return value `old_pte` not correctly handled in
> my previous patch.
>
> This patch refers to common usages of qatomic_cmpxchg like those in
> target/i386/tcg/system/excp_helper.c and target/arm/ptw.c. I also add
> a brief explanation of the correctness in the commit message.
>
> Thanks Philippe Mathieu-Daudé for offering previous review for my
> previous patch!
>
> Ziqiao Kong (1):
>   target/riscv: fix endless translation loop on big endian systems

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  target/riscv/cpu_helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> --
> 2.34.1
>
>