On Thu, Jun 2, 2022 at 8:18 AM Anup Patel <apatel@...> wrote:

On Thu, Jun 2, 2022 at 8:30 PM Ved Shanbhogue <ved@...> wrote:

On Thu, Jun 02, 2022 at 07:52:55PM +0530, Anup Patel wrote:

On Thu, Jun 2, 2022 at 6:29 PM Ved Shanbhogue <ved@...> wrote:
This is very slow for virtualized world particularly KVM RISC-V because each
SBI v0.1 putchar() or getchar() will trap to KVM RISC-V and KVM RISC-V will
forward it to user-space QEMU or KVMTOOL. This means each early print
character using SBI v0.1 putchar() will go all the way to host user-space and
come back. This is horribly slow for KVM Guest. This becomes further slower
for nested virtualization.

Since this is for debug and really early phase debug till enough of the guest
boots up to use a VFIO based char driver provided by the VMM, I am not sure
that the slowness matters.
Even this SBI call I expected the VMM to intercept and if the VMM has all
emulation in the user space VMM - e.g. the console tty is open by the user
space VMM, I am not sure this would avoid that trip to user space.
If the motivation is primarily VM debug then perhaps a standardized set of
hypercalls implemented by KVM makes more sense than SBI calls that would
need to be built into the M-mode firmware?

We have a requirement in the OS-A platform spec to mandate a particular type
of UART for early prints but there were objections on selecting one type of UART
over another.

This SBI debug console extension is also a way to avoid standardizing a
particular type of UART in OS-A platform spec.

I worry about bugs/security issues that can be caused by M-mode firmware accessing strings in untrusted memory.

The VirtIO based para-virt devices rely heavily on shared memory so I think
it is possible to address security concerns related to shared memory.

Yes, and now that I understand the motivation better why dont we define this
as a hypercall/pv-ops interface to a VMM than a SBI call to the M-mode
firmware and needing to build a virt-io like framework in firmware.

The VirtIO framework has rings in shared memory but over here it is just
shared memory without any formatted data structure.

The API as defined does not say whether the address is a virtual address or a physical address.

It is a physical address. I will clarify this in Draft v2.

Thanks. I was not sure since we had all the discussion about Svpbmt.

Based on discussion it did not seem like it needs to be much fancier than this as this is for early OS/VMM code till it has enough functionality to directly interact with a uart.

The goal of the shared memory based SBI call for early prints is to
minimize the number of traps which in-turn helps virtualization to
drastically reduce boot-time.

Understand that better now. But if that is the main motivation then
I am not understanding why we would want to push all of this into
M-mode firmware vs. defining a set of standardized pv-ops to be used
by guest OSes.

The SBI debug console also provides a standard way of early prints for
supervisor software running natively (directly under M-mode) irrespective
of the type of UART/Console available on the console so it's not just
for a virtualized world. Although, the virtualized world has an added
performance advantage due to reduced traps.

It's also helpful in early board bringup and early debugging as
pointed out by Heiko as well.
To address some of the concerns raised above, how about putting some
restrictions on sbi_debug_console_set_area ?

1. Instead of any size, restrict the shared memory region to a fixed
size (256 byte should be enough for early prints) ?
It is still a common area across all harts. Thus, atomicity needs to
be ensured by the M-mode or HS mode.
We may explore setting up a per-heart area to avoid synchronization
issues but platforms with large hart counts may have to
share multiple pages depending on the page size.

2. The shared memory region should be set up only once during a boot
cycle. Any further invocation of sbi_debug_console_set_area
should return appropriate errors.

If the per heart shared memory region approach is preferred, the above
restriction applies once per hart.

Regards,
Anup