qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PULL 27/43] target/loongarch: Add TLB instruction support


From: Peter Maydell
Subject: Re: [PULL 27/43] target/loongarch: Add TLB instruction support
Date: Thu, 7 Nov 2024 17:33:30 +0000

On Tue, 7 Jun 2022 at 00:31, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>
> This includes:
> - TLBSRCH
> - TLBRD
> - TLBWR
> - TLBFILL
> - TLBCLR
> - TLBFLUSH
> - INVTLB

Hi; running the loongarch functional tests on a build with
the clang undefined-behaviour sanitizer enabled reveals an
attempt to shift by an out-of-range amount in
helper_invtlb_page_asid_or_g():

../../target/loongarch/tcg/tlb_helper.c:470:31: runtime error: shift
exponent 244 is too large for 64-bit type 'uint64_t' (aka 'unsigned
long')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../../target/loongarch/tcg/tlb_helper.c:470:31 in

> +void helper_invtlb_page_asid_or_g(CPULoongArchState *env,
> +                                  target_ulong info, target_ulong addr)
> +{
> +    uint16_t asid = info & 0x3ff;
> +
> +    for (int i = 0; i < LOONGARCH_TLB_MAX; i++) {
> +        LoongArchTLB *tlb = &env->tlb[i];
> +        uint8_t tlb_g = FIELD_EX64(tlb->tlb_entry0, TLBENTRY, G);
> +        uint16_t tlb_asid = FIELD_EX64(tlb->tlb_misc, TLB_MISC, ASID);
> +        uint64_t vpn, tlb_vppn;
> +        uint8_t tlb_ps, compare_shift;
> +
> +        if (i >= LOONGARCH_STLB) {
> +            tlb_ps = FIELD_EX64(tlb->tlb_misc, TLB_MISC, PS);
> +        } else {
> +            tlb_ps = FIELD_EX64(env->CSR_STLBPS, CSR_STLBPS, PS);
> +        }

We read here a field from the guest, which can be 0.

> +        tlb_vppn = FIELD_EX64(tlb->tlb_misc, TLB_MISC, VPPN);
> +        vpn = (addr & TARGET_VIRT_MASK) >> (tlb_ps + 1);
> +        compare_shift = tlb_ps + 1 - R_TLB_MISC_VPPN_SHIFT;

If tlb_ps is 0, then "tlb_ps + 1 - R_TLB_MISC_VPPN_SHIFT"
is tlb_ps + 1 - 13 == tlb_ps - 12. When converted back to
uint8_t this is 244.

> +
> +        if ((tlb_g || (tlb_asid == asid)) &&
> +            (vpn == (tlb_vppn >> compare_shift))) {

Here we shift tlb_vppn by 244, which is undefined behaviour
and triggers the sanitizer.

> +            tlb->tlb_misc = FIELD_DP64(tlb->tlb_misc, TLB_MISC, E, 0);
> +        }
> +    }
> +    tlb_flush(env_cpu(env));
> +}

What's the intended behaviour here?

This likely applies also to other similar functions; this
is just the one that I found.

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]