Re: [qemu-s390x] [Qemu-devel] [PATCH v2 04/28] s390x/tcg: MVCL: Process max 2k bytes at a time

[Richard Henderson]

On 9/6/19 3:57 AM, David Hildenbrand wrote:
> Process max 2k bytes at a time, writing back registers between the
> accesses. The instruction is interruptible.
>     "For operands longer than 2K bytes, access exceptions are not
>     recognized for locations more than 2K bytes beyond the current location
>     being processed."
> 
> MVCL handling is quite different than MVCLE/MVCLU handling, so split up
> the handlers.
> 
> We'll deal with fast_memmove() and fast_memset() not probing
> reads/writes properly later. Also, we'll defer interrupt handling, as
> that will require more thought, add a TODO for that.
> 
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  target/s390x/mem_helper.c | 44 +++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 2361ed6d54..2e22c183bd 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -799,19 +799,51 @@ uint32_t HELPER(mvcl)(CPUS390XState *env, uint32_t r1, 
> uint32_t r2)
>      uint64_t srclen = env->regs[r2 + 1] & 0xffffff;
>      uint64_t src = get_address(env, r2);
>      uint8_t pad = env->regs[r2 + 1] >> 24;
> -    uint32_t cc;
> +    uint32_t cc, cur_len;
>  
>      if (is_destructive_overlap(env, dest, src, MIN(srclen, destlen))) {
>          cc = 3;
> +    } else if (srclen == destlen) {
> +        cc = 0;
> +    } else if (destlen < srclen) {
> +        cc = 1;
>      } else {
> -        cc = do_mvcl(env, &dest, &destlen, &src, &srclen, pad, 1, ra);
> +        cc = 2;
>      }
>  
> -    env->regs[r1 + 1] = deposit64(env->regs[r1 + 1], 0, 24, destlen);
> -    env->regs[r2 + 1] = deposit64(env->regs[r2 + 1], 0, 24, srclen);
> -    set_address_zero(env, r1, dest);
> -    set_address_zero(env, r2, src);
> +    /* We might have to zero-out some bits even if there was no action. */
> +    if (unlikely(!destlen || cc == 3)) {
> +        set_address_zero(env, r2, src);
> +        set_address_zero(env, r1, dest);
> +        return cc;
> +    } else if (!srclen) {
> +        set_address_zero(env, r2, src);
> +    }
>  
> +    /*
> +     * Only perform one type of type of operation (move/pad) in one step.
> +     * Process up to 2k bytes.
> +     */
> +    while (destlen) {
> +        cur_len = MIN(destlen, 2048);

The language in the PoP is horribly written, and thus confusing.  I can't
believe it really means what it appears to say, about access exceptions not
being recognized.

The code within Hercules breaks the action at every 2k address boundary -- for
both src and dest.  That's the only way that actually makes sense to me, as
otherwise we end up allowing userspace to read/write into a page without
permission.  Which is a security hole.


r~