[v7,1/2] xen/riscv: introduce early_printk basic stuff

Commit Message

Oleksii Kurochko Jan. 27, 2023, 11:39 a.m. UTC

Because printk() relies on a serial driver (like the ns16550 driver)
and drivers require working virtual memory (ioremap()) there is not
print functionality early in Xen boot.

The patch introduces the basic stuff of early_printk functionality
which will be enough to print 'hello from C environment".

Originally early_printk.{c,h} was introduced by Bobby Eshleman
(https://github.com/glg-rv/xen/commit/a3c9916bbdff7ad6030055bbee7e53d1aab71384)
but some functionality was changed:
early_printk() function was changed in comparison with the original as
common isn't being built now so there is no vscnprintf.

This commit adds early printk implementation built on the putc SBI call.

As sbi_console_putchar() is already being planned for deprecation
it is used temporarily now and will be removed or reworked after
real uart will be ready.

Signed-off-by: Bobby Eshleman <[e-mail]>
Signed-off-by: Oleksii Kurochko <[e-mail]>
Reviewed-by: Bobby Eshleman <[e-mail]>
---
Changes in V7:
    - Nothing was changed
---
Changes in V6:
    - Remove __riscv_cmodel_medany check from early_printk.c
---
Changes in V5:
    - Code style fixes
    - Change an error message of #error in case of __riscv_cmodel_medany
      isn't defined
---
Changes in V4:
    - Remove "depends on RISCV*" from Kconfig.debug as it is located in
      arch specific folder so by default RISCV configs should be ebabled.
    - Add "ifdef __riscv_cmodel_medany" to be sure that PC-relative addressing
      is used as early_*() functions can be called from head.S with MMU-off and
      before relocation (if it would be needed at all for RISC-V)
    - fix code style
---
Changes in V3:
    - reorder headers in alphabetical order
    - merge changes related to start_xen() function from "[PATCH v2 7/8]
      xen/riscv: print hello message from C env" to this patch
    - remove unneeded parentheses in definition of STACK_SIZE
---
Changes in V2:
    - introduce STACK_SIZE define.
    - use consistent padding between instruction mnemonic and operand(s)
---
 xen/arch/riscv/Kconfig.debug              |  5 ++++
 xen/arch/riscv/Makefile                   |  1 +
 xen/arch/riscv/early_printk.c             | 33 +++++++++++++++++++++++
 xen/arch/riscv/include/asm/early_printk.h | 12 +++++++++
 xen/arch/riscv/setup.c                    |  4 +++
 5 files changed, 55 insertions(+)
 create mode 100644 xen/arch/riscv/early_printk.c
 create mode 100644 xen/arch/riscv/include/asm/early_printk.h

Comments

Oleksii Kurochko Jan. 27, 2023, 2:15 p.m. UTC | #1

Hi Alistair, Bobby and community,

I would like to ask your help with the following check:
+/*
+ * early_*() can be called from head.S with MMU-off.
+ *
+ * The following requiremets should be honoured for early_*() to
+ * work correctly:
+ *    It should use PC-relative addressing for accessing symbols.
+ *    To achieve that GCC cmodel=medany should be used.
+ */
+#ifndef __riscv_cmodel_medany
+#error "early_*() can be called from head.S with MMU-off"
+#endif

Please take a look at the following messages and help me to decide if
the check mentioned above should be in early_printk.c or not:
[1]
https://lore.kernel.org/xen-devel/[e-mail]/
[2]
https://lore.kernel.org/xen-devel/[e-mail]/

Thanks in advance.

~ Oleksii

On Fri, 2023-01-27 at 13:39 +0200, Oleksii Kurochko wrote:
> Because printk() relies on a serial driver (like the ns16550 driver)
> and drivers require working virtual memory (ioremap()) there is not
> print functionality early in Xen boot.
> 
> The patch introduces the basic stuff of early_printk functionality
> which will be enough to print 'hello from C environment".
> 
> Originally early_printk.{c,h} was introduced by Bobby Eshleman
> (
> https://github.com/glg-rv/xen/commit/a3c9916bbdff7ad6030055bbee7e53d1a
> ab71384)
> but some functionality was changed:
> early_printk() function was changed in comparison with the original
> as
> common isn't being built now so there is no vscnprintf.
> 
> This commit adds early printk implementation built on the putc SBI
> call.
> 
> As sbi_console_putchar() is already being planned for deprecation
> it is used temporarily now and will be removed or reworked after
> real uart will be ready.
> 
> Signed-off-by: Bobby Eshleman <[e-mail]>
> Signed-off-by: Oleksii Kurochko <[e-mail]>
> Reviewed-by: Bobby Eshleman <[e-mail]>
> ---
> Changes in V7:
>     - Nothing was changed
> ---
> Changes in V6:
>     - Remove __riscv_cmodel_medany check from early_printk.c
> ---
> Changes in V5:
>     - Code style fixes
>     - Change an error message of #error in case of
> __riscv_cmodel_medany
>       isn't defined
> ---
> Changes in V4:
>     - Remove "depends on RISCV*" from Kconfig.debug as it is located
> in
>       arch specific folder so by default RISCV configs should be
> ebabled.
>     - Add "ifdef __riscv_cmodel_medany" to be sure that PC-relative
> addressing
>       is used as early_*() functions can be called from head.S with
> MMU-off and
>       before relocation (if it would be needed at all for RISC-V)
>     - fix code style
> ---
> Changes in V3:
>     - reorder headers in alphabetical order
>     - merge changes related to start_xen() function from "[PATCH v2
> 7/8]
>       xen/riscv: print hello message from C env" to this patch
>     - remove unneeded parentheses in definition of STACK_SIZE
> ---
> Changes in V2:
>     - introduce STACK_SIZE define.
>     - use consistent padding between instruction mnemonic and
> operand(s)
> ---
>  xen/arch/riscv/Kconfig.debug              |  5 ++++
>  xen/arch/riscv/Makefile                   |  1 +
>  xen/arch/riscv/early_printk.c             | 33
> +++++++++++++++++++++++
>  xen/arch/riscv/include/asm/early_printk.h | 12 +++++++++
>  xen/arch/riscv/setup.c                    |  4 +++
>  5 files changed, 55 insertions(+)
>  create mode 100644 xen/arch/riscv/early_printk.c
>  create mode 100644 xen/arch/riscv/include/asm/early_printk.h
> 
> diff --git a/xen/arch/riscv/Kconfig.debug
> b/xen/arch/riscv/Kconfig.debug
> index e69de29bb2..608c9ff832 100644
> --- a/xen/arch/riscv/Kconfig.debug
> +++ b/xen/arch/riscv/Kconfig.debug
> @@ -0,0 +1,5 @@
> +config EARLY_PRINTK
> +    bool "Enable early printk"
> +    default DEBUG
> +    help
> +      Enables early printk debug messages
> diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> index fd916e1004..1a4f1a6015 100644
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>  obj-$(CONFIG_RISCV_64) += riscv64/
>  obj-y += sbi.o
>  obj-y += setup.o
> diff --git a/xen/arch/riscv/early_printk.c
> b/xen/arch/riscv/early_printk.c
> new file mode 100644
> index 0000000000..b66a11f1bc
> --- /dev/null
> +++ b/xen/arch/riscv/early_printk.c
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * RISC-V early printk using SBI
> + *
> + * Copyright (C) 2021 Bobby Eshleman <[e-mail]>
> + */
> +#include <asm/early_printk.h>
> +#include <asm/sbi.h>
> +
> +/*
> + * TODO:
> + *   sbi_console_putchar is already planned for deprecation
> + *   so it should be reworked to use UART directly.
> +*/
> +void early_puts(const char *s, size_t nr)
> +{
> +    while ( nr-- > 0 )
> +    {
> +        if ( *s == '\n' )
> +            sbi_console_putchar('\r');
> +        sbi_console_putchar(*s);
> +        s++;
> +    }
> +}
> +
> +void early_printk(const char *str)
> +{
> +    while ( *str )
> +    {
> +        early_puts(str, 1);
> +        str++;
> +    }
> +}
> diff --git a/xen/arch/riscv/include/asm/early_printk.h
> b/xen/arch/riscv/include/asm/early_printk.h
> new file mode 100644
> index 0000000000..05106e160d
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/early_printk.h
> @@ -0,0 +1,12 @@
> +#ifndef __EARLY_PRINTK_H__
> +#define __EARLY_PRINTK_H__
> +
> +#include <xen/early_printk.h>
> +
> +#ifdef CONFIG_EARLY_PRINTK
> +void early_printk(const char *str);
> +#else
> +static inline void early_printk(const char *s) {};
> +#endif
> +
> +#endif /* __EARLY_PRINTK_H__ */
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index 13e24e2fe1..d09ffe1454 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,12 +1,16 @@
>  #include <xen/compile.h>
>  #include <xen/init.h>
>  
> +#include <asm/early_printk.h>
> +
>  /* Xen stack for bringing up the first CPU. */
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
>  void __init noreturn start_xen(void)
>  {
> +    early_printk("Hello from C env\n");
> +
>      for ( ;; )
>          asm volatile ("wfi");
>

Alistair Francis Jan. 31, 2023, 11:44 a.m. UTC | #2

On Sat, Jan 28, 2023 at 12:15 AM Oleksii <[e-mail]> wrote:
>
> Hi Alistair, Bobby and community,
>
> I would like to ask your help with the following check:
> +/*
> + * early_*() can be called from head.S with MMU-off.
> + *
> + * The following requiremets should be honoured for early_*() to
> + * work correctly:
> + *    It should use PC-relative addressing for accessing symbols.
> + *    To achieve that GCC cmodel=medany should be used.
> + */
> +#ifndef __riscv_cmodel_medany
> +#error "early_*() can be called from head.S with MMU-off"
> +#endif

I have never seen a check like this before. I don't really understand
what it's looking for, if the linker is unable to call early_*() I
would expect it to throw an error. I'm not sure what this is adding.

I think this is safe to remove.

Alistair

>
> Please take a look at the following messages and help me to decide if
> the check mentioned above should be in early_printk.c or not:
> [1]
> https://lore.kernel.org/xen-devel/[e-mail]/
> [2]
> https://lore.kernel.org/xen-devel/[e-mail]/
>
> Thanks in advance.
>
> ~ Oleksii
>
> On Fri, 2023-01-27 at 13:39 +0200, Oleksii Kurochko wrote:
> > Because printk() relies on a serial driver (like the ns16550 driver)
> > and drivers require working virtual memory (ioremap()) there is not
> > print functionality early in Xen boot.
> >
> > The patch introduces the basic stuff of early_printk functionality
> > which will be enough to print 'hello from C environment".
> >
> > Originally early_printk.{c,h} was introduced by Bobby Eshleman
> > (
> > https://github.com/glg-rv/xen/commit/a3c9916bbdff7ad6030055bbee7e53d1a
> > ab71384)
> > but some functionality was changed:
> > early_printk() function was changed in comparison with the original
> > as
> > common isn't being built now so there is no vscnprintf.
> >
> > This commit adds early printk implementation built on the putc SBI
> > call.
> >
> > As sbi_console_putchar() is already being planned for deprecation
> > it is used temporarily now and will be removed or reworked after
> > real uart will be ready.
> >
> > Signed-off-by: Bobby Eshleman <[e-mail]>
> > Signed-off-by: Oleksii Kurochko <[e-mail]>
> > Reviewed-by: Bobby Eshleman <[e-mail]>
> > ---
> > Changes in V7:
> >     - Nothing was changed
> > ---
> > Changes in V6:
> >     - Remove __riscv_cmodel_medany check from early_printk.c
> > ---
> > Changes in V5:
> >     - Code style fixes
> >     - Change an error message of #error in case of
> > __riscv_cmodel_medany
> >       isn't defined
> > ---
> > Changes in V4:
> >     - Remove "depends on RISCV*" from Kconfig.debug as it is located
> > in
> >       arch specific folder so by default RISCV configs should be
> > ebabled.
> >     - Add "ifdef __riscv_cmodel_medany" to be sure that PC-relative
> > addressing
> >       is used as early_*() functions can be called from head.S with
> > MMU-off and
> >       before relocation (if it would be needed at all for RISC-V)
> >     - fix code style
> > ---
> > Changes in V3:
> >     - reorder headers in alphabetical order
> >     - merge changes related to start_xen() function from "[PATCH v2
> > 7/8]
> >       xen/riscv: print hello message from C env" to this patch
> >     - remove unneeded parentheses in definition of STACK_SIZE
> > ---
> > Changes in V2:
> >     - introduce STACK_SIZE define.
> >     - use consistent padding between instruction mnemonic and
> > operand(s)
> > ---
> >  xen/arch/riscv/Kconfig.debug              |  5 ++++
> >  xen/arch/riscv/Makefile                   |  1 +
> >  xen/arch/riscv/early_printk.c             | 33
> > +++++++++++++++++++++++
> >  xen/arch/riscv/include/asm/early_printk.h | 12 +++++++++
> >  xen/arch/riscv/setup.c                    |  4 +++
> >  5 files changed, 55 insertions(+)
> >  create mode 100644 xen/arch/riscv/early_printk.c
> >  create mode 100644 xen/arch/riscv/include/asm/early_printk.h
> >
> > diff --git a/xen/arch/riscv/Kconfig.debug
> > b/xen/arch/riscv/Kconfig.debug
> > index e69de29bb2..608c9ff832 100644
> > --- a/xen/arch/riscv/Kconfig.debug
> > +++ b/xen/arch/riscv/Kconfig.debug
> > @@ -0,0 +1,5 @@
> > +config EARLY_PRINTK
> > +    bool "Enable early printk"
> > +    default DEBUG
> > +    help
> > +      Enables early printk debug messages
> > diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
> > index fd916e1004..1a4f1a6015 100644
> > --- a/xen/arch/riscv/Makefile
> > +++ b/xen/arch/riscv/Makefile
> > @@ -1,3 +1,4 @@
> > +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
> >  obj-$(CONFIG_RISCV_64) += riscv64/
> >  obj-y += sbi.o
> >  obj-y += setup.o
> > diff --git a/xen/arch/riscv/early_printk.c
> > b/xen/arch/riscv/early_printk.c
> > new file mode 100644
> > index 0000000000..b66a11f1bc
> > --- /dev/null
> > +++ b/xen/arch/riscv/early_printk.c
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * RISC-V early printk using SBI
> > + *
> > + * Copyright (C) 2021 Bobby Eshleman <[e-mail]>
> > + */
> > +#include <asm/early_printk.h>
> > +#include <asm/sbi.h>
> > +
> > +/*
> > + * TODO:
> > + *   sbi_console_putchar is already planned for deprecation
> > + *   so it should be reworked to use UART directly.
> > +*/
> > +void early_puts(const char *s, size_t nr)
> > +{
> > +    while ( nr-- > 0 )
> > +    {
> > +        if ( *s == '\n' )
> > +            sbi_console_putchar('\r');
> > +        sbi_console_putchar(*s);
> > +        s++;
> > +    }
> > +}
> > +
> > +void early_printk(const char *str)
> > +{
> > +    while ( *str )
> > +    {
> > +        early_puts(str, 1);
> > +        str++;
> > +    }
> > +}
> > diff --git a/xen/arch/riscv/include/asm/early_printk.h
> > b/xen/arch/riscv/include/asm/early_printk.h
> > new file mode 100644
> > index 0000000000..05106e160d
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/early_printk.h
> > @@ -0,0 +1,12 @@
> > +#ifndef __EARLY_PRINTK_H__
> > +#define __EARLY_PRINTK_H__
> > +
> > +#include <xen/early_printk.h>
> > +
> > +#ifdef CONFIG_EARLY_PRINTK
> > +void early_printk(const char *str);
> > +#else
> > +static inline void early_printk(const char *s) {};
> > +#endif
> > +
> > +#endif /* __EARLY_PRINTK_H__ */
> > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > index 13e24e2fe1..d09ffe1454 100644
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -1,12 +1,16 @@
> >  #include <xen/compile.h>
> >  #include <xen/init.h>
> >
> > +#include <asm/early_printk.h>
> > +
> >  /* Xen stack for bringing up the first CPU. */
> >  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
> >      __aligned(STACK_SIZE);
> >
> >  void __init noreturn start_xen(void)
> >  {
> > +    early_printk("Hello from C env\n");
> > +
> >      for ( ;; )
> >          asm volatile ("wfi");
> >
>
>

Julien Grall Jan. 31, 2023, 12:03 p.m. UTC | #3

On 31/01/2023 11:44, Alistair Francis wrote:
> On Sat, Jan 28, 2023 at 12:15 AM Oleksii <[e-mail]> wrote:
>>
>> Hi Alistair, Bobby and community,
>>
>> I would like to ask your help with the following check:
>> +/*
>> + * early_*() can be called from head.S with MMU-off.
>> + *
>> + * The following requiremets should be honoured for early_*() to
>> + * work correctly:
>> + *    It should use PC-relative addressing for accessing symbols.
>> + *    To achieve that GCC cmodel=medany should be used.
>> + */
>> +#ifndef __riscv_cmodel_medany
>> +#error "early_*() can be called from head.S with MMU-off"
>> +#endif
> 
> I have never seen a check like this before. 

The check is in the Linux code, see [3].

> I don't really understand
> what it's looking for, if the linker is unable to call early_*() I
> would expect it to throw an error. I'm not sure what this is adding.

When the MMU is off during early boot, you want any C function called to 
use PC-relative address rather than absolute address. This is because 
the physical address may not match the virtual address.

 From my understanding, on RISC-V, the use of PC-relative address is 
only guaranteed with medany. So if you were going to change the cmodel 
(Andrew suggested you would), then early_*() may end up to be broken.

This check serve as a documentation of the assumption and also help the 
developer any change in the model and take the appropriate action to 
remediate it.

> 
> I think this is safe to remove.
Based on what I wrote above, do you still think this is safe?

Cheers,

>> Please take a look at the following messages and help me to decide if
>> the check mentioned above should be in early_printk.c or not:
>> [1]
>> https://lore.kernel.org/xen-devel/[e-mail]/
>> [2]
>> https://lore.kernel.org/xen-devel/[e-mail]/

[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/mm/init.c

Alistair Francis Jan. 31, 2023, 11:17 p.m. UTC | #4

On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <[e-mail]> wrote:
>
>
>
> On 31/01/2023 11:44, Alistair Francis wrote:
> > On Sat, Jan 28, 2023 at 12:15 AM Oleksii <[e-mail]> wrote:
> >>
> >> Hi Alistair, Bobby and community,
> >>
> >> I would like to ask your help with the following check:
> >> +/*
> >> + * early_*() can be called from head.S with MMU-off.
> >> + *
> >> + * The following requiremets should be honoured for early_*() to
> >> + * work correctly:
> >> + *    It should use PC-relative addressing for accessing symbols.
> >> + *    To achieve that GCC cmodel=medany should be used.
> >> + */
> >> +#ifndef __riscv_cmodel_medany
> >> +#error "early_*() can be called from head.S with MMU-off"
> >> +#endif
> >
> > I have never seen a check like this before.
>
> The check is in the Linux code, see [3].
>
> > I don't really understand
> > what it's looking for, if the linker is unable to call early_*() I
> > would expect it to throw an error. I'm not sure what this is adding.
>
> When the MMU is off during early boot, you want any C function called to
> use PC-relative address rather than absolute address. This is because
> the physical address may not match the virtual address.

Ah!

I forgot that Xen would be compiled for virtual addresses, I have
spent too much time running on systems without an MMU recently.

>
>  From my understanding, on RISC-V, the use of PC-relative address is
> only guaranteed with medany. So if you were going to change the cmodel
> (Andrew suggested you would), then early_*() may end up to be broken.
>
> This check serve as a documentation of the assumption and also help the
> developer any change in the model and take the appropriate action to
> remediate it.
>
> >
> > I think this is safe to remove.
> Based on what I wrote above, do you still think this is safe?

With that in mind it's probably worth leaving in then. Maybe the
comment should be updated to make it explicit why we want this check
(I find the current comment not very helpful).

Alistair

>
> Cheers,
>
> >> Please take a look at the following messages and help me to decide if
> >> the check mentioned above should be in early_printk.c or not:
> >> [1]
> >> https://lore.kernel.org/xen-devel/[e-mail]/
> >> [2]
> >> https://lore.kernel.org/xen-devel/[e-mail]/
>
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/riscv/mm/init.c
>
>
> --
> Julien Grall

Andrew Cooper Feb. 1, 2023, 12:21 a.m. UTC | #5

On 31/01/2023 11:17 pm, Alistair Francis wrote:
> On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <[e-mail]> wrote:
>> On 31/01/2023 11:44, Alistair Francis wrote:
>>> On Sat, Jan 28, 2023 at 12:15 AM Oleksii <[e-mail]> wrote:
>>>
>>  From my understanding, on RISC-V, the use of PC-relative address is
>> only guaranteed with medany. So if you were going to change the cmodel
>> (Andrew suggested you would), then early_*() may end up to be broken.
>>
>> This check serve as a documentation of the assumption and also help the
>> developer any change in the model and take the appropriate action to
>> remediate it.
>>
>>> I think this is safe to remove.
>> Based on what I wrote above, do you still think this is safe?
> With that in mind it's probably worth leaving in then. Maybe the
> comment should be updated to make it explicit why we want this check
> (I find the current comment not very helpful).

The presence of this check pre-supposes that Xen will always relocate
itself, and this simply not true.  XIP for example typically does not.

Furthermore it's not checking the property wanted.  The way C is
compiled has no bearing on what relocation head.S uses to call it.

It is a given that we compile the hypervisor with a consistent code
model, meaning that the properly actually making the check do what is
wanted is also the property that means it is not needed in the first place.

This check is literally not worth the bytes it's taking up on disk, and
not worth the added compiler time, nor the wasted time of whoever comes
to support something like XIP in the future finds it to be in the way. 
Xen as a whole will really genuinely function as intended in models
other than medany, and it demonstrates a misunderstanding of the topic
to put in such a check to begin with.

~Andrew

Julien Grall Feb. 1, 2023, 9:06 a.m. UTC | #6

Hi Andrew,

On 01/02/2023 00:21, Andrew Cooper wrote:
> On 31/01/2023 11:17 pm, Alistair Francis wrote:
>> On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <[e-mail]> wrote:
>>> On 31/01/2023 11:44, Alistair Francis wrote:
>>>> On Sat, Jan 28, 2023 at 12:15 AM Oleksii <[e-mail]> wrote:
>>>>
>>>   From my understanding, on RISC-V, the use of PC-relative address is
>>> only guaranteed with medany. So if you were going to change the cmodel
>>> (Andrew suggested you would), then early_*() may end up to be broken.
>>>
>>> This check serve as a documentation of the assumption and also help the
>>> developer any change in the model and take the appropriate action to
>>> remediate it.
>>>
>>>> I think this is safe to remove.
>>> Based on what I wrote above, do you still think this is safe?
>> With that in mind it's probably worth leaving in then. Maybe the
>> comment should be updated to make it explicit why we want this check
>> (I find the current comment not very helpful).
> 
> The presence of this check pre-supposes that Xen will always relocate
> itself, and this simply not true.  XIP for example typically does not
> 
> Furthermore it's not checking the property wanted.  The way C is
> compiled has no bearing on what relocation head.S uses to call it.

I think we are still cross-talking each other because this is not my 
point. I am not sure how to explain differently...

This check is not about how head.S call early_*() but making sure the C 
function can be executed with the MMU off. In which case, you will 
likely have VA != PA.

In theory head.S could apply some relocations before hand, but it may be 
too complicated to do it before calling early_*().

> 
> It is a given that we compile the hypervisor with a consistent code
> model, meaning that the properly actually making the check do what is
> wanted is also the property that means it is not needed in the first place.

See above.

> 
> This check is literally not worth the bytes it's taking up on disk, and
> not worth the added compiler time, nor the wasted time of whoever comes
> to support something like XIP in the future finds it to be in the way.
> Xen as a whole will really genuinely function as intended in models
> other than medany, and it demonstrates a misunderstanding of the topic
> to put in such a check to begin with.

Then enlight me :). So far it looks more like you are not understanding 
my point: I am talking about C function call with MMU off (e.g. VA != 
PA) and you are talking about Xen as a whole.

I guess the only way to really know is to implement a different model. 
At which point there are three possible outcome:
   1) We had the compiler check, it fired and the developper will take 
