Re: [PATCH v3 30/34] xen/riscv: add minimal stuff to processor.h to build full Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
>>> ---
>>> 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