| Message ID | 94d722dea114defa2a5efe29e6511829f0c54b41.1677249592.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/riscv: read hart_id and dtb_base passed by bootloader | expand |
On 24.02.2023 15:48, Oleksii Kurochko wrote: > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > 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
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@gmail.com> > > --- > > 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; 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. > > Otherwise again a style nit: In the asm statements (not the register > declarations) there is a missing blank each before the opening > parenthesis. > > Jan ~Oleksii
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@gmail.com> >>> --- >>> 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
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@gmail.com> > > > > --- > > > > 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. I'll update the patch and send new version. > Jan ~ Oleksii
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@gmail.com> >>>>> --- >>>>> 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
On Mon, 2023-02-27 at 12:37 +0100, Jan Beulich wrote: > 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@gmail.com> > > > > > > --- > > > > > > 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. > It will work for the code we have now, but it will be more save to save registers a0 and a1 to some variables and pass them to start_xen() as arguments as they can be changed by something before the start_xen() call in the future. ~ Oleksii
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) ); + early_printk("Hello from C env\n"); trap_init();
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/setup.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)