action to fix it (if needed).
   2) We have no compiler check, the developper knew what to do it and 
fixed it.
   3) We have no compiler check, the developper where not aware of the 
problem and we will waste time.

Even if you disagree with the check, then I think 1 is still the best 
because it would explain our assumption. Yes it may waste a few minutes 
to the developer switching the model. But that probably nothing compare 
to the time you could waste if you don't notice it.

Anyway, Alistair has now all the information to take an informative 
decision.

Cheers,

Julien Grall Feb. 1, 2023, 9:10 a.m. UTC | #7

On 01/02/2023 09:06, Julien Grall wrote:
> Hi Andrew,
> 
> On 01/02/2023 00:21, Andrew Cooper wrote:
>> On 31/01/2023 11:17 pm, Alistair Francis wrote:
>>> On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <[e-mail]> wrote:
>>>> On 31/01/2023 11:44, Alistair Francis wrote:
>>>>> On Sat, Jan 28, 2023 at 12:15 AM Oleksii 
>>>>> <[e-mail]> wrote:
>>>>>
>>>>   From my understanding, on RISC-V, the use of PC-relative address is
>>>> only guaranteed with medany. So if you were going to change the cmodel
>>>> (Andrew suggested you would), then early_*() may end up to be broken.
>>>>
>>>> This check serve as a documentation of the assumption and also help the
>>>> developer any change in the model and take the appropriate action to
>>>> remediate it.
>>>>
>>>>> I think this is safe to remove.
>>>> Based on what I wrote above, do you still think this is safe?
>>> With that in mind it's probably worth leaving in then. Maybe the
>>> comment should be updated to make it explicit why we want this check
>>> (I find the current comment not very helpful).
>>
>> The presence of this check pre-supposes that Xen will always relocate
>> itself, and this simply not true.  XIP for example typically does not
>>
>> Furthermore it's not checking the property wanted.  The way C is
>> compiled has no bearing on what relocation head.S uses to call it.
> 
> I think we are still cross-talking each other because this is not my 
> point. I am not sure how to explain differently...
> 
> This check is not about how head.S call early_*() but making sure the C 
> function can be executed with the MMU off. In which case, you will 
> likely have VA != PA.
> 
> In theory head.S could apply some relocations before hand, but it may be 
> too complicated to do it before calling early_*().
> 
>>
>> It is a given that we compile the hypervisor with a consistent code
>> model, meaning that the properly actually making the check do what is
>> wanted is also the property that means it is not needed in the first 
>> place.
> 
> See above.
> 
>>
>> This check is literally not worth the bytes it's taking up on disk, and
>> not worth the added compiler time, nor the wasted time of whoever comes
>> to support something like XIP in the future finds it to be in the way.
>> Xen as a whole will really genuinely function as intended in models
>> other than medany, and it demonstrates a misunderstanding of the topic
>> to put in such a check to begin with.
> 
> Then enlight me :). So far it looks more like you are not understanding 
> my point: I am talking about C function call with MMU off (e.g. VA != 

Just to clarify, I meant a C function executing with MMU off here.

> PA) and you are talking about Xen as a whole.
> 
> I guess the only way to really know is to implement a different model. 
> At which point there are three possible outcome:
>    1) We had the compiler check, it fired and the developper will take 
> action to fix it (if needed).
>    2) We have no compiler check, the developper knew what to do it and 
> fixed it.
>    3) We have no compiler check, the developper where not aware of the 
> problem and we will waste time.
> 
> Even if you disagree with the check, then I think 1 is still the best 
> because it would explain our assumption. Yes it may waste a few minutes 
> to the developer switching the model. But that probably nothing compare 
> to the time you could waste if you don't notice it.
> 
> Anyway, Alistair has now all the information to take an informative 
> decision.
> 
> Cheers,
>

