Re: [PATCH v4 3/4] xen/riscv: introduce early_printk basic stuff

[Julien Grall <julien.grall@xxxxxxxxx>]

On Tue, 17 Jan 2023 at 23:57, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:

On 16/01/2023 2:39 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/riscv/Kconfig.debug b/xen/arch/riscv/Kconfig.debug
> index e69de29bb2..e139e44873 100644
> --- a/xen/arch/riscv/Kconfig.debug
> +++ b/xen/arch/riscv/Kconfig.debug
> @@ -0,0 +1,6 @@
> +config EARLY_PRINTK
> +    bool "Enable early printk"
> +    default DEBUG
> +    help
> +
> +      Enables early printk debug messages

Kconfig indentation is a little hard to get used to.

It's one tab for the main block, and one tab + 2 spaces for the help text.

Also, drop the blank line after help.

> 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..6bc29a1942
> --- /dev/null
> +++ b/xen/arch/riscv/early_printk.c
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * RISC-V early printk using SBI
> + *
> + * Copyright (C) 2021 Bobby Eshleman <bobbyeshleman@xxxxxxxxx>
> + */
> +#include <asm/early_printk.h>
> +#include <asm/sbi.h>
> +
> +/*
> + * 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 before relocate so it should not use absolute addressing."
> +#endif

This is incorrect.

Aside the part about the relocation, I do not see what is wrong with it (see below)

What *this* file is compiled with has no bearing on how head.S calls
us.  The RISC-V documentation explaining __riscv_cmodel_medany vs
__riscv_cmodel_medlow calls this point out specifically.  There's
nothing you can put here to check that head.S gets compiled with medany.

I believe you misunderstood the goal here. It is not to check how head.S will call it but to ensure the function is safe to be called when the MMU is off (e.g VA != VA).

Right now, there's nothing in this file dependent on either mode, and
it's not liable to change in the short term.

The whole point is to make sure this don’t change without us knowing. 

  Furthermore, Xen isn't
doing any relocation in the first place.

We will want to support XIP in due course, and that will be compiled
__riscv_cmodel_medlow, which is a fine and legitimate usecase.


The build system sets the model up consistently.  All you are doing by
putting this in is creating work that someone is going to have to delete
for legitimate reasons in the future.

Are you saying that a code compiled with medlow can also work with MMU off and no relocation needed?

If not, then the check is correct. It would avoid hours of debugging…

Cheers,

> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index 13e24e2fe1..9c9412152a 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,13 +1,17 @@
>  #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)
>  {
> -    for ( ;; )
> +    early_printk("Hello from C env\n");
> +
> +    for ( ; ; )

Rebasing error?

~Andrew

--

Julien Grall