Re: [Qemu-devel] [PATCH 5/6] target/s390x: fix PACK reading 1 byte less and writing 1 byte more

[David Hildenbrand <david@redhat.com>]

On 05.08.2018 20:28, Pavel Zbitskiy wrote:
> PACK fails on the test from the Principles of Operation: F1F2F3F4
> becomes 0000234C instead of 0001234C due to an off-by-one error.
> Furthermore, it overwrites one extra byte to the left of F1.
> 
> Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> ---
>  target/s390x/mem_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 704d0193b5..bacae4f503 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -1019,15 +1019,15 @@ void HELPER(pack)(CPUS390XState *env, uint32_t len, uint64_t dest, uint64_t src)
>      len_src--;
>  
>      /* now pack every value */
> -    while (len_dest >= 0) {
> +    while (len_dest > 0) {
>          b = 0;
>  
> -        if (len_src > 0) {
> +        if (len_src >= 0) {
>              b = cpu_ldub_data_ra(env, src, ra) & 0x0f;
>              src--;
>              len_src--;
>          }
> -        if (len_src > 0) {
> +        if (len_src >= 0) {
>              b |= cpu_ldub_data_ra(env, src, ra) << 4;
>              src--;
>              len_src--;
> 

In the SS format with two length fields, and in the
RSL format, L1 specifies the number of additional
operand bytes to the right of the byte designated by
the first-operand address. Therefore, the length in
bytes of the first operand is 1-16, corresponding to a
length code in L1 of 0-15. Similarly, L2. specifies the
number of additional operand bytes to the right of the
location designated by the second-operand address.
Results replace the first operand and are never
stored outside the field specified by the address and
length. If the first operand is longer than the second,
the second operand is extended on the left with zeros
up to the length of the first operand. This extension
does not modify the second operand in storage.

Doesn't this imply that we have always length + 1, and therefore the
current code is fine? ("length code vs length")

(e.g. len_dest = 0 implies one byte?)

-- 

Thanks,

David / dhildenb