Bobby Eshleman Feb. 1, 2023, 5:33 p.m. UTC | #8

On Wed, Feb 01, 2023 at 09:06:21AM +0000, Julien Grall wrote:
> Hi Andrew,
> 
> On 01/02/2023 00:21, Andrew Cooper wrote:
> > On 31/01/2023 11:17 pm, Alistair Francis wrote:
> > > On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <[e-mail]> wrote:
> > > > On 31/01/2023 11:44, Alistair Francis wrote:
> > > > > On Sat, Jan 28, 2023 at 12:15 AM Oleksii <[e-mail]> wrote:
> > > > > 
> > > >   From my understanding, on RISC-V, the use of PC-relative address is
> > > > only guaranteed with medany. So if you were going to change the cmodel
> > > > (Andrew suggested you would), then early_*() may end up to be broken.
> > > > 
> > > > This check serve as a documentation of the assumption and also help the
> > > > developer any change in the model and take the appropriate action to
> > > > remediate it.
> > > > 
> > > > > I think this is safe to remove.
> > > > Based on what I wrote above, do you still think this is safe?
> > > With that in mind it's probably worth leaving in then. Maybe the
> > > comment should be updated to make it explicit why we want this check
> > > (I find the current comment not very helpful).
> > 
> > The presence of this check pre-supposes that Xen will always relocate
> > itself, and this simply not true.  XIP for example typically does not
> > 
> > Furthermore it's not checking the property wanted.  The way C is
> > compiled has no bearing on what relocation head.S uses to call it.
> 
> I think we are still cross-talking each other because this is not my point.
> I am not sure how to explain differently...
> 
> This check is not about how head.S call early_*() but making sure the C
> function can be executed with the MMU off. In which case, you will likely
> have VA != PA.
> 
> In theory head.S could apply some relocations before hand, but it may be too
> complicated to do it before calling early_*().
> 
> > 
> > It is a given that we compile the hypervisor with a consistent code
> > model, meaning that the properly actually making the check do what is
> > wanted is also the property that means it is not needed in the first place.
> 
> See above.
> 
> > 
> > This check is literally not worth the bytes it's taking up on disk, and
> > not worth the added compiler time, nor the wasted time of whoever comes
> > to support something like XIP in the future finds it to be in the way.
> > Xen as a whole will really genuinely function as intended in models
> > other than medany, and it demonstrates a misunderstanding of the topic
> > to put in such a check to begin with.
> 
> Then enlight me :). So far it looks more like you are not understanding my
> point: I am talking about C function call with MMU off (e.g. VA != PA) and
> you are talking about Xen as a whole.
> 
> I guess the only way to really know is to implement a different model. At
> which point there are three possible outcome:
>   1) We had the compiler check, it fired and the developper will take action
> to fix it (if needed).
>   2) We have no compiler check, the developper knew what to do it and fixed
> it.
>   3) We have no compiler check, the developper where not aware of the
> problem and we will waste time.
> 

