Re: [Xen-devel] [PATCH 7/7] arm: vsmc: remove 64 bit mode check in psci handler

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 08/08/17 21:08, Volodymyr Babchuk wrote:
> PSCI handling code had helper routine that checked calling convention.
> It does not needed anymore, because:
>
>  - Generic handler checks that 64 bit calls can be made only by
>    64 bit guests.
>
>  - SMCCC requires that 64-bit handler should support both 32 and 64 bit
>    calls even if they originate from 64 bit caller.
>
> This patch removes that extra check.
>
> Also, as there are no more natural scope ( if { } ) to hold local variables,
> paramters to do_psci_*() are taken right from SMC arguments, without storing
> in intermediate local variables.
You could do:

case ....:
{
   uint64_t ...;
   uint32_t ...;

   ....
}

Now you have a scope and keep the code readable.

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> ---
> Now this patch removes { } blocks completely. Not sure if I had to
> split it into two separate patches.
>
> ---
>  xen/arch/arm/vsmc.c | 45 ++++++++++-----------------------------------
>  1 file changed, 10 insertions(+), 35 deletions(-)
>
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index ea86eea..0fd4f5a 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -84,14 +84,6 @@ static bool handle_arch(struct cpu_user_regs *regs)
>      return false;
>  }
>
> -/* helper function for checking arm mode 32/64 bit */
> -static inline int psci_mode_check(struct domain *d, register_t fid)
> -{
> -    return !( is_64bit_domain(d)^
> -              ((fid & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT)) >>
> -               ARM_SMCCC_CALL_CONV_SHIFT) );
> -}
> -
>  /* PSCI 2.0 interface */
>  static bool handle_ssc(struct cpu_user_regs *regs)
>  {
> @@ -113,8 +105,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>          return true;
>      case PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU:
>          perfc_incr(vpsci_migrate_info_up_cpu);
> -        if ( psci_mode_check(current->domain, fid) )
> -            PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
> +        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
>          return true;
>      case PSCI_0_2_FUNC_SYSTEM_OFF:
>          perfc_incr(vpsci_system_off);
> @@ -128,40 +119,24 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>          return true;
>      case PSCI_0_2_FUNC_CPU_ON:
>          perfc_incr(vpsci_cpu_on);
> -        if ( psci_mode_check(current->domain, fid) )
> -        {
> -            register_t vcpuid = PSCI_ARG(regs,1);
> -            register_t epoint = PSCI_ARG(regs,2);
> -            register_t cid = PSCI_ARG(regs,3);
> -            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> -        }
> +        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(PSCI_ARG(regs, 1),
> +                                                 PSCI_ARG(regs, 2),
> +                                                 PSCI_ARG(regs, 3)));
>          return true;
>      case PSCI_0_2_FUNC_CPU_SUSPEND:
>          perfc_incr(vpsci_cpu_suspend);
> -        if ( psci_mode_check(current->domain, fid) )
> -        {
> -            uint32_t pstate = PSCI_ARG32(regs,1);
> -            register_t epoint = PSCI_ARG(regs,2);
> -            register_t cid = PSCI_ARG(regs,3);
> -            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, 
> cid));
> -        }
> +        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(PSCI_ARG32(regs, 1),
> +                                                      PSCI_ARG(regs, 2),
> +                                                      PSCI_ARG(regs, 3)));
>          return true;
>      case PSCI_0_2_FUNC_AFFINITY_INFO:
>          perfc_incr(vpsci_cpu_affinity_info);
> -        if ( psci_mode_check(current->domain, fid) )
> -        {
> -            register_t taff = PSCI_ARG(regs,1);
> -            uint32_t laff = PSCI_ARG32(regs,2);
> -            PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
> -        }
> +        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(PSCI_ARG(regs, 1),
> +                                                        PSCI_ARG32(regs,2)));
>          return true;
>      case PSCI_0_2_FUNC_MIGRATE:
>          perfc_incr(vpsci_cpu_migrate);
> -        if ( psci_mode_check(current->domain, fid) )
> -        {
> -            uint32_t tcpu = PSCI_ARG32(regs,1);
> -            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
> -        }
> +        PSCI_SET_RESULT(regs, do_psci_0_2_migrate(PSCI_ARG32(regs, 1)));
>          return true;
>      case ARM_SMCCC_FUNC_CALL_COUNT:
>          set_user_reg(regs, 0, SSC_SMCCC_FUNCTION_COUNT);
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel