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 <david@redhat.com>
---
 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);
+        if (!srclen) {
+            fast_memset(env, dest, pad, cur_len, ra);
+        } else {
+            cur_len = MIN(cur_len, srclen);
+
+            fast_memmove(env, dest, src, cur_len, ra);
+            src = wrap_address(env, src + cur_len);
+            srclen -= cur_len;
+            env->regs[r2 + 1] = deposit64(env->regs[r2 + 1], 0, 24, srclen);
+            set_address_zero(env, r2, src);
+        }
+        dest = wrap_address(env, dest + cur_len);
+        destlen -= cur_len;
+        env->regs[r1 + 1] = deposit64(env->regs[r1 + 1], 0, 24, destlen);
+        set_address_zero(env, r1, dest);
+
+        /* TODO: Deliver interrupts. */
+    }
     return cc;
 }
 
-- 
2.21.0

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 <david@redhat.com>
> ---
>  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~

On 9/11/19 10:52 AM, Richard Henderson wrote:
> 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.

Also, doesn't "2k" come from the old esa/360 page size?

Which means that we could break at 4k pages instead of 2k now
and the program wouldn't really be able to tell the difference.


r~

On 11.09.19 17:07, Richard Henderson wrote:
> On 9/11/19 10:52 AM, Richard Henderson wrote:
>> 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.
> 
> Also, doesn't "2k" come from the old esa/360 page size?

I have no idea, I was very confused with that.

> 
> Which means that we could break at 4k pages instead of 2k now
> and the program wouldn't really be able to tell the difference.

What I had in a previous iteration was to simply process until the end
of the page(s), to not cross pages (there is one special case with 2k
vs. 4k when crossing pages when wrapping and running into a
low-address-protection). So essentially what you suggest. I can add that.

-- 

Thanks,

David / dhildenb