Ping^2 -- could one of the loongarch maintainers have
a look at this problem with the TLB code where it
does an out-of-range shift, please? This is the only
sanitizer failure in 'make check-functional' for the
whole of QEMU, so it would be nice to get this to a
100% pass.
thanks
-- PMM
On Tue, 26 Nov 2024 at 13:13, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> Ping regarding this UB due to an invalid shift --
> I think this is now the only remaining sanitizer error
> in a 'make check-functional', so it would be nice to get
> it fixed...
>
> thanks
> -- PMM
>
> On Thu, 7 Nov 2024 at 17:33, Peter Maydell <peter.mayd...@linaro.org> wrote:
> >
> > On Tue, 7 Jun 2022 at 00:31, Richard Henderson
> > <richard.hender...@linaro.org> wrote:
> > >
> > > From: Xiaojuan Yang <yangxiaoj...@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
