[v1,4/8] xen/riscv: introduce sbi call to putchar to console

Commit Message

Oleksii Jan. 6, 2023, 1:14 p.m. UTC

The patch introduce sbi_putchar() SBI call which is necessary
to implement initial early_printk

Signed-off-by: Oleksii Kurochko <[e-mail]>
---
 xen/arch/riscv/Makefile          |  1 +
 xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
 xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/sbi.h
 create mode 100644 xen/arch/riscv/sbi.c

Comments

Bobby Eshleman Dec. 20, 2022, 6:23 a.m. UTC | #1

On Fri, Jan 06, 2023 at 03:14:25PM +0200, Oleksii Kurochko wrote:
> The patch introduce sbi_putchar() SBI call which is necessary
> to implement initial early_printk
> 

I think that it might be wise to start off with an alternative to
sbi_putchar() since it is already planned for deprecation. I realize
that this will require rework, but it is almost guaranteed that
early_printk() will break on future SBI implementations if using this
SBI call. IIRC, Xen/ARM's early printk looked like a reasonable analogy
for how it could work on RISC-V, IMHO.

> Signed-off-by: Oleksii Kurochko <[e-mail]>
> ---
>  xen/arch/riscv/Makefile          |  1 +
>  xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>  xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/sbi.h
>  create mode 100644 xen/arch/riscv/sbi.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 5a67a3f493..60db415654 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_RISCV_64) += riscv64/
>  obj-y += setup.o
> +obj-y += sbi.o
>  
>  $(TARGET): $(TARGET)-syms
>  	$(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
> new file mode 100644
> index 0000000000..34b53f8eaf
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later) */
> +/*
> + * Copyright (c) 2021 Vates SAS.
> + *
> + * Taken from xvisor, modified by Bobby Eshleman ([e-mail]).
> + *
> + * Taken/modified from Xvisor project with the following copyright:
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + */
> +
> +#ifndef __CPU_SBI_H__
> +#define __CPU_SBI_H__
> +
> +#define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
> +
> +struct sbiret {
> +    long error;
> +    long value;
> +};
> +
> +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
> +        unsigned long arg1, unsigned long arg2,
> +        unsigned long arg3, unsigned long arg4,
> +        unsigned long arg5);
> +
> +/**
> + * Writes given character to the console device.
> + *
> + * @param ch The data to be written to the console.
> + */
> +void sbi_console_putchar(int ch);
> +
> +#endif // __CPU_SBI_H__
> diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
> new file mode 100644
> index 0000000000..67cf5dd982
> --- /dev/null
> +++ b/xen/arch/riscv/sbi.c
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Taken and modified from the xvisor project with the copyright Copyright (c)
> + * 2019 Western Digital Corporation or its affiliates and author Anup Patel
> + * ([e-mail]).
> + *
> + * Modified by Bobby Eshleman ([e-mail]).
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + * Copyright (c) 2021 Vates SAS.
> + */
> +
> +#include <xen/errno.h>
> +#include <asm/sbi.h>
> +
> +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
> +            unsigned long arg1, unsigned long arg2,
> +            unsigned long arg3, unsigned long arg4,
> +            unsigned long arg5)
> +{
> +    struct sbiret ret;
> +    register unsigned long a0 asm ("a0") = arg0;
> +    register unsigned long a1 asm ("a1") = arg1;
> +    register unsigned long a2 asm ("a2") = arg2;
> +    register unsigned long a3 asm ("a3") = arg3;
> +    register unsigned long a4 asm ("a4") = arg4;
> +    register unsigned long a5 asm ("a5") = arg5;
> +    register unsigned long a6 asm ("a6") = fid;
> +    register unsigned long a7 asm ("a7") = ext;
> +
> +    asm volatile ("ecall"
> +              : "+r" (a0), "+r" (a1)
> +              : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7)
> +              : "memory");
> +    ret.error = a0;
> +    ret.value = a1;
> +
> +    return ret;
> +}
> +
> +void sbi_console_putchar(int ch)
> +{
> +    sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
> +}
> -- 
> 2.38.1
> 
>

