Hello Philippe,
Any further concern regarding this series? I certainly would like to investigate
and help =).
Bests,
Ziqiao
On Mon, Apr 14, 2025 at 7:17 PM Ziqiao Kong <ziqiaok...@gmail.com> wrote:
>
> On Mon, Apr 14, 2025 at 6:41 PM Philippe Mathieu-Daudé
> <phi...@linaro.org> wrote:
> >
> > Hi,
> >
> > On 14/4/25 05:46, 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.
> > >
> >
> > Cc: qemu-sta...@nongnu.org
> > Fixes: 0c3e702aca7 ("RISC-V CPU Helpers")
> >
> > > Signed-off-by: Ziqiao Kong <ziqiaok...@gmail.com>
> > > ---
> > > target/riscv/cpu_helper.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 6c4391d96b..bc146771c8 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -1662,9 +1662,9 @@ 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));
> > > } 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));
> > > }
> > > if (old_pte != pte) {
> > > goto restart;
> >
> > If PTEs are always stored in LE order, maybe what we want is earlier:
> >
> > -- >8 --
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 619c76cc001..b6ac2800240 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -1464,5 +1464,5 @@ static int get_physical_address(CPURISCVState
> > *env, hwaddr *physical,
> > if (riscv_cpu_mxl(env) == MXL_RV32) {
> > - pte = address_space_ldl(cs->as, pte_addr, attrs, &res);
> > + pte = address_space_ldl_le(cs->as, pte_addr, attrs, &res);
> > } else {
> > - pte = address_space_ldq(cs->as, pte_addr, attrs, &res);
> > + pte = address_space_ldq_le(cs->as, pte_addr, attrs, &res);
>
> Unfortunately, this doesn't work in two ways:
>
> 1. Note pte is used in the following code and that means pte must hold
> a correct value from the
> view of host endian (in my case, big endian not little endian).
> 2. address_space_ldq_le will dispatch to ldq_le_p, while
> address_space_leq will dispatch to ldq_p.
> However, on little endian targets, ldq_p is an alias of ldq_le_p so
> making no effects.
>
> Per my testing, this patch doesn't have any effect indeed. To have a
> brief view what is happening,
> see the logs just before atomic_cmpxchg:
>
> pte_pa 0xf14000000000000 == pte 0x140f ? updated_pte 0x144f
>
> > }
> > ---
