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

[Andrew Jones <drjones@redhat.com>]

For v3, I've done the following changes (inline)

On Wed, Apr 21, 2021 at 09:02:06AM +0200, Andrew Jones wrote:
> On Tue, Apr 20, 2021 at 09:00:02PM +0200, Andrew Jones wrote:
> > The method can also be smc and it will be when running on bare metal.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arm/cstart.S       | 22 ++++++++++++++++++++++
> >  arm/cstart64.S     | 22 ++++++++++++++++++++++
> >  arm/selftest.c     | 34 +++++++---------------------------
> >  lib/arm/asm/psci.h | 10 ++++++++--
> >  lib/arm/psci.c     | 37 +++++++++++++++++++++++++++++--------
> >  lib/arm/setup.c    |  2 ++
> >  6 files changed, 90 insertions(+), 37 deletions(-)
> > 
> > diff --git a/arm/cstart.S b/arm/cstart.S
> > index 446966de350d..2401d92cdadc 100644
> > --- a/arm/cstart.S
> > +++ b/arm/cstart.S
> > @@ -95,6 +95,28 @@ start:
> >  
> >  .text
> >  
> > +/*
> > + * psci_invoke_hvc / psci_invoke_smc
> > + *
> > + * Inputs:
> > + *   r0 -- function_id
> > + *   r1 -- arg0
> > + *   r2 -- arg1
> > + *   r3 -- arg2
> > + *
> > + * Outputs:
> > + *   r0 -- return code
> > + */
> > +.globl psci_invoke_hvc
> > +psci_invoke_hvc:
> > +	hvc	#0
> > +	mov	pc, lr
> > +
> > +.globl psci_invoke_smc
> > +psci_invoke_smc:
> > +	smc	#0
> > +	mov	pc, lr
> > +
> >  enable_vfp:
> >  	/* Enable full access to CP10 and CP11: */
> >  	mov	r0, #(3 << 22 | 3 << 20)
> > diff --git a/arm/cstart64.S b/arm/cstart64.S
> > index 42ba3a3ca249..7610e28f06dd 100644
> > --- a/arm/cstart64.S
> > +++ b/arm/cstart64.S
> > @@ -109,6 +109,28 @@ start:
> >  
> >  .text
> >  
> > +/*
> > + * psci_invoke_hvc / psci_invoke_smc
> > + *
> > + * Inputs:
> > + *   x0 -- function_id

changed this comment to be 'w0 -- function_id'

> > + *   x1 -- arg0
> > + *   x2 -- arg1
> > + *   x3 -- arg2
> > + *
> > + * Outputs:
> > + *   x0 -- return code
> > + */
> > +.globl psci_invoke_hvc
> > +psci_invoke_hvc:
> > +	hvc	#0
> > +	ret
> > +
> > +.globl psci_invoke_smc
> > +psci_invoke_smc:
> > +	smc	#0
> > +	ret
> > +
> >  get_mmu_off:
> >  	adrp	x0, auxinfo
> >  	ldr	x0, [x0, :lo12:auxinfo + 8]
> > diff --git a/arm/selftest.c b/arm/selftest.c
> > index 4495b161cdd5..9f459ed3d571 100644
> > --- a/arm/selftest.c
> > +++ b/arm/selftest.c
> > @@ -400,33 +400,13 @@ static void check_vectors(void *arg __unused)
> >  	exit(report_summary());
> >  }
> >  
> > -static bool psci_check(void)
> > +static void psci_print(void)
> >  {
> > -	const struct fdt_property *method;
> > -	int node, len, ver;
> > -
> > -	node = fdt_node_offset_by_compatible(dt_fdt(), -1, "arm,psci-0.2");
> > -	if (node < 0) {
> > -		printf("PSCI v0.2 compatibility required\n");
> > -		return false;
> > -	}
> > -
> > -	method = fdt_get_property(dt_fdt(), node, "method", &len);
> > -	if (method == NULL) {
> > -		printf("bad psci device tree node\n");
> > -		return false;
> > -	}
> > -
> > -	if (len < 4 || strcmp(method->data, "hvc") != 0) {
> > -		printf("psci method must be hvc\n");
> > -		return false;
> > -	}
> > -
> > -	ver = psci_invoke(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> > -	printf("PSCI version %d.%d\n", PSCI_VERSION_MAJOR(ver),
> > -				       PSCI_VERSION_MINOR(ver));
> > -
> > -	return true;
> > +	int ver = psci_invoke(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> > +	report_info("PSCI version: %d.%d", PSCI_VERSION_MAJOR(ver),
> > +					  PSCI_VERSION_MINOR(ver));
> > +	report_info("PSCI method: %s", psci_invoke == psci_invoke_hvc ?
> > +				       "hvc" : "smc");
> >  }
> >  
> >  static void cpu_report(void *data __unused)
> > @@ -465,7 +445,7 @@ int main(int argc, char **argv)
> >  
> >  	} else if (strcmp(argv[1], "smp") == 0) {
> >  
> > -		report(psci_check(), "PSCI version");
> > +		psci_print();
> >  		on_cpus(cpu_report, NULL);
> >  		while (!cpumask_full(&ready))
> >  			cpu_relax();
> > diff --git a/lib/arm/asm/psci.h b/lib/arm/asm/psci.h
> > index 7b956bf5987d..2820c0a3afc7 100644
> > --- a/lib/arm/asm/psci.h
> > +++ b/lib/arm/asm/psci.h
> > @@ -3,8 +3,14 @@
> >  #include <libcflat.h>
> >  #include <linux/psci.h>
> >  
> > -extern int psci_invoke(unsigned long function_id, unsigned long arg0,
> > -		       unsigned long arg1, unsigned long arg2);
> > +typedef int (*psci_invoke_fn)(unsigned long function_id, unsigned long arg0,
> > +			      unsigned long arg1, unsigned long arg2);
> > +extern psci_invoke_fn psci_invoke;
> > +extern int psci_invoke_hvc(unsigned long function_id, unsigned long arg0,
> > +			   unsigned long arg1, unsigned long arg2);
> > +extern int psci_invoke_smc(unsigned long function_id, unsigned long arg0,
> > +			   unsigned long arg1, unsigned long arg2);

The prototypes are now

long invoke_fn(unsigned int function_id, unsigned long arg0,
               unsigned long arg1, unsigned long arg2)

Notice the return value changed to long and the function_id to
unsigned int.


I also improved the commit message by adding the following:

   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.

Thanks,
drew