I tend to favor the check because outcome #1 is so desirable, and I do
think that checking for medany is sufficient for the bulk of future
work. But that said, I do see Andrew's point now.

If Xen were to a) not relocate itself, b) be executed under the 2GB
range, c) be compiled under medlow, and d) the MMU is off or on but Xen
is identity mapped, then C functions should still be safe to call in
early boot. Checking medany does protect developers from accidental bad
configuration now, but it is a somewhat imperfect proxy.

One alternative that may be more long term is for the self relocation
code to be Kconfig-able and to require/force medany. Anyone wanting to
develop something like XIP could deselect relocation, which would allow
them to use medlow if they wanted/needed. The early C functions would
still be callable under both in this case. The help strings could offer
explanation too.

Thanks,
Bobby

> Even if you disagree with the check, then I think 1 is still the best
> because it would explain our assumption. Yes it may waste a few minutes to
> the developer switching the model. But that probably nothing compare to the
> time you could waste if you don't notice it.
> 
> Anyway, Alistair has now all the information to take an informative
> decision.
> 
> Cheers,
> 
> -- 
> Julien Grall
>

Alistair Francis Feb. 4, 2023, 11:59 a.m. UTC | #9

On Thu, Feb 2, 2023 at 3:34 AM Bobby Eshleman <[e-mail]> wrote:
>
> On Wed, Feb 01, 2023 at 09:06:21AM +0000, Julien Grall wrote:
> > Hi Andrew,
> >
> > On 01/02/2023 00:21, Andrew Cooper wrote:
> > > On 31/01/2023 11:17 pm, Alistair Francis wrote:
> > > > On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <[e-mail]> wrote:
> > > > > On 31/01/2023 11:44, Alistair Francis wrote:
> > > > > > On Sat, Jan 28, 2023 at 12:15 AM Oleksii <[e-mail]> wrote:
> > > > > >
> > > > >   From my understanding, on RISC-V, the use of PC-relative address is
> > > > > only guaranteed with medany. So if you were going to change the cmodel
> > > > > (Andrew suggested you would), then early_*() may end up to be broken.
> > > > >
> > > > > This check serve as a documentation of the assumption and also help the
> > > > > developer any change in the model and take the appropriate action to
> > > > > remediate it.
> > > > >
> > > > > > I think this is safe to remove.
> > > > > Based on what I wrote above, do you still think this is safe?
> > > > With that in mind it's probably worth leaving in then. Maybe the
> > > > comment should be updated to make it explicit why we want this check
> > > > (I find the current comment not very helpful).
> > >
> > > The presence of this check pre-supposes that Xen will always relocate
> > > itself, and this simply not true.  XIP for example typically does not
> > >
> > > Furthermore it's not checking the property wanted.  The way C is
> > > compiled has no bearing on what relocation head.S uses to call it.
> >
> > I think we are still cross-talking each other because this is not my point.
> > I am not sure how to explain differently...
> >
> > This check is not about how head.S call early_*() but making sure the C
> > function can be executed with the MMU off. In which case, you will likely
> > have VA != PA.
> >
> > In theory head.S could apply some relocations before hand, but it may be too
> > complicated to do it before calling early_*().
> >
> > >
> > > It is a given that we compile the hypervisor with a consistent code
> > > model, meaning that the properly actually making the check do what is
> > > wanted is also the property that means it is not needed in the first place.
> >
> > See above.
> >
> > >
> > > This check is literally not worth the bytes it's taking up on disk, and
> > > not worth the added compiler time, nor the wasted time of whoever comes
> > > to support something like XIP in the future finds it to be in the way.
> > > Xen as a whole will really genuinely function as intended in models
> > > other than medany, and it demonstrates a misunderstanding of the topic
> > > to put in such a check to begin with.
> >
> > Then enlight me :). So far it looks more like you are not understanding my
> > point: I am talking about C function call with MMU off (e.g. VA != PA) and
> > you are talking about Xen as a whole.
> >
> > I guess the only way to really know is to implement a different model. At
> > which point there are three possible outcome:
> >   1) We had the compiler check, it fired and the developper will take action
> > to fix it (if needed).
> >   2) We have no compiler check, the developper knew what to do it and fixed
> > it.
> >   3) We have no compiler check, the developper where not aware of the
> > problem and we will waste time.
> >
>
> I tend to favor the check because outcome #1 is so desirable, and I do
> think that checking for medany is sufficient for the bulk of future
> work. But that said, I do see Andrew's point now.
>
> If Xen were to a) not relocate itself, b) be executed under the 2GB
> range, c) be compiled under medlow, and d) the MMU is off or on but Xen
> is identity mapped, then C functions should still be safe to call in
> early boot. Checking medany does protect developers from accidental bad
> configuration now, but it is a somewhat imperfect proxy.

