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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]

On Mon, Dec 19, 2022 at 10:39:40AM +0000, Peter Maydell wrote:
> On Mon, 19 Dec 2022 at 06:35, David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > On Fri, Dec 16, 2022 at 09:39:19PM +0000, Peter Maydell wrote:
> > > On Fri, 16 Dec 2022 at 19:11, Daniel Henrique Barboza
> > > <danielhb413@gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > On 12/13/22 10:51, Peter Maydell wrote:
> > > > Yes, most if not all accesses are being handled as "target endian", even
> > > > though the target is always big endian.
> >
> > So "target is always big endian" is pretty misleading for POWER.  We
> > always define "TARGET_BIG_ENDIAN" in qemu, but for at least 10 years
> > the CPUs have been capable of running in either big endian or little
> > endian mode (selected at runtime).  Some variants can choose
> > endianness on a per-page basis.  Since the creation of the ISA it's
> > had "byte reversed" load and store instructions that let it use little
> > endian for specific memory accesses.
> 
> Yeah, this is like Arm (and for the purposes of this thread
> I meant essentially "TARGET_BIG_ENDIAN is always defined").

Ok.

> > Really the whole notion of an ISA having an "endianness" doesn't make
> > a lot of sense - it's an individual load or store to memory that has
> > an endianness which can depend on a bunch of factors.  When these
> > macros were created, an ISA nearly always used the same endianness,
> > but that's not really true any more - not just for POWER, but for a
> > bunch of targets.  So from that point of view, I think getting rid of
> > tswap() - particularly one that has compile time semantics, rather
> > than behaviour which can depend on cpu mode/state is a good idea.
> 
> I tend to think of the TARGET_BIG_ENDIAN/not setting as being
> something like "CPU bus endianness". At least for Arm, when you
> put the CPU into BE mode it pretty much means "the CPU byteswaps
> the data when it comes in/out", AIUI.

Hmm, I guess.  We're not really modelling down to the level of bus
byte lanes, though, so I'm not really convinced that's a meaningful
definition in the context of qemu.

> > I believe that even when running in little-endian mode, the hash page
> > tables are encoded in big-endian, so I think the proposed change makes
> > sense.
> 
> OK. I still think we should consistently change all the places that are
> accessing this data structure, though, not just half of them.

Yes, that makes sense.  Although what exactly constitutes "this data
structure" is a bit complex here.  If we mean just the spapr specific
"external HPT", then there are only a few more references to it.  If
we mean all instances of a powerpc hashed page table, then there are a
bunch more in the cpu target code.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]