Bobby Eshleman Dec. 20, 2022, 6:50 a.m. UTC | #2

On Fri, Jan 06, 2023 at 05:16:31PM +0000, Andrew Cooper wrote:
> On 20/12/2022 6:23 am, Bobby Eshleman wrote:
> > On Fri, Jan 06, 2023 at 03:14:25PM +0200, Oleksii Kurochko wrote:
> >> The patch introduce sbi_putchar() SBI call which is necessary
> >> to implement initial early_printk
> >>
> > I think that it might be wise to start off with an alternative to
> > sbi_putchar() since it is already planned for deprecation. I realize
> > that this will require rework, but it is almost guaranteed that
> > early_printk() will break on future SBI implementations if using this
> > SBI call. IIRC, Xen/ARM's early printk looked like a reasonable analogy
> > for how it could work on RISC-V, IMHO.
> 
> Hmm, we're using it as a stopgap right now in CI so we can break
> "upstream RISC-V support" into manageable chunks.
> 
> So perhaps we want to forgo plumbing sbi_putchar() into early_printk() 
> (to avoid giving the impression that this will stay around in the
> future) and use sbi_putchar() directly for the hello world print.
> 
> Next, we focus on getting the real uart driver working along with real
> printk (rather than focusing on exceptions which was going to be the
> next task), and we can drop sbi_putchar() entirely.
> 
> Thoughts?
> 
> ~Andrew

That sounds like a reasonable approach to me.

Best,
Bobby

Julien Grall Jan. 6, 2023, 1:40 p.m. UTC | #3

Hi,

On 06/01/2023 13:14, Oleksii Kurochko wrote:
> The patch introduce sbi_putchar() SBI call which is necessary
> to implement initial early_printk
> 
> Signed-off-by: Oleksii Kurochko <[e-mail]>
> ---
>   xen/arch/riscv/Makefile          |  1 +
>   xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>   xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++

IMHO, it would be better to implement sbi.c in assembly so you can use 
print in the console before you jump to C world.

>   3 files changed, 79 insertions(+)
>   create mode 100644 xen/arch/riscv/include/asm/sbi.h
>   create mode 100644 xen/arch/riscv/sbi.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 5a67a3f493..60db415654 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,5 +1,6 @@
>   obj-$(CONFIG_RISCV_64) += riscv64/
>   obj-y += setup.o
> +obj-y += sbi.o

Please order the filename alphabetically.

Cheers,

Andrew Cooper Jan. 6, 2023, 3:19 p.m. UTC | #4

On 06/01/2023 1:40 pm, Julien Grall wrote:
> Hi,
>
> On 06/01/2023 13:14, Oleksii Kurochko wrote:
>> The patch introduce sbi_putchar() SBI call which is necessary
>> to implement initial early_printk
>>
>> Signed-off-by: Oleksii Kurochko <[e-mail]>
>> ---
>>   xen/arch/riscv/Makefile          |  1 +
>>   xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>>   xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
>
> IMHO, it would be better to implement sbi.c in assembly so you can use
> print in the console before you jump to C world.

That was already requested not to happen.  Frankly, if I was an arm
maintainer I'd object to the how it's used there too, but RISCV is
massively more simple still.

Not even the pagetable setup, or the physical relocation (if even
necessary) needs doing in ASM.

I'm not completely ruling it out in the future, but someone is going to
have to come up with a very convincing argument for why they can't do
this piece of critical setup in C.

~Andrew

Michal Orzel Jan. 6, 2023, 3:19 p.m. UTC | #5

Hi Oleksii,

