Re: [PATCH v3 4/6] xen: Switch to byteswap

[Julien Grall <julien@xen.org>]

Hi,

On 10/05/2022 11:15, Lin Liu wrote:
> Update to use byteswap to swap bytes.
> 
> No functional change.
> 
> Signed-off-by: Lin Liu <lin.liu@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Wei Liu <wl@xen.org>
> Changes in v3:
> - Update xen/common/device_tree.c to use be32_to_cpu
> - Keep const in type cast in unaligned.h
> ---
>   xen/common/device_tree.c           | 44 +++++++++++++++---------------
>   xen/common/libelf/libelf-private.h |  6 ++--
>   xen/common/xz/private.h            |  2 +-
>   xen/include/xen/unaligned.h        | 24 ++++++++--------
>   4 files changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..70d3be3be6 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -171,7 +171,7 @@ bool_t dt_property_read_u32(const struct dt_device_node *np,
>       if ( !val || len < sizeof(*out_value) )
>           return 0;
>   
> -    *out_value = be32_to_cpup(val);
> +    *out_value = be32_to_cpu(*val);

This code has been taken from Linux and I would rather prefer to keep 
the *cpup* helpers to avoid any changes when backporting.

> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
> index 0a2b16d05d..16b2e6f5f0 100644
> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -20,62 +20,62 @@
>   
>   static inline uint16_t get_unaligned_be16(const void *p)
>   {
> -	return be16_to_cpup(p);
> +	return be16_to_cpu(*(const uint16_t *)p)

I haven't checked the existing implementation of be16_to_cpup(). 
However, this new approach would allow the compiler to use a single load 
instruction to read the 16-bit value from memory. So this change may 
break on platform where unaligned access is forbidden (such as arm32).

>   }
>   
>   static inline void put_unaligned_be16(uint16_t val, void *p)
>   {
> -	*(__force __be16*)p = cpu_to_be16(val);
> +	*(__be16 *)p = cpu_to_be16(val);

Why did you drop the __force?

>   }
>

Cheers,

-- 
Julien Grall