Re: [PATCH -next] powerpc/book3s64: fix link error with CONFIG_PPC_RADIX_MMU=n

[Yang Yingliang]

On 2020/9/6 14:50, Christophe Leroy wrote:
> Le 05/09/2020 à 13:25, Yang Yingliang a écrit :
> > Fix link error when CONFIG_PPC_RADIX_MMU is disabled:
> > powerpc64-linux-gnu-ld: 
> > arch/powerpc/platforms/pseries/lpar.o:(.toc+0x0): undefined reference 
> > to `mmu_pid_bits'
> >
> > Reported-by: Hulk Robot <hulkci@xxxxxxxxxx>
> > Signed-off-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
> > ---
> >   arch/powerpc/mm/book3s64/mmu_context.c | 4 ++++
>
> In your commit log, you are just mentionning 
> arch/powerpc/platforms/pseries/lpar.o, which is right.
>
> You shouldn't need to modify arch/powerpc/mm/book3s64/mmu_context.c at 
> all, see below.
>
> >   arch/powerpc/platforms/pseries/lpar.c  | 2 ++
> >   2 files changed, 6 insertions(+)
> >
> > diff --git a/arch/powerpc/mm/book3s64/mmu_context.c 
> > b/arch/powerpc/mm/book3s64/mmu_context.c
> > index 0ba30b8b935b..a8e292cd88f0 100644
> > --- a/arch/powerpc/mm/book3s64/mmu_context.c
> > +++ b/arch/powerpc/mm/book3s64/mmu_context.c
> > @@ -152,6 +152,7 @@ void hash__setup_new_exec(void)
> >     static int radix__init_new_context(struct mm_struct *mm)
> >   {
> > +#ifdef CONFIG_PPC_RADIX_MMU
>
> This shouldn't be required. radix__init_new_context() is only called 
> when radix_enabled() returns true.
> As it is a static function, when it is not called it gets optimised 
> away, so you will never get an undefined reference to `mmu_pid_bits` 
> there.
powerpc64-linux-gnu-ld: 
arch/powerpc/mm/book3s64/mmu_context.o:(.toc+0x0): undefined reference 
to `mmu_pid_bits'
powerpc64-linux-gnu-ld: 
arch/powerpc/mm/book3s64/mmu_context.o:(.toc+0x8): undefined reference 
to `mmu_base_pid'


mmu_context.c is always compiled, it uses mmu_pid_bits and mmu_base_pid.

> >       unsigned long rts_field;
> >       int index, max_id;
> >   @@ -177,6 +178,9 @@ static int radix__init_new_context(struct 
> > mm_struct *mm)
> >       mm->context.hash_context = NULL;
> >         return index;
> > +#else
> > +    return -ENOTSUPP;
> > +#endif
> >   }
> >     int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
> > diff --git a/arch/powerpc/platforms/pseries/lpar.c 
> > b/arch/powerpc/platforms/pseries/lpar.c
> > index baf24eacd268..e454e218dbba 100644
> > --- a/arch/powerpc/platforms/pseries/lpar.c
> > +++ b/arch/powerpc/platforms/pseries/lpar.c
> > @@ -1726,10 +1726,12 @@ void __init hpte_init_pseries(void)
> >     void radix_init_pseries(void)
> >   {
> > +#ifdef CONFIG_PPC_RADIX_MMU
>
> This function is only called from 
> /arch/powerpc/mm/book3s64/radix_pgtable.c which is only built when 
> CONFIG_PPC_RADIX_MMU is selected.
>
> So the entire function should be encloded in the #ifdef.
OK, I will send a v2 later.
> >       pr_info("Using radix MMU under hypervisor\n");
> >         pseries_lpar_register_process_table(__pa(process_tb),
> >                           0, PRTB_SIZE_SHIFT - 12);
> > +#endif
> >   }
> >     #ifdef CONFIG_PPC_SMLPAR
>
> Christophe
> .