| Message ID | 20190826233256.32383-1-atish.patra@wdc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for SBI version to 0.2 | expand |
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. Second just blindly moving all the existing calls to a single legacy extension doesn't seem useful. We need to differenciate the existing calls: (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 (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 (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. If we want useful acceleration of i-cache synchronization we'll need actual instructions that are much more fine grained in the future.
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.
On Tue, Aug 27, 2019 at 10:19:42PM +0000, Atish Patra wrote: > 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 ? Yes, sorry I mistead this. The way is is defined is rather non-intuitive, but actually backwards compatible. > 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. I think we actually need to fix the spec instead, even if it just the naming and not the mechanism. > > (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. The getchar/putchar calls unfortunately are fundamentally flawed, as they mean the sbi can still access the console after the host has taken it over using its own drivers. Which will lead to bugs sooner or later. And if you can bring up a console driver in opensbi it should be just as trivial to bring up the kernel version.
On Thu, 2019-08-29 at 03:59 -0700, hch@infradead.org wrote: > On Tue, Aug 27, 2019 at 10:19:42PM +0000, Atish Patra wrote: > > 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 ? > > Yes, sorry I mistead this. The way is is defined is rather > non-intuitive, but actually backwards compatible. > > > 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. > > I think we actually need to fix the spec instead, even if it just the > naming and not the mechanism. > If I understood you clearly, you want to call it legacy in the spec and just say v0.1 extensions. The whole idea of marking them as legacy extensions to indicate that it would be obsolete in the future. But I am not too worried about the semantics here. So I am fine with just changing the text to v0.1 if that avoids confusion. > > > (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. > > The getchar/putchar calls unfortunately are fundamentally flawed, as > they mean the sbi can still access the console after the host has > taken > it over using its own drivers. Which will lead to bugs sooner or > later. > > And if you can bring up a console driver in opensbi it should be just > as trivial to bring up the kernel version.
On Fri, Aug 30, 2019 at 11:13:25PM +0000, Atish Patra wrote: > If I understood you clearly, you want to call it legacy in the spec and > just say v0.1 extensions. > > The whole idea of marking them as legacy extensions to indicate that it > would be obsolete in the future. > > But I am not too worried about the semantics here. So I am fine with > just changing the text to v0.1 if that avoids confusion. So my main problems is that we are lumping all the "legacy" extensions together. While some of them are simply a bad idea and shouldn't really be implemented for anything new ever, others like the sfence.vma and ipi ones are needed until we have hardware support to avoid them and possibly forever for virtualization. So either we use different markers of legacy for them, or we at least define new extensions that replace them at the same time. What I want to avoid is the possibіly of an implementation using the really legacy bits and new extensions at the same time.
On Fri, Sep 13, 2019 at 08:54:27AM -0700, Palmer Dabbelt wrote: > On Tue, Sep 3, 2019 at 12:38 AM hch@infradead.org <hch@infradead.org> wrote: > > > On Fri, Aug 30, 2019 at 11:13:25PM +0000, Atish Patra wrote: > > > If I understood you clearly, you want to call it legacy in the spec and > > > just say v0.1 extensions. > > > > > > The whole idea of marking them as legacy extensions to indicate that it > > > would be obsolete in the future. > > > > > > But I am not too worried about the semantics here. So I am fine with > > > just changing the text to v0.1 if that avoids confusion. > > > > So my main problems is that we are lumping all the "legacy" extensions > > together. While some of them are simply a bad idea and shouldn't > > really be implemented for anything new ever, others like the sfence.vma > > and ipi ones are needed until we have hardware support to avoid them > > and possibly forever for virtualization. > > > > So either we use different markers of legacy for them, or we at least > > define new extensions that replace them at the same time. What I > > want to avoid is the possibіly of an implementation using the really > > legacy bits and new extensions at the same time. > > > > Nominally we've got to replace these as well because we didn't include > the length of the hart mask. Well, let's do that as part of definining the first real post-0.1 SBI then, and don't bother defining the old ones as legacy at all. Just two different specs that don't interact except that we reserve extension space in the new one for the old one so that one SBI spec can implement both.
On Sun, 15 Sep 2019 23:54:46 PDT (-0700), Christoph Hellwig wrote: > On Fri, Sep 13, 2019 at 08:54:27AM -0700, Palmer Dabbelt wrote: >> On Tue, Sep 3, 2019 at 12:38 AM hch@infradead.org <hch@infradead.org> wrote: >> >> > On Fri, Aug 30, 2019 at 11:13:25PM +0000, Atish Patra wrote: >> > > If I understood you clearly, you want to call it legacy in the spec and >> > > just say v0.1 extensions. >> > > >> > > The whole idea of marking them as legacy extensions to indicate that it >> > > would be obsolete in the future. >> > > >> > > But I am not too worried about the semantics here. So I am fine with >> > > just changing the text to v0.1 if that avoids confusion. >> > >> > So my main problems is that we are lumping all the "legacy" extensions >> > together. While some of them are simply a bad idea and shouldn't >> > really be implemented for anything new ever, others like the sfence.vma >> > and ipi ones are needed until we have hardware support to avoid them >> > and possibly forever for virtualization. >> > >> > So either we use different markers of legacy for them, or we at least >> > define new extensions that replace them at the same time. What I >> > want to avoid is the possibіly of an implementation using the really >> > legacy bits and new extensions at the same time. >> > >> >> Nominally we've got to replace these as well because we didn't include >> the length of the hart mask. > > Well, let's do that as part of definining the first real post-0.1 > SBI then, and don't bother defining the old ones as legacy at all. > > Just two different specs that don't interact except that we reserve > extension space in the new one for the old one so that one SBI spec > can implement both. Makes sense. We're getting finish with this "just go write everything down" exercise, so we can start actually doing things now :).