Re: [PATCH kvm-unit-tests v3 8/8] arm/arm64: psci: Don't assume method is hvc

[Andrew Jones <drjones@redhat.com>]

On Thu, May 13, 2021 at 10:08:09AM +0100, Alexandru Elisei wrote:
> Hi Drew,
> 
> On 5/13/21 8:08 AM, Andrew Jones wrote:
> > On Wed, May 12, 2021 at 05:14:24PM +0100, Alexandru Elisei wrote:
> >> Hi Drew,
> >>
> >> On 4/29/21 5:41 PM, Andrew Jones wrote:
> >>> The method can be smc in addition to hvc, and it will be when running
> >>> on bare metal. Additionally, we move the invocations to assembly so
> >>> we don't have to rely on compiler assumptions. We also fix the
> >>> prototype of psci_invoke. It should return long, not int, and
> >>> function_id should be an unsigned int, not an unsigned long.
> >> Sorry to harp on this again, but to be honest, it's still not clear to me why the
> >> psci_invoke_{hvc,smc} functions return a long int.
> >>
> >> If we only expect the PSCI functions to return error codes, then the PSCI spec
> >> says these are 32-bit signed integers. If we want to support PSCI functions
> >> returning other values, like PSCI_STAT_{RESIDENCY,COUNT}, then the invoke
> >> functions should return an unsigned value.
> >>
> >> The only case we're supporting is the error return for the SMC calling convention
> >> (which says that error codes are 32/64bit signed integers).
> > psci_invoke_{hvc,smc} should implement the SMC calling convention, since
> > they're just wrapping the smc/hvc call. PSCI calls that build on that,
> > e.g. psci_cpu_on, can define their own return type and then translate
> > the signed long returned by SMC into, e.g. 32-bit signed integers. Indeed
> > that's what psci_cpu_on does.
> >
> > I would write something like that in the commit message or rename
> > psci_invoke to smc_invoke.
> 
> I agree that psci_invoke_* use the SMC calling convention, but we're not
> implementing *all* the features of the SMC calling convention, because SMCCC can
> return more than one result in registers r0-r3. In my opinion, I think the easiest
> solution and the most consistent with both specifications would be to keep the
> current names and change the return value either to an int, and put a comment
> saying that we only support PSCI functions that return an error, either to a long
> unsigned int, meaning that we support *all* PSCI functions as defined in ARM DEN
> 0022D.
> 
> What do you think? Does that make sense?
>

OK, I'll change it back to an int return for psci_invoke and remove the
sentences in the commit message about it. I won't write any new comments
though, because I'd actually rather reduce the common PSCI code, than
to try and ensure we support it completely. We should do as little PSCI
in lib/arm* as possible. The psci unit test (arm/psci.c) is the one that
should worry the most about the specifications, and it should actually
define its own interfaces in order to inspect all bits, including the ones
that aren't supposed to be used, when it does its tests.

Thanks,
drew