Re: [PATCH] xen/riscv: read hart_id and dtb_base passed by bootloader

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2023 17:36, Oleksii wrote:
> On Fri, 2023-02-24 at 16:04 +0100, Jan Beulich wrote:
>> On 24.02.2023 15:48, Oleksii Kurochko wrote:
>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>> ---
>>>  xen/arch/riscv/setup.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
>>> index b3f8b10f71..154bf3a0bc 100644
>>> --- a/xen/arch/riscv/setup.c
>>> +++ b/xen/arch/riscv/setup.c
>>> @@ -26,6 +26,18 @@ static void test_macros_from_bug_h(void)
>>>  
>>>  void __init noreturn start_xen(void)
>>>  {
>>> +    /*
>>> +     * The following things are passed by bootloader:
>>> +     *   a0 -> hart_id
>>> +     *   a1 -> dtb_base
>>> +    */
>>> +    register unsigned long hart_id  asm("a0");
>>> +    register unsigned long dtb_base asm("a1");
>>> +
>>> +    asm volatile( "mv %0, a0" : "=r" (hart_id) );
>>> +
>>> +    asm volatile( "mv %0, a1" : "=r" (dtb_base) );
>>
>> Are you sure this (a) works and (b) is what you want? You're
>> inserting
> Oh, yeah... it should be:
>   unsigned long hart_id;
>   unsigned long dtb_base;

As per below - no, I don't think so. I continue to think these want
to be function parameters.

> I did experiments with 'register' and 'asm()' and after rebase of my
> internal branches git backed these changes...
> 
> Sorry for that I have to double check patches next time.
> 
> It looks like it works only because the variables aren't used anywhere.
>> "mov a0, a0" and "mov a1, a1". I suppose as long as the two local
>> variables aren't used further down in the function, the compiler will
>> be able to recognize both registers as dead, and hence use them for
>> argument passing to later functions that you call. But I would expect
>> that to break once you actually consume either of the variables.
>>
>> Fundamentally I think this is the kind of thing you want do to in
>> assembly. However, in the specific case here, can't you simply have
>>
>> void __init noreturn start_xen(unsigned long hard_id,
>>                                unsigned long dtb_base)
>> {
>>     ...
>>
>> and all is going to be fine, without any asm()?
> One of the things that I would like to do is to not use an assembler as
> much as possible. And as we have C environment ready after a few
> assembly instructions in head.S I thought it would be OK to do it in C
> code somewhere at the start so someone/sonething doesn't alter register
> a0 and a1.

Avoiding assembly code where possible if of course appreciated, because
generally C code is easier to maintain. But of course this can only be
done if things can be expressed correctly. And you need to keep in mind
that asm() statements also are assembly code, just lower volume.

Jan