On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé <phi...@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