Yeah, I agree. It probably is worth leaving the check in for now, even
if it's imperfect. We can always remove it in the future if required.

Alistair

>
> One alternative that may be more long term is for the self relocation
> code to be Kconfig-able and to require/force medany. Anyone wanting to
> develop something like XIP could deselect relocation, which would allow
> them to use medlow if they wanted/needed. The early C functions would
> still be callable under both in this case. The help strings could offer
> explanation too.
>
> Thanks,
> Bobby
>
> > Even if you disagree with the check, then I think 1 is still the best
> > because it would explain our assumption. Yes it may waste a few minutes to
> > the developer switching the model. But that probably nothing compare to the
> > time you could waste if you don't notice it.
> >
> > Anyway, Alistair has now all the information to take an informative
> > decision.
> >
> > Cheers,
> >
> > --
> > Julien Grall
> >

Oleksii Kurochko Feb. 6, 2023, 5:30 p.m. UTC | #10

Hi all,

On Wed, 2023-02-01 at 09:06 +0000, Julien Grall wrote:
> Hi Andrew,
> 
> On 01/02/2023 00:21, Andrew Cooper wrote:
> > On 31/01/2023 11:17 pm, Alistair Francis wrote:
> > > On Tue, Jan 31, 2023 at 10:03 PM Julien Grall <[e-mail]>
> > > wrote:
> > > > On 31/01/2023 11:44, Alistair Francis wrote:
> > > > > On Sat, Jan 28, 2023 at 12:15 AM Oleksii
> > > > > <[e-mail]> wrote:
> > > > > 
> > > >   From my understanding, on RISC-V, the use of PC-relative
> > > > address is
> > > > only guaranteed with medany. So if you were going to change the
> > > > cmodel
> > > > (Andrew suggested you would), then early_*() may end up to be
> > > > broken.
> > > > 
> > > > This check serve as a documentation of the assumption and also
> > > > help the
> > > > developer any change in the model and take the appropriate
> > > > action to
> > > > remediate it.
> > > > 
> > > > > I think this is safe to remove.
> > > > Based on what I wrote above, do you still think this is safe?
> > > With that in mind it's probably worth leaving in then. Maybe the
> > > comment should be updated to make it explicit why we want this
> > > check
> > > (I find the current comment not very helpful).
> > 
> > The presence of this check pre-supposes that Xen will always
> > relocate
> > itself, and this simply not true.  XIP for example typically does
> > not
> > 
> > Furthermore it's not checking the property wanted.  The way C is
> > compiled has no bearing on what relocation head.S uses to call it.
> 
> I think we are still cross-talking each other because this is not my 
> point. I am not sure how to explain differently...
> 
> This check is not about how head.S call early_*() but making sure the
> C 
> function can be executed with the MMU off. In which case, you will 
> likely have VA != PA.
> 
> In theory head.S could apply some relocations before hand, but it may
> be 
> too complicated to do it before calling early_*().
> 
> > 
> > It is a given that we compile the hypervisor with a consistent code
> > model, meaning that the properly actually making the check do what
> > is
> > wanted is also the property that means it is not needed in the
> > first place.
> 
> See above.
> 
> > 
> > This check is literally not worth the bytes it's taking up on disk,
> > and
> > not worth the added compiler time, nor the wasted time of whoever
> > comes
> > to support something like XIP in the future finds it to be in the
> > way.
> > Xen as a whole will really genuinely function as intended in models
> > other than medany, and it demonstrates a misunderstanding of the
> > topic
> > to put in such a check to begin with.
> 
> Then enlight me :). So far it looks more like you are not
> understanding 
> my point: I am talking about C function call with MMU off (e.g. VA !=
> PA) and you are talking about Xen as a whole.
> 
> I guess the only way to really know is to implement a different
> model. 
> At which point there are three possible outcome:
>    1) We had the compiler check, it fired and the developper will
> take 
> action to fix it (if needed).
>    2) We have no compiler check, the developper knew what to do it
> and 
> fixed it.
>    3) We have no compiler check, the developper where not aware of
> the 
> problem and we will waste time.
> 
> Even if you disagree with the check, then I think 1 is still the best
> because it would explain our assumption. Yes it may waste a few
> minutes 
> to the developer switching the model. But that probably nothing
> compare 
> to the time you could waste if you don't notice it.
> 
> Anyway, Alistair has now all the information to take an informative 
> decision.
> 