On 06/01/2023 14:14, Oleksii Kurochko wrote:
> 
> 
> The patch introduce sbi_putchar() SBI call which is necessary
> to implement initial early_printk
> 
> Signed-off-by: Oleksii Kurochko <[e-mail]>
> ---
>  xen/arch/riscv/Makefile          |  1 +
>  xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>  xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/sbi.h
>  create mode 100644 xen/arch/riscv/sbi.c
> 
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index 5a67a3f493..60db415654 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_RISCV_64) += riscv64/
>  obj-y += setup.o
> +obj-y += sbi.o
> 
>  $(TARGET): $(TARGET)-syms
>         $(OBJCOPY) -O binary -S $< $@
> diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
> new file mode 100644
> index 0000000000..34b53f8eaf
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later) */
> +/*
> + * Copyright (c) 2021 Vates SAS.
> + *
> + * Taken from xvisor, modified by Bobby Eshleman ([e-mail]).
> + *
> + * Taken/modified from Xvisor project with the following copyright:
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + */
> +
> +#ifndef __CPU_SBI_H__
> +#define __CPU_SBI_H__
I wonder where does CPU come from. Shouldn't this be called __ASM_RISCV_SBI_H__ ?

> +
> +#define SBI_EXT_0_1_CONSOLE_PUTCHAR            0x1
> +
> +struct sbiret {
> +    long error;
> +    long value;
> +};
> +
> +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
> +        unsigned long arg1, unsigned long arg2,
> +        unsigned long arg3, unsigned long arg4,
> +        unsigned long arg5);
The arguments needs to be aligned.

> +
> +/**
> + * Writes given character to the console device.
> + *
> + * @param ch The data to be written to the console.
> + */
> +void sbi_console_putchar(int ch);
> +
> +#endif // __CPU_SBI_H__
// should be replaced with /* */

> diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
> new file mode 100644
> index 0000000000..67cf5dd982
> --- /dev/null
> +++ b/xen/arch/riscv/sbi.c
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Taken and modified from the xvisor project with the copyright Copyright (c)
> + * 2019 Western Digital Corporation or its affiliates and author Anup Patel
> + * ([e-mail]).
> + *
> + * Modified by Bobby Eshleman ([e-mail]).
> + *
> + * Copyright (c) 2019 Western Digital Corporation or its affiliates.
> + * Copyright (c) 2021 Vates SAS.
> + */
> +
> +#include <xen/errno.h>
> +#include <asm/sbi.h>
> +
> +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
> +            unsigned long arg1, unsigned long arg2,
> +            unsigned long arg3, unsigned long arg4,
> +            unsigned long arg5)
The arguments needs to be aligned.

> +{
> +    struct sbiret ret;
Could you please add an empty line here.

> +    register unsigned long a0 asm ("a0") = arg0;
> +    register unsigned long a1 asm ("a1") = arg1;
> +    register unsigned long a2 asm ("a2") = arg2;
> +    register unsigned long a3 asm ("a3") = arg3;
> +    register unsigned long a4 asm ("a4") = arg4;
> +    register unsigned long a5 asm ("a5") = arg5;
> +    register unsigned long a6 asm ("a6") = fid;
> +    register unsigned long a7 asm ("a7") = ext;
> +
> +    asm volatile ("ecall"
> +              : "+r" (a0), "+r" (a1)
> +              : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7)
> +              : "memory");
> +    ret.error = a0;
> +    ret.value = a1;
> +
> +    return ret;
> +}
> +
> +void sbi_console_putchar(int ch)
> +{
> +    sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
> +}
> --
> 2.38.1
> 
> 

~Michal

Julien Grall Jan. 6, 2023, 3:39 p.m. UTC | #6

Hi Andrew,

