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

[Jan Beulich <jbeulich@xxxxxxxx>]

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
"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()?

Otherwise again a style nit: In the asm statements (not the register
declarations) there is a missing blank each before the opening
parenthesis.

Jan