Re: [Xen-devel] [PATCH v4 2/2] xen/arm : emulation of arm's PSCI v0.2 standard

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-07-14 at 19:21 +0530, Parth Dixit wrote:

Thanks.

> @@ -1082,31 +1064,84 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
> unsigned int code)
>  #ifdef CONFIG_ARM_64
>  #define PSCI_OP_REG(r) (r)->x0
>  #define PSCI_RESULT_REG(r) (r)->x0
> -#define PSCI_ARGS(r) (r)->x1, (r)->x2
> +#define PSCI_ARGS(r) (r)->x1, (r)->x2, (r)->x3
> +#define PSCI_ARG(r,n) (r)->x##n

It seems like one or the other of these is redundant. Special casing
functions which have exactly three arguments is unnecessary.

>  #else
>  #define PSCI_OP_REG(r) (r)->r0
>  #define PSCI_RESULT_REG(r) (r)->r0
> -#define PSCI_ARGS(r) (r)->r1, (r)->r2
> +#define PSCI_ARGS(r) (r)->r1, (r)->r2, (r)->r3
> +#define PSCI_ARG(r,n) (r)->r##n
>  #endif
>  
>  static void do_trap_psci(struct cpu_user_regs *regs)
>  {
> -    arm_psci_fn_t psci_call = NULL;
> -
> -    if ( PSCI_OP_REG(regs) >= ARRAY_SIZE(arm_psci_table) )
> -    {
> -        domain_crash_synchronous();
> -        return;
> -    }
> -
> -    psci_call = arm_psci_table[PSCI_OP_REG(regs)].fn;
> -    if ( psci_call == NULL )
> +    switch( PSCI_OP_REG(regs) )
>      {
> +    case PSCI_cpu_off:
> +        {
> +            uint32_t pstate = (u32)( PSCI_ARG(regs,1) );

Don't mix and match uintNN_t with uNN please. We generally prefer the
uintXX_t version. If you wanted you could incorporate this into a
PSCI_ARG32 macro (up to you).

I think the braces around PSCI_ARG are not needed, are they? Or if they
are then they should be in the macro I think.

> +            PSCI_RESULT_REG(regs) = do_psci_cpu_off(pstate);

I think you also need to cast the result of any SMC32 call, in order to
properly zero the top bits of x0 in the SMC64 case, to match the
architectural behaviour of writing to w0.

> +    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
> +    case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:

You need to check is_32bit_domain here and reject calls from the wrong
mode for each fn type with INVALID_PARAMETERS.

Perhaps either split the two cases, or a psci_mode_check(v,
PSCI_OP_REG(regs)) helper which just checks bit 30? Sadly you can't pull
that check to the top since some SMC32 calls are valid for aarch64 and
some aren't.

> +        PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_up_cpu();

Need to be careful here. An AArch32 guest running on an AArch64
hypervisor might end up with a truncated result. Perhaps that is OK,
since all of their MIDR/cpu_target stuff is already truncated.

I'd be happier if the top half of the register state for an AArch32
guest remained clear though, I so I think a cast should be used (which
suggests that splitting the two cases will be cleaner).

> +        break;
> +    case PSCI_0_2_FN_SYSTEM_OFF:
> +        do_psci_0_2_system_off();
> +        break;
> +    case PSCI_0_2_FN_SYSTEM_RESET:
> +        do_psci_0_2_system_reset();

I guess neither of these return, but it seems wise to preload the return
value with PSCI_RET_INTERNAL_FAILURE just in case.

> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index 1ceb8cb..2ce96d4 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -17,24 +17,92 @@
>  #include <asm/current.h>
>  #include <asm/gic.h>
>  #include <asm/psci.h>
> +#include <public/sched.h>
> +#include <asm-arm/event.h>
> +
> +static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
> +                       register_t context_id,int ver);

Please order the functions in the file to avoid forward decls where
possible.
 
Ian.


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