Re: [PATCH 5/5] target/loongarch/tcg: Add hardware page table walker support

[Richard Henderson]

On 9/5/24 01:27, gaosong wrote:
> How about adding a variable to determine if tlb needs to be modified?
> like this:
>
> @@ -248,7 +250,7 @@ hwaddr loongarch_cpu_get_phys_page_debug(CPUState *cs, 
> vaddr addr)
>       int prot;
>
>       if (get_physical_address(env, &phys_addr, &prot, addr, MMU_DATA_LOAD,
> -                             cpu_mmu_index(cs, false)) != 0) {
> +                             cpu_mmu_index(cs, false) != 0, false)) {
>           return -1;
>       }

Yes, that sort of thing.  In other targets the flags is called 'debug'.

> > > +        entry = ldq_phys(cs->as, tmp0) & TARGET_PHYS_MASK;
> > > +
> > > +        if (entry == 0) {
> > > +            return ret;
> > > +        }
> > > +
> > > +        /* Check entry, and do tlb modify. */
> > > +        if ((tlb_error == TLBRET_INVALID) &&
> > > +            (access_type == MMU_DATA_LOAD ||
> > > +             access_type == MMU_INST_FETCH )) {
> > > +            if (!(FIELD_EX64(entry, TLBENTRY, PRESENT))) {
> > > +                break;
> > > +            }
> > > +            entry = FIELD_DP64(entry, TLBENTRY, V, 1);
> > > +        } else if ((tlb_error == TLBRET_INVALID) &&
> > > +                   access_type == MMU_DATA_STORE) {
> > > +            if (!((FIELD_EX64(entry, TLBENTRY, PRESENT) &&
> > > +                  (FIELD_EX64(entry, TLBENTRY, WRITE))))){
> > > +                break;
> > > +            }
> > > +            entry = FIELD_DP64(entry, TLBENTRY, V, 1);
> > > +            entry = FIELD_DP64(entry, TLBENTRY, D, 1);
> > > +        } else if (tlb_error ==  TLBRET_DIRTY) {
> > > +            if (!(FIELD_EX64(entry, TLBENTRY, WRITE))) {
> > > +                break;
> > > +            }
> > > +            entry = FIELD_DP64(entry, TLBENTRY, D, 1);
> > > +            entry = FIELD_DP64(entry, TLBENTRY, V, 1);
> > > +        }
> > > +        stq_phys(cs->as, tmp0, entry);
> >
> > You certainly want to use a compare and swap here, restarting if the compare 
> > fails.
> Sorry ,  I don't understand here, could you explain it in detail?

A plain store will have an smp race condition with the guest kernel.
The update needs to be atomic.

Compare:

  target/arm/ptw.c, arm_casq_ptw()
  target/riscv/cpu_helper.c, get_physical_address(), s/cmpxchg/
  target/i386/tcg/sysemu/excp_helper.c, ptw_setl()


r~