Re: Deprecate the ppc405 boards in QEMU?

[Thomas Huth]

On 21/10/2021 08.48, Christophe Leroy wrote:
> Le 20/10/2021 à 15:16, Christophe Leroy a écrit :
> > Le 20/10/2021 à 14:43, Cédric Le Goater a écrit :
> > > On 10/20/21 13:42, BALATON Zoltan wrote:
> > > > On Wed, 20 Oct 2021, Philippe Mathieu-Daudé wrote:
> > > > > On 10/5/21 14:29, Thomas Huth wrote:
> > > > > > On 05/10/2021 14.20, BALATON Zoltan wrote:
> > > > > > > On Tue, 5 Oct 2021, Cédric Le Goater wrote:
> > > > > > > > On 10/5/21 08:18, Alexey Kardashevskiy wrote:
> > > > > > > > > On 05/10/2021 15:44, Christophe Leroy wrote:
> > > > > > > > > > Le 05/10/2021 à 02:48, David Gibson a écrit :
> > > > > > > > > > > On Fri, Oct 01, 2021 at 04:18:49PM +0200, Thomas Huth wrote:
> > > > > > > > > > > > On 01/10/2021 15.04, Christophe Leroy wrote:
> > > > > > > > > > > > > Le 01/10/2021 à 14:04, Thomas Huth a écrit :
> > > > > > > > > > > > > > On 01/10/2021 13.12, Peter Maydell wrote:
> > > > > > > > > > > > > > > On Fri, 1 Oct 2021 at 10:43, Thomas Huth <thuth@redhat.com>
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > > Nevertheless, as long as nobody has a hint where to find that
> > > > > > > > > > > > > > > > ppc405_rom.bin, I think both boards are pretty useless in
> > > > > > > > > > > > > > > > QEMU (as far as I
> > > > > > > > > > > > > > > > can see, they do not work without the bios at all, so it's
> > > > > > > > > > > > > > > > also not possible
> > > > > > > > > > > > > > > > to use a Linux image with the "-kernel" CLI option directly).
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > It is at least in theory possible to run bare-metal code on
> > > > > > > > > > > > > > > either board, by passing either a pflash or a bios argument.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > True. I did some more research, and seems like there was once
> > > > > > > > > > > > > > support for those boards in u-boot, but it got removed there a
> > > > > > > > > > > > > > couple of years ago already:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > https://gitlab.com/qemu-project/u-boot/-/commit/98f705c9cefdf
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > https://gitlab.com/qemu-project/u-boot/-/commit/b147ff2f37d5b
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > https://gitlab.com/qemu-project/u-boot/-/commit/7514037bcdc37
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > But I agree that there seem to be no signs of anybody actually
> > > > > > > > > > > > > > > successfully using these boards for anything, so we should
> > > > > > > > > > > > > > > deprecate-and-delete them.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Yes, let's mark them as deprecated now ... if someone still uses
> > > > > > > > > > > > > > them and speaks up, we can still revert the deprecation again.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I really would like to be able to use them to validate Linux 
> > > > > > > > > > > > > Kernel
> > > > > > > > > > > > > changes, hence looking for that missing BIOS.
> > > > > > > > > > > > >
> > > > > > > > > > > > > If we remove ppc405 from QEMU, we won't be able to do any
> > > > > > > > > > > > > regression
> > > > > > > > > > > > > tests of Linux Kernel on those processors.
> > > > > > > > > > > >
> > > > > > > > > > > > If you/someone managed to compile an old version of u-boot for
> > > > > > > > > > > > one of these
> > > > > > > > > > > > two boards, so that we would finally have something for
> > > > > > > > > > > > regression testing,
> > > > > > > > > > > > we can of course also keep the boards in QEMU...
> > > > > > > > > > >
> > > > > > > > > > > I can see that it would be usefor for some cases, but unless someone
> > > > > > > > > > > volunteers to track down the necessary firmware and look after it, I
> > > > > > > > > > > think we do need to deprecate it - I certainly don't have the
> > > > > > > > > > > capacity
> > > > > > > > > > > to look into this.
> > > > > > > > > >
> > > > > > > > > > I will look at it, please allow me a few weeks though.
> > > > > > > > >
> > > > > > > > > Well, building it was not hard but now I'd like to know what board
> > > > > > > > > QEMU actually emulates, there are way too many codenames and PVRs.
> > > > > > > >
> > > > > > > > yes. We should try to reduce the list below. Deprecating embedded
> > > > > > > > machines
> > > > > > > > is one way.
> > > > > > >
> > > > > > > Why should we reduce that list? It's good to have different cpu
> > > > > > > options when one wants to test code for different PPC versions (maybe
> > > > > > > also in user mode) or just to have a quick list of these at one place.
> > > > > >
> > > > > > I think there are many CPUs in that list which cannot be used with any
> > > > > > board, some of them might be also in a very incomplete state. So
> > > > > > presenting such a big list to the users is confusing and might create
> > > > > > wrong expectations. It would be good to remove at least the CPUs which
> > > > > > are really completely useless.
> > > > >
> > > > > Maybe only remove some from system emulation but keep all of them
> > > > > in user emulation?
> > > >
> > > > Or keep them all but mark those that are not tested/may be incomplete? 
> > > > So the used can see what is expected to work and what may need to be 
> > > > fixed. That way somebody may try and fix it whereas if it's not there 
> > > > they are unlikely to try to add it.
> > >
> > >
> > > The bamboo machine with 440 CPUs is booting with the latest kernel
> > > and we have an acceptance test for it now, thanks to Thomas. There
> > > is not much effort in keeping them in a working state until someone
> > > volunteers. Hopefully, Christophe is making sure that we are not
> > > breaking anything with Linux support.
> > >
> > > The 405 machine are still close to deprecation I think. We are still
> > > struggling to boot one with mainline Linux, using uboot provided by
> > > Thomas which skips SDRAM init. It is not clear to me if u-boot is
> > > strictly necessary. It depends if Linux relies on it to do some
> > > pre-initialization of HW. I guess once we find a good DTS for it, or
> > > not, we can take a decision.
> >
> > I now have a hacked configuration booting linux with the hotfoot DTS and 
> > the kernel is booting up to starting /init
> >
> > Then it is faulting forever taking a DSI for write protection. The problem 
> > is now likely in Linux and I'm investigating it, but I'm having problem 
> > with GDB (7.8.1), I'm hitting the bug 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=17700
> >
> > And GDB 11.1 coredumps while reading vmlinux's symbols
> >
> > Hopefully I'll find a GDB version between 7.8 and 11 that works.
>
> I bisected the issue to 
> https://github.com/linuxppc/linux/commit/52ae92cc290f0506eef9ad5466bb453ce4a9e80e 

