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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

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.

Right now, there's nothing in this file dependent on either mode, and
it's not liable to change in the short term.  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.

> 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