Re: [Xen-devel] [PATCH v3 12/17] xen/arm: vpsci: Remove parameter 'ver' from do_common_cpu

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 15/02/18 15:02, Julien Grall wrote:
> Currently, the behavior of do_common_cpu will slightly change depending
> on the PSCI version passed in parameter. Looking at the code, more the
> specific 0.2 behavior could move out of the function or adapted for 0.1:
> 
>     - x0/r0 can be updated on PSCI 0.1 because general purpose registers
>     are undefined upon CPU on.

Is that explicitly mentioned somewhere in the spec? I couldn't find
anything in the original DEN0022A. Or is that because it does *not*
mention anything about the GPR state that we are safe to put anything in
there?

>     - PSCI 0.1 does not defined PSCI_ALREADY_ON. However, it would be
>     safer to bail out if the CPU is already on.
> 
> Based on this, the parameter 'ver' is removed and do_psci_cpu_on
> (implementation for PSCI 0.1) is adapted to avoid returning
> PSCI_ALREADY_ON.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@xxxxxxxx>

Given that it's safe to clobber x0/r0 on CPU_ON in PSCI 0.1, the rest
looks correct to me:

Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>

Cheers,
Andre.

> ---
>     The reviewed-by was kept despite move this patch towards the end
>     of the series because there was no clash with the rest of the series.
> 
>     Changes in v2:
>         - Move the patch towards the end of the series as not strictly
>         necessary for SP2.
>         - Add Volodymyr's reviewed-by
> ---
>  xen/arch/arm/vpsci.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index 19ee7caeb4..7ea3ea58e3 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -22,7 +22,7 @@
>  #include <public/sched.h>
>  
>  static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
> -                       register_t context_id,int ver)
> +                            register_t context_id)
>  {
>      struct vcpu *v;
>      struct domain *d = current->domain;
> @@ -40,8 +40,7 @@ static int do_common_cpu_on(register_t target_cpu, 
> register_t entry_point,
>      if ( is_64bit_domain(d) && is_thumb )
>          return PSCI_INVALID_PARAMETERS;
>  
> -    if ( (ver == PSCI_VERSION(0, 2)) &&
> -            !test_bit(_VPF_down, &v->pause_flags) )
> +    if ( !test_bit(_VPF_down, &v->pause_flags) )
>          return PSCI_ALREADY_ON;
>  
>      if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
> @@ -55,18 +54,21 @@ static int do_common_cpu_on(register_t target_cpu, 
> register_t entry_point,
>      ctxt->ttbr0 = 0;
>      ctxt->ttbr1 = 0;
>      ctxt->ttbcr = 0; /* Defined Reset Value */
> +
> +    /*
> +     * x0/r0_usr are always updated because for PSCI 0.1 the general
> +     * purpose registers are undefined upon CPU_on.
> +     */
>      if ( is_32bit_domain(d) )
>      {
>          ctxt->user_regs.cpsr = PSR_GUEST32_INIT;
> -        if ( ver == PSCI_VERSION(0, 2) )
> -            ctxt->user_regs.r0_usr = context_id;
> +        ctxt->user_regs.r0_usr = context_id;
>      }
>  #ifdef CONFIG_ARM_64
>      else
>      {
>          ctxt->user_regs.cpsr = PSR_GUEST64_INIT;
> -        if ( ver == PSCI_VERSION(0, 2) )
> -            ctxt->user_regs.x0 = context_id;
> +        ctxt->user_regs.x0 = context_id;
>      }
>  #endif
>  
> @@ -93,7 +95,14 @@ static int do_common_cpu_on(register_t target_cpu, 
> register_t entry_point,
>  
>  static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
>  {
> -    return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1));
> +    int32_t ret;
> +
> +    ret = do_common_cpu_on(vcpuid, entry_point, 0);
> +    /*
> +     * PSCI 0.1 does not define the return code PSCI_ALREADY_ON.
> +     * Instead, return PSCI_INVALID_PARAMETERS.
> +     */
> +    return (ret == PSCI_ALREADY_ON) ? PSCI_INVALID_PARAMETERS : ret;
>  }
>  
>  static int32_t do_psci_cpu_off(uint32_t power_state)
> @@ -137,8 +146,7 @@ static int32_t do_psci_0_2_cpu_on(register_t target_cpu,
>                                    register_t entry_point,
>                                    register_t context_id)
>  {
> -    return do_common_cpu_on(target_cpu, entry_point, context_id,
> -                            PSCI_VERSION(0, 2));
> +    return do_common_cpu_on(target_cpu, entry_point, context_id);
>  }
>  
>  static const unsigned long target_affinity_mask[] = {
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel