| Message ID | 5bd7c5db6638f09dabdc13a6e12f0b204eacb234.1703255175.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 22.12.2023 16:13, Oleksii Kurochko wrote: > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in V3: > - Update the commit message ??? (again) > @@ -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) > +{ > + /* Encoding of the pause instruction */ > + __asm__ __volatile__ ( ".insn 0x100000F" ); binutils 2.40 knows "pause" - why use .insn then? > + barrier(); Why? Jan
On Tue, 2024-01-23 at 12:39 +0100, Jan Beulich wrote: > On 22.12.2023 16:13, Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > --- > > Changes in V3: > > - Update the commit message > > ??? (again) The same as with previous. asm/processor.h was changed to processor.h > > > @@ -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) > > +{ > > + /* Encoding of the pause instruction */ > > + __asm__ __volatile__ ( ".insn 0x100000F" ); > > binutils 2.40 knows "pause" - why use .insn then? I thought that for this instruction it is needed to have extension ZIHINTPAUSE ( according to Linux Kernel source code [1] ) and to cover older version. But looking at [2] and considering that I announced that we expected binutils >= 2.40 ( or 2.39 as I mentioned as an reply to patch to README file ) it can be used pause instruction. > > > + barrier(); > > Why? Just to be aligned with Linux kernel implemetation from where this function was taken. ~ Oleksii [1] https://elixir.bootlin.com/linux/latest/source/arch/riscv/include/asm/vdso/processor.h#L9 [2]https://elixir.bootlin.com/linux/latest/source/arch/riscv/Kconfig#L582
On 23.01.2024 18:08, Oleksii wrote: > On Tue, 2024-01-23 at 12:39 +0100, Jan Beulich wrote: >> On 22.12.2023 16:13, Oleksii Kurochko wrote: >>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >>> --- >>> Changes in V3: >>> - Update the commit message >> >> ??? (again) > The same as with previous. asm/processor.h was changed to processor.h > >> >>> @@ -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) >>> +{ >>> + /* Encoding of the pause instruction */ >>> + __asm__ __volatile__ ( ".insn 0x100000F" ); >> >> binutils 2.40 knows "pause" - why use .insn then? > I thought that for this instruction it is needed to have extension > ZIHINTPAUSE ( according to Linux Kernel source code [1] ) and to cover > older version. Well, of course you'll need to enable the extension then for gas. But as long as you use the insn unconditionally, that's all fine and natural. Another thing would be if you meant to also run on systems not supporting the extension: Then the above use of .insn would need to become conditional anyway. >>> + barrier(); >> >> Why? > Just to be aligned with Linux kernel implemetation from where this > function was taken. Hmm, looking more closely we have an (open-coded) barrier even on x86. So I suppose it's really wanted (to keep the compiler from moving memory accesses around this construct), but then you may want to consider using __asm__ __volatile__ ( "pause" ::: "memory" ); here. First and foremost because at least in the general case two separate asm()s aren't the same as one combined one (volatile ones are more restricted, but I'd always err on the safe side, even if just to avoid giving bad examples which later on may be taken as a basis for deriving other code). Jan
On Wed, 2024-01-24 at 09:19 +0100, Jan Beulich wrote: > On 23.01.2024 18:08, Oleksii wrote: > > On Tue, 2024-01-23 at 12:39 +0100, Jan Beulich wrote: > > > On 22.12.2023 16:13, Oleksii Kurochko wrote: > > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > > > --- > > > > Changes in V3: > > > > - Update the commit message > > > > > > ??? (again) > > The same as with previous. asm/processor.h was changed to > > processor.h > > > > > > > > > @@ -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) > > > > +{ > > > > + /* Encoding of the pause instruction */ > > > > + __asm__ __volatile__ ( ".insn 0x100000F" ); > > > > > > binutils 2.40 knows "pause" - why use .insn then? > > I thought that for this instruction it is needed to have extension > > ZIHINTPAUSE ( according to Linux Kernel source code [1] ) and to > > cover > > older version. > > Well, of course you'll need to enable the extension then for gas. But > as long as you use the insn unconditionally, that's all fine and > natural. Another thing would be if you meant to also run on systems > not supporting the extension: Then the above use of .insn would need > to become conditional anyway. Then it makes sense to use "pause". Let's assume that for now we are running only on systems which support the extension until we won't face compilation issue for some system. > > > > > + barrier(); > > > > > > Why? > > Just to be aligned with Linux kernel implemetation from where this > > function was taken. > > Hmm, looking more closely we have an (open-coded) barrier even on > x86. > So I suppose it's really wanted (to keep the compiler from moving > memory accesses around this construct), but then you may want to > consider using > > __asm__ __volatile__ ( "pause" ::: "memory" ); > > here. First and foremost because at least in the general case two > separate asm()s aren't the same as one combined one (volatile ones > are more restricted, but I'd always err on the safe side, even if > just to avoid giving bad examples which later on may be taken as a > basis for deriving other code). It makes sense, I'll update inline assembler line code. Thanks. ~ Oleksii
On 24.01.2024 11:12, Oleksii wrote: > On Wed, 2024-01-24 at 09:19 +0100, Jan Beulich wrote: >> On 23.01.2024 18:08, Oleksii wrote: >>> On Tue, 2024-01-23 at 12:39 +0100, Jan Beulich wrote: >>>> On 22.12.2023 16:13, Oleksii Kurochko wrote: >>>>> @@ -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) >>>>> +{ >>>>> + /* Encoding of the pause instruction */ >>>>> + __asm__ __volatile__ ( ".insn 0x100000F" ); >>>> >>>> binutils 2.40 knows "pause" - why use .insn then? >>> I thought that for this instruction it is needed to have extension >>> ZIHINTPAUSE ( according to Linux Kernel source code [1] ) and to >>> cover >>> older version. >> >> Well, of course you'll need to enable the extension then for gas. But >> as long as you use the insn unconditionally, that's all fine and >> natural. Another thing would be if you meant to also run on systems >> not supporting the extension: Then the above use of .insn would need >> to become conditional anyway. > Then it makes sense to use "pause". > Let's assume that for now we are running only on systems which support > the extension until we won't face compilation issue for some system. Gives me the impression that you still don't properly separate the two aspects: One is what systems Xen is to run on, and other is what's needed to make Xen build properly. The first needs documenting (and ideally at some point actually enforcing), while the latter may require e.g. compiler command line option adjustments. Jan
On Wed, 2024-01-24 at 12:27 +0100, Jan Beulich wrote: > On 24.01.2024 11:12, Oleksii wrote: > > On Wed, 2024-01-24 at 09:19 +0100, Jan Beulich wrote: > > > On 23.01.2024 18:08, Oleksii wrote: > > > > On Tue, 2024-01-23 at 12:39 +0100, Jan Beulich wrote: > > > > > On 22.12.2023 16:13, Oleksii Kurochko wrote: > > > > > > @@ -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) > > > > > > +{ > > > > > > + /* Encoding of the pause instruction */ > > > > > > + __asm__ __volatile__ ( ".insn 0x100000F" ); > > > > > > > > > > binutils 2.40 knows "pause" - why use .insn then? > > > > I thought that for this instruction it is needed to have > > > > extension > > > > ZIHINTPAUSE ( according to Linux Kernel source code [1] ) and > > > > to > > > > cover > > > > older version. > > > > > > Well, of course you'll need to enable the extension then for gas. > > > But > > > as long as you use the insn unconditionally, that's all fine and > > > natural. Another thing would be if you meant to also run on > > > systems > > > not supporting the extension: Then the above use of .insn would > > > need > > > to become conditional anyway. > > Then it makes sense to use "pause". > > Let's assume that for now we are running only on systems which > > support > > the extension until we won't face compilation issue for some > > system. > > Gives me the impression that you still don't properly separate the > two > aspects: One is what systems Xen is to run on, and other is what's > needed to make Xen build properly. The first needs documenting (and > ideally at some point actually enforcing), while the latter may > require > e.g. compiler command line option adjustments. I understand that it will be required update "-march=..._zihintpause" and it should be a check that this extension is supported by a toolchain. But I am not sure that I know how can I enforce that a system should have this extension, and considering Linux kernel implementation which uses always pause instruction, it looks like all available systems support this extension. But I agree what I wrote above isn't fully correct. ~ Oleksii
On 24.01.2024 16:33, Oleksii wrote: > On Wed, 2024-01-24 at 12:27 +0100, Jan Beulich wrote: >> On 24.01.2024 11:12, Oleksii wrote: >>> On Wed, 2024-01-24 at 09:19 +0100, Jan Beulich wrote: >>>> On 23.01.2024 18:08, Oleksii wrote: >>>>> On Tue, 2024-01-23 at 12:39 +0100, Jan Beulich wrote: >>>>>> On 22.12.2023 16:13, Oleksii Kurochko wrote: >>>>>>> @@ -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) >>>>>>> +{ >>>>>>> + /* Encoding of the pause instruction */ >>>>>>> + __asm__ __volatile__ ( ".insn 0x100000F" ); >>>>>> >>>>>> binutils 2.40 knows "pause" - why use .insn then? >>>>> I thought that for this instruction it is needed to have >>>>> extension >>>>> ZIHINTPAUSE ( according to Linux Kernel source code [1] ) and >>>>> to >>>>> cover >>>>> older version. >>>> >>>> Well, of course you'll need to enable the extension then for gas. >>>> But >>>> as long as you use the insn unconditionally, that's all fine and >>>> natural. Another thing would be if you meant to also run on >>>> systems >>>> not supporting the extension: Then the above use of .insn would >>>> need >>>> to become conditional anyway. >>> Then it makes sense to use "pause". >>> Let's assume that for now we are running only on systems which >>> support >>> the extension until we won't face compilation issue for some >>> system. >> >> Gives me the impression that you still don't properly separate the >> two >> aspects: One is what systems Xen is to run on, and other is what's >> needed to make Xen build properly. The first needs documenting (and >> ideally at some point actually enforcing), while the latter may >> require >> e.g. compiler command line option adjustments. > I understand that it will be required update "-march=..._zihintpause" > and it should be a check that this extension is supported by a > toolchain. > > But I am not sure that I know how can I enforce that a system should > have this extension, and considering Linux kernel implementation which > uses always pause instruction, it looks like all available systems > support this extension. Which is why I said documenting will suffice, at least for now. Jan
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h index 6db681d805..a3bff6c9c3 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 smp_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) +{ + /* Encoding of the pause instruction */ + __asm__ __volatile__ ( ".insn 0x100000F" ); + + barrier(); +} + static inline void wfi(void) { __asm__ __volatile__ ("wfi");
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V3: - Update the commit message - rename get_processor_id to smp_processor_id - code style fixes - update the cpu_relax instruction: use pause instruction instead of div %0, %0, zero --- Changes in V2: - Nothing changed. Only rebase. --- xen/arch/riscv/include/asm/processor.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)