Re: [kvm-unit-tests PATCH v2 2/3] riscv: lib: Add SSE assembly entry handling

[Andrew Jones <andrew.jones@linux.dev>]

On Mon, Nov 25, 2024 at 09:46:48AM +0100, Clément Léger wrote:
> 
> 
> On 22/11/2024 17:20, Andrew Jones wrote:
> > On Fri, Nov 22, 2024 at 03:04:56PM +0100, Clément Léger wrote:
> >> Add a SSE entry assembly code to handle SSE events. Events should be
> >> registered with a struct sse_handler_arg containing a correct stack and
> >> handler function.
> >>
> >> Signed-off-by: Clément Léger <cleger@rivosinc.com>
> >> ---
> >>  riscv/Makefile          |   1 +
> >>  lib/riscv/asm/sse.h     |  16 +++++++
> >>  lib/riscv/sse-entry.S   | 100 ++++++++++++++++++++++++++++++++++++++++
> > 
> > Let's just add the entry function to riscv/sbi-asm.S and the
> > sse_handler_arg struct definition to riscv/sbi-tests.h
> 
> Hi drew,
> 
> I need to have some offset generated using asm-offsets.c which is in
> lib/riscv. If I move the sse_handler_arg in riscv/sbi-tests.h, that will
> be really off to include that file in the lib/riscv/asm-offsets.c.

That's true, but it's also not great to put a test-specific definition of
an arg structure in lib code. It seems like we'll eventually want a neater
solution to this, though, since using asm-offsets for test-specific
structures makes sense. However, we could put it off for now, since each
member of the structure that SSE tests need is the same size,
sizeof(long), so we can do the same thing that HSM and SUSP do, which is
to define some indices and access with ASMARR().

> Except if you have some other solution.

ASMARR(), even though I'm not a huge fan of that approach either...

> 
> > 
> >>  lib/riscv/asm-offsets.c |   9 ++++
> >>  4 files changed, 126 insertions(+)
> >>  create mode 100644 lib/riscv/asm/sse.h
> >>  create mode 100644 lib/riscv/sse-entry.S
> >>
> >> diff --git a/riscv/Makefile b/riscv/Makefile
> >> index 28b04156..e50621ad 100644
> >> --- a/riscv/Makefile
> >> +++ b/riscv/Makefile
> >> @@ -39,6 +39,7 @@ cflatobjs += lib/riscv/sbi.o
> >>  cflatobjs += lib/riscv/setjmp.o
> >>  cflatobjs += lib/riscv/setup.o
> >>  cflatobjs += lib/riscv/smp.o
> >> +cflatobjs += lib/riscv/sse-entry.o
> >>  cflatobjs += lib/riscv/stack.o
> >>  cflatobjs += lib/riscv/timer.o
> >>  ifeq ($(ARCH),riscv32)
> >> diff --git a/lib/riscv/asm/sse.h b/lib/riscv/asm/sse.h
> >> new file mode 100644
> >> index 00000000..557f6680
> >> --- /dev/null
> >> +++ b/lib/riscv/asm/sse.h
> >> @@ -0,0 +1,16 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +#ifndef _ASMRISCV_SSE_H_
> >> +#define _ASMRISCV_SSE_H_
> >> +
> >> +typedef void (*sse_handler_fn)(void *data, struct pt_regs *regs, unsigned int hartid);
> >> +
> >> +struct sse_handler_arg {
> >> +	unsigned long reg_tmp;
> >> +	sse_handler_fn handler;
> >> +	void *handler_data;
> >> +	void *stack;
> >> +};
> >> +
> >> +extern void sse_entry(void);
> >> +
> >> +#endif /* _ASMRISCV_SSE_H_ */
> >> diff --git a/lib/riscv/sse-entry.S b/lib/riscv/sse-entry.S
> >> new file mode 100644
> >> index 00000000..bedc47e9
> >> --- /dev/null
> >> +++ b/lib/riscv/sse-entry.S
> >> @@ -0,0 +1,100 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +/*
> >> + * SBI SSE entry code
> >> + *
> >> + * Copyright (C) 2024, Rivos Inc., Clément Léger <cleger@rivosinc.com>
> >> + */
> >> +#include <asm/asm.h>
> >> +#include <asm/asm-offsets.h>
> >> +#include <asm/csr.h>
> >> +
> >> +.global sse_entry
> >> +sse_entry:
> >> +	/* Save stack temporarily */
> >> +	REG_S sp, SSE_REG_TMP(a6)

While thinking about the asm-offsets issue, I took a closer look at this
and noticed that this should be a7, since ENTRY_ARG is specified to be
set to A7. It looks like we have A6 (hartid) and A7 (arg) swapped. The
opensbi implementation also has them swapped, allowing this test to pass.
Both need to be fixed.

Thanks,
drew