On 06/01/2023 15:19, Andrew Cooper wrote:
> On 06/01/2023 1:40 pm, Julien Grall wrote:
>> Hi,
>>
>> On 06/01/2023 13:14, Oleksii Kurochko wrote:
>>> The patch introduce sbi_putchar() SBI call which is necessary
>>> to implement initial early_printk
>>>
>>> Signed-off-by: Oleksii Kurochko <[e-mail]>
>>> ---
>>>    xen/arch/riscv/Makefile          |  1 +
>>>    xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>>>    xen/arch/riscv/sbi.c             | 44 ++++++++++++++++++++++++++++++++
>>
>> IMHO, it would be better to implement sbi.c in assembly so you can use
>> print in the console before you jump to C world.
> 
> That was already requested not to happen.  Frankly, if I was an arm
> maintainer I'd object to the how it's used there too, but RISCV is
> massively more simple still.

There are a few reasons:
   1) Xen is not fully position independent. Even if -fpic is enabled, 
you would still need apply some relocation in order if you want to use 
it before running in the virtual address space.
   2) Doing any memory access before the MMU is setup requires some 
careful though (see [1]). With function implemented in C, you can't 
really control which memory accesses are done.

> 
> Not even the pagetable setup, or the physical relocation (if even
> necessary) needs doing in ASM.
> 
> I'm not completely ruling it out in the future, but someone is going to
> have to come up with a very convincing argument for why they can't do
> this piece of critical setup in C.

That's great if RISC-V doesn't have the issues I mentioned above. On 
Arm, I don't really think we can get away. But feel free to explain how 
this could be done...

Cheers,

[1] 
https://events.static.linuxfound.org/sites/events/files/slides/slides_10.pdf

Andrew Cooper Jan. 6, 2023, 5:16 p.m. UTC | #7

On 20/12/2022 6:23 am, Bobby Eshleman wrote:
> On Fri, Jan 06, 2023 at 03:14:25PM +0200, Oleksii Kurochko wrote:
>> The patch introduce sbi_putchar() SBI call which is necessary
>> to implement initial early_printk
>>
> I think that it might be wise to start off with an alternative to
> sbi_putchar() since it is already planned for deprecation. I realize
> that this will require rework, but it is almost guaranteed that
> early_printk() will break on future SBI implementations if using this
> SBI call. IIRC, Xen/ARM's early printk looked like a reasonable analogy
> for how it could work on RISC-V, IMHO.

Hmm, we're using it as a stopgap right now in CI so we can break
"upstream RISC-V support" into manageable chunks.

So perhaps we want to forgo plumbing sbi_putchar() into early_printk() 
(to avoid giving the impression that this will stay around in the
future) and use sbi_putchar() directly for the hello world print.

Next, we focus on getting the real uart driver working along with real
printk (rather than focusing on exceptions which was going to be the
next task), and we can drop sbi_putchar() entirely.

Thoughts?

~Andrew

Oleksii Jan. 9, 2023, 9:04 a.m. UTC | #8

Hi,

On Fri, 2023-01-06 at 13:40 +0000, Julien Grall wrote:
> Hi,
> 
> On 06/01/2023 13:14, Oleksii Kurochko wrote:
> > The patch introduce sbi_putchar() SBI call which is necessary
> > to implement initial early_printk
> > 
> > Signed-off-by: Oleksii Kurochko <[e-mail]>
> > ---
> >   xen/arch/riscv/Makefile          |  1 +
> >   xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
> >   xen/arch/riscv/sbi.c             | 44
> > ++++++++++++++++++++++++++++++++
> 
> IMHO, it would be better to implement sbi.c in assembly so you can
> use 
> print in the console before you jump to C world.
> 
I thought that we can live with C version as we set up stack from the
start and then we can call early_printk() from assembly code too.
Is it bad approach?

> >   3 files changed, 79 insertions(+)
> >   create mode 100644 xen/arch/riscv/include/asm/sbi.h
> >   create mode 100644 xen/arch/riscv/sbi.c
> > 
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 5a67a3f493..60db415654 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,5 +1,6 @@
> >   obj-$(CONFIG_RISCV_64) += riscv64/
> >   obj-y += setup.o
> > +obj-y += sbi.o
> 
> Please order the filename alphabetically.
> 
> Cheers,
>

