Re: [PATCH 3/3] target/riscv: fix the trap generation for conditional store

[Richard Henderson]

On 12/13/24 10:31, Konrad, Frederic wrote:
> +CC maintainers
>
> > -----Original Message-----
> > From: qemu-devel-bounces+fkonrad=amd.com@nongnu.org 
> > <qemu-devel-bounces+fkonrad=amd.com@nongnu.org> On
> > Behalf Of Richard Henderson
> > Sent: 11 December 2024 22:43
> > To: qemu-devel@nongnu.org
> > Subject: Re: [PATCH 3/3] target/riscv: fix the trap generation for conditional 
> > store
> >
> > On 12/11/24 15:19, Frederic Konrad wrote:
> > > +    /*
> > > +     * A misaligned store trap should be triggered even if the store should
> > > +     * fail due to the reservation.
> > > +     */
> > > +    tcg_gen_andi_tl(tmp, src1, ~((uint64_t)0) << memop_alignment_bits(mop));
> >
> > The constant is incorrect for testing the low bits.
>
> Hmm, I don't get it, basically with that I'm trying to do:
>    MO_8: src1 == (src1 & 0xFFFFFFFF)
>    MO_16: src1 == (src1 & 0xFFFFFFFE)
>    MO_32: src1 == (src1 & 0xFFFFFFFC)
>    etc
>
> what am I missing?
>
> > > +    tcg_gen_brcond_tl(TCG_COND_EQ, tmp, src1, l3);

Ah, I missed the form of the equality.  I had been expecting

    tmp = src1 & 1
    brcond tmp != 0

and so mis-read the mask.

> About making the fallthrough path be the common case, If I do it I'll need to 
> jump anyway and the
> end of this instruction ie:
>
>      if not aligned go to misaligned label:
>      ...
>      do the normal operation
>      ...
>      go to done label
>      misaligned label
>      ...
>      trigger the exception
>      ...
>      done label
>
> Is that what you had in mind?

There is another code block in there.  We can sort as

        if not aligned goto misaligned;
        if reservation mismatch goto mismatch

        normal operation
        goto done

        misaligned:
        raise exception

        mismatch:
        mb, set failure

        done:
        clear reservation

so that we save adding a branch on the mismatch path.


r~