[PATCH v5 5/8] lib: sbi: Implement SBI debug console extension
Anup Patel
apatel at ventanamicro.com
Thu Feb 9 21:29:28 PST 2023
On Fri, Feb 10, 2023 at 10:50 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>
> On 10 Feb 2023, at 04:23, Anup Patel <anup at brainfault.org> wrote:
> >
> > On Wed, Feb 1, 2023 at 2:28 PM Andrew Jones <ajones at ventanamicro.com> wrote:
> >>
> >> On Fri, Jan 13, 2023 at 05:11:07PM +0530, Anup Patel wrote:
> >>> We implement SBI debug console extension as one of the replacement
> >>> SBI extensions. This extension is only available when OpenSBI platform
> >>> provides a console device to generic library.
> >>>
> >>> Signed-off-by: Anup Patel <apatel at ventanamicro.com>
> >>> Reviewed-by: Atish Patra <atishp at rivosinc.com>
> >>> Reviewed-by: Xiang W <wxjstz at 126.com>
> >>> Reviewed-by: Bin Meng <bmeng at tinylab.org>
> >>> ---
> >>> lib/sbi/Kconfig | 4 +++
> >>> lib/sbi/objects.mk | 3 ++
> >>> lib/sbi/sbi_ecall_dbcn.c | 72 ++++++++++++++++++++++++++++++++++++++++
> >>> 3 files changed, 79 insertions(+)
> >>> create mode 100644 lib/sbi/sbi_ecall_dbcn.c
> >>>
> >>> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> >>> index df74bba..ef6728b 100644
> >>> --- a/lib/sbi/Kconfig
> >>> +++ b/lib/sbi/Kconfig
> >>> @@ -26,6 +26,10 @@ config SBI_ECALL_PMU
> >>> bool "Performance Monitoring Unit extension"
> >>> default y
> >>>
> >>> +config SBI_ECALL_DBCN
> >>> + bool "Debug Console extension"
> >>> + default y
> >>> +
> >>> config SBI_ECALL_LEGACY
> >>> bool "SBI v0.1 legacy extensions"
> >>> default y
> >>> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> >>> index c774ebb..319f38d 100644
> >>> --- a/lib/sbi/objects.mk
> >>> +++ b/lib/sbi/objects.mk
> >>> @@ -37,6 +37,9 @@ libsbi-objs-$(CONFIG_SBI_ECALL_SRST) += sbi_ecall_srst.o
> >>> carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_PMU) += ecall_pmu
> >>> libsbi-objs-$(CONFIG_SBI_ECALL_PMU) += sbi_ecall_pmu.o
> >>>
> >>> +carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_DBCN) += ecall_dbcn
> >>> +libsbi-objs-$(CONFIG_SBI_ECALL_DBCN) += sbi_ecall_dbcn.o
> >>> +
> >>> carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_LEGACY) += ecall_legacy
> >>> libsbi-objs-$(CONFIG_SBI_ECALL_LEGACY) += sbi_ecall_legacy.o
> >>>
> >>> diff --git a/lib/sbi/sbi_ecall_dbcn.c b/lib/sbi/sbi_ecall_dbcn.c
> >>> new file mode 100644
> >>> index 0000000..bfa40e9
> >>> --- /dev/null
> >>> +++ b/lib/sbi/sbi_ecall_dbcn.c
> >>> @@ -0,0 +1,72 @@
> >>> +/*
> >>> + * SPDX-License-Identifier: BSD-2-Clause
> >>> + *
> >>> + * Copyright (c) 2022 Ventana Micro Systems Inc.
> >>> + *
> >>> + * Authors:
> >>> + * Anup Patel <apatel at ventanamicro.com>
> >>> + */
> >>> +
> >>> +#include <sbi/sbi_console.h>
> >>> +#include <sbi/sbi_domain.h>
> >>> +#include <sbi/sbi_error.h>
> >>> +#include <sbi/sbi_ecall.h>
> >>> +#include <sbi/sbi_ecall_interface.h>
> >>> +#include <sbi/sbi_trap.h>
> >>> +#include <sbi/riscv_asm.h>
> >>> +
> >>> +static int sbi_ecall_dbcn_handler(unsigned long extid, unsigned long funcid,
> >>> + const struct sbi_trap_regs *regs,
> >>> + unsigned long *out_val,
> >>> + struct sbi_trap_info *out_trap)
> >>> +{
> >>> + ulong smode = (csr_read(CSR_MSTATUS) & MSTATUS_MPP) >>
> >>> + MSTATUS_MPP_SHIFT;
> >>> +
> >>> + switch (funcid) {
> >>> + case SBI_EXT_DBCN_CONSOLE_WRITE:
> >>> + case SBI_EXT_DBCN_CONSOLE_READ:
> >>> + /*
> >>> + * On RV32, the M-mode can only access the first 4GB of
> >>> + * the physical address space because M-mode does not have
> >>> + * MMU to access full 34-bit physical address space.
> >>> + *
> >>> + * Based on above, we simply fail if the upper 32bits of
> >>> + * the physical address (i.e. a2 register) is non-zero on
> >>> + * RV32.
> >>> + */
> >>> +#if __riscv_xlen == 32
> >>> + if (regs->a2)
> >>> + return SBI_ERR_FAILED;
> >>
> >> The spec drafts says SBI_ERR_FAILED is only for I/O errors. I think
> >> we should extend the spec draft to point this situation out and return
> >> SBI_ERR_INVALID_ADDRESS.
> >
> > This is a limitation for OpenSBI RV32 because to support a 34bit
> > physical address for RV32, we will need special infrastructure
> > using a temporary page table. Due to this reason, twe reat this
> > limitation as I/O error on OpenSBI RV32 side.
>
> Were the interface to take a virtual address (or, really, pointer) this
> would not be an issue as the supervisor would already have handled the
> mapping for you. Which is what I’ve kept advocating for.
There are two major issues in passing virtual address:
1) The virtual address could point to anything so SBI implementation
(Both M-mode firmware and hypervisors) have to translate the
virtual address into physical address and then check sanity of
the virtual address. This will involve doing software page table walks
which are very expensive.
2) SBI implementation will have to use unpriv load/stores for accessing
data pointed by virtual address which can potentially trap if the page
table mapping is changed by OS while the SBI implementation was
accessing it.
Regards,
Anup
>
> Jess
>
> >>
> >>> +#endif
> >>> + if (!sbi_domain_check_addr_range(sbi_domain_thishart_ptr(),
> >>> + regs->a1, regs->a0, smode,
> >>> + SBI_DOMAIN_READ|SBI_DOMAIN_WRITE))
> >>> + return SBI_EINVALID_ADDR;
> >>
> >> The spec draft says this should be SBI_ERR_INVALID_PARAM
> >
> > Okay, I will update.
> >
> >>
> >>> + if (funcid == SBI_EXT_DBCN_CONSOLE_WRITE)
> >>> + sbi_nputs((const char *)regs->a1, regs->a0);
> >>> + else
> >>> + *out_val = sbi_ngets((char *)regs->a1, regs->a0);
> >>> + return 0;
> >>> + case SBI_EXT_DBCN_CONSOLE_WRITE_BYTE:
> >>> + sbi_putc(regs->a0);
> >>> + return 0;
> >>> + default:
> >>> + break;
> >>> + }
> >>> +
> >>> + return SBI_ENOTSUPP;
> >>> +}
> >>> +
> >>> +static int sbi_ecall_dbcn_probe(unsigned long extid, unsigned long *out_val)
> >>> +{
> >>> + *out_val = (sbi_console_get_device()) ? 1 : 0;
> >>
> >> nit: Unnecessary ()
> >
> > Okay, I will update.
> >
> >>
> >>> + return 0;
> >>> +}
> >>> +
> >>> +struct sbi_ecall_extension ecall_dbcn = {
> >>> + .extid_start = SBI_EXT_DBCN,
> >>> + .extid_end = SBI_EXT_DBCN,
> >>> + .handle = sbi_ecall_dbcn_handler,
> >>> + .probe = sbi_ecall_dbcn_probe,
> >>> +};
> >>> --
> >>> 2.34.1
> >>>
> >>>
> >>
> >> Thanks,
> >> drew
> >
> > Regards,
> > Anup
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
>
More information about the opensbi
mailing list