Re: [PATCH 1/2] powerpc/entry: convert to common and generic entry

[Luming Yu]

On Fri, Sep 13, 2024 at 08:54:12AM +0200, Christophe Leroy wrote:
> 
> 
> Le 13/09/2024 à 03:40, Luming Yu a écrit :
> > On Thu, Sep 12, 2024 at 12:23:29PM +0200, Christophe Leroy wrote:
> > > 
> > > 
> > > Le 12/09/2024 à 10:24, Luming Yu a écrit :
> > > > From: Yu Luming <luming.yu@xxxxxxxxx>
> > > > 
> > > > convert powerpc entry code in syscall and fault to use syscall_work
> > > > and irqentry_state as well as common calls from generic entry infrastructure.
> > > > 
> > > > Signed-off-by: Luming Yu <luming.yu@xxxxxxxxxxxx>
> > > > ---
> > > >    arch/powerpc/Kconfig                   | 1 +
> > > >    arch/powerpc/include/asm/hw_irq.h      | 5 +++++
> > > >    arch/powerpc/include/asm/processor.h   | 6 ++++++
> > > >    arch/powerpc/include/asm/syscall.h     | 5 +++++
> > > >    arch/powerpc/include/asm/thread_info.h | 1 +
> > > >    arch/powerpc/kernel/syscall.c          | 6 +++++-
> > > >    arch/powerpc/mm/fault.c                | 5 +++++
> > > >    7 files changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > There is another build problem:
> > > 
> > >    CC      kernel/entry/common.o
> > > kernel/entry/common.c: In function 'irqentry_exit':
> > > kernel/entry/common.c:335:21: error: implicit declaration of function
> > > 'regs_irqs_disabled'; did you mean 'raw_irqs_disabled'?
> > > [-Werror=implicit-function-declaration]
> > >    335 |         } else if (!regs_irqs_disabled(regs)) {
> > >        |                     ^~~~~~~~~~~~~~~~~~
> > >        |                     raw_irqs_disabled
> > > 
> > > 
> > > You have put regs_irqs_disabled() in a section dedicated to PPC64, so it
> > > fails on PPC32.
> > > 
> > > 
> > > After fixing this problem and providing an empty asm/entry-common.h it is
> > > now possible to build the kernel. But that's not enough, the board is stuck
> > > after:
> > > 
> > > ...
> > > [    2.871391] Freeing unused kernel image (initmem) memory: 1228K
> > > [    2.877990] Run /init as init process
> > 
> > Thanks for these questions. :-)
> > I haven't gotten chance to run it in ppc32 qemu.
> > the common syscall trace enter lost this hunk
> > -       if (!is_32bit_task())
> > -               audit_syscall_entry(regs->gpr[0], regs->gpr[3], regs->gpr[4],
> > -                                   regs->gpr[5], regs->gpr[6]);
> > -       else
> > -               audit_syscall_entry(regs->gpr[0],
> > -                                   regs->gpr[3] & 0xffffffff,
> > -                                   regs->gpr[4] & 0xffffffff,
> > -                                   regs->gpr[5] & 0xffffffff,
> > -                                   regs->gpr[6] & 0xffffffff);
> > which I don't understand whether we need a arch callbacks for it.
> 
> I don't thing so.
> 
> As far as I can see, audit_syscall_entry() is called by
> syscall_enter_audit() in kernel/entry/common.c
> 
> And the masking of arguments based on is_32bit_task() is done in
> syscall_get_arguments() with is called by
> syscall_enter_audit() just before calling audit_syscall_entry() and which is
> an arch callback that does the same as the removed hunk.
so, syscall_get_arguments is the ppc arch callback. thanks. :-)
> > 
> > Before I sent out the RFC patch set, the very limited compile and boot test goes well with a ppc64 qemu VM. Surely, there will be a lot of test, debug and following up patch set update that is necessary to make it a complete convert.
> 
> Even on ppc64 it doesn't build, at the first place because
> arch/powerpc/include/asm/entry-common.h is missing in your patch. Did you
> forget to 'git add' it ?
oh, I forget that I was testing this patch on top of the early user notifier patch:
https://github.com/linuxppc/issues/issues/477, https://patchwork.ozlabs.org/project/linuxppc-dev/patch/1FD36D52828D2506+20231218031309.2063-1-luming.yu@xxxxxxxxxxxx/ 
and the entry-common.h is as follows:
[root@localhost linux]# cat arch/powerpc/include/asm/entry-common.h
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef ARCH_POWERPC_ENTRY_COMMON_H
#define ARCH_POWERPC_ENTRY_COMMON_H

#include <linux/user-return-notifier.h>

static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
                                                  unsigned long ti_work)
{
        if (ti_work & _TIF_USER_RETURN_NOTIFY)
                fire_user_return_notifiers();
}

#define arch_exit_to_user_mode_prepare arch_exit_to_user_mode_prepare

#endif

As you could see , it looks irrelevant.
> 
> And same as with PPC32, when I build PPC64 with an empty asm/entry-common.h,
> it doesn't work. So, I guess you had some needed code in that file and you
> have to send it.

please send me your kernel config and let me reproduce and double check 
if there could be another bit of code make the difference. My test p8 64bit VM boots
an unmodified fedora 38 user space just fine with the patch. 
After boot, the only difference is make install of kernel could hang for a while, that 
I'm not sure how that could happen yet. and I plan to do some unit test for many common
features like : lockdep, ptrace, rcu, audit...
>