Re: [PATCH v3 7/7] hw/ppc/epapr: Do not swap ePAPR magic value

[BALATON Zoltan]

On Thu, 19 Dec 2024, Nicholas Piggin wrote:
> On Thu Dec 19, 2024 at 5:18 AM AEST, BALATON Zoltan wrote:
> > On Wed, 18 Dec 2024, Philippe Mathieu-Daudé wrote:
> > > The ePAPR magic value in $r6 doesn't need to be byte swapped.
> > >
> > > See ePAPR-v1.1.pdf chapter 5.4.1 "Boot CPU Initial Register State"
> > > and the following mailing-list thread:
> > > https://lore.kernel.org/qemu-devel/CAFEAcA_NR4XW5DNL4nq7vnH4XRH5UWbhQCxuLyKqYk6_FCBrAA@mail.gmail.com/
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > > hw/ppc/sam460ex.c     | 2 +-
> > > hw/ppc/virtex_ml507.c | 2 +-
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> > > index 78e2a46e753..db9c8f3fa6e 100644
> > > --- a/hw/ppc/sam460ex.c
> > > +++ b/hw/ppc/sam460ex.c
> > > @@ -234,7 +234,7 @@ static void main_cpu_reset(void *opaque)
> > >
> > >         /* Create a mapping for the kernel.  */
> > >         booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1 << 31);
> > > -        env->gpr[6] = tswap32(EPAPR_MAGIC);
> > > +        env->gpr[6] = EPAPR_MAGIC;
> >
> > I don't know how to test this (or if anything actually uses it).
>
> The Linux kernel boot wrapper tests it AFAIKS. Does this mean
> they never worked on LE hosts?

Linux boots on the sam460ex on LE host, I don't know if it works on BE 
host as I don't have any so this could be broken on BE host but should 
work on LE host. If this breaks Linux boot somebody may report it, I don't 
have time to try testing it now.

Regards,
BALATON Zoltan

> > not sure about is what endianness env->gpr is? It's a host array so maybe
> > it needs to match the host endianness which is little endian most of the
> > time as opposed to PPC big endian on this machine. So maybe tswap is wrong
> > but is removing it right? Maybe we need to only swap on LE hosts. I think
> > it's only used by Linux kernels so maybe trying to boot one could test
> > this change but I'm not sure.
>
> Yes env->gpr is host-endian, and emulated target code will see
> the same value that the host does. It can't be correct because
> the machine running in emulation can't possibly know what endian
> the host is.
>
> I think we should just take it, it looks trivially correct
> (always dangerous words when dealing with early boot code, lol).
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> > Regards,
> > BALATON Zoltan
> >
> > >         env->gpr[7] = (16 * MiB) - 8; /* bi->ima_size; */
> > >
> > >     } else {
> > > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > > index f378e5c4a90..6197d31d88f 100644
> > > --- a/hw/ppc/virtex_ml507.c
> > > +++ b/hw/ppc/virtex_ml507.c
> > > @@ -119,7 +119,7 @@ static void main_cpu_reset(void *opaque)
> > >     /* Create a mapping spanning the 32bit addr space. */
> > >     booke_set_tlb(&env->tlb.tlbe[0], 0, 0, 1U << 31);
> > >     booke_set_tlb(&env->tlb.tlbe[1], 0x80000000, 0x80000000, 1U << 31);
> > > -    env->gpr[6] = tswap32(EPAPR_MAGIC);
> > > +    env->gpr[6] = EPAPR_MAGIC;
> > >     env->gpr[7] = bi->ima_size;
> > > }