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

[Jan Beulich <jbeulich@xxxxxxxx>]

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