Re: [Qemu-devel] [PATCH 8/9] target/arm: Implement CAS and CASP

[Peter Maydell]

On 3 May 2018 at 18:32, Richard Henderson <address@hidden> wrote:
> On 05/03/2018 07:55 AM, Peter Maydell wrote:
>>> +        /* If compare equal, write back new data, else write back old 
>>> data.  */
>>> +        tcg_gen_movcond_i64(TCG_COND_NE, c1, c2, zero, t1, d1);
>>> +        tcg_gen_movcond_i64(TCG_COND_NE, c2, c2, zero, t2, d2);
>>> +        tcg_gen_qemu_st_i64(c1, addr, memidx, MO_64 | s->be_data);
>>> +        tcg_gen_qemu_st_i64(c2, a2, memidx, MO_64 | s->be_data);
>>
>> I think this has the wrong behaviour if you do a CASP-with-mismatched-value
>> to read-only memory -- architecturally this should fail the comparison
>> and return the memory value in registers, it's not allowed to do a
>> memory write and take a data abort because the memory isn't writable.
>
> If this is true [...]

Fortunately it turns out that it is not true. In the pseudocode,
the CAS accesses are made with an acctype of either AccType_ORDEREDRW
or AccType_ATOMICRW, and when this gets down into the translation table
walk code AArch64.CheckPermission() handles those access types
specially and generates a permission fault on !r || !w, so will fault
the read part of the read-modify-write.

I wouldn't be too surprised if we get the ESR_ELx WnR bit wrong
for this (it is supposed to be "0 if a plain read would fault,
otherwise 1"), but let's not worry about that for now...

Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM