Re: [RFC PATCH 0/2] Add support for SBI version to 0.2

[Atish Patra]

On Tue, 2019-08-27 at 07:46 -0700, Christoph Hellwig wrote:
> On Mon, Aug 26, 2019 at 04:32:54PM -0700, Atish Patra wrote:
> > This patch series aims to add support for SBI specification version
> > v0.2. It doesn't break compatibility with any v0.1 implementation.
> > Internally, all the v0.1 calls are just renamed to legacy to be in
> > sync with specification [1].
> > 
> > The patches for v0.2 support in OpenSBI are available at
> > http://lists.infradead.org/pipermail/opensbi/2019-August/000422.html
> > 
> > [1] 
> > https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
> 
> I really don't like the current design of that SBI 0.2 spec,
> and don't think implementing it as-is is helpful.
> 
> For one the way how the extension id is placed creates a compatibilty
> problem, not allowing your to implement a backwards compatible sbi,
> which seems bad.
> 

I did not understand this part. All the legacy SBI calls are defined as
a separate extension ID not single extension. How did it break the
backward compatibility ?

Here are the few possible usecases

1. New kernel can use SBI v0.2 calling convention for older legacy
calls. It will just set a6 to zero (function id) & not use the return
value in a1. a0 is still report error value.

2. New kernel with older SBI implementation (i.e. BBL) will also work
as sbi_get_spec_version will return error and spec_version will be set
0.1. BBL never checks a6 or set a1 which works for the legacy calls.

3. Older kernel with new SBI implementation(i.e OpenSBI) will never use
v0.2 calling conventions. OpenSBI never use a6 or set a1 for legacy
calls anyways.

Did I miss any usecase ? 

> Second just blindly moving all the existing calls to a single legacy
> extension doesn't seem useful.  We need to differenciate the existing
> calls:

I think the confusion is because of legacy renaming. They are not
single legacy extension. They are all separate extensions. The spec
just called all those extensions as collectively as legacy. So I just
tried to make the patch sync with the spec.

If that's the source of confusion, I can rename it to sbi_0.1_x in
stead of legacy.
> 
>  (1) actually board specific and have not place in a cpu abstraction
>      layer: getchar/putchar, these should just never be advertised in
> a
>       non-legacy setup, and the drivers using them should not probe
>       on a sbi 0.2+ system

In that case, we have to update the drivers(earlycon-riscv-sbi &
hvc_riscv_sbi) in kernel as well. Once these patches are merged, nobody
will be able to use earlycon=sbi feature in mainline kernel.

Personally, I am fine with it. But there were some interest during
RISC-V workshop in keeping these for now for easy debugging and early
bringup.


>  (2) useful for currently taped out cpus and in the long run for
>      virtualization to avoid mmio traps:  ipis, timers, tlb
> shootdown.
>      These should stay backwards compatible, but for sbi 0.2 be
>      negotiated individually

We still can do that with existing scheme.

>  (3) in theory useful, but given how much of a big hammer sfence.i
>      not useful in theory: SBI_REMOTE_FENCE_I we can decide if we
> want
>      to either not allow it for sbi 0.2+ or also negotiate it.  I'd
>      personally favor not advertising it and just use ipis to
> implement
>      it.  

Once we have native IPIs, sure. Otherwise, sfence.i using IPI via SBI
will be even slower compared today. Gary had done the same thing for
sfence.vma and it was not enocouraged.

https://patchwork.kernel.org/patch/10845959/#22576987

> If we want useful acceleration of i-cache synchronization
>      we'll need actual instructions that are much more fine grained
>      in the future.


-- 
Regards,
Atish