| Message ID | 20170525210508.4910-16-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
On 05/25/2017 02:04 PM, Aurelien Jarno wrote: > There are multiple issues with the COMPARE LOGICAL LONG EXTENDED > instruction: > - The test between the two operands is inverted, leading to an inversion > of the cc values 1 and 2. > - The address and length of an operand continue to be decreased after > reaching the end of this operand. These values are then wrong write > back to the registers. > - We should limit the amount of bytes to process, so that interrupts can > be served correctly. > > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > --- > target/s390x/mem_helper.c | 36 ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 1dc71fe5f0..bd3bce3623 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -716,28 +716,48 @@ uint32_t HELPER(clcle)(CPUS390XState *env, uint32_t r1, uint64_t a2, > uint64_t srclen = get_length(env, r3 + 1); > uint64_t src = get_address(env, r3); > uint8_t pad = a2 & 0xff; > + uint64_t len = MAX(srclen, destlen); > uint32_t cc = 0; > > if (!(destlen || srclen)) { Might as well replace this with len == 0 now. > if (v1 != v2) { > - cc = (v1 < v2) ? 1 : 2; > + cc = (v1 > v2) ? 1 : 2; This would have been less confusing, perhaps, if we'd not used "src" and "dest" -- and doubly so since they are two sources -- but rather "src1" and "src3", which corresponds better to the PoO text. That said, Reviewed-by: Richard Henderson <rth@twiddle.net> r~
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 1dc71fe5f0..bd3bce3623 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -716,28 +716,48 @@ uint32_t HELPER(clcle)(CPUS390XState *env, uint32_t r1, uint64_t a2, uint64_t srclen = get_length(env, r3 + 1); uint64_t src = get_address(env, r3); uint8_t pad = a2 & 0xff; + uint64_t len = MAX(srclen, destlen); uint32_t cc = 0; if (!(destlen || srclen)) { return cc; } - if (srclen > destlen) { - srclen = destlen; + /* Lest we fail to service interrupts in a timely manner, limit the + amount of work we're willing to do. For now, let's cap at 8k. */ + if (len > 0x2000) { + len = 0x2000; + cc = 3; } - for (; destlen || srclen; src++, dest++, destlen--, srclen--) { - uint8_t v1 = srclen ? cpu_ldub_data_ra(env, src, ra) : pad; - uint8_t v2 = destlen ? cpu_ldub_data_ra(env, dest, ra) : pad; + for (; len; len--) { + uint8_t v1 = pad; + uint8_t v2 = pad; + + if (srclen) { + v1 = cpu_ldub_data_ra(env, src, ra); + } + if (destlen) { + v2 = cpu_ldub_data_ra(env, dest, ra); + } + if (v1 != v2) { - cc = (v1 < v2) ? 1 : 2; + cc = (v1 > v2) ? 1 : 2; break; } + + if (srclen) { + src++; + srclen--; + } + if (destlen) { + dest++; + destlen--; + } } set_length(env, r1 + 1, destlen); - /* can't use srclen here, we trunc'ed it */ - set_length(env, r3 + 1, env->regs[r3 + 1] - src - env->regs[r3]); + set_length(env, r3 + 1, srclen); set_address(env, r1, dest); set_address(env, r3, src);
There are multiple issues with the COMPARE LOGICAL LONG EXTENDED instruction: - The test between the two operands is inverted, leading to an inversion of the cc values 1 and 2. - The address and length of an operand continue to be decreased after reaching the end of this operand. These values are then wrong write back to the registers. - We should limit the amount of bytes to process, so that interrupts can be served correctly. Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- target/s390x/mem_helper.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)