2023-08-14 08:14:19

by Nicholas Piggin

[permalink] [raw]
Subject: Re: [PATCH] powerpc/radix: Move some functions into #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE

On Wed Aug 9, 2023 at 6:01 PM AEST, Christophe Leroy wrote:
> With skiboot_defconfig, Clang reports:
>
> CC arch/powerpc/mm/book3s64/radix_tlb.o
> arch/powerpc/mm/book3s64/radix_tlb.c:419:20: error: unused function '_tlbie_pid_lpid' [-Werror,-Wunused-function]
> static inline void _tlbie_pid_lpid(unsigned long pid, unsigned long lpid,
> ^
> arch/powerpc/mm/book3s64/radix_tlb.c:663:20: error: unused function '_tlbie_va_range_lpid' [-Werror,-Wunused-function]
> static inline void _tlbie_va_range_lpid(unsigned long start, unsigned long end,
> ^
>
> This is because those functions are only called from functions
> enclosed in a #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>
> Move below functions inside that #ifdef
> * __tlbie_pid_lpid(unsigned long pid,
> * __tlbie_va_lpid(unsigned long va, unsigned long pid,
> * fixup_tlbie_pid_lpid(unsigned long pid, unsigned long lpid)
> * _tlbie_pid_lpid(unsigned long pid, unsigned long lpid,
> * fixup_tlbie_va_range_lpid(unsigned long va,
> * __tlbie_va_range_lpid(unsigned long start, unsigned long end,
> * _tlbie_va_range_lpid(unsigned long start, unsigned long end,

Thanks for doing this. Functions vaguely belong where they are, which
makes it slightly annoying to move them. Is it also annoying to add
ifdefs for each one where they are?

Thanks,
Nick


2023-08-14 10:20:58

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH] powerpc/radix: Move some functions into #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE



Le 14/08/2023 à 08:24, Nicholas Piggin a écrit :
> On Wed Aug 9, 2023 at 6:01 PM AEST, Christophe Leroy wrote:
>> With skiboot_defconfig, Clang reports:
>>
>> CC arch/powerpc/mm/book3s64/radix_tlb.o
>> arch/powerpc/mm/book3s64/radix_tlb.c:419:20: error: unused function '_tlbie_pid_lpid' [-Werror,-Wunused-function]
>> static inline void _tlbie_pid_lpid(unsigned long pid, unsigned long lpid,
>> ^
>> arch/powerpc/mm/book3s64/radix_tlb.c:663:20: error: unused function '_tlbie_va_range_lpid' [-Werror,-Wunused-function]
>> static inline void _tlbie_va_range_lpid(unsigned long start, unsigned long end,
>> ^
>>
>> This is because those functions are only called from functions
>> enclosed in a #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>>
>> Move below functions inside that #ifdef
>> * __tlbie_pid_lpid(unsigned long pid,
>> * __tlbie_va_lpid(unsigned long va, unsigned long pid,
>> * fixup_tlbie_pid_lpid(unsigned long pid, unsigned long lpid)
>> * _tlbie_pid_lpid(unsigned long pid, unsigned long lpid,
>> * fixup_tlbie_va_range_lpid(unsigned long va,
>> * __tlbie_va_range_lpid(unsigned long start, unsigned long end,
>> * _tlbie_va_range_lpid(unsigned long start, unsigned long end,
>
> Thanks for doing this. Functions vaguely belong where they are, which
> makes it slightly annoying to move them. Is it also annoying to add
> ifdefs for each one where they are?

Looking at it once more, we can even move all those functions out in a
separate file that would only be built when
CONFIG_KVM_BOOK3S_HV_POSSIBLE is selected. The only dependency with
other part of the file is the static tlb_single_page_flush_ceiling,
which can easily be made global.

In principle we try to avoid #ifdefs in C-files.

Why would we want to keep those functions at the current place and
spreat several more #ifdefs inside the file ?

Christophe