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

[Peter Maydell]

On Fri, 16 Dec 2022 at 19:11, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
>
>
> On 12/13/22 10:51, Peter Maydell wrote:
> > 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).
>
> Yes, most if not all accesses are being handled as "target endian", even
> though the target is always big endian.
>
> IIUC the idea behind Phil's cleanups is exactly to replace uses of
> "target-something" if the endianess of the host is irrelevant, which
> is the case for ppc64. We would then change the semantics of the code
> gradually to make it consistent again.

I would be happier if we just did all the functions that read and
write this byte array at once -- there are not many of them.

thanks
-- PMM