Re: [PATCH] feat: add loongarch page table walker support for debugger memory access

[bibo mao]

On 2024/12/31 上午10:08, Miao Hao wrote:
> On 2024/12/31 09:18, bibo mao wrote:
> > On 2024/12/31 上午9:09, bibo mao wrote:
> > > On 2024/12/30 下午3:04, Miao Hao wrote:
> > > > Hi Bibo,
> > > >
> > > > Thanks for your review. I apologize for my late respond due to some 
> > > > personal reasons.
> > > >
> > > > On 2024/12/19 17:57, bibo mao wrote:
> > > > > Hi Miao,
> > > > >
> > > > > Thanks for doing this. It is useful to debug VM.
> > > > >
> > > > > On 2024/12/19 上午11:24, Miao Hao wrote:
> > > > > > Signed-off-by: Miao Hao <haomiao23s@ict.ac.cn>
> > > > > > ---
> > > > > >   target/loongarch/cpu_helper.c     | 104 
> > > > > > ++++++++++++++++++++++++++++--
> > > > > >   target/loongarch/internals.h      |   4 +-
> > > > > >   target/loongarch/tcg/tlb_helper.c |   4 +-
> > > > > >   3 files changed, 104 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/target/loongarch/cpu_helper.c 
> > > > > > b/target/loongarch/cpu_helper.c
> > > > > > index 580362ac3e..c0828a813d 100644
> > > > > > --- a/target/loongarch/cpu_helper.c
> > > > > > +++ b/target/loongarch/cpu_helper.c
> > > > > > @@ -141,9 +141,95 @@ bool loongarch_tlb_search(CPULoongArchState 
> > > > > > *env, target_ulong vaddr,
> > > > > >       return false;
> > > > > >   }
> > > > > >   +static int loongarch_page_table_walker(CPULoongArchState *env, 
> > > > > > hwaddr *physical,
> > > > > > +                                 int *prot, target_ulong address)
> > > > > > +{
> > > > > > +    CPUState *cs = env_cpu(env);
> > > > > > +    target_ulong index, phys;
> > > > > > +    int shift;
> > > > > > +    uint64_t dir_base, dir_width;
> > > > > > +    uint64_t base;
> > > > > > +    int level;
> > > > > > +
> > > > > > +    /* 0:64bit, 1:128bit, 2:192bit, 3:256bit */
> > > > > > +    shift = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTEWIDTH);
> > > > > > +    shift = (shift + 1) * 3;
> > > >
> > > > the assignment of variable shift and the corresponding comment is 
> > > > incorrect here, and details are logged in the v1.03 change log of 
> > > > LoongArch specification volume1 
> > > > (https://loongson.cn/uploads/images/2023102309132647981.%E9%BE%99%E8%8A%AF%E6%9E%B6%E6%9E%84%E5%8F%82%E8%80%83%E6%89%8B%E5%86%8C%E5%8D%B7%E4%B8%80_r1p10.pdf) 
> > > >
> > > >
> > > >      /* 0:64bit, 1:128bit, 2:256bit, 3:512bit */
> > > >      shift = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTEWIDTH);
> > > >      shift = shift + 3;
> > > Ok, I see.
> > > It seems that this is right, thanks for the detailed explanation.
> > >
> > > > > > +
> > > > > > +    if ((address >> 63) & 0x1) {
> > > > > > +        base = env->CSR_PGDH;
> > > > > > +    } else {
> > > > > > +        base = env->CSR_PGDL;
> > > > > > +    }
> > > > > > +    base &= TARGET_PHYS_MASK;
> > > > > > +
> > > > > > +    for (level = 4; level > 0; level--) {
> > > > > > +        get_dir_base_width(env, &dir_base, &dir_width, level);
> > > > > > +
> > > > > > +        if (dir_width != 0) {
> > > > > how about check whether it equeal to 0 firstly like this?
> > > > >            if (dir_width == 0)
> > > > >                continue;
> > > > It's good to reduce code nesting, I will adopt this suggestion.
> > > > > > +            /* get next level page directory */
> > > > > > +            index = (address >> dir_base) & ((1 << dir_width) - 1);
> > > > > > +            phys = base | index << shift;
> > > > > Here will only load first 64bit if shift is not 0, such as 
> > > > > 1:128bit, 2:192bit, 3:256bit
> > > > After fixing the assignment of shift, this issue no longer exists. 
> > > > Shift is less than or equal to 6, and index is 6 bit. Thus, index << 
> > > > shift is at most 12 bit.
> > > Supposing one pte entry is 128bit, value of shift is 4.
> > >     phys = base | index << shift;  will be the same as
> > >     phys = base | index * 16;
> > > however all accessed pte entry is 16 bytes aligned, pte entry with 
> > > "phys = base | index * 16 + 8" will never be accessed. Is that right?
> I see what you mean, but since all accessed pte entry is 16 bytes 
> aligned, why there is a pte entry with "phys = base | index * 16 + 8"? 
> On the other hand, a 128 bit pte entry remains undefined in LoongArch 
> specification volume1, which only defines 64 bit pte entry (in section 
> 5.4.5). Perhaps the CSR_PWCL.PTEWIDTH is for future extension, so I 
> think we can just leave it alone since it works well on 64 bit pte.
IIRC, for 128 bit pte entry, it presents two pages. physical address of 
page0 is located phys = base | index * 16, page1 can be located at
  phys = base | index * 16 + 8

If we want to only care about 64 bit pte, the following sentences are 
suggested to removed.
   +    /* 0:64bit, 1:128bit, 2:192bit, 3:256bit */
   +    shift = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTEWIDTH);
   +    shift = (shift + 1) * 3;

Regards
Bibo Mao
> > > I think it should be something like this.
> > >        index = (address >> dir_base) & ((1 << (dir_width + shift) - 1);
> > >        phys = base | index << 3;
> > Sorry, it should be shift - 3.
> >  index = (address >> dir_base) & (1 << (dir_width + shift - 3) - 1);
> >  phys = base | index << 3;
> In my understanding, assignment "index = (address >> dir_base) & ((1 << 
> dir_width) - 1)" retrieves the corresponding bits in the virtual address 
> set in CSR_PWC[L|H] as the index (Figure 5-2 in LoongArch specification 
> volume1), and CSR_PWCL.PTEWIDTH pads zeros to retrieved bits to align.
>
> Here's an example, a 3 level page table with 16KB pages in Loongarch 
> linux setup:
>
> virtual address:
>
> +---------------+---------------+---------------+---------------+
>
> |46               36|35               25|24 14|13                 0|
>
> +---------------+---------------+---------------+---------------+
>
>     dir3_index    dir1_index    pt_index      page_offset
>
> CSR_PWCL:
>
> |00|00000|00000|11001|01011|01110|01011|
>
> CSR_PWCH:
>
> |0000000|0|000000|000000|100100|001011|
>
> Note that dir3_index, dir1_index and pt_index are 11 bit and 
> CSR_PWCL.PTEWidth=0. To index a 64 bit pte in the 16KB(2^14B) page 
> table, we should pad (0 + 3) zeros to pt_index.
>
>
> Regards
>
> Miao, Hao
>
> > > Regards
> > > Bibo, Mao
> > > > > > +            base = ldq_phys(cs->as, phys) & TARGET_PHYS_MASK;
> > > > > > +            if (!FIELD_EX64(base, TLBENTRY, HUGE)) {
> > > > > > +                /* mask off page dir permission bits */
> > > > > > +                base &= TARGET_PAGE_MASK;
> > > > > > +            } else {
> > > > > > +                /* base is a huge pte */
> > > > > > +                break;
> > > > > > +            }
> > > > > > +
> > > > > > +            if (base == 0) {
> > > > > physical adddress 0 is valid and Valid bit will be checked in 
> > > > > later. Can we remove this?
> > > > the value of base equals to 0 means that the current page directory 
> > > > entry does not point to next level page directory, so we return here
> > > > > > +                return TLBRET_NOMATCH;
> > > > > > +            }
> > > > >
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > > +    /* pte */
> > > > > > +    if (FIELD_EX64(base, TLBENTRY, HUGE)) {
> > > > > > +        /* Huge Page. base is pte */
> > > > > > +        base = FIELD_DP64(base, TLBENTRY, LEVEL, 0);
> > > > > > +        base = FIELD_DP64(base, TLBENTRY, HUGE, 0);
> > > > > > +        if (FIELD_EX64(base, TLBENTRY, HGLOBAL)) {
> > > > > > +            base = FIELD_DP64(base, TLBENTRY, HGLOBAL, 0);
> > > > > > +            base = FIELD_DP64(base, TLBENTRY, G, 1);
> > > > > > +        }
> > > > > > +    } else {
> > > > > > +        /* Normal Page. base points to pte */
> > > > > > +        get_dir_base_width(env, &dir_base, &dir_width, 0);
> > > > > > +        index = (address >> dir_base) & ((1 << dir_width) - 1);
> > > > > > +        phys = base | index << shift;
> > > > > Ditto, shift may be wider than 64-bit.
> > > > >
> > > > > Regards
> > > > > Bibo Mao
> > > >
> > > > Ditto, shift is less than or equal to 6.
> > > >
> > > >
> > > > Regards
> > > >
> > > > Miao Hao
> > > >
> > > > > > +        base = ldq_phys(cs->as, phys);
> > > > > > +    }
> > > > > > +
> > > > > > +    /* TODO: check plv and other bits? */
> > > > > > +
> > > > > > +    /* base is pte, in normal pte format */
> > > > > > +    if (!FIELD_EX64(base, TLBENTRY, V)) {
> > > > > > +        return TLBRET_NOMATCH;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (!FIELD_EX64(base, TLBENTRY, D)) {
> > > > > > +        *prot = PAGE_READ;
> > > > > > +    } else {
> > > > > > +        *prot = PAGE_READ | PAGE_WRITE;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* get TARGET_PAGE_SIZE aligned physical address */
> > > > > > +    base += (address & TARGET_PHYS_MASK) & ((1 << dir_base) - 1);
> > > > > > +    /* mask RPLV, NX, NR bits */
> > > > > > +    base = FIELD_DP64(base, TLBENTRY_64, RPLV, 0);
> > > > > > +    base = FIELD_DP64(base, TLBENTRY_64, NX, 0);
> > > > > > +    base = FIELD_DP64(base, TLBENTRY_64, NR, 0);
> > > > > > +    /* mask other attribute bits */
> > > > > > +    *physical = base & TARGET_PAGE_MASK;
> > > > > > +
> > > > > > +    return 0;
> > > > > > +}
> > > > > > +
> > > > > >   static int loongarch_map_address(CPULoongArchState *env, hwaddr 
> > > > > > *physical,
> > > > > >                                    int *prot, target_ulong address,
> > > > > > -                                 MMUAccessType access_type, int 
> > > > > > mmu_idx)
> > > > > > +                                 MMUAccessType access_type, int 
> > > > > > mmu_idx,
> > > > > > +                                 int is_debug)
> > > > > >   {
> > > > > >       int index, match;
> > > > > >   @@ -151,6 +237,13 @@ static int 
> > > > > > loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
> > > > > >       if (match) {
> > > > > >           return loongarch_map_tlb_entry(env, physical, prot,
> > > > > >                                          address, access_type, 
> > > > > > index, mmu_idx);
> > > > > > +    } else if (is_debug) {
> > > > > > +        /*
> > > > > > +         * For debugger memory access, we want to do the map when 
> > > > > > there is a
> > > > > > +         * legal mapping, even if the mapping is not yet in TLB. 
> > > > > > return 0 if
> > > > > > +         * there is a valid map, else none zero.
> > > > > > +         */
> > > > > > +        return loongarch_page_table_walker(env, physical, prot, 
> > > > > > address);
> > > > > >       }
> > > > > >         return TLBRET_NOMATCH;
> > > > > > @@ -158,7 +251,8 @@ static int 
> > > > > > loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
> > > > > >   #else
> > > > > >   static int loongarch_map_address(CPULoongArchState *env, hwaddr 
> > > > > > *physical,
> > > > > >                                    int *prot, target_ulong address,
> > > > > > -                                 MMUAccessType access_type, int 
> > > > > > mmu_idx)
> > > > > > +                                 MMUAccessType access_type, int 
> > > > > > mmu_idx,
> > > > > > +                                 int is_debug)
> > > > > >   {
> > > > > >       return TLBRET_NOMATCH;
> > > > > >   }
> > > > > > @@ -178,7 +272,7 @@ static hwaddr dmw_va2pa(CPULoongArchState 
> > > > > > *env, target_ulong va,
> > > > > >     int get_physical_address(CPULoongArchState *env, hwaddr 
> > > > > > *physical,
> > > > > >                            int *prot, target_ulong address,
> > > > > > -                         MMUAccessType access_type, int mmu_idx)
> > > > > > +                         MMUAccessType access_type, int mmu_idx, 
> > > > > > int is_debug)
> > > > > >   {
> > > > > >       int user_mode = mmu_idx == MMU_USER_IDX;
> > > > > >       int kernel_mode = mmu_idx == MMU_KERNEL_IDX;
> > > > > > @@ -222,7 +316,7 @@ int get_physical_address(CPULoongArchState 
> > > > > > *env, hwaddr *physical,
> > > > > >         /* Mapped address */
> > > > > >       return loongarch_map_address(env, physical, prot, address,
> > > > > > -                                 access_type, mmu_idx);
> > > > > > +                                 access_type, mmu_idx, is_debug);
> > > > > >   }
> > > > > >     hwaddr loongarch_cpu_get_phys_page_debug(CPUState *cs, vaddr 
> > > > > > addr)
> > > > > > @@ -232,7 +326,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), 1) != 0) {
> > > > > >           return -1;
> > > > > >       }
> > > > > >       return phys_addr;