Re: [Qemu-devel] [PATCH 1/7] target/mips: compare virtual addresses in LL/SC sequence

[Alex Bennée]

Aleksandar Markovic <address@hidden> writes:

> From: Leon Alrae <address@hidden>
>
> Do only virtual addresses comaprisons in LL/SC sequence.

Doesn't this make things less correct? I know we currently use virtual
addresses for the ARM code but in theory you could map two virtual pages
to the same physical page and then violate the LL/SC behaviour.

Of course I can't imagine why you might do that.

>
> Until this patch, physical addresses had been compared in LL/SC
> sequence. Unfortunately, that meant that, on each SC, the address
> translation had to be done, which is a quite complex operation.
> Getting rid of that allows throwing away SC helpers and having
> common SC implementations in user and system mode, avoiding two
> separate implementations selected by #ifdef CONFIG_USER_ONLY.
>
> Given that LL/SC emulation was already very simplified (as only the
> address and value were compared), using virtual addresses instead of
> physical does not seem to be a violation. Correct guest software
> should not rely on LL/SC if they accesses the same physical address
> via different virtual addresses or if page mapping gets changed
> between LL/SC due to manipulating TLB entries. MIPS Instruction Set
> Manual clearly says that an RMW sequence must use the same address
> in the LL and SC (virtual address, physical address, cacheability
> and coherency attributes must be identical). Otherwise, the result of
> the SC is not predictable. This patch takes advantage of this fact
> and removes the virtual->physical address translation from SC helper.
>
> lladdr served as Coprocessor 0 LLAddr register which captures physical
> address of the most recent LL instruction, and also lladdr was used
> for comparison with following SC physical address. This patch changes
> the meaning of lladdr - now it will only keep the virtual address of
> the most recent LL. Additionally we introduce CP0_LLAddr which is the
> actual Coperocessor 0 LLAddr register that guest can access.
>
> Signed-off-by: Leon Alrae <address@hidden>
> Signed-off-by: Miodrag Dinic <address@hidden>
> Signed-off-by: Aleksandar Markovic <address@hidden>
> ---
>  target/mips/cpu.h       |  3 ++-
>  target/mips/machine.c   |  7 ++++---
>  target/mips/op_helper.c | 29 +++++++++++++++++------------
>  target/mips/translate.c |  4 ++--
>  4 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index 7f8ba5f..57ca861 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -480,10 +480,11 @@ struct CPUMIPSState {
>  #define CP0C5_NFExists   0
>      int32_t CP0_Config6;
>      int32_t CP0_Config7;
> +    uint64_t CP0_LLAddr;
>      uint64_t CP0_MAAR[MIPS_MAAR_MAX];
>      int32_t CP0_MAARI;
>      /* XXX: Maybe make LLAddr per-TC? */
> -    uint64_t lladdr;
> +    target_ulong lladdr; /* LL virtual address compared against SC */
>      target_ulong llval;
>      target_ulong llnewval;
>      target_ulong llreg;
> diff --git a/target/mips/machine.c b/target/mips/machine.c
> index 20100d5..155170c 100644
> --- a/target/mips/machine.c
> +++ b/target/mips/machine.c
> @@ -212,8 +212,8 @@ const VMStateDescription vmstate_tlb = {
>
>  const VMStateDescription vmstate_mips_cpu = {
>      .name = "cpu",
> -    .version_id = 10,
> -    .minimum_version_id = 10,
> +    .version_id = 11,
> +    .minimum_version_id = 11,
>      .post_load = cpu_post_load,
>      .fields = (VMStateField[]) {
>          /* Active TC */
> @@ -283,9 +283,10 @@ const VMStateDescription vmstate_mips_cpu = {
>          VMSTATE_INT32(env.CP0_Config3, MIPSCPU),
>          VMSTATE_INT32(env.CP0_Config6, MIPSCPU),
>          VMSTATE_INT32(env.CP0_Config7, MIPSCPU),
> +        VMSTATE_UINT64(env.CP0_LLAddr, MIPSCPU),
>          VMSTATE_UINT64_ARRAY(env.CP0_MAAR, MIPSCPU, MIPS_MAAR_MAX),
>          VMSTATE_INT32(env.CP0_MAARI, MIPSCPU),
> -        VMSTATE_UINT64(env.lladdr, MIPSCPU),
> +        VMSTATE_UINTTL(env.lladdr, MIPSCPU),
>          VMSTATE_UINTTL_ARRAY(env.CP0_WatchLo, MIPSCPU, 8),
>          VMSTATE_INT32_ARRAY(env.CP0_WatchHi, MIPSCPU, 8),
>          VMSTATE_UINTTL(env.CP0_XContext, MIPSCPU),
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index e537a8b..283669c 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -255,15 +255,15 @@ static inline hwaddr do_translate_address(CPUMIPSState 
> *env,
>                                                        target_ulong address,
>                                                        int rw, uintptr_t 
> retaddr)
>  {
> -    hwaddr lladdr;
> +    hwaddr paddr;
>      CPUState *cs = CPU(mips_env_get_cpu(env));
>
> -    lladdr = cpu_mips_translate_address(env, address, rw);
> +    paddr = cpu_mips_translate_address(env, address, rw);
>
> -    if (lladdr == -1LL) {
> +    if (paddr == -1LL) {
>          cpu_loop_exit_restore(cs, retaddr);
>      } else {
> -        return lladdr;
> +        return paddr;
>      }
>  }
>
> @@ -274,7 +274,8 @@ target_ulong helper_##name(CPUMIPSState *env, 
> target_ulong arg, int mem_idx)  \
>          env->CP0_BadVAddr = arg;                                             
>  \
>          do_raise_exception(env, EXCP_AdEL, GETPC());                         
>  \
>      }                                                                        
>  \
> -    env->lladdr = do_translate_address(env, arg, 0, GETPC());                
>  \
> +    env->CP0_LLAddr = do_translate_address(env, arg, 0, GETPC());            
>  \
> +    env->lladdr = arg;                                                       
>  \
>      env->llval = do_##insn(env, arg, mem_idx, GETPC());                      
>  \
>      return env->llval;                                                       
>  \
>  }
> @@ -294,7 +295,7 @@ target_ulong helper_##name(CPUMIPSState *env, 
> target_ulong arg1,              \
>          env->CP0_BadVAddr = arg2;                                            
>  \
>          do_raise_exception(env, EXCP_AdES, GETPC());                         
>  \
>      }                                                                        
>  \
> -    if (do_translate_address(env, arg2, 1, GETPC()) == env->lladdr) {        
>  \
> +    if (arg2 == env->lladdr) {                                               
>  \
>          tmp = do_##ld_insn(env, arg2, mem_idx, GETPC());                     
>  \
>          if (tmp == env->llval) {                                             
>  \
>              do_##st_insn(env, arg2, arg1, mem_idx, GETPC());                 
>  \
> @@ -873,7 +874,7 @@ target_ulong helper_mftc0_status(CPUMIPSState *env)
>
>  target_ulong helper_mfc0_lladdr(CPUMIPSState *env)
>  {
> -    return (int32_t)(env->lladdr >> env->CP0_LLAddr_shift);
> +    return (int32_t)(env->CP0_LLAddr >> env->CP0_LLAddr_shift);
>  }
>
>  target_ulong helper_mfc0_maar(CPUMIPSState *env)
> @@ -949,7 +950,7 @@ target_ulong helper_dmfc0_tcschefback(CPUMIPSState *env)
>
>  target_ulong helper_dmfc0_lladdr(CPUMIPSState *env)
>  {
> -    return env->lladdr >> env->CP0_LLAddr_shift;
> +    return env->CP0_LLAddr >> env->CP0_LLAddr_shift;
>  }
>
>  target_ulong helper_dmfc0_maar(CPUMIPSState *env)
> @@ -1177,7 +1178,8 @@ void helper_mtc0_tcrestart(CPUMIPSState *env, 
> target_ulong arg1)
>  {
>      env->active_tc.PC = arg1;
>      env->active_tc.CP0_TCStatus &= ~(1 << CP0TCSt_TDS);
> -    env->lladdr = 0ULL;
> +    env->CP0_LLAddr = 0;
> +    env->lladdr = 0;
>      /* MIPS16 not implemented. */
>  }
>
> @@ -1189,12 +1191,14 @@ void helper_mttc0_tcrestart(CPUMIPSState *env, 
> target_ulong arg1)
>      if (other_tc == other->current_tc) {
>          other->active_tc.PC = arg1;
>          other->active_tc.CP0_TCStatus &= ~(1 << CP0TCSt_TDS);
> -        other->lladdr = 0ULL;
> +        other->CP0_LLAddr = 0;
> +        other->lladdr = 0;
>          /* MIPS16 not implemented. */
>      } else {
>          other->tcs[other_tc].PC = arg1;
>          other->tcs[other_tc].CP0_TCStatus &= ~(1 << CP0TCSt_TDS);
> -        other->lladdr = 0ULL;
> +        other->CP0_LLAddr = 0;
> +        other->lladdr = 0;
>          /* MIPS16 not implemented. */
>      }
>  }
> @@ -1620,7 +1624,7 @@ void helper_mtc0_lladdr(CPUMIPSState *env, target_ulong 
> arg1)
>  {
>      target_long mask = env->CP0_LLAddr_rw_bitmask;
>      arg1 = arg1 << env->CP0_LLAddr_shift;
> -    env->lladdr = (env->lladdr & ~mask) | (arg1 & mask);
> +    env->CP0_LLAddr = (env->CP0_LLAddr & ~mask) | (arg1 & mask);
>  }
>
>  #define MTC0_MAAR_MASK(env) \
> @@ -2318,6 +2322,7 @@ static inline void exception_return(CPUMIPSState *env)
>  void helper_eret(CPUMIPSState *env)
>  {
>      exception_return(env);
> +    env->CP0_LLAddr = 1;
>      env->lladdr = 1;
>  }
>
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index d05ee67..c9104a7 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -4913,7 +4913,7 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>      case 17:
>          switch (sel) {
>          case 0:
> -            gen_mfhc0_load64(arg, offsetof(CPUMIPSState, lladdr),
> +            gen_mfhc0_load64(arg, offsetof(CPUMIPSState, CP0_LLAddr),
>                               ctx->CP0_LLAddr_shift);
>              rn = "LLAddr";
>              break;
> @@ -20440,7 +20440,7 @@ void mips_cpu_dump_state(CPUState *cs, FILE *f, 
> fprintf_function cpu_fprintf,
>                  env->CP0_Status, env->CP0_Cause, env->CP0_EPC);
>      cpu_fprintf(f, "    Config0 0x%08x Config1 0x%08x LLAddr 0x%016"
>                  PRIx64 "\n",
> -                env->CP0_Config0, env->CP0_Config1, env->lladdr);
> +                env->CP0_Config0, env->CP0_Config1, env->CP0_LLAddr);
>      cpu_fprintf(f, "    Config2 0x%08x Config3 0x%08x\n",
>                  env->CP0_Config2, env->CP0_Config3);
>      cpu_fprintf(f, "    Config4 0x%08x Config5 0x%08x\n",


--
Alex Bennée