Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)

[Peter Maydell]

On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The tswap64() calls introduced in commit 4be21d561d ("pseries:
> savevm support for pseries machine") are used to store the HTAB
> in the migration stream (see savevm_htab_handlers) and are in
> big-endian format.

I think they're reading the run-time spapr->htab data structure
(some of which is stuck onto the wire as a stream-of-bytes buffer
and some of which is not). But either way, it's a target-endian
data structure, because the code in hw/ppc/spapr_softmmu.c which
reads and writes entries in it is using ldq_p() and stq_p(),
and the current in-tree version of these macros is doing a
"read host 64-bit and convert to/from target endianness wih tswap64".

>  #define HPTE(_table, _i)   (void *)(((uint64_t *)(_table)) + ((_i) * 2))
> -#define HPTE_VALID(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & HPTE64_V_VALID)
> -#define HPTE_DIRTY(_hpte)  (tswap64(*((uint64_t *)(_hpte))) & 
> HPTE64_V_HPTE_DIRTY)
> -#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= 
> tswap64(~HPTE64_V_HPTE_DIRTY))
> -#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= 
> tswap64(HPTE64_V_HPTE_DIRTY))
> +#define HPTE_VALID(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & 
> HPTE64_V_VALID)
> +#define HPTE_DIRTY(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & 
> HPTE64_V_HPTE_DIRTY)
> +#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= 
> cpu_to_be64(~HPTE64_V_HPTE_DIRTY))
> +#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= 
> cpu_to_be64(HPTE64_V_HPTE_DIRTY))

This means we now have one file that's accessing this data structure
as "this is target-endian", and one file that's accessing it as
"this is big-endian". It happens that that ends up meaning the same
thing because PPC is always TARGET_BIG_ENDIAN, but it seems a bit
inconsistent.

We should decide whether we're thinking of the data structure
as target-endian or big-endian and change all the accessors
appropriately (or none of them -- currently we're completely
consistent about treating it as "target endian", I think).

I also think that "cast a pointer into a byte-array to uint64_t*
and then dereference it" is less preferable than using ldq_p()
and stq_p(), but that's arguably a separate thing.

thanks
-- PMM