Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue

[lixianglai]

Hi Richard:
> On 3/13/24 15:33, Xianglai Li wrote:
> > +    if (unlikely((level == 0) || (level > 4))) {
> > +        return base;
> > +    }
> > +
> > +    if (FIELD_EX64(base, TLBENTRY, HUGE)) {
> > +        if (FIELD_EX64(base, TLBENTRY, LEVEL)) {
> > +            return base;
> > +        } else {
> > +            return  FIELD_DP64(base, TLBENTRY, LEVEL, level);
> > +        }
> > +
> > +        if (unlikely(level == 4)) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "Attempted use of level %lu huge page\n", 
> > level);
> > +        }
>
> This block is unreachable, because you've already returned.
> Perhaps it would be worthwhile to add another for the level==0 or > 4 
> case above?
A normal level 4 page table should not print an error log,

only if a level 4 page is large, so we should put it in

    if (FIELD_EX64(base, TLBENTRY, HUGE)) {
        if (unlikely(level == 4)) {
            qemu_log_mask(LOG_GUEST_ERROR,
                          "Attempted use of level %lu huge page\n", level);
        }

        if (FIELD_EX64(base, TLBENTRY, LEVEL)) {
            return base;
        } else {
            return  FIELD_DP64(base, TLBENTRY, LEVEL, level);
        }
    }


Thanks!

Xianglai.


> > @@ -530,20 +553,34 @@ void helper_ldpte(CPULoongArchState *env, 
> > target_ulong base, target_ulong odd,
> >       CPUState *cs = env_cpu(env);
> >       target_ulong phys, tmp0, ptindex, ptoffset0, ptoffset1, ps, badv;
> >       int shift;
> > -    bool huge = (base >> LOONGARCH_PAGE_HUGE_SHIFT) & 0x1;
> >       uint64_t ptbase = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTBASE);
> >       uint64_t ptwidth = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTWIDTH);
> > +    uint64_t dir_base, dir_width;
> >         base = base & TARGET_PHYS_MASK;
> > +    if (FIELD_EX64(base, TLBENTRY, HUGE)) {
> > +        /*
> > +         * Gets the huge page level and Gets huge page size
> > +         * Clears the huge page level information in the address
> > +         * Clears huge page bit
> > +         */
> > +        get_dir_base_width(env, &dir_base, &dir_width,
> > +                           FIELD_EX64(base, TLBENTRY, LEVEL));
> > +
> > +        FIELD_DP64(base, TLBENTRY, LEVEL, 0);
> > +        FIELD_DP64(base, TLBENTRY, HUGE, 0);
> > +        if (FIELD_EX64(base, TLBENTRY, HG)) {
> > +            FIELD_DP64(base, TLBENTRY, HG, 0);
> > +            FIELD_DP64(base, TLBENTRY, G, 1);
>
> FIELD_DP64 returns a value.  You need
>
>     base = FIELD_DP64(base, ...);
>
>
> r~