Re: [PATCH v3 05/18] accel/tcg: Add endian-specific cpu_{ld, st}* operations

[Peter Maydell]

On Wed, 22 Apr 2020 at 05:33, Richard Henderson
<address@hidden> wrote:
>
> We currently have target-endian versions of these operations,
> but no easy way to force a specific endianness.  This can be
> helpful if the target has endian-specific operations, or a mode
> that swaps endianness.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  docs/devel/loads-stores.rst |  39 +++--
>  include/exec/cpu_ldst.h     | 277 +++++++++++++++++++++++++++---------
>  accel/tcg/cputlb.c          | 236 ++++++++++++++++++++++--------
>  accel/tcg/user-exec.c       | 211 ++++++++++++++++++++++-----
>  4 files changed, 587 insertions(+), 176 deletions(-)
>
> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> index 0d99eb24c1..9a944ef1af 100644
> --- a/docs/devel/loads-stores.rst
> +++ b/docs/devel/loads-stores.rst
> @@ -97,9 +97,9 @@ function, which is a return address into the generated code.
>
>  Function names follow the pattern:
>
> -load: ``cpu_ld{sign}{size}_mmuidx_ra(env, ptr, mmuidx, retaddr)``
> +load: ``cpu_ld{sign}{size}{end}_mmuidx_ra(env, ptr, mmuidx, retaddr)``
>
> -store: ``cpu_st{size}_mmuidx_ra(env, ptr, val, mmuidx, retaddr)``
> +store: ``cpu_st{size}{end}_mmuidx_ra(env, ptr, val, mmuidx, retaddr)``
>
>  ``sign``
>   - (empty) : for 32 or 64 bit sizes
> @@ -112,9 +112,14 @@ store: ``cpu_st{size}_mmuidx_ra(env, ptr, val, mmuidx, 
> retaddr)``
>   - ``l`` : 32 bits
>   - ``q`` : 64 bits
>
> +``end``
> + - (empty) : for target endian, or 8 bit sizes
> + - ``_be`` : big endian
> + - ``_le`` : little endian
> +
>  Regexes for git grep:
> - - ``\<cpu_ld[us]\?[bwlq]_mmuidx_ra\>``
> - - ``\<cpu_st[bwlq]_mmuidx_ra\>``
> + - ``\<cpu_ld[us]\?[bwlq](_[bl]e)\?_mmuidx_ra\>``
> + - ``\<cpu_st[bwlq](_[bl]e)\?_mmuidx_ra\>``
>
>  ``cpu_{ld,st}*_data_ra``
>  ~~~~~~~~~~~~~~~~~~~~~~~~
> @@ -129,9 +134,9 @@ be performed with a context other than the default.
>
>  Function names follow the pattern:
>
> -load: ``cpu_ld{sign}{size}_data_ra(env, ptr, ra)``
> +load: ``cpu_ld{sign}{size}{end}_data_ra(env, ptr, ra)``
>
> -store: ``cpu_st{size}_data_ra(env, ptr, val, ra)``
> +store: ``cpu_st{size}{end}_data_ra(env, ptr, val, ra)``
>
>  ``sign``
>   - (empty) : for 32 or 64 bit sizes
> @@ -144,9 +149,14 @@ store: ``cpu_st{size}_data_ra(env, ptr, val, ra)``
>   - ``l`` : 32 bits
>   - ``q`` : 64 bits
>
> +``end``
> + - (empty) : for target endian, or 8 bit sizes
> + - ``_be`` : big endian
> + - ``_le`` : little endian
> +
>  Regexes for git grep:
> - - ``\<cpu_ld[us]\?[bwlq]_data_ra\>``
> - - ``\<cpu_st[bwlq]_data_ra\>``
> + - ``\<cpu_ld[us]\?[bwlq](_[bl]e)\?_data_ra\>``
> + - ``\<cpu_st[bwlq](_[bl]e)\?_data_ra\>``
>
>  ``cpu_{ld,st}*_data``
>  ~~~~~~~~~~~~~~~~~~~~~
> @@ -163,9 +173,9 @@ the CPU state anyway.
>
>  Function names follow the pattern:
>
> -load: ``cpu_ld{sign}{size}_data(env, ptr)``
> +load: ``cpu_ld{sign}{size}{end}_data(env, ptr)``
>
> -store: ``cpu_st{size}_data(env, ptr, val)``
> +store: ``cpu_st{size}{end}_data(env, ptr, val)``
>
>  ``sign``
>   - (empty) : for 32 or 64 bit sizes
> @@ -178,9 +188,14 @@ store: ``cpu_st{size}_data(env, ptr, val)``
>   - ``l`` : 32 bits
>   - ``q`` : 64 bits
>
> +``end``
> + - (empty) : for target endian, or 8 bit sizes
> + - ``_be`` : big endian
> + - ``_le`` : little endian
> +
>  Regexes for git grep
> - - ``\<cpu_ld[us]\?[bwlq]_data\>``
> - - ``\<cpu_st[bwlq]_data\+\>``
> + - ``\<cpu_ld[us]\?[bwlq](_[bl]e)\?_data\>``
> + - ``\<cpu_st[bwlq](_[bl]e)\?_data\+\>``
>
>  ``cpu_ld*_code``
>  ~~~~~~~~~~~~~~~~
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index 53de19753a..1ba515bfcc 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -26,12 +26,18 @@
>   * The syntax for the accessors is:
>   *
>   * load:  cpu_ld{sign}{size}_{mmusuffix}(env, ptr)
> + *        cpu_ld{sign}{size}{end}_{mmusuffix}(env, ptr)
>   *        cpu_ld{sign}{size}_{mmusuffix}_ra(env, ptr, retaddr)
> + *        cpu_ld{sign}{size}{end}_{mmusuffix}_ra(env, ptr, retaddr)
>   *        cpu_ld{sign}{size}_mmuidx_ra(env, ptr, mmu_idx, retaddr)
> + *        cpu_ld{sign}{size}{end}_mmuidx_ra(env, ptr, mmu_idx, retaddr)
>   *
>   * store: cpu_st{size}_{mmusuffix}(env, ptr, val)
> + *        cpu_st{size}{end}_{mmusuffix}(env, ptr, val)
>   *        cpu_st{size}_{mmusuffix}_ra(env, ptr, val, retaddr)
> + *        cpu_st{size}{end}_{mmusuffix}_ra(env, ptr, val, retaddr)
>   *        cpu_st{size}_mmuidx_ra(env, ptr, val, mmu_idx, retaddr)
> + *        cpu_st{size}{end}_mmuidx_ra(env, ptr, val, mmu_idx, retaddr)

Since '{end}' can be the empty string, these new lines should
replace the old ones, not just be added. (The other changes
in this doc do the right thing.)

>   *
>   * sign is:
>   * (empty): for 32 and 64 bit sizes
> @@ -44,6 +50,11 @@
>   *   l: 32 bits
>   *   q: 64 bits
>   *
> + * end is:
> + * (empty): for target native endian, or for 8 bit access
> + *     _be: for forced big endian
> + *     _le: for forced little endian
> + *
>   * mmusuffix is one of the generic suffixes "data" or "code", or "mmuidx".
>   * The "mmuidx" suffix carries an extra mmu_idx argument that specifies
>   * the index to use; the "data" and "code" suffixes take the index from

Otherwise

Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM