Re: [PATCH v3 20/20] linux-user: Add RISC-V zicfilp support in VDSO

[Deepak Gupta]

On Wed, Aug 07, 2024 at 01:41:37PM +1000, Richard Henderson wrote:
> On 8/7/24 10:06, Deepak Gupta wrote:
> > Add zicfilp support in VDSO. VDSO functions need lpad instruction
> > so that userspace could call this function when landing pad extension is
> > enabled. This solution only works when toolchain always use landing pad
> > label 1.
>
> Well, no, the lpad insns *could* use imm=0.
> Why would the toolchain always use label 1?
>
> Much more explanation is required here.

Sorry this is amiss on my end. label=1 is kind of experimental, dev enabling
to ensure that label checkings are working. Eventually label scheme would be
hash (20bit truncates) based label scheme. Hash calculated over function
prototype.

This is again chicken and an egg problem. Things have to make it to upstream
in toolchain and libc for this scheme to be usable.

For now, I am thinking of doing `lpad 0` (as you also hinted) for vDSO.
I hope that's fine.

> > +/* GNU_PROPERTY_RISCV64_* macros from elf.h for use in asm code.  */
> > +#define FEATURE_1_AND 0xc0000000
> > +
> > +#define GNU_PROPERTY(type, value)      \
> > +  .section .note.gnu.property, "a";  \
> > +  .p2align 3;                       \
> > +  .word 4;                          \
> > +  .word 16;                         \
> > +  .word 5;                          \
> > +  .asciz "GNU";                     \
> > +  .word type;                       \
> > +  .word 4;                          \
> > +  .word value;                      \
> > +  .word 0;                          \
> > +  .text
> > +
> > +/* Add GNU property note with the supported features to all asm code
> > +   where sysdep.h is included.  */
> > +#undef __VALUE_FOR_FEATURE_1_AND
> > +#if defined (__riscv_zicfilp) || defined (__riscv_zicfiss)
> > +#  if defined (__riscv_zicfilp)
> > +#    if defined (__riscv_zicfiss)
>
> Why are you checking __riscv_* symbols, for when the toolchain
> has these features enabled on the command-line?

Yes, you're right.

> This is the kind of feature you want enabled always.

Yes, you're right here as well.
It should be compiled into vDSO unconditionally.

Will do that.

> > +#ifdef __riscv_zicfilp
> > +# define LPAD       lpad 1
> > +#else
> > +# define LPAD
> > +#endif
>
> Here, especially, you should be using ".insn", not omitting the lpad insn.
>
>
> r~