Re: [PATCH] RISC-V: Fixup clear_page export when using Zicboz

Ben Dooks
2023-02-24T14:42:27Z

On 24/02/2023 14:18, Andrew Jones wrote:
> On Fri, Feb 24, 2023 at 01:58:44PM +0000, Ben Dooks wrote:
>> When the clear_page() via Zicboz is enabled, the module build
>> fails as clear_page() is not marked as a ksym entry. Fix this
>> by changing the asm code to use <asm-generic/export.h> to add
>> the correct export.
>>
>> Also remove the weak clear_page() as there's nothing else in
>> the build defining this symbol, so just make it the entry when
>> the Zicboz is enabled.
>>
>> Fixes modpost errors such as this:
>> ERROR: modpost: "clear_page" [drivers/gpu/drm/ttm/ttm.ko] undefined!
>>
>> Reported-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> arch/riscv/lib/clear_page.S | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/riscv/lib/clear_page.S b/arch/riscv/lib/clear_page.S
>> index 7c7fa45b5ab5..4ed5fef52d80 100644
>> --- a/arch/riscv/lib/clear_page.S
>> +++ b/arch/riscv/lib/clear_page.S
>> @@ -6,6 +6,7 @@
>> #include <linux/linkage.h>
>> #include <asm/asm.h>
>> #include <asm/alternative-macros.h>
>> +#include <asm-generic/export.h>
>> #include <asm/hwcap.h>
>> #include <asm/insn-def.h>
>> #include <asm/page.h>
>> @@ -17,7 +18,7 @@
>>
>> /* void clear_page(void *page) */
>> ENTRY(__clear_page)
>> -WEAK(clear_page)
>> +SYM_FUNC_START(clear_page)
>> li a2, PAGE_SIZE
>>
>> /*
>> @@ -70,4 +71,5 @@ WEAK(clear_page)
>> .Lno_zicboz:
>> li a1, 0
>> tail __memset
>> -END(__clear_page)
>> +SYM_FUNC_END(clear_page)
>> +EXPORT_SYMBOL(clear_page)
>> --
>> 2.39.1
>>
>
> Hi Ben,
>
> This looks good to me. I agree that I shouldn't have made clear_page weak,
> but not because there isn't currently anything else defining it, but
> because it's not something a subsystem is likely to ever define. Exporting
> for modules is also definitely needed.
>
> Is there any reason I shouldn't just squash this into the patch which
> introduces clear_page? That patch is still under review, and merging
> it broken just to immediately fix it would break bisection for no reason.

Yeah, as long as you note fixes from myself an sudip, then go for it.

--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius