| Message ID | 20241218182106.78800-8-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ppc: Remove tswap() calls | expand |
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). What I'm 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. 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; > } > >
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? > 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; > > } > > > >
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; >>> } >>> >>> > >
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; 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; }
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(-)