Oleksii Jan. 9, 2023, 9:06 a.m. UTC | #9

Hi Michal,

On Fri, 2023-01-06 at 16:19 +0100, Michal Orzel wrote:
> Hi Oleksii,
> 
> On 06/01/2023 14:14, Oleksii Kurochko wrote:
> > 
> > 
> > The patch introduce sbi_putchar() SBI call which is necessary
> > to implement initial early_printk
> > 
> > Signed-off-by: Oleksii Kurochko <[e-mail]>
> > ---
> >  xen/arch/riscv/Makefile          |  1 +
> >  xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
> >  xen/arch/riscv/sbi.c             | 44
> > ++++++++++++++++++++++++++++++++
> >  3 files changed, 79 insertions(+)
> >  create mode 100644 xen/arch/riscv/include/asm/sbi.h
> >  create mode 100644 xen/arch/riscv/sbi.c
> > 
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index 5a67a3f493..60db415654 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,5 +1,6 @@
> >  obj-$(CONFIG_RISCV_64) += riscv64/
> >  obj-y += setup.o
> > +obj-y += sbi.o
> > 
> >  $(TARGET): $(TARGET)-syms
> >         $(OBJCOPY) -O binary -S $< $@
> > diff --git a/xen/arch/riscv/include/asm/sbi.h
> > b/xen/arch/riscv/include/asm/sbi.h
> > new file mode 100644
> > index 0000000000..34b53f8eaf
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/sbi.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-or-later) */
> > +/*
> > + * Copyright (c) 2021 Vates SAS.
> > + *
> > + * Taken from xvisor, modified by Bobby Eshleman
> > ([e-mail]).
> > + *
> > + * Taken/modified from Xvisor project with the following
> > copyright:
> > + *
> > + * Copyright (c) 2019 Western Digital Corporation or its
> > affiliates.
> > + */
> > +
> > +#ifndef __CPU_SBI_H__
> > +#define __CPU_SBI_H__
> I wonder where does CPU come from. Shouldn't this be called
> __ASM_RISCV_SBI_H__ ?
> 
I missed that when get this files from Bobby's patch series.
It makes sense to rename the define.
Thanks.
> > +
> > +#define SBI_EXT_0_1_CONSOLE_PUTCHAR            0x1
> > +
> > +struct sbiret {
> > +    long error;
> > +    long value;
> > +};
> > +
> > +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid,
> > unsigned long arg0,
> > +        unsigned long arg1, unsigned long arg2,
> > +        unsigned long arg3, unsigned long arg4,
> > +        unsigned long arg5);
> The arguments needs to be aligned.
> 
> > +
> > +/**
> > + * Writes given character to the console device.
> > + *
> > + * @param ch The data to be written to the console.
> > + */
> > +void sbi_console_putchar(int ch);
> > +
> > +#endif // __CPU_SBI_H__
> // should be replaced with /* */
> 
> > diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
> > new file mode 100644
> > index 0000000000..67cf5dd982
> > --- /dev/null
> > +++ b/xen/arch/riscv/sbi.c
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Taken and modified from the xvisor project with the copyright
> > Copyright (c)
> > + * 2019 Western Digital Corporation or its affiliates and author
> > Anup Patel
> > + * ([e-mail]).
> > + *
> > + * Modified by Bobby Eshleman ([e-mail]).
> > + *
> > + * Copyright (c) 2019 Western Digital Corporation or its
> > affiliates.
> > + * Copyright (c) 2021 Vates SAS.
> > + */
> > +
> > +#include <xen/errno.h>
> > +#include <asm/sbi.h>
> > +
> > +struct sbiret sbi_ecall(unsigned long ext, unsigned long fid,
> > unsigned long arg0,
> > +            unsigned long arg1, unsigned long arg2,
> > +            unsigned long arg3, unsigned long arg4,
> > +            unsigned long arg5)
> The arguments needs to be aligned.
> 
It looks that I mixed tabs with space or vice versa.
Will double check.
Thanks.
> > +{
> > +    struct sbiret ret;
> Could you please add an empty line here.
> 
> > +    register unsigned long a0 asm ("a0") = arg0;
> > +    register unsigned long a1 asm ("a1") = arg1;
> > +    register unsigned long a2 asm ("a2") = arg2;
> > +    register unsigned long a3 asm ("a3") = arg3;
> > +    register unsigned long a4 asm ("a4") = arg4;
> > +    register unsigned long a5 asm ("a5") = arg5;
> > +    register unsigned long a6 asm ("a6") = fid;
> > +    register unsigned long a7 asm ("a7") = ext;
> > +
> > +    asm volatile ("ecall"
> > +              : "+r" (a0), "+r" (a1)
> > +              : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6),
> > "r" (a7)
> > +              : "memory");
> > +    ret.error = a0;
> > +    ret.value = a1;
> > +
> > +    return ret;
> > +}
> > +
> > +void sbi_console_putchar(int ch)
> > +{
> > +    sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
> > +}
> > --
> > 2.38.1
> > 
> > 
> 
> ~Michal
~Oleksii

