| Message ID | 11f177882b74c60233626075a69bdd00d3da2311.1700761381.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 24.11.2023 11:30, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/processor.h > +++ b/xen/arch/riscv/include/asm/processor.h > @@ -12,6 +12,9 @@ > > #ifndef __ASSEMBLY__ > > +/* TODO: need to be implemeted */ > +#define get_processor_id() 0 Please don't re-introduce this - it was just recently dropped from the code base. > @@ -53,6 +56,18 @@ struct cpu_user_regs > unsigned long pregs; > }; > > +/* TODO: need to implement */ > +#define cpu_to_core(_cpu) (0) > +#define cpu_to_socket(_cpu) (0) No need for leading underscores here. > +static inline void cpu_relax(void) > +{ > + int dummy; > + /* In lieu of a halt instruction, induce a long-latency stall. */ > + __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy)); Any reason for this, when Arm's is just barrier(), and apparently they got away with this quite fine? Also isn't this causing a division by zero, which I'd expect to cause some kind of exception? (Terminology-wise I'm of course biased by x86, where "halt instruction" wouldn't be suitable to use here. But if that terminology is fine on RISC-V, then obviously no objection.) Jan
On Thu, 2023-12-14 at 17:04 +0100, Jan Beulich wrote: > On 24.11.2023 11:30, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/include/asm/processor.h > > +++ b/xen/arch/riscv/include/asm/processor.h > > @@ -12,6 +12,9 @@ > > > > #ifndef __ASSEMBLY__ > > > > +/* TODO: need to be implemeted */ > > +#define get_processor_id() 0 > > Please don't re-introduce this - it was just recently dropped from > the > code base. Thanks for remind me that. I'll drop it in the next version. > > > @@ -53,6 +56,18 @@ struct cpu_user_regs > > unsigned long pregs; > > }; > > > > +/* TODO: need to implement */ > > +#define cpu_to_core(_cpu) (0) > > +#define cpu_to_socket(_cpu) (0) > > No need for leading underscores here. Sure. Thanks. > > > +static inline void cpu_relax(void) > > +{ > > + int dummy; > > + /* In lieu of a halt instruction, induce a long-latency > > stall. */ > > + __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy)); > > Any reason for this, when Arm's is just barrier(), and apparently > they got > away with this quite fine? Also isn't this causing a division by > zero, > which I'd expect to cause some kind of exception? (Terminology-wise > I'm of > course biased by x86, where "halt instruction" wouldn't be suitable > to use > here. But if that terminology is fine on RISC-V, then obviously no > objection.) It was based on Linux kernel code: https://elixir.bootlin.com/linux/latest/source/arch/riscv/include/asm/vdso/processor.h#L9 But looks I missed barrier()... Probably it will be better update cpu_relax() to: /* Encoding of the pause instruction */ __asm__ __volatile__ (".4byte 0x100000F"); barrier(); ~ Oleksii
On 18.12.2023 11:49, Oleksii wrote: > On Thu, 2023-12-14 at 17:04 +0100, Jan Beulich wrote: >> On 24.11.2023 11:30, Oleksii Kurochko wrote: >>> +static inline void cpu_relax(void) >>> +{ >>> + int dummy; >>> + /* In lieu of a halt instruction, induce a long-latency >>> stall. */ >>> + __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy)); >> >> Any reason for this, when Arm's is just barrier(), and apparently >> they got >> away with this quite fine? Also isn't this causing a division by >> zero, >> which I'd expect to cause some kind of exception? (Terminology-wise >> I'm of >> course biased by x86, where "halt instruction" wouldn't be suitable >> to use >> here. But if that terminology is fine on RISC-V, then obviously no >> objection.) > It was based on Linux kernel code: > https://elixir.bootlin.com/linux/latest/source/arch/riscv/include/asm/vdso/processor.h#L9 > > But looks I missed barrier()... > Probably it will be better update cpu_relax() to: > > /* Encoding of the pause instruction */ > __asm__ __volatile__ (".4byte 0x100000F"); > > barrier(); But definitely without .4byte, which defines a piece of data. If for whatever reason you don't want to use "pause" directly, please use .insn. Jan
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h index 6db681d805..b6218a00a7 100644 --- a/xen/arch/riscv/include/asm/processor.h +++ b/xen/arch/riscv/include/asm/processor.h @@ -12,6 +12,9 @@ #ifndef __ASSEMBLY__ +/* TODO: need to be implemeted */ +#define get_processor_id() 0 + /* On stack VCPU state */ struct cpu_user_regs { @@ -53,6 +56,18 @@ struct cpu_user_regs unsigned long pregs; }; +/* TODO: need to implement */ +#define cpu_to_core(_cpu) (0) +#define cpu_to_socket(_cpu) (0) + +static inline void cpu_relax(void) +{ + int dummy; + /* In lieu of a halt instruction, induce a long-latency stall. */ + __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy)); + barrier(); +} + static inline void wfi(void) { __asm__ __volatile__ ("wfi");
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V2: - Nothing changed. Only rebase. --- xen/arch/riscv/include/asm/processor.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)