[PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Andrew Jones]

On Thu, Oct 12, 2023 at 07:25:11PM +0200, Andrew Jones wrote:
> On Thu, Oct 12, 2023 at 10:09:44PM +0530, Anup Patel wrote:
> > On Thu, Oct 12, 2023 at 9:31 PM Andrew Jones <ajones at ventanamicro.com> wrote:
> > >
> > > On Thu, Oct 12, 2023 at 02:32:46PM +0100, Conor Dooley wrote:
> > > > On Thu, Oct 12, 2023 at 02:30:02PM +0100, Conor Dooley wrote:
> > > > > Yo,
> > > > >
> > > > > On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote:
> > > > > > When the SUSP SBI extension is present it implies that the standard
> > > > > > "suspend to RAM" type is available. Wire it up to the generic
> > > > > > platform suspend support, also applying the already present support
> > > > > > for non-retentive CPU suspend. When the kernel is built with
> > > > > > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend.
> > > > > > Resumption will occur when a platform-specific wake-up event arrives.
> > > > > >
> > > > > > Signed-off-by: Andrew Jones <ajones at ventanamicro.com>
> > > > >
> > > > > > +static int __init sbi_system_suspend_init(void)
> > > > > > +{
> > > > > > + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) {
> > > > >
> > > > > Random thought I had reading this, was that it'll be possible to have a
> > > > > firmware that implements SBI < 2.0 that provides the SUSP extension.
> > > > > FWIW, I don't think that that is problematic, but maybe I am missing
> > > > > something that would make it so.
> > > >
> > > > Hmm, next patch I look at is from Anup's debug console series, and he
> > > > does check that the SBI implementation is at least version 2.0 before
> > > > probing for the extension. We should probably have the same policy
> > > > everywhere.
> > >
> > > Yeah, the main reason I wrote in the other response that I'd prefer not
> > > to always check version when probing is because in most (I think all
> > > except PMU) cases it would only reduce the platforms where the extension
> > > can be used, but without any reason to do so. For example, right now QEMU
> > > bundles an OpenSBI v1.3.1 binary and that version supports both SUSP and
> > > DBCN, but, if we were to add SBI 2.0 checks in Linux for those extensions,
> > > then users will need to update their SBI implementations in order to use
> > > them. While requiring users to update their SBI implementations makes
> > > sense for new functionality or fixes, it seems like a lot to ask for them
> > > to just get a bigger number in their version check.
> > >
> > > (And then there's downstream SBI implementations which may end up cherry
> > > picking extensions, so their version numbers would be hard to define.)
> > >
> > > So my vote for Linux policy would be to only do version checks when
> > > necessary. And my preference for SBI would be to try and avoid specifying
> > > extensions which require clients to check versions.
> > 
> > In addition to SBI PMU, even SBI STA and SBI NACL requires spec version
> > check because these new SBI extensions use SBI_ERR_NO_SHMEM
> > error code which is not available in SBI v1.0 (or lower).
> 
> But when those extensions are present, they must implement their
> specifications, which means SBI_ERR_NO_SHMEM must also be present.
> From a client's perspective getting an affirmative that the extension is
> present is enough. Also checking the SBI version is basically turning the
> client into an SBI implementation sanity checker.
> 
> > 
> > My recommendation is that SBI implementation should comply with the
> > minimum SBI spec version required by the SBI extensions which it
> > implements.
> >
> 
> I agree, but SBI implementation versions don't get bumped until after
> spec freezes, even though they incorporate everything needed for the
> new extensions and the extensions themselves.
> 
> Linux can certainly choose to be strict about the SBI implementations
> from which it enables extensions, but I'm still not sure it's necessary.
>

I was just reviewing the DBCN series and was about to raise my thoughts
there again about not being so strict about SBI implementation versions,
but then I thought some more about it. I guess we should be strict about
the version since it's otherwise not possible to any confidence that the
extension advertised is the extension described in the frozen/ratified
version of the spec (there could be implementations of draft spec versions
which are not compatible with the final version and those will have the
same extension ID). The most confidence Linux can have in an SBI
extension's implementation being what it expects to be will come from
both the extension's presence and the SBI implementation's version
being at least as big as the version in which the extension was frozen.

Long story short, I'll change the above condition to look for 2.0 and
ask QEMU folks to put an OpenSBI binary which advertises 2.0 into its
repo as soon as possible.

Thanks,
drew