Re: [qemu-s390x] [Qemu-devel] [PATCH v2 16/28] s390x/tcg: Fault-safe memmove

[David Hildenbrand]

On 12.09.19 00:03, Richard Henderson wrote:
> On 9/6/19 3:57 AM, David Hildenbrand wrote:
>> +static void access_memmove_idx(CPUS390XState *env, vaddr dest, vaddr src,
>> +                               int size, int dest_idx, int src_idx,
>> +                               uintptr_t ra)
>> +{
>> +    S390Access srca = access_prepare_idx(env, src, size, MMU_DATA_LOAD, 
>> src_idx,
>> +                                         ra);
>> +    S390Access desta = access_prepare_idx(env, dest, size, MMU_DATA_STORE,
>> +                                          dest_idx, ra);
> 
> I was just thinking that it might be worth passing in these Access structures.
>  It seems that usually we wind up computing them in several locations.
> 
> Hoisting it up it would make MVC look like
> 
>     midx = cpu_mmu_index(env);
>     srca = access_prepare_idx(env, src, size, LOAD, midx, ra);
>     dsta = access_prepare_idx(env, dst, size, STORE, midx, ra);
> 
>     if (dst == src + 1) {
>         uint8_t x = access_get_byte(env, &srca, 0, ra);
>         access_memset(env, &dsta, x, size, ra);
>     } else if (!is_destructive_overlap(env, dst, src, size)) {
>         access_memmove(env, &dsta, &srca, size, ra);
>     } else {
>         // byte by byte loop, but still need srca, dsta.
>     }
> 
> which seems even More Correct, since the current access_memset path does not
> check for read watchpoints or faults on all of [src, src+size-1].
> 

I had precisely that in previous versions :) Can switch to that model.

-- 

Thanks,

David / dhildenb