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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.02.2023 12:19, Oleksii wrote:
> On Mon, 2023-02-27 at 10:17 +0100, Jan Beulich wrote:
>> 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.
>>
> Let's get hard_id and dtb_base in head.S and pass them as arguments of
> start_xen() function as you mentioned before.

Still looks like a misunderstanding to me. Aiui a0 and a1 are the
registers to hold first and second function arguments each. If
firmware places the two values in these two registers, then
start_xen(), with the adjusted declaration, will fit the purpose
without any assembly code.

Jan