PSCI v0.2 Emulation support in xen ( arm )

PSCI v0.2 Emulation support in xen ( arm )

[Parth Dixit]

This patchest implements PSCI v0.2 standard specified by arm
for invoking power related calls from guest's to hypervisor.
Version 0.1 support is already present in xen 4.4, some new
functions are added by arm for system shutdown/reboot/suspend
etc. We want to add this new functionality while mainating
backward compatibilty with PSCI v0.1.

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

[Parth Dixit]

From: parthd <parth.dixit@linaro.org>

Arm based virtual machines dom0/guest will request power related functionality
from xen through psci interface. This patch implements version 0.2 of
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) )
     {
-        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;
+            break;
+        case PSCI_0_2_FN_AFFINITY_INFO:
+        case PSCI_0_2_FN64_AFFINITY_INFO:
+            fn_index = PSCI_0_2_affinity_info;
+            break;
+        case PSCI_0_2_FN_MIGRATE:
+        case PSCI_0_2_FN64_MIGRATE:
+            fn_index = PSCI_0_2_migrate;
+            break;
+        case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
+            fn_index = PSCI_0_2_migrate_info_type;
+            break;
+        case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
+        case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
+            fn_index = PSCI_0_2_migrate_info_up_cpu;
+            break;
+        case PSCI_0_2_FN_SYSTEM_OFF:
+            fn_index = PSCI_0_2_system_off;
+            break;
+        case PSCI_0_2_FN_SYSTEM_RESET:
+            fn_index = PSCI_0_2_system_reset;
+            break;
+        default:
+            domain_crash_synchronous();
+		    return;
     }
 
-    psci_call = arm_psci_table[PSCI_OP_REG(regs)].fn;
+    psci_call = arm_psci_table[fn_index].fn;
     if ( psci_call == NULL )
     {
         domain_crash_synchronous();
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;
+    vcpu_block();
+    return PSCI_RET_SUCCESS;
+}
+
+int do_psci_0_2_affinity_info(uint32_t target_affinity, uint32_t lowest_affinity_level)
+{
+    unsigned long target_affinity_mask;
+    unsigned int mpidr;
+    struct vcpu *v = current;
+
+    switch ( lowest_affinity_level )
+    {
+    case MPIDR_AFF0_SHIFT:
+    case MPIDR_AFF1_SHIFT:
+    case MPIDR_AFF2_SHIFT:
+        target_affinity_mask
+            = ( MPIDR_HWID_MASK & AFFINITY_MASK( lowest_affinity_level ) );
+        break;
+    case MPIDR_AFF3_SHIFT:
+        target_affinity_mask
+            = ( MPIDR_HWID_MASK & AFFINITY_MASK(lowest_affinity_level+1) );
+        break;
+    default:
+        return PSCI_RET_INVALID_PARAMS;
+    }
+
+    target_affinity &= target_affinity_mask;
+    mpidr = v->arch.vmpidr;
+    if ( ( mpidr & target_affinity_mask ) == target_affinity )
+        return PSCI_0_2_AFFINITY_LEVEL_ON;
+    else
+        return PSCI_0_2_AFFINITY_LEVEL_OFF;
+
+}
+
+int do_psci_0_2_migrate(uint32_t vcpuid)
+{
+    return PSCI_RET_NOT_SUPPORTED;
+}
+
+
+int do_psci_0_2_migrate_info_type(void)
+{
+    return PSCI_0_2_TOS_MP;
+}
+
+
+int do_psci_0_2_migrate_info_up_cpu(void)
+{
+    return PSCI_RET_NOT_SUPPORTED;
+}
+
+
+void do_psci_0_2_system_off( void )
+{
+    struct domain *d = current->domain;
+    domain_shutdown(d,0);
+}
+
+
+void do_psci_0_2_system_reset(void)
+{
+    struct domain *d = current->domain;
+    domain_shutdown(d,1);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 9267c1b..20f02d5 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -13,9 +13,15 @@
 #define _MPIDR_SMP          (31)
 #define MPIDR_SMP           (_AC(1,U) << _MPIDR_SMP)
 #define MPIDR_AFF0_SHIFT    (0)
+#define MPIDR_AFF1_SHIFT    (1)
+#define MPIDR_AFF2_SHIFT    (2)
+#define MPIDR_AFF3_SHIFT    (3)
 #define MPIDR_AFF0_MASK     (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
 #define MPIDR_HWID_MASK     _AC(0xffffff,U)
 #define MPIDR_INVALID       (~MPIDR_HWID_MASK)
+#define MPIDR_AFF_BIT_SHIFT (8)
+#define AFFINITY_MASK(level) (_AC(0xff,U) << ((level)*MPIDR_AFF_BIT_SHIFT)) 
+
 
 /* TTBCR Translation Table Base Control Register */
 #define TTBCR_EAE    _AC(0x80000000,U)
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 189964b..dbaa885 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -18,6 +18,24 @@ int do_psci_cpu_off(uint32_t power_state);
 int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
 int do_psci_migrate(uint32_t vcpuid);
 
+/* PSCI 0.2 functions to handle guest PSCI requests */
+int do_psci_0_2_psci_version(uint32_t vcpuid);
+int do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point);
+int do_psci_0_2_cpu_off(uint32_t power_state);
+int do_psci_0_2_cpu_on(uint32_t vcpuid, register_t entry_point);
+int do_psci_0_2_affinity_info(uint32_t target_affinity, uint32_t lowest_affinity_level);
+int do_psci_0_2_migrate(uint32_t vcpuid);
+int do_psci_0_2_migrate_info_type(void);
+int do_psci_0_2_migrate_info_up_cpu(void);
+void do_psci_0_2_system_off(void);
+void do_psci_0_2_system_reset(void);
+int do_psci_0_2_fn64_cpu_suspend(uint32_t power_state, uint32_t entry_point);
+int do_psci_0_2_fn64_cpu_on(uint32_t vcpuid, register_t entry_point);
+int do_psci_0_2_fn64_affinity_info(uint32_t target_affinity, uint32_t lowest_affinity_level);
+int do_psci_0_2_fn64_migrate(uint32_t vcpuid);
+int do_psci_0_2_fn64_migrate_info_up_cpu(void);
+
+
 #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
