Re: [PATCH] riscv, bpf: Emit fence.i for BPF_NOSPEC

[Luis Gerhorst <luis.gerhorst@fau.de>]

Samuel Holland <samuel.holland@sifive.com> writes:

> The Zifencei extension is no longer a mandatory part of the ISA, but it is
> mandatory for Linux. Linux requires at least "rv32ima or rv64ima, as defined by
> version 2.2 of the user ISA and version 1.10 of the privileged ISA".
>
> Notably, in version 2.2 of the user ISA, the Zifencei extension was still an
> unnamed subset of the I extension, so it is included in the above requirement.
> It was later removed from the I extension and given its own name, which is why
> we have weirdness like the code below. (You can see in arch/riscv/Makefile where
> we unconditionally add either ISA version 2.2 or Zifencei to CFLAGS.)
>
>> RISC-V cpufeature.c had this in [2]:
>> 
>> 		/*
>> 		 * Linux requires the following extensions, so we may as well
>> 		 * always set them.
>> 		 */
>> 		set_bit(RISCV_ISA_EXT_ZICSR, this_isa);
>> 		set_bit(RISCV_ISA_EXT_ZIFENCEI, this_isa);
>> 
>> But as of [1] it was changed to:
>> 
>> 		if (acpi_disabled) {
>> 			set_bit(RISCV_ISA_EXT_ZICSR, source_isa);
>> 			set_bit(RISCV_ISA_EXT_ZIFENCEI, source_isa);
>> 
>> So based on that I would assume fence.i may not always be supported.
>
> This is just a quirk of the parsing code. As mentioned in [1], older devicetrees
> were written while Zifencei was an implicit part of I, so we don't expect it to
> appear in the devicetree. The ACPI table definition was written after Zifencei
> was a separate extension, so we do expect Zifencei to appear on its own in the
> ACPI table. But it is still required. (We don't currently check that all
> extensions required by the kernel are actually present; this will be done as
> part of the RVA23 enablement.)
>
>> But I also found that 921ebd8f2c08 ("RISC-V: Allow userspace to flush
>> the instruction cache") seems to assume that fence.i always works (see
>> local_flush_icache_all() which I assume runs in the kernel).
>
> Yes, the kernel definitely won't run if fence.i is missing, so we don't need to
> worry about such hardware. We could probably document this better.

Thanks for clarifying this! In that case the patch looks good to me.

I assume some testing has been done to ensure the instruction coding
works. (I think the eBPF CI does not have RISC-V yet but [1] previously
worked fine for me.)

Acked-by: Luis Gerhorst <luis.gerhorst@fau.de>

[1] https://github.com/pulehui/riscv-bpf-vmtest