You could argue that this commit introduced a bug, or at least a bad 
behavior in the kernel: According to the 405 user's manual that I have:

 10.2 Reserved Fields
 For all registers having fields marked as reserved, the reserved
 fields should be written as zero and read as undefined. That is,
 when writing to a reseved field, write a 0 to the field. When
 reading from a reserved field, ignore the field.

Thus the kernel should not write non-zero bits into the upper bits of this 
register.

> The problem is in QEMU, it should ignore upper bits of PID register.

Agreed, that's certainly necessary, too.

> The following change fixes the issue, don't know it is it the right way to 
> fix though
>
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index 754509e556..44f4fa5280 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -570,7 +570,7 @@ static int mmu40x_get_physical_address(CPUPPCState *env, 
> mmu_ctx_t *ctx,
>       for (i = 0; i < env->nb_tlb; i++) {
>           tlb = &env->tlb.tlbe[i];
>           if (ppcemb_tlb_check(env, tlb, &raddr, address,
> -                             env->spr[SPR_40x_PID], 0, i) < 0) {
> +                             env->spr[SPR_40x_PID] & 0xff, 0, i) < 0) {
>               continue;
>           }
>           zsel = (tlb->attr >> 4) & 0xF;
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 2cb98c5169..9331830f34 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -789,7 +789,7 @@ void helper_4xx_tlbwe_hi(CPUPPCState *env, target_ulong 
> entry,
>       } else {
>           tlb->prot &= ~PAGE_VALID;
>       }
> -    tlb->PID = env->spr[SPR_40x_PID]; /* PID */
> +    tlb->PID = env->spr[SPR_40x_PID] & 0xff; /* PID */
>       LOG_SWTLB("%s: set up TLB %d RPN " TARGET_FMT_plx " EPN " TARGET_FMT_lx
>                 " size " TARGET_FMT_lx " prot %c%c%c%c PID %d\n", __func__,
>                 (int)entry, tlb->RPN, tlb->EPN, tlb->size,
> @@ -837,7 +837,7 @@ void helper_4xx_tlbwe_lo(CPUPPCState *env, target_ulong 
> entry,
>
>   target_ulong helper_4xx_tlbsx(CPUPPCState *env, target_ulong address)
>   {
> -    return ppcemb_tlb_search(env, address, env->spr[SPR_40x_PID]);
> +    return ppcemb_tlb_search(env, address, env->spr[SPR_40x_PID] & 0xff);
>   }

The modications looks sane to me, could you please send this as a proper 
patch (with Signed-off-by line) to the mailing list?

Alternatively, it might be possible to do the masking only once in 
helper_booke_setpid() in mmu_helper.c:

void helper_booke_setpid(CPUPPCState *env, uint32_t pidn, target_ulong pid)
{
    if (pid == SPR_40x_PID) {
        pid &= 0xff;
    }
    env->spr[pidn] = pid;
    /* changing PIDs mean we're in a different address space now */
    tlb_flush(env_cpu(env));
}

... not sure whether that is the better approach here, though ... it maybe 
depends whether the reserved bits read back as zeros on a real 405 or not...

 Thomas