@@ -380,11 +380,98 @@ typedef uint64_t xen_callback_t;
 #define GUEST_TIMER_PHYS_NS_PPI 30
 #define GUEST_EVTCHN_PPI        31
 
+/* PSCI version */
+#define XEN_PSCI_V_0_1 1
+#define XEN_PSCI_V_0_2 2
+
 /* PSCI functions */
-#define PSCI_cpu_suspend 0
-#define PSCI_cpu_off     1
-#define PSCI_cpu_on      2
-#define PSCI_migrate     3
+#define PSCI_migrate     17
+#define PSCI_cpu_suspend 18
+#define PSCI_cpu_off     0
+#define PSCI_cpu_on      1
+
+#define PSCI_0_2_psci_version             2
+#define PSCI_0_2_cpu_suspend              3
+#define PSCI_0_2_affinity_info            4
+#define PSCI_0_2_migrate                  5
+#define PSCI_0_2_migrate_info_type        6
+#define PSCI_0_2_migrate_info_up_cpu      7
+#define PSCI_0_2_system_off               8
+#define PSCI_0_2_system_reset             9
+#define PSCI_0_2_cpu_on                   10
+#define PSCI_0_2_cpu_off                  11
+#define PSCI_0_2_fn64_cpu_suspend         12
+#define PSCI_0_2_fn64_cpu_on              13
+#define PSCI_0_2_fn64_affinity_info       14
+#define PSCI_0_2_fn64_migrate             15
+#define PSCI_0_2_fn64_migrate_info_up_cpu 16
+
+/* 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
+#define PSCI_0_2_FN64_BASE			\
+					(PSCI_0_2_FN_BASE + PSCI_0_2_64BIT)
+#define PSCI_0_2_FN64(n)			(PSCI_0_2_FN64_BASE + (n))
+
+#define PSCI_0_2_FN_PSCI_VERSION		PSCI_0_2_FN(0)
+#define PSCI_0_2_FN_CPU_SUSPEND			PSCI_0_2_FN(1)
+#define PSCI_0_2_FN_CPU_OFF			PSCI_0_2_FN(2)
+#define PSCI_0_2_FN_CPU_ON			PSCI_0_2_FN(3)
+#define PSCI_0_2_FN_AFFINITY_INFO		PSCI_0_2_FN(4)
+#define PSCI_0_2_FN_MIGRATE			PSCI_0_2_FN(5)
+#define PSCI_0_2_FN_MIGRATE_INFO_TYPE		PSCI_0_2_FN(6)
+#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU		PSCI_0_2_FN(7)
+#define PSCI_0_2_FN_SYSTEM_OFF			PSCI_0_2_FN(8)
+#define PSCI_0_2_FN_SYSTEM_RESET		PSCI_0_2_FN(9)
+
+#define PSCI_0_2_FN64_CPU_SUSPEND		PSCI_0_2_FN64(1)
+#define PSCI_0_2_FN64_CPU_ON			PSCI_0_2_FN64(3)
+#define PSCI_0_2_FN64_AFFINITY_INFO		PSCI_0_2_FN64(4)
+#define PSCI_0_2_FN64_MIGRATE			PSCI_0_2_FN64(5)
+#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU	PSCI_0_2_FN64(7)
+
+/* PSCI v0.2 power state encoding for CPU_SUSPEND function */
+#define PSCI_0_2_POWER_STATE_ID_MASK		0xffff
+#define PSCI_0_2_POWER_STATE_ID_SHIFT		0
+#define PSCI_0_2_POWER_STATE_TYPE_SHIFT		16
+#define PSCI_0_2_POWER_STATE_TYPE_MASK		\
+				(0x1 << PSCI_0_2_POWER_STATE_TYPE_SHIFT)
+#define PSCI_0_2_POWER_STATE_AFFL_SHIFT		24
+#define PSCI_0_2_POWER_STATE_AFFL_MASK		\
+				(0x3 << PSCI_0_2_POWER_STATE_AFFL_SHIFT)
+
+/* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
+#define PSCI_0_2_AFFINITY_LEVEL_ON		0
+#define PSCI_0_2_AFFINITY_LEVEL_OFF		1
+#define PSCI_0_2_AFFINITY_LEVEL_ON_PENDING	2
+
+/* PSCI v0.2 multicore support in Trusted OS returned by MIGRATE_INFO_TYPE */
+#define PSCI_0_2_TOS_UP_MIGRATE			0
+#define PSCI_0_2_TOS_UP_NO_MIGRATE		1
+#define PSCI_0_2_TOS_MP				2
+
+/* PSCI version decoding (independent of PSCI version) */
+#define PSCI_VERSION_MAJOR_SHIFT		16
+#define PSCI_VERSION_MINOR_MASK			\
+		((1U << PSCI_VERSION_MAJOR_SHIFT) - 1)
+#define PSCI_VERSION_MAJOR_MASK			~PSCI_VERSION_MINOR_MASK
+#define PSCI_VERSION_MAJOR(ver)			\
+		(((ver) & PSCI_VERSION_MAJOR_MASK) >> PSCI_VERSION_MAJOR_SHIFT)
+#define PSCI_VERSION_MINOR(ver)			\
+		((ver) & PSCI_VERSION_MINOR_MASK)
+
+/* 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
+
 
 #endif
 
-- 
1.9.1

Re: PSCI v0.2 Emulation support in xen ( arm )

[Julien Grall]

Hi Parth,

On 06/19/2014 11:14 AM, Parth Dixit wrote:
> This patchest implements PSCI v0.2 standard specified by arm
> for invoking power related calls from guest's to hypervisor.
> Version 0.1 support is already present in xen 4.4, some new
> functions are added by arm for system shutdown/reboot/suspend
> etc. We want to add this new functionality while mainating
> backward compatibilty with PSCI v0.1.

Thank you for this series.

We use to cc the relevant maintainers on every patch send to the mailing
list.

There is a wiki page [1] explaining how to submit a patch on Xen devel.

Regards,

[1]http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Cc_the_maintainer_of_the_code_you_are_modifying

-- 
Julien Grall

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

[Julien Grall]

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

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

[Stefano Stabellini]

On Thu, 19 Jun 2014, Parth Dixit wrote:
> From: parthd <parth.dixit@linaro.org>
> 
> Arm based virtual machines dom0/guest will request power related functionality
> from xen through psci interface. This patch implements version 0.2 of
> 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;
>  

Even though you are adding the psci-0.2 compatible string, I don't see
the new PSCI_0_2_FN_* function numbers being exposed to the guest yet.


> 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) )
>      {
> -        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;
> +            break;
> +        case PSCI_0_2_FN_AFFINITY_INFO:
> +        case PSCI_0_2_FN64_AFFINITY_INFO:
> +            fn_index = PSCI_0_2_affinity_info;
> +            break;
> +        case PSCI_0_2_FN_MIGRATE:
> +        case PSCI_0_2_FN64_MIGRATE:
> +            fn_index = PSCI_0_2_migrate;
> +            break;
> +        case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> +            fn_index = PSCI_0_2_migrate_info_type;
> +            break;
> +        case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
> +        case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> +            fn_index = PSCI_0_2_migrate_info_up_cpu;
> +            break;
> +        case PSCI_0_2_FN_SYSTEM_OFF:
> +            fn_index = PSCI_0_2_system_off;
> +            break;
> +        case PSCI_0_2_FN_SYSTEM_RESET:
> +            fn_index = PSCI_0_2_system_reset;
> +            break;
> +        default:
> +            domain_crash_synchronous();
> +		    return;
>      }
>  
> -    psci_call = arm_psci_table[PSCI_OP_REG(regs)].fn;
> +    psci_call = arm_psci_table[fn_index].fn;
>      if ( psci_call == NULL )
>      {
>          domain_crash_synchronous();
> 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;
>  }

You haven't modified do_psci_cpu_on, but the current implementation
takes a 32bit vcpu id instead of a register_t to specify the target cpu.
Also do_psci_cpu_on doesn't currently take a context_id as parameter.


> +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;
> +    vcpu_block();
> +    return PSCI_RET_SUCCESS;
> +}
> +
> +int do_psci_0_2_affinity_info(uint32_t target_affinity, uint32_t lowest_affinity_level)
> +{
> +    unsigned long target_affinity_mask;
> +    unsigned int mpidr;
> +    struct vcpu *v = current;
> +
> +    switch ( lowest_affinity_level )
> +    {
> +    case MPIDR_AFF0_SHIFT:
> +    case MPIDR_AFF1_SHIFT:
> +    case MPIDR_AFF2_SHIFT:
> +        target_affinity_mask
> +            = ( MPIDR_HWID_MASK & AFFINITY_MASK( lowest_affinity_level ) );
> +        break;
> +    case MPIDR_AFF3_SHIFT:
> +        target_affinity_mask
> +            = ( MPIDR_HWID_MASK & AFFINITY_MASK(lowest_affinity_level+1) );
> +        break;
> +    default:
> +        return PSCI_RET_INVALID_PARAMS;
> +    }
> +
> +    target_affinity &= target_affinity_mask;
> +    mpidr = v->arch.vmpidr;
> +    if ( ( mpidr & target_affinity_mask ) == target_affinity )
> +        return PSCI_0_2_AFFINITY_LEVEL_ON;
> +    else
> +        return PSCI_0_2_AFFINITY_LEVEL_OFF;
> +
> +}
> +
> +int do_psci_0_2_migrate(uint32_t vcpuid)
> +{
> +    return PSCI_RET_NOT_SUPPORTED;
> +}
> +
> +
> +int do_psci_0_2_migrate_info_type(void)
> +{
> +    return PSCI_0_2_TOS_MP;
> +}
> +
> +
> +int do_psci_0_2_migrate_info_up_cpu(void)
> +{
> +    return PSCI_RET_NOT_SUPPORTED;
> +}
> +
> +
> +void do_psci_0_2_system_off( void )

code style


> +{
> +    struct domain *d = current->domain;
> +    domain_shutdown(d,0);
> +}
> +
> +
> +void do_psci_0_2_system_reset(void)
> +{
> +    struct domain *d = current->domain;
> +    domain_shutdown(d,1);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 9267c1b..20f02d5 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -13,9 +13,15 @@
>  #define _MPIDR_SMP          (31)
>  #define MPIDR_SMP           (_AC(1,U) << _MPIDR_SMP)
>  #define MPIDR_AFF0_SHIFT    (0)
> +#define MPIDR_AFF1_SHIFT    (1)
> +#define MPIDR_AFF2_SHIFT    (2)
> +#define MPIDR_AFF3_SHIFT    (3)
>  #define MPIDR_AFF0_MASK     (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
>  #define MPIDR_HWID_MASK     _AC(0xffffff,U)
>  #define MPIDR_INVALID       (~MPIDR_HWID_MASK)
> +#define MPIDR_AFF_BIT_SHIFT (8)
> +#define AFFINITY_MASK(level) (_AC(0xff,U) << ((level)*MPIDR_AFF_BIT_SHIFT)) 
> +
>  
>  /* TTBCR Translation Table Base Control Register */
>  #define TTBCR_EAE    _AC(0x80000000,U)
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index 189964b..dbaa885 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -18,6 +18,24 @@ int do_psci_cpu_off(uint32_t power_state);
>  int do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
>  int do_psci_migrate(uint32_t vcpuid);
>  
> +/* PSCI 0.2 functions to handle guest PSCI requests */
> +int do_psci_0_2_psci_version(uint32_t vcpuid);
> +int do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point);
> +int do_psci_0_2_cpu_off(uint32_t power_state);
> +int do_psci_0_2_cpu_on(uint32_t vcpuid, register_t entry_point);
> +int do_psci_0_2_affinity_info(uint32_t target_affinity, uint32_t lowest_affinity_level);
> +int do_psci_0_2_migrate(uint32_t vcpuid);
> +int do_psci_0_2_migrate_info_type(void);
> +int do_psci_0_2_migrate_info_up_cpu(void);
> +void do_psci_0_2_system_off(void);
> +void do_psci_0_2_system_reset(void);
> +int do_psci_0_2_fn64_cpu_suspend(uint32_t power_state, uint32_t entry_point);
> +int do_psci_0_2_fn64_cpu_on(uint32_t vcpuid, register_t entry_point);
> +int do_psci_0_2_fn64_affinity_info(uint32_t target_affinity, uint32_t lowest_affinity_level);
> +int do_psci_0_2_fn64_migrate(uint32_t vcpuid);
> +int do_psci_0_2_fn64_migrate_info_up_cpu(void);
> +
> +
>  #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
> @@ -380,11 +380,98 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_TIMER_PHYS_NS_PPI 30
>  #define GUEST_EVTCHN_PPI        31
>  
> +/* PSCI version */
> +#define XEN_PSCI_V_0_1 1
> +#define XEN_PSCI_V_0_2 2
> +
>  /* PSCI functions */
> -#define PSCI_cpu_suspend 0
> -#define PSCI_cpu_off     1
> -#define PSCI_cpu_on      2
> -#define PSCI_migrate     3
> +#define PSCI_migrate     17
> +#define PSCI_cpu_suspend 18
> +#define PSCI_cpu_off     0
> +#define PSCI_cpu_on      1