Julien Grall Jan. 9, 2023, 11:11 a.m. UTC | #10

Hi Oleksii,

On 09/01/2023 09:04, Oleksii wrote:
> On Fri, 2023-01-06 at 13:40 +0000, Julien Grall wrote:
>> Hi,
>>
>> On 06/01/2023 13:14, Oleksii Kurochko wrote:
>>> The patch introduce sbi_putchar() SBI call which is necessary
>>> to implement initial early_printk
>>>
>>> Signed-off-by: Oleksii Kurochko <[e-mail]>
>>> ---
>>>    xen/arch/riscv/Makefile          |  1 +
>>>    xen/arch/riscv/include/asm/sbi.h | 34 ++++++++++++++++++++++++
>>>    xen/arch/riscv/sbi.c             | 44
>>> ++++++++++++++++++++++++++++++++
>>
>> IMHO, it would be better to implement sbi.c in assembly so you can
>> use
>> print in the console before you jump to C world.
>>
> I thought that we can live with C version as we set up stack from the
> start and then we can call early_printk() from assembly code too.
> Is it bad approach?

It depends on how early you want to call it. For Arm, we chose to use 
assembly because the C code may not be PIE (and even with PIE it may 
need some relocation work).

Andrew suggested that this may not be a problem with RISC-V. I have 
looked a bit more around and notice that the kernel is also calling some 
C function very early (like setup_vm()). But they ensure that the code 
is built with -mcmodel=medany.

It looks like you are already building Xen with this option. So all 
looks good for RISC-V. That said, I would suggest to check that 
__riscv_cmodel_medany is defined in files where you implement C function 
called from early assembly code.

Cheers,

Oleksii Jan. 9, 2023, 1:01 p.m. UTC | #11

On Fri, 2023-01-06 at 17:16 +0000, Andrew Cooper wrote:
> On 20/12/2022 6:23 am, Bobby Eshleman wrote:
> > On Fri, Jan 06, 2023 at 03:14:25PM +0200, Oleksii Kurochko wrote:
> > > The patch introduce sbi_putchar() SBI call which is necessary
> > > to implement initial early_printk
> > > 
> > I think that it might be wise to start off with an alternative to
> > sbi_putchar() since it is already planned for deprecation. I
> > realize
> > that this will require rework, but it is almost guaranteed that
> > early_printk() will break on future SBI implementations if using
> > this
> > SBI call. IIRC, Xen/ARM's early printk looked like a reasonable
> > analogy
> > for how it could work on RISC-V, IMHO.
> 
> Hmm, we're using it as a stopgap right now in CI so we can break
> "upstream RISC-V support" into manageable chunks.
> 
> So perhaps we want to forgo plumbing sbi_putchar() into
> early_printk() 
> (to avoid giving the impression that this will stay around in the
> future) and use sbi_putchar() directly for the hello world print.
> 
> Next, we focus on getting the real uart driver working along with
> real
> printk (rather than focusing on exceptions which was going to be the
> next task), and we can drop sbi_putchar() entirely.
> 
> Thoughts?
> 
I think it would be better to remain it as it is done now and add TODO
comment "TODO: rewrite early_printk() to use uart directly as
sbi_putchar() is already planned for deprecation" and update the commit
message too.
After real uart will be ready it will be easy to remove sbi_putchar()
and do something similar to ARM early printk implementation.
> ~Andrew

