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 <[email protected]> >>>>> --- >>>>> 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
