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

[Andrew Jones <drjones@redhat.com>]

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