Re: [PATCH v3 1/2] tests/tcg/multiarch: Make the system memory test work on big-endian

["Alex Bennée" <alex.bennee@linaro.org>]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> Store the bytes in descending order on big-endian.
> Invert the logic in the multi-byte signed tests on big-endian.
> Make the checks in the multi-byte signed tests stricter.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tests/tcg/multiarch/system/memory.c | 64 +++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 22 deletions(-)
>
> diff --git a/tests/tcg/multiarch/system/memory.c b/tests/tcg/multiarch/system/memory.c
> index 214f7d4f54b..eaae6929cb3 100644
> --- a/tests/tcg/multiarch/system/memory.c
> +++ b/tests/tcg/multiarch/system/memory.c
> @@ -40,18 +40,21 @@ static void pdot(int count)
>  }
>  
>  /*
> - * Helper macros for shift/extract so we can keep our endian handling
> - * in one place.
> + * Helper macros for endian handling.
>   */
> -#define BYTE_SHIFT(b, pos) ((uint64_t)b << (pos * 8))
> -#define BYTE_EXTRACT(b, pos) ((b >> (pos * 8)) & 0xff)
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +#define BYTE_SHIFT(b, pos) (b << (pos * 8))
> +#define BYTE_NEXT(b) ((b)++)
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +#define BYTE_SHIFT(b, pos) (b << ((sizeof(b) - 1 - (pos)) * 8))
> +#define BYTE_NEXT(b) ((b)--)

I guess we don't want to start count high so we'll see:

255, 254, 253

instead of

0, 255, 254, 253?

> +#else
> +#error Unsupported __BYTE_ORDER__
> +#endif
>  
>  /*
> - * Fill the data with ascending value bytes.
> - *
> - * Currently we only support Little Endian machines so write in
> - * ascending address order. When we read higher address bytes should
> - * either be zero or higher than the lower bytes.
> + * Fill the data with ascending (for little-endian) or descending (for
> + * big-endian) value bytes.
>   */

There is also a comment later on that needs fixing:

  /*
   * Read the test data and verify at various offsets
   *
   * For everything except bytes all our reads should be either positive
   * or negative depending on what offset we are reading from. Currently
   * we only handle LE systems.
   */

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Would you be able to re-base and respin v3 so I can pull it cleanly?


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro