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

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

Hi Parth,

Thank you for the patch.

On 06/26/2014 06:26 AM, Parth Dixit wrote:
> +void vcpu_block_event(struct vcpu *v)
> +{
> +    vcpu_block();
> +    /* The ARM spec declares that even if local irqs are masked in
> +    * the CPSR register, an irq should wake up a cpu from WFI anyway.
> +    * For this reason we need to check for irqs that need delivery,
> +    * ignoring the CPSR register, *after* calling SCHEDOP_block to
> +    * avoid races with vgic_vcpu_inject_irq.
> +    */

The comment is out of date here and you should move it on top of the
function.

Also, you've introduce this function but didn't replace the code in WFI
trap.

Furthermore, I would move this change in a separate patch. It will be
more clear.

> +    if ( local_events_need_delivery_nomask() )
> +        vcpu_unblock(current);
> +}
> +

[..]

> +int do_psci_0_2_cpu_off(void)
> +{
> +    uint32_t power_state = 0 ;

Coding style require a newline between the declarations.

Also, defining power_state looks pointless here.

> +    return do_psci_cpu_off(power_state);
> +}
> +
> +int do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point,
> +                       register_t context_id)


This function is 95% the same code. I would merge as much as possible
with do_psci_cpu_on to avoid code duplication.

[..]

> +
> +int do_psci_0_2_migrate(uint32_t target_cpu)
> +{
> +    return PSCI_ENOSYS;
> +}
> +
> +

Only one newline.

> +int do_psci_0_2_migrate_info_type(void)
> +{
> +    return PSCI_0_2_TOS_MP;
> +}
> +
> +

Same here.

> +register_t do_psci_0_2_migrate_info_up_cpu(void)
> +{
> +    return PSCI_ENOSYS;
> +}
> +
> +

Same here.

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

Same here.

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

[..]

>  static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
>  {
> @@ -55,6 +56,7 @@ static inline int arch_virq_is_global(int virq)
>      return 1;
>  }
>  
> +

Spurious change.

> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index 189964b..3487380 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -1,11 +1,6 @@
>  #ifndef __ASM_PSCI_H__
>  #define __ASM_PSCI_H__
>  
> -#define PSCI_SUCCESS  0
> -#define PSCI_ENOSYS  -1
> -#define PSCI_EINVAL  -2
> -#define PSCI_DENIED  -3
> -

Why did you just moved them at the end of the file and change the
indentation?

> +/* PSCI version */
> +#define XEN_PSCI_V_0_1 1

This doesn't seem to be used, right?

> +
> +/* PSCI v0.2 interface */
> +#define PSCI_0_2_FN_BASE			0x84000000
> +#define PSCI_0_2_FN(n)				(PSCI_0_2_FN_BASE + (n))
> +#define PSCI_0_2_64BIT				0x40000000

In general we don't use hard-tab in Xen coding style. Can you remove them?


[..]

> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 7496556..93803e4 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -385,6 +385,7 @@ typedef uint64_t xen_callback_t;
>  #define PSCI_cpu_off     1
>  #define PSCI_cpu_on      2
>  #define PSCI_migrate     3
> +#define PSCI_0_1_MAX     4

Why do you expose PSCI_0_1_MAX?

-- 
Julien Grall