[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Daniel Henrique Barboza
Subject: Re: [RFC PATCH-for-8.0 2/3] hw/ppc/spapr: Replace tswap64(HPTE) by cpu_to_be64(HPTE)
Date: Fri, 16 Dec 2022 16:10:56 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1

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))) & 
-#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= 
-#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= 
+#define HPTE_VALID(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & 
+#define HPTE_DIRTY(_hpte)  (be64_to_cpu(*((uint64_t *)(_hpte))) & 
+#define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= 
+#define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= 

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

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.

However, I don't feel comfortable acking this patch alone since 4be21d561d
is from David and I don't know if there's a great design behind the use of
tswap64() to manipulate the hpte. David, would you care to comment?


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.

-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]