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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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 <oleksii.kurochko@xxxxxxxxx>
> > ---
> >  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
> > (bobby.eshleman@xxxxxxxxx).
> > + *
> > + * 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
> > + * (anup.patel@xxxxxxx).
> > + *
> > + * Modified by Bobby Eshleman (bobby.eshleman@xxxxxxxxx).
> > + *
> > + * 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