Interestingly this is going to change the existing function numbers.
Linux reads them from device tree so that should be OK. It is definitely
worth noting that you are making this change in the commit message.


> +#define PSCI_0_2_psci_version             2
> +#define PSCI_0_2_cpu_suspend              3
> +#define PSCI_0_2_affinity_info            4
> +#define PSCI_0_2_migrate                  5
> +#define PSCI_0_2_migrate_info_type        6
> +#define PSCI_0_2_migrate_info_up_cpu      7
> +#define PSCI_0_2_system_off               8
> +#define PSCI_0_2_system_reset             9
> +#define PSCI_0_2_cpu_on                   10
> +#define PSCI_0_2_cpu_off                  11
> +#define PSCI_0_2_fn64_cpu_suspend         12
> +#define PSCI_0_2_fn64_cpu_on              13
> +#define PSCI_0_2_fn64_affinity_info       14
> +#define PSCI_0_2_fn64_migrate             15
> +#define PSCI_0_2_fn64_migrate_info_up_cpu 16

These are just internal between traps.c and vpsci.c, right?
You should write that down.


> +/* 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
> +#define PSCI_0_2_FN64_BASE			\
> +					(PSCI_0_2_FN_BASE + PSCI_0_2_64BIT)
> +#define PSCI_0_2_FN64(n)			(PSCI_0_2_FN64_BASE + (n))
> +
> +#define PSCI_0_2_FN_PSCI_VERSION		PSCI_0_2_FN(0)
> +#define PSCI_0_2_FN_CPU_SUSPEND			PSCI_0_2_FN(1)
> +#define PSCI_0_2_FN_CPU_OFF			PSCI_0_2_FN(2)
> +#define PSCI_0_2_FN_CPU_ON			PSCI_0_2_FN(3)
> +#define PSCI_0_2_FN_AFFINITY_INFO		PSCI_0_2_FN(4)
> +#define PSCI_0_2_FN_MIGRATE			PSCI_0_2_FN(5)
> +#define PSCI_0_2_FN_MIGRATE_INFO_TYPE		PSCI_0_2_FN(6)
> +#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU		PSCI_0_2_FN(7)
> +#define PSCI_0_2_FN_SYSTEM_OFF			PSCI_0_2_FN(8)
> +#define PSCI_0_2_FN_SYSTEM_RESET		PSCI_0_2_FN(9)
> +
> +#define PSCI_0_2_FN64_CPU_SUSPEND		PSCI_0_2_FN64(1)
> +#define PSCI_0_2_FN64_CPU_ON			PSCI_0_2_FN64(3)
> +#define PSCI_0_2_FN64_AFFINITY_INFO		PSCI_0_2_FN64(4)
> +#define PSCI_0_2_FN64_MIGRATE			PSCI_0_2_FN64(5)
> +#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU	PSCI_0_2_FN64(7)
> +
> +/* PSCI v0.2 power state encoding for CPU_SUSPEND function */
> +#define PSCI_0_2_POWER_STATE_ID_MASK		0xffff
> +#define PSCI_0_2_POWER_STATE_ID_SHIFT		0
> +#define PSCI_0_2_POWER_STATE_TYPE_SHIFT		16
> +#define PSCI_0_2_POWER_STATE_TYPE_MASK		\
> +				(0x1 << PSCI_0_2_POWER_STATE_TYPE_SHIFT)
> +#define PSCI_0_2_POWER_STATE_AFFL_SHIFT		24
> +#define PSCI_0_2_POWER_STATE_AFFL_MASK		\
> +				(0x3 << PSCI_0_2_POWER_STATE_AFFL_SHIFT)
> +
> +/* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
> +#define PSCI_0_2_AFFINITY_LEVEL_ON		0
> +#define PSCI_0_2_AFFINITY_LEVEL_OFF		1
> +#define PSCI_0_2_AFFINITY_LEVEL_ON_PENDING	2
> +
> +/* PSCI v0.2 multicore support in Trusted OS returned by MIGRATE_INFO_TYPE */
> +#define PSCI_0_2_TOS_UP_MIGRATE			0
> +#define PSCI_0_2_TOS_UP_NO_MIGRATE		1
> +#define PSCI_0_2_TOS_MP				2
> +
> +/* PSCI version decoding (independent of PSCI version) */
> +#define PSCI_VERSION_MAJOR_SHIFT		16
> +#define PSCI_VERSION_MINOR_MASK			\
> +		((1U << PSCI_VERSION_MAJOR_SHIFT) - 1)
> +#define PSCI_VERSION_MAJOR_MASK			~PSCI_VERSION_MINOR_MASK
> +#define PSCI_VERSION_MAJOR(ver)			\
> +		(((ver) & PSCI_VERSION_MAJOR_MASK) >> PSCI_VERSION_MAJOR_SHIFT)
> +#define PSCI_VERSION_MINOR(ver)			\
> +		((ver) & PSCI_VERSION_MINOR_MASK)
> +
> +/* 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

Be careful: at the moment the functions in vpsci.c return the error
codes specified in xen/include/asm-arm/psci.h.
We should define the PSCI return codes only in one place.

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

[Julien Grall]

On 06/19/2014 05:00 PM, Stefano Stabellini wrote:
> On Thu, 19 Jun 2014, Parth Dixit wrote:
>> From: parthd <parth.dixit@linaro.org>
>>
>> Arm based virtual machines dom0/guest will request power related functionality
>> from xen through psci interface. This patch implements version 0.2 of
>> 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;
>>  
> 
> Even though you are adding the psci-0.2 compatible string, I don't see
> the new PSCI_0_2_FN_* function numbers being exposed to the guest yet.

These function numbers are defined by the spec. There is no need to
expose to the guest.

IIRC, the DT binding for PSCI v0.2 only contain the method to call psci
and the compatible string.

>> +/* 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
> 
> Be careful: at the moment the functions in vpsci.c return the error
> codes specified in xen/include/asm-arm/psci.h.
> We should define the PSCI return codes only in one place.

It looks like this has been cut & paste from Linux patch (see
http://www.spinics.net/lists/arm-kernel/msg319712.html).

But this should not be exposed to neither the toolstack nor the guest.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 19 Jun 2014, Julien Grall wrote:
> On 06/19/2014 05:00 PM, Stefano Stabellini wrote:
> > On Thu, 19 Jun 2014, Parth Dixit wrote:
> >> From: parthd <parth.dixit@linaro.org>
> >>
> >> Arm based virtual machines dom0/guest will request power related functionality
> >> from xen through psci interface. This patch implements version 0.2 of
> >> 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;
> >>  
> > 
> > Even though you are adding the psci-0.2 compatible string, I don't see
> > the new PSCI_0_2_FN_* function numbers being exposed to the guest yet.
> 
> These function numbers are defined by the spec. There is no need to
> expose to the guest.

I disagree, it makes things clearer leaving less margin for errors.


> IIRC, the DT binding for PSCI v0.2 only contain the method to call psci
> and the compatible string.

That is not what the spec uses as example of DT bindings at page 46,
chapter 5.12.


> >> +/* 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
> > 
> > Be careful: at the moment the functions in vpsci.c return the error
> > codes specified in xen/include/asm-arm/psci.h.
> > We should define the PSCI return codes only in one place.
> 
> It looks like this has been cut & paste from Linux patch (see
> http://www.spinics.net/lists/arm-kernel/msg319712.html).
> 
> But this should not be exposed to neither the toolstack nor the guest.

Indeed

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

[Julien Grall]

On 19/06/14 18:22, Stefano Stabellini wrote:
> On Thu, 19 Jun 2014, Julien Grall wrote:
>> On 06/19/2014 05:00 PM, Stefano Stabellini wrote:
>>> On Thu, 19 Jun 2014, Parth Dixit wrote:
>>>> From: parthd <parth.dixit@linaro.org>
>>>>
>>>> Arm based virtual machines dom0/guest will request power related functionality
>>>> from xen through psci interface. This patch implements version 0.2 of
>>>> 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;
>>>>
>>>
>>> Even though you are adding the psci-0.2 compatible string, I don't see
>>> the new PSCI_0_2_FN_* function numbers being exposed to the guest yet.
>>
>> These function numbers are defined by the spec. There is no need to
>> expose to the guest.
>
> I disagree, it makes things clearer leaving less margin for errors.

The PSCI spec clearly define standard values (see every functions 
description in the doc). Those values won't change in the PSCI 0.2 version.

The properties function = <id> should only be created if we want PSCI 
0.1 use them.

That made me think, I don't think we need to have specific values for 
PSCI 0.1 function, unless they are not compatible.

>
>> IIRC, the DT binding for PSCI v0.2 only contain the method to call psci
>> and the compatible string.
>
> That is not what the spec uses as example of DT bindings at page 46,
> chapter 5.12.

The properties function = <id> will only be used by guest support only 
PSCI 0.1. I suspect the spec try to make the device node compliant for 
both version (see the compatible string).

Even tho it's not yet merged, it looks like Linux will go to the 
following binding:

http://www.spinics.net/lists/kvm/msg103313.html

Regards,

-- 
Julien Grall

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

[Christoffer Dall]

On 19 June 2014 20:02, Julien Grall <julien.grall@linaro.org> wrote:
>
>
> On 19/06/14 18:22, Stefano Stabellini wrote:
>>
>> On Thu, 19 Jun 2014, Julien Grall wrote:
>>>
>>> On 06/19/2014 05:00 PM, Stefano Stabellini wrote:
>>>>
>>>> On Thu, 19 Jun 2014, Parth Dixit wrote:
>>>>>
>>>>> From: parthd <parth.dixit@linaro.org>
>>>>>
>>>>> Arm based virtual machines dom0/guest will request power related
>>>>> functionality
>>>>> from xen through psci interface. This patch implements version 0.2 of
>>>>> 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;
>>>>>
>>>>
>>>> Even though you are adding the psci-0.2 compatible string, I don't see
>>>> the new PSCI_0_2_FN_* function numbers being exposed to the guest yet.
>>>
>>>
>>> These function numbers are defined by the spec. There is no need to
>>> expose to the guest.
>>
>>
>> I disagree, it makes things clearer leaving less margin for errors.
>
>
> The PSCI spec clearly define standard values (see every functions
> description in the doc). Those values won't change in the PSCI 0.2 version.
>
> The properties function = <id> should only be created if we want PSCI 0.1
> use them.
>
> That made me think, I don't think we need to have specific values for PSCI
> 0.1 function, unless they are not compatible.
>
>
>>
>>> IIRC, the DT binding for PSCI v0.2 only contain the method to call psci
>>> and the compatible string.
>>
>>
>> That is not what the spec uses as example of DT bindings at page 46,
>> chapter 5.12.
>
>
> The properties function = <id> will only be used by guest support only PSCI
> 0.1. I suspect the spec try to make the device node compliant for both
> version (see the compatible string).
>
> Even tho it's not yet merged, it looks like Linux will go to the following
> binding:
>
> http://www.spinics.net/lists/kvm/msg103313.html
>
It's merged in 3.16-rc1.

And yes, the idea is to have a single device tree node, and then psci
v0.1 only kernels can still use psci v0.1, and psci v0.2 can ignore
the function IDs and use the ones defined as per the spec.

-Christoffer

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

[Stefano Stabellini]

On Thu, 19 Jun 2014, Christoffer Dall wrote:
> On 19 June 2014 20:02, Julien Grall <julien.grall@linaro.org> wrote:
> >
> >
> > On 19/06/14 18:22, Stefano Stabellini wrote:
> >>
> >> On Thu, 19 Jun 2014, Julien Grall wrote:
> >>>
> >>> On 06/19/2014 05:00 PM, Stefano Stabellini wrote:
> >>>>
> >>>> On Thu, 19 Jun 2014, Parth Dixit wrote:
> >>>>>
> >>>>> From: parthd <parth.dixit@linaro.org>
> >>>>>
> >>>>> Arm based virtual machines dom0/guest will request power related
> >>>>> functionality
> >>>>> from xen through psci interface. This patch implements version 0.2 of
> >>>>> 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;
> >>>>>
> >>>>
> >>>> Even though you are adding the psci-0.2 compatible string, I don't see
> >>>> the new PSCI_0_2_FN_* function numbers being exposed to the guest yet.
> >>>
> >>>
> >>> These function numbers are defined by the spec. There is no need to
> >>> expose to the guest.
> >>
> >>
> >> I disagree, it makes things clearer leaving less margin for errors.
> >
> >
> > The PSCI spec clearly define standard values (see every functions
> > description in the doc). Those values won't change in the PSCI 0.2 version.
> >
> > The properties function = <id> should only be created if we want PSCI 0.1
> > use them.
> >
> > That made me think, I don't think we need to have specific values for PSCI
> > 0.1 function, unless they are not compatible.
> >
> >
> >>
> >>> IIRC, the DT binding for PSCI v0.2 only contain the method to call psci
> >>> and the compatible string.
> >>
> >>
> >> That is not what the spec uses as example of DT bindings at page 46,
> >> chapter 5.12.
> >
> >
> > The properties function = <id> will only be used by guest support only PSCI
> > 0.1. I suspect the spec try to make the device node compliant for both
> > version (see the compatible string).
> >
> > Even tho it's not yet merged, it looks like Linux will go to the following
> > binding:
> >
> > http://www.spinics.net/lists/kvm/msg103313.html
> >
> It's merged in 3.16-rc1.
> 
> And yes, the idea is to have a single device tree node, and then psci
> v0.1 only kernels can still use psci v0.1, and psci v0.2 can ignore
> the function IDs and use the ones defined as per the spec.

OK. From the Xen POV we need to expose a psci node compatible with both
psci-0.2 and psci, because we want 3.15 to be able to boot with multiple
vcpus on Xen 4.5.

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

[Julien Grall]

On 20/06/14 12:48, Stefano Stabellini wrote:
>> And yes, the idea is to have a single device tree node, and then psci
>> v0.1 only kernels can still use psci v0.1, and psci v0.2 can ignore
>> the function IDs and use the ones defined as per the spec.
>
> OK. From the Xen POV we need to expose a psci node compatible with both
> psci-0.2 and psci, because we want 3.15 to be able to boot with multiple
> vcpus on Xen 4.5.
>

AFAIU parth's patch, this is already possible. He doesn't add the 
properties such as suspend because we don't support them on Xen for PSCI 
v0.1

-- 
Julien Grall

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

[Parth Dixit]

[-- Attachment #1.1: Type: text/plain, Size: 1390 bytes --]

Next version of my patch is ready except for following things on which i
need your suggestion

1. Exposing PSCI v0.2 functions in device tree - This was not done because
it gives the impression that you can modify the function id's and kernel
will call the function id's based on function id's exposed in device tree
whereas kernel ignores it for PSCI v0.2 while it follows it for PSCI v0.1
which can be confusing. Either way is fine with me.
2.  Why do you clear the IRQ flag in psci_suspend - I am taking cue from
the "vcpu_block_enable_events" in xen/common/schedule.c where flag is
cleared to enable interrupts before pausing the cpu.

I have taken care of rest of the comments.

Regards
parth


On 20 June 2014 17:32, Julien Grall <julien.grall@linaro.org> wrote:

>
>
> On 20/06/14 12:48, Stefano Stabellini wrote:
>
>> And yes, the idea is to have a single device tree node, and then psci
>>> v0.1 only kernels can still use psci v0.1, and psci v0.2 can ignore
>>> the function IDs and use the ones defined as per the spec.
>>>
>>
>> OK. From the Xen POV we need to expose a psci node compatible with both
>> psci-0.2 and psci, because we want 3.15 to be able to boot with multiple
>> vcpus on Xen 4.5.
>>
>>
> AFAIU parth's patch, this is already possible. He doesn't add the
> properties such as suspend because we don't support them on Xen for PSCI
> v0.1
>
> --
> Julien Grall
>

[-- Attachment #1.2: Type: text/html, Size: 2169 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Christoffer Dall]

On 23 June 2014 07:28, Parth Dixit <parth.dixit@linaro.org> wrote:
> Next version of my patch is ready except for following things on which i
> need your suggestion
>
> 1. Exposing PSCI v0.2 functions in device tree - This was not done because
> it gives the impression that you can modify the function id's and kernel
> will call the function id's based on function id's exposed in device tree
> whereas kernel ignores it for PSCI v0.2 while it follows it for PSCI v0.1
> which can be confusing. Either way is fine with me.

I don't see why this has become such a large issue of debate; the
device tree is a stable ABI, we have agreed on the bindings for PSCI
v0.2:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/arm/psci.txt

Just provide the IDs that apply for PSCI v0.1 so older kernels will
still work with Xen - newer versions of Linux will ignore the function
IDs and use the ones defined in the spec.

-Christoffer

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

[Stefano Stabellini]

On Mon, 23 Jun 2014, Christoffer Dall wrote:
> On 23 June 2014 07:28, Parth Dixit <parth.dixit@linaro.org> wrote:
> > Next version of my patch is ready except for following things on which i
> > need your suggestion
> >
> > 1. Exposing PSCI v0.2 functions in device tree - This was not done because
> > it gives the impression that you can modify the function id's and kernel
> > will call the function id's based on function id's exposed in device tree
> > whereas kernel ignores it for PSCI v0.2 while it follows it for PSCI v0.1
> > which can be confusing. Either way is fine with me.
> 
> I don't see why this has become such a large issue of debate; the
> device tree is a stable ABI, we have agreed on the bindings for PSCI
> v0.2:
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/arm/psci.txt
> 
> Just provide the IDs that apply for PSCI v0.1 so older kernels will
> still work with Xen - newer versions of Linux will ignore the function
> IDs and use the ones defined in the spec.

Right

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On Mon, 23 Jun 2014, Parth Dixit wrote:
> Next version of my patch is ready except for following things on which i need your suggestion
> 1. Exposing PSCI v0.2 functions in device tree - This was not done because it gives the impression that you can modify the function id's
> and kernel will call the function id's based on function id's exposed in device tree whereas kernel ignores it for PSCI v0.2 while it
> follows it for PSCI v0.1 which can be confusing. Either way is fine with me.
> 2.  Why do you clear the IRQ flag in psci_suspend - I am taking cue from the "vcpu_block_enable_events" in xen/common/schedule.c where
> flag is cleared to enable interrupts before pausing the cpu.

Keep in mind that vcpu_block_enable_events is common code, while
local_event_delivery_enable is the arm specific implementation.

In the arm case local_event_delivery_enable is implemented by clearing
PSR_IRQ_MASK because effectively that's what is needed to enable event
delivery. Events are just a Xen specific kind of interrupts.

vcpu_block_enable_events calls local_event_delivery_enable before
blocking a vcpu, to make sure it can wake the vcpu up if an event needs
to be delivered to it.

We need to clear PSR_IRQ_MASK because the CPU_SUSPEND call "is intended
for use in idle subsystems where the core is expected to return to
execution through a wake up event". The vcpu is never going to come up
again if we don't clear PSR_IRQ_MASK, because events wouldn't be
delivered to it.

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Julien Grall]

On 06/23/2014 11:40 AM, Stefano Stabellini wrote:
> On Mon, 23 Jun 2014, Parth Dixit wrote:
>> Next version of my patch is ready except for following things on which i need your suggestion
>> 1. Exposing PSCI v0.2 functions in device tree - This was not done because it gives the impression that you can modify the function id's
>> and kernel will call the function id's based on function id's exposed in device tree whereas kernel ignores it for PSCI v0.2 while it
>> follows it for PSCI v0.1 which can be confusing. Either way is fine with me.
>> 2.  Why do you clear the IRQ flag in psci_suspend - I am taking cue from the "vcpu_block_enable_events" in xen/common/schedule.c where
>> flag is cleared to enable interrupts before pausing the cpu.
> 
> Keep in mind that vcpu_block_enable_events is common code, while
> local_event_delivery_enable is the arm specific implementation.
> 
> In the arm case local_event_delivery_enable is implemented by clearing
> PSR_IRQ_MASK because effectively that's what is needed to enable event
> delivery. Events are just a Xen specific kind of interrupts.
> 
> vcpu_block_enable_events calls local_event_delivery_enable before
> blocking a vcpu, to make sure it can wake the vcpu up if an event needs
> to be delivered to it.
> 
> We need to clear PSR_IRQ_MASK because the CPU_SUSPEND call "is intended
> for use in idle subsystems where the core is expected to return to
> execution through a wake up event". The vcpu is never going to come up
> again if we don't clear PSR_IRQ_MASK, because events wouldn't be
> delivered to it.

With this solution Xen will return into the guest with IRQ enable
unconditionally.

I don't see anything in the specification that allow a such change. So
the guest may assume that the IRQs are still disabled. This would break it.

Couldn't we use the same trick as WFI ie:

vcpu_block();
if ( local_events_delivery_nomask() )
  vcpu_unblock(current);

It might be better to introduce a new helper for this purpose.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Mon, 23 Jun 2014, Julien Grall wrote:
> On 06/23/2014 11:40 AM, Stefano Stabellini wrote:
> > On Mon, 23 Jun 2014, Parth Dixit wrote:
> >> Next version of my patch is ready except for following things on which i need your suggestion
> >> 1. Exposing PSCI v0.2 functions in device tree - This was not done because it gives the impression that you can modify the function id's
> >> and kernel will call the function id's based on function id's exposed in device tree whereas kernel ignores it for PSCI v0.2 while it
> >> follows it for PSCI v0.1 which can be confusing. Either way is fine with me.
> >> 2.  Why do you clear the IRQ flag in psci_suspend - I am taking cue from the "vcpu_block_enable_events" in xen/common/schedule.c where
> >> flag is cleared to enable interrupts before pausing the cpu.
> > 
> > Keep in mind that vcpu_block_enable_events is common code, while
> > local_event_delivery_enable is the arm specific implementation.
> > 
> > In the arm case local_event_delivery_enable is implemented by clearing
> > PSR_IRQ_MASK because effectively that's what is needed to enable event
> > delivery. Events are just a Xen specific kind of interrupts.
> > 
> > vcpu_block_enable_events calls local_event_delivery_enable before
> > blocking a vcpu, to make sure it can wake the vcpu up if an event needs
> > to be delivered to it.
> > 
> > We need to clear PSR_IRQ_MASK because the CPU_SUSPEND call "is intended
> > for use in idle subsystems where the core is expected to return to
> > execution through a wake up event". The vcpu is never going to come up
> > again if we don't clear PSR_IRQ_MASK, because events wouldn't be
> > delivered to it.
> 
> With this solution Xen will return into the guest with IRQ enable
> unconditionally.
> 
> I don't see anything in the specification that allow a such change. So
> the guest may assume that the IRQs are still disabled. This would break it.
> 
> Couldn't we use the same trick as WFI ie:
> 
> vcpu_block();
> if ( local_events_delivery_nomask() )
>   vcpu_unblock(current);
> 
> It might be better to introduce a new helper for this purpose.

Actually the spec says:

"5. The caller must ensure that appropriate wake-up events are enabled
to allow resumption from that state."

so maybe we could allow the guest kernel to shut itself in the foot and
avoiding clearing PSR_IRQ_MASK.

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

[Julien Grall]

On 06/23/2014 05:57 PM, Stefano Stabellini wrote:
> On Mon, 23 Jun 2014, Julien Grall wrote:
>> On 06/23/2014 11:40 AM, Stefano Stabellini wrote:
>>> On Mon, 23 Jun 2014, Parth Dixit wrote:
>>>> Next version of my patch is ready except for following things on which i need your suggestion
>>>> 1. Exposing PSCI v0.2 functions in device tree - This was not done because it gives the impression that you can modify the function id's
>>>> and kernel will call the function id's based on function id's exposed in device tree whereas kernel ignores it for PSCI v0.2 while it
>>>> follows it for PSCI v0.1 which can be confusing. Either way is fine with me.
>>>> 2.  Why do you clear the IRQ flag in psci_suspend - I am taking cue from the "vcpu_block_enable_events" in xen/common/schedule.c where
>>>> flag is cleared to enable interrupts before pausing the cpu.
>>>
>>> Keep in mind that vcpu_block_enable_events is common code, while
>>> local_event_delivery_enable is the arm specific implementation.
>>>
>>> In the arm case local_event_delivery_enable is implemented by clearing
>>> PSR_IRQ_MASK because effectively that's what is needed to enable event
>>> delivery. Events are just a Xen specific kind of interrupts.
>>>
>>> vcpu_block_enable_events calls local_event_delivery_enable before
>>> blocking a vcpu, to make sure it can wake the vcpu up if an event needs
>>> to be delivered to it.
>>>
>>> We need to clear PSR_IRQ_MASK because the CPU_SUSPEND call "is intended
>>> for use in idle subsystems where the core is expected to return to
>>> execution through a wake up event". The vcpu is never going to come up
>>> again if we don't clear PSR_IRQ_MASK, because events wouldn't be
>>> delivered to it.
>>
>> With this solution Xen will return into the guest with IRQ enable
>> unconditionally.
>>
>> I don't see anything in the specification that allow a such change. So
>> the guest may assume that the IRQs are still disabled. This would break it.
>>
>> Couldn't we use the same trick as WFI ie:
>>
>> vcpu_block();
>> if ( local_events_delivery_nomask() )
>>   vcpu_unblock(current);
>>
>> It might be better to introduce a new helper for this purpose.
> 
> Actually the spec says:
> 
> "5. The caller must ensure that appropriate wake-up events are enabled
> to allow resumption from that state."
> 
> so maybe we could allow the guest kernel to shut itself in the foot and
> avoiding clearing PSR_IRQ_MASK.

It's annoying that a wake-up events is not described in the spec. I
guess we can get the definition of a wake-up events from the ARM ARM
(see B1.8.13). Which say, among other events, that an interrupt is
considered as a wake-up event if PSR_IRQ_MASK is not set.

-- 
Julien Grall

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

[Christoffer Dall]

On 23 June 2014 19:19, Julien Grall <julien.grall@linaro.org> wrote:
> On 06/23/2014 05:57 PM, Stefano Stabellini wrote:
>> On Mon, 23 Jun 2014, Julien Grall wrote:
>>> On 06/23/2014 11:40 AM, Stefano Stabellini wrote:
>>>> On Mon, 23 Jun 2014, Parth Dixit wrote:
>>>>> Next version of my patch is ready except for following things on which i need your suggestion
>>>>> 1. Exposing PSCI v0.2 functions in device tree - This was not done because it gives the impression that you can modify the function id's
>>>>> and kernel will call the function id's based on function id's exposed in device tree whereas kernel ignores it for PSCI v0.2 while it
>>>>> follows it for PSCI v0.1 which can be confusing. Either way is fine with me.
>>>>> 2.  Why do you clear the IRQ flag in psci_suspend - I am taking cue from the "vcpu_block_enable_events" in xen/common/schedule.c where
>>>>> flag is cleared to enable interrupts before pausing the cpu.
>>>>
>>>> Keep in mind that vcpu_block_enable_events is common code, while
>>>> local_event_delivery_enable is the arm specific implementation.
>>>>
>>>> In the arm case local_event_delivery_enable is implemented by clearing
>>>> PSR_IRQ_MASK because effectively that's what is needed to enable event
>>>> delivery. Events are just a Xen specific kind of interrupts.
>>>>
>>>> vcpu_block_enable_events calls local_event_delivery_enable before
>>>> blocking a vcpu, to make sure it can wake the vcpu up if an event needs
>>>> to be delivered to it.
>>>>
>>>> We need to clear PSR_IRQ_MASK because the CPU_SUSPEND call "is intended
>>>> for use in idle subsystems where the core is expected to return to
>>>> execution through a wake up event". The vcpu is never going to come up
>>>> again if we don't clear PSR_IRQ_MASK, because events wouldn't be
>>>> delivered to it.
>>>
>>> With this solution Xen will return into the guest with IRQ enable
>>> unconditionally.
>>>
>>> I don't see anything in the specification that allow a such change. So
>>> the guest may assume that the IRQs are still disabled. This would break it.
>>>
>>> Couldn't we use the same trick as WFI ie:
>>>
>>> vcpu_block();
>>> if ( local_events_delivery_nomask() )
>>>   vcpu_unblock(current);
>>>
>>> It might be better to introduce a new helper for this purpose.
>>
>> Actually the spec says:
>>
>> "5. The caller must ensure that appropriate wake-up events are enabled
>> to allow resumption from that state."
>>
>> so maybe we could allow the guest kernel to shut itself in the foot and
>> avoiding clearing PSR_IRQ_MASK.
>
> It's annoying that a wake-up events is not described in the spec. I
> guess we can get the definition of a wake-up events from the ARM ARM
> (see B1.8.13). Which say, among other events, that an interrupt is
> considered as a wake-up event if PSR_IRQ_MASK is not set.
>
AFAIU, wake-up events are implementation-defined.  So KVM takes the
approach to directly define what wake-up events are on that system,
which is simply interrupts:
https://github.com/torvalds/linux/blob/master/arch/arm/kvm/psci.c

But yes, agreed, it's annoying that the wake-up business is a bit vague.

-Christoffer