I'll bring back the check and send the new version of the patch
tomorrow as Bobby&Alistair lean toward it.

Andrew, do you have other thoughts?

> Cheers,
> 
~ Oleksii

Patch

diff --git a/xen/arch/riscv/Kconfig.debug b/xen/arch/riscv/Kconfig.debug
index e69de29bb2..608c9ff832 100644
--- a/xen/arch/riscv/Kconfig.debug
+++ b/xen/arch/riscv/Kconfig.debug
@@ -0,0 +1,5 @@ 
+config EARLY_PRINTK
+    bool "Enable early printk"
+    default DEBUG
+    help
+      Enables early printk debug messages
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index fd916e1004..1a4f1a6015 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -1,3 +1,4 @@ 
+obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-$(CONFIG_RISCV_64) += riscv64/
 obj-y += sbi.o
 obj-y += setup.o
diff --git a/xen/arch/riscv/early_printk.c b/xen/arch/riscv/early_printk.c
new file mode 100644
index 0000000000..b66a11f1bc
--- /dev/null
+++ b/xen/arch/riscv/early_printk.c
@@ -0,0 +1,33 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * RISC-V early printk using SBI
+ *
+ * Copyright (C) 2021 Bobby Eshleman <[e-mail]>
+ */
+#include <asm/early_printk.h>
+#include <asm/sbi.h>
+
+/*
+ * TODO:
+ *   sbi_console_putchar is already planned for deprecation
+ *   so it should be reworked to use UART directly.
+*/
+void early_puts(const char *s, size_t nr)
+{
+    while ( nr-- > 0 )
+    {
+        if ( *s == '\n' )
+            sbi_console_putchar('\r');
+        sbi_console_putchar(*s);
+        s++;
+    }
+}
+
+void early_printk(const char *str)
+{
+    while ( *str )
+    {
+        early_puts(str, 1);
+        str++;
+    }
+}
diff --git a/xen/arch/riscv/include/asm/early_printk.h b/xen/arch/riscv/include/asm/early_printk.h
new file mode 100644
index 0000000000..05106e160d
--- /dev/null
+++ b/xen/arch/riscv/include/asm/early_printk.h
@@ -0,0 +1,12 @@ 
+#ifndef __EARLY_PRINTK_H__
+#define __EARLY_PRINTK_H__
+
+#include <xen/early_printk.h>
+
+#ifdef CONFIG_EARLY_PRINTK
+void early_printk(const char *str);
+#else
+static inline void early_printk(const char *s) {};
+#endif
+
+#endif /* __EARLY_PRINTK_H__ */
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 13e24e2fe1..d09ffe1454 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,12 +1,16 @@ 
 #include <xen/compile.h>
 #include <xen/init.h>
 
+#include <asm/early_printk.h>
+
 /* Xen stack for bringing up the first CPU. */
 unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
     __aligned(STACK_SIZE);
 
 void __init noreturn start_xen(void)
 {
+    early_printk("Hello from C env\n");
+
     for ( ;; )
         asm volatile ("wfi");