Patch

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 5a67a3f493..60db415654 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,5 +1,6 @@ 
 obj-$(CONFIG_RISCV_64) += riscv64/
 obj-y += setup.o
+obj-y += sbi.o
 
 $(TARGET): $(TARGET)-syms
 	$(OBJCOPY) -O binary -S $< $@
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
new file mode 100644
index 0000000000..34b53f8eaf
--- /dev/null
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -0,0 +1,34 @@ 
+/* SPDX-License-Identifier: (GPL-2.0-or-later) */
+/*
+ * Copyright (c) 2021 Vates SAS.
+ *
+ * Taken from xvisor, modified by Bobby Eshleman ([e-mail]).
+ *
+ * Taken/modified from Xvisor project with the following copyright:
+ *
+ * Copyright (c) 2019 Western Digital Corporation or its affiliates.
+ */
+
+#ifndef __CPU_SBI_H__
+#define __CPU_SBI_H__
+
+#define SBI_EXT_0_1_CONSOLE_PUTCHAR		0x1
+
+struct sbiret {
+    long error;
+    long value;
+};
+
+struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
+        unsigned long arg1, unsigned long arg2,
+        unsigned long arg3, unsigned long arg4,
+        unsigned long arg5);
+
+/**
+ * Writes given character to the console device.
+ *
+ * @param ch The data to be written to the console.
+ */
+void sbi_console_putchar(int ch);
+
+#endif // __CPU_SBI_H__
diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
new file mode 100644
index 0000000000..67cf5dd982
--- /dev/null
+++ b/xen/arch/riscv/sbi.c
@@ -0,0 +1,44 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Taken and modified from the xvisor project with the copyright Copyright (c)
+ * 2019 Western Digital Corporation or its affiliates and author Anup Patel
+ * ([e-mail]).
+ *
+ * Modified by Bobby Eshleman ([e-mail]).
+ *
+ * Copyright (c) 2019 Western Digital Corporation or its affiliates.
+ * Copyright (c) 2021 Vates SAS.
+ */
+
+#include <xen/errno.h>
+#include <asm/sbi.h>
+
+struct sbiret sbi_ecall(unsigned long ext, unsigned long fid, unsigned long arg0,
+            unsigned long arg1, unsigned long arg2,
+            unsigned long arg3, unsigned long arg4,
+            unsigned long arg5)
+{
+    struct sbiret ret;
+    register unsigned long a0 asm ("a0") = arg0;
+    register unsigned long a1 asm ("a1") = arg1;
+    register unsigned long a2 asm ("a2") = arg2;
+    register unsigned long a3 asm ("a3") = arg3;
+    register unsigned long a4 asm ("a4") = arg4;
+    register unsigned long a5 asm ("a5") = arg5;
+    register unsigned long a6 asm ("a6") = fid;
+    register unsigned long a7 asm ("a7") = ext;
+
+    asm volatile ("ecall"
+              : "+r" (a0), "+r" (a1)
+              : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7)
+              : "memory");
+    ret.error = a0;
+    ret.value = a1;
+
+    return ret;
+}
+
+void sbi_console_putchar(int ch)
+{
+    sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
+}