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

[Richard Henderson <richard.henderson@linaro.org>]

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~