Re: [Qemu-devel] [PATCH 04/13] target-ppc: implement lxvll instruction

[Richard Henderson]

On 12/05/2016 09:52 AM, Richard Henderson wrote:
> On 12/05/2016 03:25 AM, Nikunj A Dadhania wrote:
>> +void helper_lxvll(CPUPPCState *env, target_ulong addr,
>> +                  target_ulong xt_num, target_ulong rb)
>> +{
>> +    ppc_vsr_t xt;
>> +
>> +    getVSR(xt_num, &xt, env);
>> +    if (unlikely((rb & 0xFF) == 0)) {
>> +        xt.s128 = int128_make128(0, 0);
> 
> Nit: int128_zero.
> 
>> +    } else {
>> +        target_ulong end = ((rb & 0xFF) * 8) - 1;
>> +        if (msr_le) {
>> +            xt.u64[LO_IDX] = cpu_ldq_data_ra(env, addr, GETPC());
>> +            addr = addr_add(env, addr, 8);
>> +            xt.u64[HI_IDX] = cpu_ldq_data_ra(env, addr, GETPC());
>> +            xt.s128 = int128_and(xt.s128, mask_u128(127 - end, 127));
> 
> The ISA document says that this is a sequence of byte operations.  Which means
> that END < 127 will not access bytes outside of the length.  Which means that
> your code will trigger SIGSEGV near page boundaries when real hardware won't.
> 
> I also don't see how this does the right thing for little-endian.

Oh, and one more thing:

Do you need to perform the permission check on all NB bytes before writing any
of them?  I suspect that real hardware does, otherwise the instruction might
not be restartable.

Are there any atomicity guarantees made by real hardware?  If so, you may need
to implement this differently.  If not, a comment to that effect would be 
helpful.


r~