On Mon, Apr 19, 2021 at 05:33:37PM +0100, Alexandru Elisei wrote:
> On 4/7/21 7:59 PM, Andrew Jones wrote:
> > +psci_invoke_fn psci_invoke;
>
> In setup(), we set the conduit after we call assert() several time. If the asert()
> fails, then psci_system_off() will end up calling a NULL function. Maybe there
> should be some sort of check for that?
I can initialize psci_invoke to something that will fail in a more obvious
manner.
>
> > +
> > __attribute__((noinline))
> > -int psci_invoke(unsigned long function_id, unsigned long arg0,
> > - unsigned long arg1, unsigned long arg2)
> > +int psci_invoke_hvc(unsigned long function_id, unsigned long arg0,
> > + unsigned long arg1, unsigned long arg2)
> > {
> > asm volatile(
> > "hvc #0"
> > @@ -22,6 +24,17 @@ int psci_invoke(unsigned long function_id, unsigned long arg0,
> > return function_id;
> > }
> >
> > +__attribute__((noinline))
> > +int psci_invoke_smc(unsigned long function_id, unsigned long arg0,
> > + unsigned long arg1, unsigned long arg2)
> > +{
> > + asm volatile(
> > + "smc #0"
> > + : "+r" (function_id)
> > + : "r" (arg0), "r" (arg1), "r" (arg2));
> > + return function_id;
>
> I haven't been able to figure out what prevents the compiler from shuffling the
> arguments around before executing the inline assembly, such that x0-x3 doesn't
> contain the arguments in the order we are expecting.
We know the arguments will be in r0-r3 because of the noinline and that
shuffling them wouldn't make much sense, but I agree that this is in the
realm of [too] fragile assumptions.
>
> Some excerpts from the extended asm help page [1] that make me believe that the
> compiler doesn't provide any guarantees:
>
> "If you must use a specific register, but your Machine Constraints do not provide
> sufficient control to select the specific register you want, local register
> variables may provide a solution"
>
> "Using the generic ‘r’ constraint instead of a constraint for a specific register
> allows the compiler to pick the register to use, which can result in more
> efficient code."
>
> Same with psci_invoke_hvc(). Doing both in assembly (like Linux) should be
> sufficient and fairly straightforward.
OK, I'll just use assembly to avoid the assumptions.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm
>
> > +}
> > +
> > int psci_cpu_on(unsigned long cpuid, unsigned long entry_point)
> > {
> > #ifdef __arm__
> > diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> > index 5cda2d919d2b..e595a9e5a167 100644
> > --- a/lib/arm/setup.c
> > +++ b/lib/arm/setup.c
> > @@ -25,6 +25,7 @@
> > #include <asm/processor.h>
> > #include <asm/smp.h>
> > #include <asm/timer.h>
> > +#include <asm/psci.h>
> >
> > #include "io.h"
> >
> > @@ -55,6 +56,26 @@ int mpidr_to_cpu(uint64_t mpidr)
> > return -1;
> > }
> >
> > +static void psci_set_conduit(void)
> > +{
> > + const void *fdt = dt_fdt();
> > + const struct fdt_property *method;
> > + int node, len;
> > +
> > + node = fdt_node_offset_by_compatible(fdt, -1, "arm,psci-0.2");
> > + assert_msg(node >= 0, "PSCI v0.2 compatibility required");
> > +
> > + method = fdt_get_property(fdt, node, "method", &len);
> > + assert(method != NULL && len == 4);
> > +
> > + if (strcmp(method->data, "hvc") == 0)
> > + psci_invoke = psci_invoke_hvc;
> > + else if (strcmp(method->data, "smc") == 0)
> > + psci_invoke = psci_invoke_smc;
> > + else
> > + assert_msg(false, "Unknown PSCI conduit: %s", method->data);
> > +}
>
> Any particular reason for doing this here instead of in psci.c? This looks like
> something that belongs to that file, but that might just be my personal preference.
I don't have a strong preference on this, so I'll move it.
Thanks,
drew
