Re: [PATCH v3 3/5] riscv: support early isa ext and use it to optimize pgtable_l4|l5_enabled

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2779 bytes --]

On Wed, Sep 09, 2026 at 11:01:56PM +0800, Jisheng Zhang wrote:
> The pgtable_l4|[l5]_enabled check sits at hot code path, performance
> is impacted a lot. Since pgtable_l4|[l5]_enabled isn't changed after
> boot, we can use alternative mechanism to optimize them.
> 
> So the question is whether we can add RISCV_ISA_EXT_SV48/SV5 and use
> riscv_has_extension_*() or not. Per [1] and [2], SV48 and SV57 are ISA
> exensions too. From another side, riscv_has_extension_[un]likely() and
> other related functions report whether the extension is supported and
> enabled on the platform. So SV48 and SV57 can be supported with current
> isa extension alternative mechanism.
> 
> However, to use it to optimize pgtable_l4|l5_enabled, we have support
> the "early" characteristic, I.E besides risc_isa bitmap setting, we
> need to support appling alternative early before MMU on.
> 
> After that, use it to optimize pgtable_l4|l5_enabled.
> 
> For the typical access_ok(addr, 1);
> before the patch:
> 
> ...
> auipc	a5,0xb43
> lbu	a5,100(a5) # ffffffff80b51f68 <pgtable_l5_enabled>
> bnez	a5,ffffffff8000ef46 <foo+0x56>
> auipc	a5,0xb43
> lbu	a5,91(a5) # ffffffff80b51f69 <pgtable_l4_enabled>
> beqz	a5,ffffffff8000ef5a <foo+0x6a>
> ...
> 
> after the patch:
> These memory load and test branch instructions are replaced with only
> two j or nop instructions.
> 
> Initial test lmbench's lat_syscall write on TH1520 platforms shows that
> the write syscall latency is reduced by about 2.38%.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> Link: https://github.com/riscv/riscv-isa-manual/blob/main/src/profiles/profiles.adoc [1]
> Link: https://riscv.atlassian.net/wiki/spaces/HOME/pages/16154732/Ratified+ISA+Extensions [2]

I don't love the early business at all, but I do prefer this diff to the
previous one.

> @@ -1147,6 +1159,8 @@ void __init riscv_fill_hwcap(void)
>  	isa2hwcap[RISCV_ISA_EXT_C] = COMPAT_HWCAP_ISA_C;
>  	isa2hwcap[RISCV_ISA_EXT_V] = COMPAT_HWCAP_ISA_V;
>  
> +	bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX);
> +

The only real gripe I have is that this should probably have a comment
explaining why it is being zeroed. I can see reasons to zero it (fresh
slate) and to keep it (otherwise nothing will set the sv48/sv57 bits in
the register, since we don't actually read the associated devicetree
properties after this point).

From my point of view, with this niggle resolved, I'm happy with the
code that's here but I have no opinion on whether or not this should be
done in the first place. Conditional on the comment,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.


>  	if (!acpi_disabled) {
>  		riscv_fill_hwcap_from_isa_string(isa2hwcap);
>  	} else {


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]