Re: [PATCH RFC] xen/arm : emulation of arm's psci v0.2 standard

[Julien Grall <julien.grall@linaro.org>]

Hi Parth,

Cced the maintainers here.

On 06/19/2014 11:14 AM, Parth Dixit wrote:
> From: parthd <parth.dixit@linaro.org>

This will be use as the commit author. IIRC, we request to have the full
name here.

> 
> Arm based virtual machines dom0/guest will request power related functionality
> from xen through psci interface. This patch implements version 0.2 of

s/psci/PSCI

> PSCI standard specified by arm for 64bit and 32 bit arm machines.
> 
> Signed-off-by: Parth Dixit <parth.dixit@linaro.org>
> ---
>  xen/arch/arm/domain_build.c     |  5 ++-
>  xen/arch/arm/traps.c            | 56 ++++++++++++++++++++++--
>  xen/arch/arm/vpsci.c            | 75 ++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/processor.h |  6 +++
>  xen/include/asm-arm/psci.h      | 18 ++++++++
>  xen/include/public/arch-arm.h   | 95 +++++++++++++++++++++++++++++++++++++++--
>  6 files changed, 246 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c424793..ebd4170 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -388,6 +388,9 @@ static int make_hypervisor_node(struct domain *d,
>  static int make_psci_node(void *fdt, const struct dt_device_node *parent)
>  {
>      int res;
> +    const char compat[] =
> +        "arm,psci-0.2""\0"
> +        "arm,psci";
>  
>      DPRINT("Create PSCI node\n");
>  
> @@ -396,7 +399,7 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent)
>      if ( res )
>          return res;
>  
> -    res = fdt_property_string(fdt, "compatible", "arm,psci");
> +    res = fdt_property(fdt, "compatible", compat, sizeof(compat));
>      if ( res )
>          return res;
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 03a3da6..dc4ff56 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1046,6 +1046,15 @@ typedef struct {
>  static arm_psci_t arm_psci_table[] = {
>      PSCI(cpu_off, 1),
>      PSCI(cpu_on, 2),
> +    PSCI(0_2_psci_version,1),
> +    PSCI(0_2_cpu_suspend,2),
> +    PSCI(0_2_affinity_info,2),
> +    PSCI(0_2_migrate,1),
> +    PSCI(0_2_migrate_info_type,0),
> +    PSCI(0_2_migrate_info_up_cpu,0),
> +    PSCI(0_2_system_off,0),
> +    PSCI(0_2_system_reset,0),
> +
>  };
>  
>  #ifndef NDEBUG
> @@ -1092,14 +1101,53 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  static void do_trap_psci(struct cpu_user_regs *regs)
>  {
>      arm_psci_fn_t psci_call = NULL;
> +    int fn_index = -1;
>  
> -    if ( PSCI_OP_REG(regs) >= ARRAY_SIZE(arm_psci_table) )
> +    switch ( PSCI_OP_REG(regs) )

I don't much like this switch case. Isn't any other solution to get the
fn_index?

>      {
> -        domain_crash_synchronous();
> -        return;
> +        case PSCI_0_2_FN_PSCI_VERSION:
> +            fn_index = PSCI_0_2_psci_version;
> +            break;
> +        case PSCI_0_2_FN_CPU_SUSPEND:
> +        case PSCI_0_2_FN64_CPU_SUSPEND:
> +            fn_index = PSCI_0_2_cpu_suspend;
> +            break;
> +        case PSCI_cpu_off:
> +        case PSCI_0_2_FN_CPU_OFF:
> +            fn_index = PSCI_cpu_off;
> +            break;
> +        case PSCI_cpu_on:
> +        case PSCI_0_2_FN_CPU_ON:
> +        case PSCI_0_2_FN64_CPU_ON:
> +            fn_index = PSCI_cpu_on;

You have to modified the behavior of PSCI_cpu_on. On PSCI v0.2, the
function should return ALREADY_ON, if the vcpu is online.

We don't handle this return value for now.

> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index 1ceb8cb..c1243d4 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -83,6 +83,81 @@ int do_psci_cpu_off(uint32_t power_state)
>      return PSCI_SUCCESS;
>  }
>  
> +int do_psci_0_2_psci_version(uint32_t vcpuid)
> +{
> +    return XEN_PSCI_V_0_2;
> +}
> +
> +int do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    regs->cpsr &= ~PSR_IRQ_MASK;

Why do you clear the IRQ flag here?

> +    vcpu_block();
> +    return PSCI_RET_SUCCESS;
> +}

> +void do_psci_0_2_system_off( void )
> +{
> +    struct domain *d = current->domain;
> +    domain_shutdown(d,0);

Please use SHUTDOWN_poweroff rather than hardcoding 0.

> +}
> +
> +
> +void do_psci_0_2_system_reset(void)
> +{
> +    struct domain *d = current->domain;
> +    domain_shutdown(d,1);

SHUTDOWN_reboot

>  #endif /* __ASM_PSCI_H__ */
>  
>  /*
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 7496556..1663a3e 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h


Most of the defines you added here some belongs to include/asm-arm/psci.h.

The file arch-arm.h, contains define exposed to the guest. Here the
define are only used within the hypervisor. Therefore they should not
belong to this file.

> +/* PSCI return values (inclusive of all PSCI versions) */
> +#define PSCI_RET_SUCCESS			 0
> +#define PSCI_RET_NOT_SUPPORTED			-1
> +#define PSCI_RET_INVALID_PARAMS			-2
> +#define PSCI_RET_DENIED				-3
> +#define PSCI_RET_ALREADY_ON			-4
> +#define PSCI_RET_ON_PENDING			-5
> +#define PSCI_RET_INTERNAL_FAILURE		-6
> +#define PSCI_RET_NOT_PRESENT			-7
> +#define PSCI_RET_DISABLED			-8
> +

Please look at include/asm-arm/psci.h, there is already the definition
of PSCI_{DENIED,SUCCESS,...}.

Regards,

-- 
Julien Grall