[PATCH v3 0/7] Handle SMCs and HVCs in conformance with SMCCC

[PATCH v3 0/7] Handle SMCs and HVCs in conformance with SMCCC

[Volodymyr Babchuk]

Hello all,

This is third version. Instead of 4 patches, there are 7 now.
As part of the series, I make some functions in traps.c
available globally, moved SMC conditional check into
separate patch, changed how PSCI functiond numbers are defined.

Per-patch changes are described in corresponding patch messages.


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

[PATCH 1/7] arm: traps: psci: use generic register accessors

[Volodymyr Babchuk]

From: Volodymyr Babchuk <vlad.babchuk@gmail.com>

There are standard functions set_user_reg() and get_user_reg(). We can
use them in PSCI_SET_RESULT()/PSCI_ARG() macroses instead of relying on
CONFIG_ARM_64 definition.

Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
---
 xen/arch/arm/traps.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6cf9ee7..ed78b36 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1449,13 +1449,12 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 }
 #endif
 
+#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
+#define PSCI_ARG(reg,n) get_user_reg(reg, n)
+
 #ifdef CONFIG_ARM_64
-#define PSCI_RESULT_REG(reg) (reg)->x0
-#define PSCI_ARG(reg,n) (reg)->x##n
-#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
+#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
 #else
-#define PSCI_RESULT_REG(reg) (reg)->r0
-#define PSCI_ARG(reg,n) (reg)->r##n
 #define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
 #endif
 
@@ -1470,14 +1469,14 @@ static void do_trap_psci(struct cpu_user_regs *regs)
     register_t fid = PSCI_ARG(regs,0);
 
     /* preloading in case psci_mode_check fails */
-    PSCI_RESULT_REG(regs) = PSCI_INVALID_PARAMETERS;
+    PSCI_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
     switch( fid )
     {
     case PSCI_cpu_off:
         {
             uint32_t pstate = PSCI_ARG32(regs,1);
             perfc_incr(vpsci_cpu_off);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_off(pstate);
+            PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
         }
         break;
     case PSCI_cpu_on:
@@ -1485,36 +1484,36 @@ static void do_trap_psci(struct cpu_user_regs *regs)
             uint32_t vcpuid = PSCI_ARG32(regs,1);
             register_t epoint = PSCI_ARG(regs,2);
             perfc_incr(vpsci_cpu_on);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_on(vcpuid, epoint);
+            PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
         }
         break;
     case PSCI_0_2_FN_PSCI_VERSION:
         perfc_incr(vpsci_version);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_version();
+        PSCI_SET_RESULT(regs, do_psci_0_2_version());
         break;
     case PSCI_0_2_FN_CPU_OFF:
         perfc_incr(vpsci_cpu_off);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_cpu_off();
+        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
         break;
     case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
         perfc_incr(vpsci_migrate_info_type);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_type();
+        PSCI_SET_RESULT(regs, do_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:
         perfc_incr(vpsci_migrate_info_up_cpu);
         if ( psci_mode_check(current->domain, fid) )
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_up_cpu();
+            PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
         break;
     case PSCI_0_2_FN_SYSTEM_OFF:
         perfc_incr(vpsci_system_off);
         do_psci_0_2_system_off();
-        PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         break;
     case PSCI_0_2_FN_SYSTEM_RESET:
         perfc_incr(vpsci_system_reset);
         do_psci_0_2_system_reset();
-        PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         break;
     case PSCI_0_2_FN_CPU_ON:
     case PSCI_0_2_FN64_CPU_ON:
@@ -1524,8 +1523,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
             register_t vcpuid = PSCI_ARG(regs,1);
             register_t epoint = PSCI_ARG(regs,2);
             register_t cid = PSCI_ARG(regs,3);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_cpu_on(vcpuid, epoint, cid);
+            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_CPU_SUSPEND:
@@ -1536,8 +1534,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
             uint32_t pstate = PSCI_ARG32(regs,1);
             register_t epoint = PSCI_ARG(regs,2);
             register_t cid = PSCI_ARG(regs,3);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_cpu_suspend(pstate, epoint, cid);
+            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_AFFINITY_INFO:
@@ -1547,8 +1544,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
         {
             register_t taff = PSCI_ARG(regs,1);
             uint32_t laff = PSCI_ARG32(regs,2);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_affinity_info(taff, laff);
+            PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
         }
         break;
     case PSCI_0_2_FN_MIGRATE:
@@ -1557,7 +1553,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
         if ( psci_mode_check(current->domain, fid) )
         {
             uint32_t tcpu = PSCI_ARG32(regs,1);
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate(tcpu);
+            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
         }
         break;
     default:
-- 
2.7.4


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

Re: [PATCH 1/7] arm: traps: psci: use generic register accessors

[Andrew Cooper]

On 08/08/2017 21:08, Volodymyr Babchuk wrote:
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6cf9ee7..ed78b36 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  }
>  #endif
>  
> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
> +
>  #ifdef CONFIG_ARM_64
> -#define PSCI_RESULT_REG(reg) (reg)->x0
> -#define PSCI_ARG(reg,n) (reg)->x##n
> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)

There is no need for the mask as well as the explicit (uint32_t) cast. 
I'd recommend dropping the mask entirely.

If you insist on keeping the mask, then it should be 0xffffffffu or
0x00000000ffffffffull to be compliant with the C standard (A pedantic
compiler will complain that the literal is out of range of int).

Also as you are changing all of these macros, it would be nice to apply
correct style to them, by inserting spaces after all the commas.

~Andrew

>  #else
> -#define PSCI_RESULT_REG(reg) (reg)->r0
> -#define PSCI_ARG(reg,n) (reg)->r##n
>  #define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
>  #endif
>  
>


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

Re: [PATCH 1/7] arm: traps: psci: use generic register accessors

[Volodymyr Babchuk]

Hi Andrew

On 08.08.17 23:37, Andrew Cooper wrote:
> On 08/08/2017 21:08, Volodymyr Babchuk wrote:
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 6cf9ee7..ed78b36 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>>   }
>>   #endif
>>   
>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
>> +
>>   #ifdef CONFIG_ARM_64
>> -#define PSCI_RESULT_REG(reg) (reg)->x0
>> -#define PSCI_ARG(reg,n) (reg)->x##n
>> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
>> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
> 
> There is no need for the mask as well as the explicit (uint32_t) cast.
> I'd recommend dropping the mask entirely.
Yes, I know this. But Julien asked me to keep this ([1])

> If you insist on keeping the mask, then it should be 0xffffffffu or
> 0x00000000ffffffffull to be compliant with the C standard (A pedantic
> compiler will complain that the literal is out of range of int).
> 
> Also as you are changing all of these macros, it would be nice to apply
> correct style to them, by inserting spaces after all the commas.
Yep, will do. Thanks.

> ~Andrew
> 
>>   #else
>> -#define PSCI_RESULT_REG(reg) (reg)->r0
>> -#define PSCI_ARG(reg,n) (reg)->r##n
>>   #define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
>>   #endif
>>   
>>
> 
[1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg113198.html

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

Re: [PATCH 1/7] arm: traps: psci: use generic register accessors

[Julien Grall]

On 08/08/17 21:37, Andrew Cooper wrote:
> On 08/08/2017 21:08, Volodymyr Babchuk wrote:
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 6cf9ee7..ed78b36 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>>  }
>>  #endif
>>
>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
>> +
>>  #ifdef CONFIG_ARM_64
>> -#define PSCI_RESULT_REG(reg) (reg)->x0
>> -#define PSCI_ARG(reg,n) (reg)->x##n
>> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
>> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
>
> There is no need for the mask as well as the explicit (uint32_t) cast.
> I'd recommend dropping the mask entirely.

I want to avoid the implicit cast from 64-bit register to 32-bit that 
Volodymyr introduced in his series.

uint32_t pstate = get_user_reg(regs, 1);

IHMO this is a call to mistake. Another solution is to provide 3 helpers
	- get_user_reg32(...)
	- get_user_reg64(...)
	- get_user_reg(...) -> Return the full register (32-bit on ARM32, 
64-bit on ARM64).

This would at least document the return value of get_user_reg*.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 1/7] arm: traps: psci: use generic register accessors

[Andrew Cooper]

On 09/08/17 10:52, Julien Grall wrote:
>
>
> On 08/08/17 21:37, Andrew Cooper wrote:
>> On 08/08/2017 21:08, Volodymyr Babchuk wrote:
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 6cf9ee7..ed78b36 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct
>>> cpu_user_regs *regs, unsigned int code)
>>>  }
>>>  #endif
>>>
>>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>>> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
>>> +
>>>  #ifdef CONFIG_ARM_64
>>> -#define PSCI_RESULT_REG(reg) (reg)->x0
>>> -#define PSCI_ARG(reg,n) (reg)->x##n
>>> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n &
>>> 0x00000000FFFFFFFF )
>>> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) &
>>> 0x00000000FFFFFFFF)
>>
>> There is no need for the mask as well as the explicit (uint32_t) cast.
>> I'd recommend dropping the mask entirely.
>
> I want to avoid the implicit cast from 64-bit register to 32-bit that
> Volodymyr introduced in his series.
>
> uint32_t pstate = get_user_reg(regs, 1);

This is how we'd expect code to look on the x86 side, but you are the
maintainer here, so have prerogative.

>
> IHMO this is a call to mistake. Another solution is to provide 3 helpers
>     - get_user_reg32(...)
>     - get_user_reg64(...)
>     - get_user_reg(...) -> Return the full register (32-bit on ARM32,
> 64-bit on ARM64).
>
> This would at least document the return value of get_user_reg*.

None of this is an explanation for having both an explicit uint32_t cast
and mask hidden inside PSCI_ARG32().  They are redundant.

~Andrew

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

Re: [PATCH 1/7] arm: traps: psci: use generic register accessors

[Julien Grall]

Hi Andrew,

On 09/08/17 12:12, Andrew Cooper wrote:
> On 09/08/17 10:52, Julien Grall wrote:
>>
>>
>> On 08/08/17 21:37, Andrew Cooper wrote:
>>> On 08/08/2017 21:08, Volodymyr Babchuk wrote:
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 6cf9ee7..ed78b36 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -1449,13 +1449,12 @@ static void do_debug_trap(struct
>>>> cpu_user_regs *regs, unsigned int code)
>>>>  }
>>>>  #endif
>>>>
>>>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>>>> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
>>>> +
>>>>  #ifdef CONFIG_ARM_64
>>>> -#define PSCI_RESULT_REG(reg) (reg)->x0
>>>> -#define PSCI_ARG(reg,n) (reg)->x##n
>>>> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n &
>>>> 0x00000000FFFFFFFF )
>>>> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) &
>>>> 0x00000000FFFFFFFF)
>>>
>>> There is no need for the mask as well as the explicit (uint32_t) cast.
>>> I'd recommend dropping the mask entirely.
>>
>> I want to avoid the implicit cast from 64-bit register to 32-bit that
>> Volodymyr introduced in his series.
>>
>> uint32_t pstate = get_user_reg(regs, 1);
>
> This is how we'd expect code to look on the x86 side, but you are the
> maintainer here, so have prerogative.

This is a bit error-prone. But less than passing directly the return as 
an argument. I.e

foo(get_user_reg(regs, 1))

And assuming foo will do the cast for you. My point of explicit size is 
avoid potential mis-usage in the code.

>
>>
>> IHMO this is a call to mistake. Another solution is to provide 3 helpers
>>     - get_user_reg32(...)
>>     - get_user_reg64(...)
>>     - get_user_reg(...) -> Return the full register (32-bit on ARM32,
>> 64-bit on ARM64).
>>
>> This would at least document the return value of get_user_reg*.
>
> None of this is an explanation for having both an explicit uint32_t cast
> and mask hidden inside PSCI_ARG32().  They are redundant.

If you are using a macro, you need to keep at least the cast to prevent 
a user misusing the variable. The other solution is a static inline as I 
suggested above.

Cheers,

-- 
Julien Grall

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

[PATCH 2/7] arm: make processor-specific functions from traps.c globaly visible

[Volodymyr Babchuk]

From: Volodymyr Babchuk <vlad.babchuk@gmail.com>

The following list of functions:

 - advance_pc()
 - check_conditional_instr()
 - inject_undef_exception()
 - inject_iabt_exception()
 - inject_dabt_exception()
 - inject_vabt_exception()

are now globaly visible. This change is needed becase we plan to handle SMCs
in different file and handler code needs to alter state of a guest.

Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
---
 xen/arch/arm/traps.c            | 16 ++++++----------
 xen/include/asm-arm/processor.h | 11 +++++++++++
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ed78b36..0171c1c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -616,8 +616,7 @@ static void inject_iabt64_exception(struct cpu_user_regs *regs,
 
 #endif
 
-static void inject_undef_exception(struct cpu_user_regs *regs,
-                                   const union hsr hsr)
+void inject_undef_exception(struct cpu_user_regs *regs, const union hsr hsr)
 {
         if ( is_32bit_domain(current->domain) )
             inject_undef32_exception(regs);
@@ -627,8 +626,7 @@ static void inject_undef_exception(struct cpu_user_regs *regs,
 #endif
 }
 
-static void inject_iabt_exception(struct cpu_user_regs *regs,
-                                  register_t addr,
+void inject_iabt_exception(struct cpu_user_regs *regs, register_t addr,
                                   int instr_len)
 {
         if ( is_32bit_domain(current->domain) )
@@ -639,8 +637,7 @@ static void inject_iabt_exception(struct cpu_user_regs *regs,
 #endif
 }
 
-static void inject_dabt_exception(struct cpu_user_regs *regs,
-                                  register_t addr,
+void inject_dabt_exception(struct cpu_user_regs *regs, register_t addr,
                                   int instr_len)
 {
         if ( is_32bit_domain(current->domain) )
@@ -652,7 +649,7 @@ static void inject_dabt_exception(struct cpu_user_regs *regs,
 }
 
 /* Inject a virtual Abort/SError into the guest. */
-static void inject_vabt_exception(struct cpu_user_regs *regs)
+void inject_vabt_exception(struct cpu_user_regs *regs)
 {
     const union hsr hsr = { .bits = regs->hsr };
 
@@ -1706,8 +1703,7 @@ static const unsigned short cc_map[16] = {
         0                       /* NV                     */
 };
 
-static int check_conditional_instr(struct cpu_user_regs *regs,
-                                   const union hsr hsr)
+int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr)
 {
     unsigned long cpsr, cpsr_cond;
     int cond;
@@ -1752,7 +1748,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs,
     return 1;
 }
 
-static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
+void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
 {
     unsigned long itbits, cond, cpsr = regs->cpsr;
 
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 855ded1..6347048 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -686,6 +686,17 @@ void init_traps(void);
 
 void panic_PAR(uint64_t par);
 
+void advance_pc(struct cpu_user_regs *regs, const union hsr hsr);
+
+int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr);
+
+void inject_undef_exception(struct cpu_user_regs *regs, const union hsr hsr);
+void inject_iabt_exception(struct cpu_user_regs *regs, register_t addr,
+                           int instr_len);
+void inject_dabt_exception(struct cpu_user_regs *regs, register_t addr,
+                           int instr_len);
+void inject_vabt_exception(struct cpu_user_regs *regs);
+
 void show_execution_state(struct cpu_user_regs *regs);
 void show_registers(struct cpu_user_regs *regs);
 //#define dump_execution_state() run_in_exception_handler(show_execution_state)
-- 
2.7.4


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

Re: [PATCH 2/7] arm: make processor-specific functions from traps.c globaly visible

[Julien Grall]

Hi Volodymyr,

On 08/08/17 21:08, Volodymyr Babchuk wrote:
> From: Volodymyr Babchuk <vlad.babchuk@gmail.com>
>
> The following list of functions:
>
>  - advance_pc()
>  - check_conditional_instr()
>  - inject_undef_exception()
>  - inject_iabt_exception()
>  - inject_dabt_exception()
>  - inject_vabt_exception()
>
> are now globaly visible. This change is needed becase we plan to handle SMCs
> in different file and handler code needs to alter state of a guest.
>
> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
> ---
>  xen/arch/arm/traps.c            | 16 ++++++----------
>  xen/include/asm-arm/processor.h | 11 +++++++++++

I would much prefer if you introduce a new header traps.h rather than 
piggy-back on the now growing processor.h.

I have actually a series that will do that, but I am happy if you 
introduce it before me.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/7] arm: make processor-specific functions from traps.c globaly visible

[Volodymyr Babchuk]

Hi Julien,

On 09.08.17 12:53, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>> From: Volodymyr Babchuk <vlad.babchuk@gmail.com>
>>
>> The following list of functions:
>>
>>  - advance_pc()
>>  - check_conditional_instr()
>>  - inject_undef_exception()
>>  - inject_iabt_exception()
>>  - inject_dabt_exception()
>>  - inject_vabt_exception()
>>
>> are now globaly visible. This change is needed becase we plan to 
>> handle SMCs
>> in different file and handler code needs to alter state of a guest.
>>
>> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
>> ---
>>  xen/arch/arm/traps.c            | 16 ++++++----------
>>  xen/include/asm-arm/processor.h | 11 +++++++++++
> 
> I would much prefer if you introduce a new header traps.h rather than 
> piggy-back on the now growing processor.h.
Probably, better idea is to move this functions to processor.c or to 
newlycreated file like processor_state.c, because actually this 
functions can
have broader use, than traps handling. What do you think?

> I have actually a series that will do that, but I am happy if you 
> introduce it before me.
> 
> Cheers,
> 

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

Re: [PATCH 2/7] arm: make processor-specific functions from traps.c globaly visible

[Julien Grall]

On 09/08/2017 20:26, Volodymyr Babchuk wrote:
> Hi Julien,

Hi Volodymyr,

>
> On 09.08.17 12:53, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>> From: Volodymyr Babchuk <vlad.babchuk@gmail.com>
>>>
>>> The following list of functions:
>>>
>>>  - advance_pc()
>>>  - check_conditional_instr()
>>>  - inject_undef_exception()
>>>  - inject_iabt_exception()
>>>  - inject_dabt_exception()
>>>  - inject_vabt_exception()
>>>
>>> are now globaly visible. This change is needed becase we plan to
>>> handle SMCs
>>> in different file and handler code needs to alter state of a guest.
>>>
>>> Signed-off-by: Volodymyr Babchuk <vlad.babchuk@gmail.com>
>>> ---
>>>  xen/arch/arm/traps.c            | 16 ++++++----------
>>>  xen/include/asm-arm/processor.h | 11 +++++++++++
>>
>> I would much prefer if you introduce a new header traps.h rather than
>> piggy-back on the now growing processor.h.
> Probably, better idea is to move this functions to processor.c or to
> newlycreated file like processor_state.c, because actually this
> functions can
> have broader use, than traps handling. What do you think?

processor.c deals with the physical processor whilst those are for the 
guest. The same for processor_state.c.

IHMO, they are fine to stay in traps.c for now as it is dealing with 
traps. If you really want to move it in the separate file, then please 
introduce vtraps.c.

Cheers,

-- 
Julien Grall

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

[PATCH 3/7] arm: traps: check if SMC was conditional before handling it

[Volodymyr Babchuk]

On certain ARM arhcitectures SMC instruction can be conditional
(ARM DDI 0487A.k page D7-1949) and we need to check if that
conditional was meet.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
This patch was separated from the next one

---
 xen/arch/arm/traps.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 0171c1c..e14e7c0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2773,6 +2773,12 @@ static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int rc = 0;
 
+    if ( !check_conditional_instr(regs, hsr) )
+    {
+        advance_pc(regs, hsr);
+        return;
+    }
+
     if ( current->domain->arch.monitor.privileged_call_enabled )
         rc = monitor_smc();
 
-- 
2.7.4


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

Re: [PATCH 3/7] arm: traps: check if SMC was conditional before handling it

[Julien Grall]

Hi,

On 08/08/17 21:08, Volodymyr Babchuk wrote:
> On certain ARM arhcitectures SMC instruction can be conditional
> (ARM DDI 0487A.k page D7-1949) and we need to check if that
> conditional was meet.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> This patch was separated from the next one

Nowhere in this series you mention the dependencies on your other series 
[1]. This patch should not be merged until [1] has been merged.

Cheers,

>
> ---
>  xen/arch/arm/traps.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 0171c1c..e14e7c0 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2773,6 +2773,12 @@ static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
>  {
>      int rc = 0;
>
> +    if ( !check_conditional_instr(regs, hsr) )
> +    {
> +        advance_pc(regs, hsr);
> +        return;
> +    }
> +
>      if ( current->domain->arch.monitor.privileged_call_enabled )
>          rc = monitor_smc();
>
>

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg02988.html

-- 
Julien Grall

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

[PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk]

SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
SMCCC states that both HVC and SMC are valid conduits to call to a different
firmware functions. Thus, for example PSCI calls can be made both by
SMC or HVC. Also SMCCC defines function number coding for such calls.
Besides functional calls there are query calls, which allows underling
OS determine version, UID and number of functions provided by service
provider.

This patch adds new file `vsmc.c`, which handles both generic SMCs
and HVC according to SMC. At this moment it implements only one
service: Standard Hypervisor Service.

Standard Hypervisor Service only supports query calls, so caller can
ask about hypervisor UID and determine that it is XEN running.

This change allows more generic handling for SMCs and HVCs and it can
be easily extended to support new services and functions.

But, before SMC is forwarded to standard SMCCC handler, it can be routed
to a domain monitor, if one is installed.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 - Updated description to indicate that this patch affects only SMC call path.
 - added "xen_" prefix to definitions in include/public/arch-arm/smc.h
 - moved do_trap_smc() into vsmc.c from traps.c
 - replaced all tabs with spaces

---
 xen/arch/arm/Makefile             |   1 +
 xen/arch/arm/traps.c              |  19 +-----
 xen/arch/arm/vsmc.c               | 139 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vsmc.h        |  94 ++++++++++++++++++++++++++
 xen/include/public/arch-arm/smc.h |  68 +++++++++++++++++++
 5 files changed, 303 insertions(+), 18 deletions(-)
 create mode 100644 xen/arch/arm/vsmc.c
 create mode 100644 xen/include/asm-arm/vsmc.h
 create mode 100644 xen/include/public/arch-arm/smc.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2..4efd01c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_HAS_GICV3) += vgic-v3.o
 obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
 obj-y += vm_event.o
 obj-y += vtimer.o
+obj-y += vsmc.o
 obj-y += vpsci.o
 obj-y += vuart.o
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e14e7c0..b119dc0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -43,7 +43,7 @@
 #include <asm/mmio.h>
 #include <asm/cpufeature.h>
 #include <asm/flushtlb.h>
-#include <asm/monitor.h>
+#include <asm/vsmc.h>
 
 #include "decode.h"
 #include "vtimer.h"
@@ -2769,23 +2769,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     inject_dabt_exception(regs, info.gva, hsr.len);
 }
 
-static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
-{
-    int rc = 0;
-
-    if ( !check_conditional_instr(regs, hsr) )
-    {
-        advance_pc(regs, hsr);
-        return;
-    }
-
-    if ( current->domain->arch.monitor.privileged_call_enabled )
-        rc = monitor_smc();
-
-    if ( rc != 1 )
-        inject_undef_exception(regs, hsr);
-}
-
 static void enter_hypervisor_head(struct cpu_user_regs *regs)
 {
     if ( guest_mode(regs) )
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
new file mode 100644
index 0000000..5ef6167
--- /dev/null
+++ b/xen/arch/arm/vsmc.c
@@ -0,0 +1,139 @@
+/*
+ * xen/arch/arm/vsmc.c
+ *
+ * Generic handler for SMC and HVC calls according to
+ * ARM SMC calling convention
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <public/arch-arm/smc.h>
+#include <asm/monitor.h>
+#include <asm/vsmc.h>
+#include <asm/regs.h>
+
+/* Number of functions currently supported by Hypervisor Service. */
+#define XEN_SMCCC_FUNCTION_COUNT 3
+
+/* SMCCC interface for hypervisor. Tell about itself. */
+static bool handle_hypervisor(struct cpu_user_regs *regs)
+{
+    switch ( ARM_SMCCC_FUNC_NUM(get_user_reg(regs, 0)) )
+    {
+    case ARM_SMCCC_FUNC_CALL_COUNT:
+        set_user_reg(regs, 0, XEN_SMCCC_FUNCTION_COUNT);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_UID:
+        set_user_reg(regs, 0, XEN_SMCCC_UID.a[0]);
+        set_user_reg(regs, 1, XEN_SMCCC_UID.a[1]);
+        set_user_reg(regs, 2, XEN_SMCCC_UID.a[2]);
+        set_user_reg(regs, 3, XEN_SMCCC_UID.a[3]);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_REVISION:
+        set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
+        set_user_reg(regs, 1, XEN_SMCCC_MINOR_REVISION);
+        return true;
+    }
+    return false;
+}
+
+/*
+ * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
+ * returns true if that was valid SMCCC call (even if function number
+ * was unkown).
+ */
+static bool vsmc_handle_call(struct cpu_user_regs *regs)
+{
+    bool handled = false;
+    const union hsr hsr = { .bits = regs->hsr };
+
+    /*
+     * Check immediate value for HVC32, HVC64 and SMC64.
+     * It is not so easy to check immediate value for SMC32,
+     * because it is not stored in HSR.ISS field. To get immediate
+     * value we need to dissasemble instruction at current pc, which
+     * is expensive. So we will assume that it is 0x0.
+     */
+    switch ( hsr.ec )
+    {
+    case HSR_EC_HVC32:
+    case HSR_EC_HVC64:
+    case HSR_EC_SMC64:
+        if ( (hsr.iss & 0xFF) != 0)
+            return false;
+        break;
+    case HSR_EC_SMC32:
+        break;
+    default:
+        return false;
+    }
+
+    /* 64 bit calls are allowed only from 64 bit domains */
+    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
+         is_32bit_domain(current->domain) )
+    {
+        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
+        return true;
+    }
+
+    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
+    {
+    case ARM_SMCCC_OWNER_HYPERVISOR:
+        handled = handle_hypervisor(regs);
+        break;
+    }
+
+    if ( !handled )
+    {
+        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
+                get_user_reg(regs, 0));
+        /* Inform caller that function is not supported */
+        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
+    }
+
+    return true;
+}
+
+/* This function will be called from traps.c */
+void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    int rc = 0;
+
+    if ( !check_conditional_instr(regs, hsr) )
+    {
+        advance_pc(regs, hsr);
+        return;
+    }
+
+    /* If monitor is enabled, let it handle the call */
+    if ( current->domain->arch.monitor.privileged_call_enabled )
+        rc = monitor_smc();
+
+    if ( rc == 1 )
+        return;
+
+    /* Use standard routines to handle the call */
+    if ( vsmc_handle_call(regs) )
+        advance_pc(regs, hsr);
+    else
+        inject_undef_exception(regs, hsr);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
new file mode 100644
index 0000000..7baabef
--- /dev/null
+++ b/xen/include/asm-arm/vsmc.h
@@ -0,0 +1,94 @@
+/*
+ * Copyright (c) 2017, EPAM Systems
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#ifndef __ASM_ARM_VSMC_H__
+#define __ASM_ARM_VSMC_H__
+
+#include <xen/types.h>
+
+/*
+ * This file provides common defines for ARM SMC Calling Convention as
+ * specified in
+ * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
+ */
+
+#define ARM_SMCCC_STD_CALL              0U
+#define ARM_SMCCC_FAST_CALL             1U
+#define ARM_SMCCC_TYPE_SHIFT            31
+
+#define ARM_SMCCC_SMC_32                0U
+#define ARM_SMCCC_SMC_64                1U
+#define ARM_SMCCC_CALL_CONV_SHIFT       30
+
+#define ARM_SMCCC_OWNER_MASK            0x3F
+#define ARM_SMCCC_OWNER_SHIFT           24
+
+#define ARM_SMCCC_FUNC_MASK             0xFFFF
+
+/* Check if this is fast call */
+#define ARM_SMCCC_IS_FAST_CALL(smc_val) \
+    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
+
+/* Check if this is 64 bit call  */
+#define ARM_SMCCC_IS_64(smc_val)                                        \
+    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
+
+/* Get function number from function identifier */
+#define ARM_SMCCC_FUNC_NUM(smc_val)     ((smc_val) & ARM_SMCCC_FUNC_MASK)
+
+/* Get service owner number from function identifier */
+#define ARM_SMCCC_OWNER_NUM(smc_val)                                    \
+    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
+
+/*
+ * Construct function identifier from call type (fast or standard),
+ * calling convention (32 or 64 bit), service owner and function number
+ */
+#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num)   \
+    (((type) << ARM_SMCCC_TYPE_SHIFT) |                                 \
+         ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) |          \
+         (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) |  \
+         ((func_num) & ARM_SMCCC_FUNC_MASK))
+
+/* List of known service owners */
+#define ARM_SMCCC_OWNER_ARCH            0
+#define ARM_SMCCC_OWNER_CPU             1
+#define ARM_SMCCC_OWNER_SIP             2
+#define ARM_SMCCC_OWNER_OEM             3
+#define ARM_SMCCC_OWNER_STANDARD        4
+#define ARM_SMCCC_OWNER_HYPERVISOR      5
+#define ARM_SMCCC_OWNER_TRUSTED_APP     48
+#define ARM_SMCCC_OWNER_TRUSTED_APP_END 49
+#define ARM_SMCCC_OWNER_TRUSTED_OS      50
+#define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
+
+/* List of generic function numbers */
+#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
+#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
+#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
+
+/* Only one error code defined in SMCCC */
+#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
+
+void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
+
+#endif  /* __ASM_ARM_VSMC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:b
+ */
diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
new file mode 100644
index 0000000..42f3165
--- /dev/null
+++ b/xen/include/public/arch-arm/smc.h
@@ -0,0 +1,68 @@
+/*
+ * smc.h
+ *
+ * SMC/HVC interface in accordance with SMC Calling Convention.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright 2017 (C) EPAM Systems
+ */
+
+#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
+#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
+
+typedef struct {
+    uint32_t a[4];
+} xen_arm_smccc_uid;
+
+#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
+    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
+        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
+        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
+
+/*
+ * Hypervisor Service version.
+ *
+ * We can't use XEN version here, because of SMCCC requirements:
+ * Major revision should change every time SMC/HVC function is removed.
+ * Minor revision should change every time SMC/HVC function is added.
+ * So, it is SMCCC protocol revision code, not XEN version.
+ *
+ * Those values are subjected to change, when interface will be extended.
+ * They should not be stored in public/asm-arm/smc.h because they should
+ * be queried by guest using SMC/HVC interface.
+ */
+#define XEN_SMCCC_MAJOR_REVISION 0
+#define XEN_SMCCC_MINOR_REVISION 1
+
+/* Hypervisor Service UID. Randomly generated with 3rd party tool.  */
+#define XEN_SMCCC_UID XEN_ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
+                                        0x9a, 0xcf, 0x79, 0xd1, \
+                                        0x8d, 0xde, 0xe6, 0x67)
+
+#endif  /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:b
+ */
-- 
2.7.4


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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Julien Grall]

Hi Volodymyr,

CC "THE REST" maintainers to get an opinion on the public headers.

On 08/08/17 21:08, Volodymyr Babchuk wrote:
> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
> SMCCC states that both HVC and SMC are valid conduits to call to a different
> firmware functions. Thus, for example PSCI calls can be made both by
> SMC or HVC. Also SMCCC defines function number coding for such calls.
> Besides functional calls there are query calls, which allows underling
> OS determine version, UID and number of functions provided by service
> provider.
>
> This patch adds new file `vsmc.c`, which handles both generic SMCs
> and HVC according to SMC. At this moment it implements only one
> service: Standard Hypervisor Service.
>
> Standard Hypervisor Service only supports query calls, so caller can
> ask about hypervisor UID and determine that it is XEN running.
>
> This change allows more generic handling for SMCs and HVCs and it can
> be easily extended to support new services and functions.
>
> But, before SMC is forwarded to standard SMCCC handler, it can be routed
> to a domain monitor, if one is installed.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  - Updated description to indicate that this patch affects only SMC call path.
>  - added "xen_" prefix to definitions in include/public/arch-arm/smc.h
>  - moved do_trap_smc() into vsmc.c from traps.c
>  - replaced all tabs with spaces

I would have really appreciated a summary of the discussion we had on 
the previous version regarding the bindings. This is a real blocker for 
this series and should not be ignored.

>
> ---
>  xen/arch/arm/Makefile             |   1 +
>  xen/arch/arm/traps.c              |  19 +-----
>  xen/arch/arm/vsmc.c               | 139 ++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vsmc.h        |  94 ++++++++++++++++++++++++++
>  xen/include/public/arch-arm/smc.h |  68 +++++++++++++++++++
>  5 files changed, 303 insertions(+), 18 deletions(-)
>  create mode 100644 xen/arch/arm/vsmc.c
>  create mode 100644 xen/include/asm-arm/vsmc.h
>  create mode 100644 xen/include/public/arch-arm/smc.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2..4efd01c 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_HAS_GICV3) += vgic-v3.o
>  obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
>  obj-y += vm_event.o
>  obj-y += vtimer.o
> +obj-y += vsmc.o
>  obj-y += vpsci.o
>  obj-y += vuart.o
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index e14e7c0..b119dc0 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -43,7 +43,7 @@
>  #include <asm/mmio.h>
>  #include <asm/cpufeature.h>
>  #include <asm/flushtlb.h>
> -#include <asm/monitor.h>
> +#include <asm/vsmc.h>
>
>  #include "decode.h"
>  #include "vtimer.h"
> @@ -2769,23 +2769,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      inject_dabt_exception(regs, info.gva, hsr.len);
>  }
>
> -static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
> -{
> -    int rc = 0;
> -
> -    if ( !check_conditional_instr(regs, hsr) )
> -    {
> -        advance_pc(regs, hsr);
> -        return;
> -    }
> -
> -    if ( current->domain->arch.monitor.privileged_call_enabled )
> -        rc = monitor_smc();
> -
> -    if ( rc != 1 )
> -        inject_undef_exception(regs, hsr);
> -}
> -
>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
>  {
>      if ( guest_mode(regs) )
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> new file mode 100644
> index 0000000..5ef6167
> --- /dev/null
> +++ b/xen/arch/arm/vsmc.c
> @@ -0,0 +1,139 @@
> +/*
> + * xen/arch/arm/vsmc.c
> + *
> + * Generic handler for SMC and HVC calls according to
> + * ARM SMC calling convention
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +
> +#include <xen/lib.h>
> +#include <xen/types.h>
> +#include <public/arch-arm/smc.h>
> +#include <asm/monitor.h>
> +#include <asm/vsmc.h>
> +#include <asm/regs.h>

<asm/regs.h> should be before <asm/vsmc.h> to keep the headers ordered 
alphabetically.

> +
> +/* Number of functions currently supported by Hypervisor Service. */
> +#define XEN_SMCCC_FUNCTION_COUNT 3
> +
> +/* SMCCC interface for hypervisor. Tell about itself. */
> +static bool handle_hypervisor(struct cpu_user_regs *regs)
> +{
> +    switch ( ARM_SMCCC_FUNC_NUM(get_user_reg(regs, 0)) )
> +    {
> +    case ARM_SMCCC_FUNC_CALL_COUNT:
> +        set_user_reg(regs, 0, XEN_SMCCC_FUNCTION_COUNT);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_UID:
> +        set_user_reg(regs, 0, XEN_SMCCC_UID.a[0]);
> +        set_user_reg(regs, 1, XEN_SMCCC_UID.a[1]);
> +        set_user_reg(regs, 2, XEN_SMCCC_UID.a[2]);
> +        set_user_reg(regs, 3, XEN_SMCCC_UID.a[3]);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_REVISION:
> +        set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
> +        set_user_reg(regs, 1, XEN_SMCCC_MINOR_REVISION);
> +        return true;
> +    }
> +    return false;
> +}
> +
> +/*
> + * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
> + * returns true if that was valid SMCCC call (even if function number
> + * was unkown).
> + */
> +static bool vsmc_handle_call(struct cpu_user_regs *regs)

Something is wrong here. The comment says "Handle SMC/HVC" but the name 
of the function is "vsmc".

> +{
> +    bool handled = false;
> +    const union hsr hsr = { .bits = regs->hsr };
> +
> +    /*
> +     * Check immediate value for HVC32, HVC64 and SMC64.
> +     * It is not so easy to check immediate value for SMC32,
> +     * because it is not stored in HSR.ISS field. To get immediate
> +     * value we need to dissasemble instruction at current pc, which

s/dissasemble/disassemble/

> +     * is expensive. So we will assume that it is 0x0.
> +     */
> +    switch ( hsr.ec )
> +    {
> +    case HSR_EC_HVC32:
> +    case HSR_EC_HVC64:
> +    case HSR_EC_SMC64:
> +        if ( (hsr.iss & 0xFF) != 0)

The immediate is 16 bits. So it should be 0xFFFF. It would also be nice 
to have a comment explaining it and probably a define.

> +            return false;
> +        break;
> +    case HSR_EC_SMC32:
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    /* 64 bit calls are allowed only from 64 bit domains */

Missing full stop.

> +    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
> +         is_32bit_domain(current->domain) )
> +    {
> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
> +        return true;
> +    }
> +
> +    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
> +    {
> +    case ARM_SMCCC_OWNER_HYPERVISOR:
> +        handled = handle_hypervisor(regs);
> +        break;
> +    }
> +
> +    if ( !handled )
> +    {
> +        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
> +                get_user_reg(regs, 0));
> +        /* Inform caller that function is not supported */
> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
> +    }
> +
> +    return true;
> +}
> +
> +/* This function will be called from traps.c */
> +void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
> +{
> +    int rc = 0;
> +
> +    if ( !check_conditional_instr(regs, hsr) )
> +    {
> +        advance_pc(regs, hsr);
> +        return;
> +    }
> +
> +    /* If monitor is enabled, let it handle the call */
> +    if ( current->domain->arch.monitor.privileged_call_enabled )
> +        rc = monitor_smc();
> +
> +    if ( rc == 1 )
> +        return;
> +
> +    /* Use standard routines to handle the call */
> +    if ( vsmc_handle_call(regs) )
> +        advance_pc(regs, hsr);
> +    else
> +        inject_undef_exception(regs, hsr);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
> new file mode 100644
> index 0000000..7baabef
> --- /dev/null
> +++ b/xen/include/asm-arm/vsmc.h
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (c) 2017, EPAM Systems
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +#ifndef __ASM_ARM_VSMC_H__
> +#define __ASM_ARM_VSMC_H__
> +
> +#include <xen/types.h>
> +
> +/*
> + * This file provides common defines for ARM SMC Calling Convention as
> + * specified in
> + * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
> + */
> +
> +#define ARM_SMCCC_STD_CALL              0U
> +#define ARM_SMCCC_FAST_CALL             1U
> +#define ARM_SMCCC_TYPE_SHIFT            31
> +
> +#define ARM_SMCCC_SMC_32                0U
> +#define ARM_SMCCC_SMC_64                1U
> +#define ARM_SMCCC_CALL_CONV_SHIFT       30
> +
> +#define ARM_SMCCC_OWNER_MASK            0x3F
> +#define ARM_SMCCC_OWNER_SHIFT           24
> +
> +#define ARM_SMCCC_FUNC_MASK             0xFFFF
> +
> +/* Check if this is fast call */
> +#define ARM_SMCCC_IS_FAST_CALL(smc_val) \
> +    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
> +
> +/* Check if this is 64 bit call  */
> +#define ARM_SMCCC_IS_64(smc_val)                                        \
> +    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
> +
> +/* Get function number from function identifier */
> +#define ARM_SMCCC_FUNC_NUM(smc_val)     ((smc_val) & ARM_SMCCC_FUNC_MASK)
> +
> +/* Get service owner number from function identifier */
> +#define ARM_SMCCC_OWNER_NUM(smc_val)                                    \
> +    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
> +
> +/*
> + * Construct function identifier from call type (fast or standard),
> + * calling convention (32 or 64 bit), service owner and function number
> + */
> +#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num)   \
> +    (((type) << ARM_SMCCC_TYPE_SHIFT) |                                 \
> +         ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) |          \
> +         (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) |  \
> +         ((func_num) & ARM_SMCCC_FUNC_MASK))
> +
> +/* List of known service owners */
> +#define ARM_SMCCC_OWNER_ARCH            0
> +#define ARM_SMCCC_OWNER_CPU             1
> +#define ARM_SMCCC_OWNER_SIP             2
> +#define ARM_SMCCC_OWNER_OEM             3
> +#define ARM_SMCCC_OWNER_STANDARD        4
> +#define ARM_SMCCC_OWNER_HYPERVISOR      5
> +#define ARM_SMCCC_OWNER_TRUSTED_APP     48
> +#define ARM_SMCCC_OWNER_TRUSTED_APP_END 49
> +#define ARM_SMCCC_OWNER_TRUSTED_OS      50
> +#define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
> +
> +/* List of generic function numbers */
> +#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
> +#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
> +#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
> +
> +/* Only one error code defined in SMCCC */
> +#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
> +
> +void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
> +
> +#endif  /* __ASM_ARM_VSMC_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:b
> + */
> diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
> new file mode 100644
> index 0000000..42f3165
> --- /dev/null
> +++ b/xen/include/public/arch-arm/smc.h
> @@ -0,0 +1,68 @@
> +/*
> + * smc.h
> + *
> + * SMC/HVC interface in accordance with SMC Calling Convention.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright 2017 (C) EPAM Systems
> + */
> +
> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
> +
> +typedef struct {
> +    uint32_t a[4];
> +} xen_arm_smccc_uid;
> +
> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
> +
> +/*
> + * Hypervisor Service version.
> + *
> + * We can't use XEN version here, because of SMCCC requirements:
> + * Major revision should change every time SMC/HVC function is removed.
> + * Minor revision should change every time SMC/HVC function is added.
> + * So, it is SMCCC protocol revision code, not XEN version.
> + *
> + * Those values are subjected to change, when interface will be extended.
> + * They should not be stored in public/asm-arm/smc.h because they should
> + * be queried by guest using SMC/HVC interface.
> + */
> +#define XEN_SMCCC_MAJOR_REVISION 0
> +#define XEN_SMCCC_MINOR_REVISION 1
> +
> +/* Hypervisor Service UID. Randomly generated with 3rd party tool.  */
> +#define XEN_SMCCC_UID XEN_ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
> +                                        0x9a, 0xcf, 0x79, 0xd1, \
> +                                        0x8d, 0xde, 0xe6, 0x67)
> +
> +#endif  /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:b
> + */
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Jan Beulich]

>>> On 09.08.17 at 12:10, <julien.grall@arm.com> wrote:
> CC "THE REST" maintainers to get an opinion on the public headers.

Please be more specific as to what you expect - the only public
header affected here is ARM-specific.

> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
>> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
>> +
>> +typedef struct {
>> +    uint32_t a[4];
>> +} xen_arm_smccc_uid;

This is not the normal way of encoding a UID type.

>> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
>> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \

This is not C89 compatible.

>> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
>> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>> +
>> +/*
>> + * Hypervisor Service version.
>> + *
>> + * We can't use XEN version here, because of SMCCC requirements:
>> + * Major revision should change every time SMC/HVC function is removed.
>> + * Minor revision should change every time SMC/HVC function is added.
>> + * So, it is SMCCC protocol revision code, not XEN version.

I don't understand this explanation - how is the situation here
different from some arbitrary, non-toolstack-only hypercall?

>> + * Those values are subjected to change, when interface will be extended.
>> + * They should not be stored in public/asm-arm/smc.h because they should
>> + * be queried by guest using SMC/HVC interface.
>> + */
>> +#define XEN_SMCCC_MAJOR_REVISION 0
>> +#define XEN_SMCCC_MINOR_REVISION 1

The comment says the values shouldn't be put here, but then
they're still being put here?

>> +/* Hypervisor Service UID. Randomly generated with 3rd party tool.  */
>> +#define XEN_SMCCC_UID XEN_ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
>> +                                        0x9a, 0xcf, 0x79, 0xd1, \
>> +                                        0x8d, 0xde, 0xe6, 0x67)

Why is the right side using _ARM_ in its name, but the macro being
defined isn't?

Jan


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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk]

On 09.08.17 14:58, Jan Beulich wrote:
>>>> On 09.08.17 at 12:10, <julien.grall@arm.com> wrote:
>> CC "THE REST" maintainers to get an opinion on the public headers.
> 
> Please be more specific as to what you expect - the only public
> header affected here is ARM-specific.
> 
>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
>>> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
>>> +
>>> +typedef struct {
>>> +    uint32_t a[4];
>>> +} xen_arm_smccc_uid;
> 
> This is not the normal way of encoding a UID type.
Just to be clear: you are proposing to store UID in such struct
struct uuid_t {
     unsigned32  time_low;
     unsigned16  time_mid;
     unsigned16  time_hi_and_version;
     unsigned8   clock_seq_hi_and_reserved;
     unsigned8   clock_seq_low;
     byte        node[6];
};
right?


>>> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
>>> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
>
> This is not C89 compatible.
Oops, sorry. Didn't knew that XEN should be C89 compatible.
Is there any guide for novices? I didn't found anything useful in docs/ 
(not even coding style document). On wiki I have found only 
"Submitting_Xen_Project_Patches" page, which is very helpful, but it 
does not cover topics like which C standard to use.

>>> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
>>> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>>> +
>>> +/*
>>> + * Hypervisor Service version.
>>> + *
>>> + * We can't use XEN version here, because of SMCCC requirements:
>>> + * Major revision should change every time SMC/HVC function is removed.
>>> + * Minor revision should change every time SMC/HVC function is added.
>>> + * So, it is SMCCC protocol revision code, not XEN version.
> 
> I don't understand this explanation - how is the situation here
> different from some arbitrary, non-toolstack-only hypercall?
Because this is generic interface that should be supported by all 
hypervisors, including XEN. Think about this as a way for a guest to 
determine under which hypervisor it operates, and which functions it 
provides.

>>> + * Those values are subjected to change, when interface will be extended.
>>> + * They should not be stored in public/asm-arm/smc.h because they should
>>> + * be queried by guest using SMC/HVC interface.
>>> + */
>>> +#define XEN_SMCCC_MAJOR_REVISION 0
>>> +#define XEN_SMCCC_MINOR_REVISION 1
> 
> The comment says the values shouldn't be put here, but then
> they're still being put here?
Yeah, sorry. Missed this.

>>> +/* Hypervisor Service UID. Randomly generated with 3rd party tool.  */
>>> +#define XEN_SMCCC_UID XEN_ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
>>> +                                        0x9a, 0xcf, 0x79, 0xd1, \
>>> +                                        0x8d, 0xde, 0xe6, 0x67)
> 
> Why is the right side using _ARM_ in its name, but the macro being
> defined isn't?
> 
> Jan
> 

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Jan Beulich]

>>> On 09.08.17 at 23:39, <volodymyr_babchuk@epam.com> wrote:
> On 09.08.17 14:58, Jan Beulich wrote:
>>>>> On 09.08.17 at 12:10, <julien.grall@arm.com> wrote:
>>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>>> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
>>>> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
>>>> +
>>>> +typedef struct {
>>>> +    uint32_t a[4];
>>>> +} xen_arm_smccc_uid;
>> 
>> This is not the normal way of encoding a UID type.
> Just to be clear: you are proposing to store UID in such struct
> struct uuid_t {
>      unsigned32  time_low;
>      unsigned16  time_mid;
>      unsigned16  time_hi_and_version;
>      unsigned8   clock_seq_hi_and_reserved;
>      unsigned8   clock_seq_low;
>      byte        node[6];
> };
> right?

Type-wise yes; the names of the fields look uncommon to me.

>>>> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
>>>> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
>>
>> This is not C89 compatible.
> Oops, sorry. Didn't knew that XEN should be C89 compatible.
> Is there any guide for novices? I didn't found anything useful in docs/ 
> (not even coding style document). On wiki I have found only 
> "Submitting_Xen_Project_Patches" page, which is very helpful, but it 
> does not cover topics like which C standard to use.

The public headers are required to the C89 compatible; Xen
code in general is fine to use extensions.

>>>> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
>>>> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>>>> +
>>>> +/*
>>>> + * Hypervisor Service version.
>>>> + *
>>>> + * We can't use XEN version here, because of SMCCC requirements:
>>>> + * Major revision should change every time SMC/HVC function is removed.
>>>> + * Minor revision should change every time SMC/HVC function is added.
>>>> + * So, it is SMCCC protocol revision code, not XEN version.
>> 
>> I don't understand this explanation - how is the situation here
>> different from some arbitrary, non-toolstack-only hypercall?
> Because this is generic interface that should be supported by all 
> hypervisors, including XEN. Think about this as a way for a guest to 
> determine under which hypervisor it operates, and which functions it 
> provides.

In which case - why the XEN_ prefixes?

Jan


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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Julien Grall]

Hi,

On 10/08/17 08:30, Jan Beulich wrote:
>>>> On 09.08.17 at 23:39, <volodymyr_babchuk@epam.com> wrote:
>> On 09.08.17 14:58, Jan Beulich wrote:
>>>>>> On 09.08.17 at 12:10, <julien.grall@arm.com> wrote:
>>>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>>>> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
>>>>> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
>>>>> +
>>>>> +typedef struct {
>>>>> +    uint32_t a[4];
>>>>> +} xen_arm_smccc_uid;
>>>
>>> This is not the normal way of encoding a UID type.
>> Just to be clear: you are proposing to store UID in such struct
>> struct uuid_t {
>>      unsigned32  time_low;
>>      unsigned16  time_mid;
>>      unsigned16  time_hi_and_version;
>>      unsigned8   clock_seq_hi_and_reserved;
>>      unsigned8   clock_seq_low;
>>      byte        node[6];
>> };
>> right?
>
> Type-wise yes; the names of the fields look uncommon to me.
>
>>>>> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
>>>>> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
>>>
>>> This is not C89 compatible.
>> Oops, sorry. Didn't knew that XEN should be C89 compatible.
>> Is there any guide for novices? I didn't found anything useful in docs/
>> (not even coding style document). On wiki I have found only
>> "Submitting_Xen_Project_Patches" page, which is very helpful, but it
>> does not cover topics like which C standard to use.
>
> The public headers are required to the C89 compatible; Xen
> code in general is fine to use extensions.
>
>>>>> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
>>>>> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>>>>> +
>>>>> +/*
>>>>> + * Hypervisor Service version.
>>>>> + *
>>>>> + * We can't use XEN version here, because of SMCCC requirements:
>>>>> + * Major revision should change every time SMC/HVC function is removed.
>>>>> + * Minor revision should change every time SMC/HVC function is added.
>>>>> + * So, it is SMCCC protocol revision code, not XEN version.
>>>
>>> I don't understand this explanation - how is the situation here
>>> different from some arbitrary, non-toolstack-only hypercall?
>> Because this is generic interface that should be supported by all
>> hypervisors, including XEN. Think about this as a way for a guest to
>> determine under which hypervisor it operates, and which functions it
>> provides.
>
> In which case - why the XEN_ prefixes?

Because the version depends on the interface implemented which is Xen 
specific.

You have to match the UID (Xen Specific) and the version to know what is 
actually supported.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk]

Hello Jan,

On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote:

> 
> > On 08/08/17 21:08, Volodymyr Babchuk wrote:
> >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
> >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
> >> +
> >> +typedef struct {
> >> +    uint32_t a[4];
> >> +} xen_arm_smccc_uid;
> 
> This is not the normal way of encoding a UID type.
I thought about this. According to RFC 4122, UUID should be defined like this:
struct xen_uuid_rfc_4122 {
    u32     time_low;                  /* low part of timestamp */
    u16     time_mid;                  /* mid part of timestamp */
    u16     time_hi_and_version;       /* high part of timestamp and version */
    u8      clock_seq_hi_and_reserved; /* clock seq hi and variant */
    u8      clock_seq_low;             /* clock seq low */
    u8      node[6];                   /* nodes */
};

This resembles structure of RFC, but it is highly inconvenient to use. The most
used operation for UUIDs is comparison, I think. Next popular operations are
serialization and deserialization. All those are very trivial, if you are using
array instead of separate fields. I just checked Linux kernel, it uses array
of 16 u8s. I used array of four u32s because this is how it is represented in
SMC convention.
Now I'm going to create separate public header for UUIDs. And I'm not sure
that RFC 4122 approach is the best. Serialization code for that structure
will require some fiddling with binary shifts. Personally I stick to the
Linux way (uint8_t data[16]).
So, I'm interested in maintainers opinion.

> >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
> >> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
> 
> This is not C89 compatible.

I'm sorry, but I not quite sure why this is not C89 compatible. According to [1]
C89 supports initializer lists.

> 
> >> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
> >> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
> >> +

[1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Jan Beulich]

>>> On 16.08.17 at 23:41, <volodymyr_babchuk@epam.com> wrote:
> Hello Jan,
> 
> On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote:
> 
>> 
>> > On 08/08/17 21:08, Volodymyr Babchuk wrote:
>> >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
>> >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
>> >> +
>> >> +typedef struct {
>> >> +    uint32_t a[4];
>> >> +} xen_arm_smccc_uid;
>> 
>> This is not the normal way of encoding a UID type.
> I thought about this. According to RFC 4122, UUID should be defined like 
> this:
> struct xen_uuid_rfc_4122 {
>     u32     time_low;                  /* low part of timestamp */
>     u16     time_mid;                  /* mid part of timestamp */
>     u16     time_hi_and_version;       /* high part of timestamp and version */
>     u8      clock_seq_hi_and_reserved; /* clock seq hi and variant */
>     u8      clock_seq_low;             /* clock seq low */
>     u8      node[6];                   /* nodes */
> };
> 
> This resembles structure of RFC, but it is highly inconvenient to use. The most
> used operation for UUIDs is comparison, I think. Next popular operations are
> serialization and deserialization. All those are very trivial, if you are using
> array instead of separate fields. I just checked Linux kernel, it uses array
> of 16 u8s. I used array of four u32s because this is how it is represented in
> SMC convention.

In our tree, see e.g. struct xenpf_efi_guid and EFI_GUID plus the
"conversion" function between them (cast_guid()).

> Now I'm going to create separate public header for UUIDs.

I don't see the need for a separate header.

>And I'm not sure
> that RFC 4122 approach is the best. Serialization code for that structure
> will require some fiddling with binary shifts. Personally I stick to the
> Linux way (uint8_t data[16]).
> So, I'm interested in maintainers opinion.
> 
>> >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
>> >> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
>> 
>> This is not C89 compatible.
> 
> I'm sorry, but I not quite sure why this is not C89 compatible. According to [1]
> C89 supports initializer lists.

It's the (<type>){<initializers>} style which C89 doesn't support
afaik, and ...

>> >> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
>> >> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>> >> +
> 
> [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7 

... this also doesn't have anything like that.

Jan


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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk]

Hi Jan,

On Thu, Aug 17, 2017 at 01:45:54AM -0600, Jan Beulich wrote:
> >>> On 16.08.17 at 23:41, <volodymyr_babchuk@epam.com> wrote:
> > Hello Jan,
> > 
> > On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote:
> > 
> >> 
> >> > On 08/08/17 21:08, Volodymyr Babchuk wrote:
> >> >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
> >> >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
> >> >> +
> >> >> +typedef struct {
> >> >> +    uint32_t a[4];
> >> >> +} xen_arm_smccc_uid;
> >> 
> >> This is not the normal way of encoding a UID type.
> > I thought about this. According to RFC 4122, UUID should be defined like 
> > this:
> > struct xen_uuid_rfc_4122 {
> >     u32     time_low;                  /* low part of timestamp */
> >     u16     time_mid;                  /* mid part of timestamp */
> >     u16     time_hi_and_version;       /* high part of timestamp and version */
> >     u8      clock_seq_hi_and_reserved; /* clock seq hi and variant */
> >     u8      clock_seq_low;             /* clock seq low */
> >     u8      node[6];                   /* nodes */
> > };
> > 
> > This resembles structure of RFC, but it is highly inconvenient to use. The most
> > used operation for UUIDs is comparison, I think. Next popular operations are
> > serialization and deserialization. All those are very trivial, if you are using
> > array instead of separate fields. I just checked Linux kernel, it uses array
> > of 16 u8s. I used array of four u32s because this is how it is represented in
> > SMC convention.
> 
> In our tree, see e.g. struct xenpf_efi_guid and EFI_GUID plus the
> "conversion" function between them (cast_guid()).
There are two EFI_GUID definitions in xen tree. One is in tools/libxc/xc_efi.h,
where guid defined in linux style:
typedef struct {
        uint8_t b[16];
} efi_guid_t;

Another is in xen/include/efi/efidef.h, and it is defined in Microsoft style:
typedef struct {          
    UINT32  Data1;
    UINT16  Data2;
    UINT16  Data3;
    UINT8   Data4[8]; 
} EFI_GUID;  

I assume, that you meant second one.
Anyways, both of them are for EFI code and are not publicly exported.

> > Now I'm going to create separate public header for UUIDs.
> 
> I don't see the need for a separate header.
Look, I was asked to provide XEN SMCCC UUID in public headers. To do that,
I need some structure to hold UUID. There are two ways: I can either
introduce public struct xen_uuid, which later can be used by anyone.
In this case it should have some generic structure (RFC4122 compatible,
Linux compatible, Microsoft compatible, or some XEN way). In another words,
everyone should be happy with it.

Either it can be SMCCC-only structure. Then it can live in public SMCCC header,
it can have definition that is suitable for SMCCC use, and I'm not obligued
to make it compatible with any other UUID definition standard. According to
SMCCC, UUID is presented as four 32-bit fields. This is exactly what I did
there.

So, if you are saying, that there are no need for a separate header, then I
can use the second approach, right?

> >And I'm not sure
> > that RFC 4122 approach is the best. Serialization code for that structure
> > will require some fiddling with binary shifts. Personally I stick to the
> > Linux way (uint8_t data[16]).
> > So, I'm interested in maintainers opinion.
> > 
> >> >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
> >> >> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
> >> 
> >> This is not C89 compatible.
> > 
> > I'm sorry, but I not quite sure why this is not C89 compatible. According to [1]
> > C89 supports initializer lists.
> 
> It's the (<type>){<initializers>} style which C89 doesn't support
> afaik, and ...
I wrote this small test program:

#include <stdio.h>

struct test
{
	int a[2];
};

int main (int argc, char* argv[])
{
	printf("%d\n", ((struct test){{1,2}}).a[0]);
	return 0;
}

It is compiles with gcc --std=c89 without warnings.

> >> >> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
> >> >> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
> >> >> +
> > 
> > [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7 
> 
> ... this also doesn't have anything like that.
I'm sorry, I don't get what do you mean under "this".

> Jan
> 

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Julien Grall]

Hi,

On 17/08/17 13:35, Volodymyr Babchuk wrote:
> I wrote this small test program:
>
> #include <stdio.h>
>
> struct test
> {
> 	int a[2];
> };
>
> int main (int argc, char* argv[])
> {
> 	printf("%d\n", ((struct test){{1,2}}).a[0]);
> 	return 0;
> }
>
> It is compiles with gcc --std=c89 without warnings.

This is because GCC supports compounds literals in C89 as an extension 
(see [1]).

>
>>>>>> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
>>>>>> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>>>>>> +
>>>
>>> [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7
>>
>> ... this also doesn't have anything like that.
> I'm sorry, I don't get what do you mean under "this".

The web page does not mention (<type>){<initializers>}. Which is normal 
because this also appears in c99 (see [2]).

>
>> Jan
>>

Cheers,

[1] https://gcc.gnu.org/onlinedocs/gcc-4.3.1/gcc/Compound-Literals.html
[2]  http://en.cppreference.com/w/c/language/compound_literal

-- 
Julien Grall

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Jan Beulich]

>>> On 17.08.17 at 14:35, <volodymyr_babchuk@epam.com> wrote:
> On Thu, Aug 17, 2017 at 01:45:54AM -0600, Jan Beulich wrote:
>> >>> On 16.08.17 at 23:41, <volodymyr_babchuk@epam.com> wrote:
>> > On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote:
>> >> > On 08/08/17 21:08, Volodymyr Babchuk wrote:
>> >> >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
>> >> >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
>> >> >> +
>> >> >> +typedef struct {
>> >> >> +    uint32_t a[4];
>> >> >> +} xen_arm_smccc_uid;
>> >> 
>> >> This is not the normal way of encoding a UID type.
>> > I thought about this. According to RFC 4122, UUID should be defined like 
>> > this:
>> > struct xen_uuid_rfc_4122 {
>> >     u32     time_low;                  /* low part of timestamp */
>> >     u16     time_mid;                  /* mid part of timestamp */
>> >     u16     time_hi_and_version;       /* high part of timestamp and version */
>> >     u8      clock_seq_hi_and_reserved; /* clock seq hi and variant */
>> >     u8      clock_seq_low;             /* clock seq low */
>> >     u8      node[6];                   /* nodes */
>> > };
>> > 
>> > This resembles structure of RFC, but it is highly inconvenient to use. The most
>> > used operation for UUIDs is comparison, I think. Next popular operations are
>> > serialization and deserialization. All those are very trivial, if you are using
>> > array instead of separate fields. I just checked Linux kernel, it uses array
>> > of 16 u8s. I used array of four u32s because this is how it is represented in
>> > SMC convention.
>> 
>> In our tree, see e.g. struct xenpf_efi_guid and EFI_GUID plus the
>> "conversion" function between them (cast_guid()).
> There are two EFI_GUID definitions in xen tree. One is in 
> tools/libxc/xc_efi.h,

When I talk about the Xen tree, I normally mean the xen/ sub-tree
of xen.git.

> where guid defined in linux style:
> typedef struct {
>         uint8_t b[16];
> } efi_guid_t;
> 
> Another is in xen/include/efi/efidef.h, and it is defined in Microsoft 
> style:
> typedef struct {          
>     UINT32  Data1;
>     UINT16  Data2;
>     UINT16  Data3;
>     UINT8   Data4[8]; 
> } EFI_GUID;  
> 
> I assume, that you meant second one.
> Anyways, both of them are for EFI code and are not publicly exported.

But struct xenpf_efi_guid is.

>> > Now I'm going to create separate public header for UUIDs.
>> 
>> I don't see the need for a separate header.
> Look, I was asked to provide XEN SMCCC UUID in public headers. To do that,
> I need some structure to hold UUID. There are two ways: I can either
> introduce public struct xen_uuid, which later can be used by anyone.
> In this case it should have some generic structure (RFC4122 compatible,
> Linux compatible, Microsoft compatible, or some XEN way). In another words,
> everyone should be happy with it.

Right, but that _still_ does not require you to add a new header. I'd
see such a relatively generic type go into xen.h.

> Either it can be SMCCC-only structure. Then it can live in public SMCCC header,
> it can have definition that is suitable for SMCCC use, and I'm not obligued
> to make it compatible with any other UUID definition standard. According to
> SMCCC, UUID is presented as four 32-bit fields. This is exactly what I did
> there.
> 
> So, if you are saying, that there are no need for a separate header, then I
> can use the second approach, right?

As per above - no, not really.

>> >And I'm not sure
>> > that RFC 4122 approach is the best. Serialization code for that structure
>> > will require some fiddling with binary shifts. Personally I stick to the
>> > Linux way (uint8_t data[16]).
>> > So, I'm interested in maintainers opinion.
>> > 
>> >> >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)      \
>> >> >> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                     \
>> >> 
>> >> This is not C89 compatible.
>> > 
>> > I'm sorry, but I not quite sure why this is not C89 compatible. According to [1]
>> > C89 supports initializer lists.
>> 
>> It's the (<type>){<initializers>} style which C89 doesn't support
>> afaik, and ...
> I wrote this small test program:
> 
> #include <stdio.h>
> 
> struct test
> {
> 	int a[2];
> };
> 
> int main (int argc, char* argv[])
> {
> 	printf("%d\n", ((struct test){{1,2}}).a[0]);
> 	return 0;
> }
> 
> It is compiles with gcc --std=c89 without warnings.

Sure, because afaik gcc keeps its extensions enabled in that mode.
Try a compiler that is _not_ gcc or C99 compatible.

>> >> >> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
>> >> >> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>> >> >> +
>> > 
>> > [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7 
>> 
>> ... this also doesn't have anything like that.
> I'm sorry, I don't get what do you mean under "this".

It means the document you referred to.

Jan

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk]

Hi Julien,

On 09.08.17 13:10, Julien Grall wrote:
> Hi Volodymyr,
> 
> CC "THE REST" maintainers to get an opinion on the public headers.
> 
> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
>> SMCCC states that both HVC and SMC are valid conduits to call to a 
>> different
>> firmware functions. Thus, for example PSCI calls can be made both by
>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>> Besides functional calls there are query calls, which allows underling
>> OS determine version, UID and number of functions provided by service
>> provider.
>>
>> This patch adds new file `vsmc.c`, which handles both generic SMCs
>> and HVC according to SMC. At this moment it implements only one
>> service: Standard Hypervisor Service.
>>
>> Standard Hypervisor Service only supports query calls, so caller can
>> ask about hypervisor UID and determine that it is XEN running.
>>
>> This change allows more generic handling for SMCs and HVCs and it can
>> be easily extended to support new services and functions.
>>
>> But, before SMC is forwarded to standard SMCCC handler, it can be routed
>> to a domain monitor, if one is installed.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  - Updated description to indicate that this patch affects only SMC 
>> call path.
>>  - added "xen_" prefix to definitions in include/public/arch-arm/smc.h
>>  - moved do_trap_smc() into vsmc.c from traps.c
>>  - replaced all tabs with spaces
> 
> I would have really appreciated a summary of the discussion we had on 
> the previous version regarding the bindings. This is a real blocker for 
> this series and should not be ignored.
> 

While I agree that question about bindings is important, I can't see how 
it affects this patch series. This patch series does not break anything.
Because

1. This series add only new feature: generic hypervisor service with no 
immediate use. All ARM guests are already aware that they are running on 
XEN. All ARM guests know that they call *only* PSCI.

2. I see importance of this patch series for embedded platforms, where 
developer exactly knows what software of which version he/she will run. 
I doubt that server platforms will need something beyond PSCI, which I 
preserved as is.

A I'm not denying importance of SMC bindings, but I think it is not 
blocker for my patches. We can add bindings later, when there will be 
consensus on how they should look. In meantime SMC handler can be used 
by anyone who knows that is available.

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Julien Grall]

On 10/08/17 16:33, Volodymyr Babchuk wrote:
> Hi Julien,
>
> On 09.08.17 13:10, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> CC "THE REST" maintainers to get an opinion on the public headers.
>>
>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
>>> SMCCC states that both HVC and SMC are valid conduits to call to a
>>> different
>>> firmware functions. Thus, for example PSCI calls can be made both by
>>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>>> Besides functional calls there are query calls, which allows underling
>>> OS determine version, UID and number of functions provided by service
>>> provider.
>>>
>>> This patch adds new file `vsmc.c`, which handles both generic SMCs
>>> and HVC according to SMC. At this moment it implements only one
>>> service: Standard Hypervisor Service.
>>>
>>> Standard Hypervisor Service only supports query calls, so caller can
>>> ask about hypervisor UID and determine that it is XEN running.
>>>
>>> This change allows more generic handling for SMCs and HVCs and it can
>>> be easily extended to support new services and functions.
>>>
>>> But, before SMC is forwarded to standard SMCCC handler, it can be routed
>>> to a domain monitor, if one is installed.
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>  - Updated description to indicate that this patch affects only SMC
>>> call path.
>>>  - added "xen_" prefix to definitions in include/public/arch-arm/smc.h
>>>  - moved do_trap_smc() into vsmc.c from traps.c
>>>  - replaced all tabs with spaces
>>
>> I would have really appreciated a summary of the discussion we had on
>> the previous version regarding the bindings. This is a real blocker
>> for this series and should not be ignored.
>>
>
> While I agree that question about bindings is important, I can't see how
> it affects this patch series. This patch series does not break anything.
> Because
>
> 1. This series add only new feature: generic hypervisor service with no
> immediate use. All ARM guests are already aware that they are running on
> XEN. All ARM guests know that they call *only* PSCI.
>
> 2. I see importance of this patch series for embedded platforms, where
> developer exactly knows what software of which version he/she will run.
> I doubt that server platforms will need something beyond PSCI, which I
> preserved as is.

I disagree here. SMCCC could be used to provide Dom0 a way to interact 
with the firmware if necessary.

>
> A I'm not denying importance of SMC bindings, but I think it is not
> blocker for my patches. We can add bindings later, when there will be
> consensus on how they should look. In meantime SMC handler can be used
> by anyone who knows that is available.

I am not in favor on getting something merged in Xen until we agree on 
the way for the guest to know it is there. It means you have to carry 
hack in your kernel in order to use SMC. Maybe this is fine for you, but 
I don't want to make this assumption on Xen upstream today.

This is a change in the interface that should be notified to the guest. 
If we expose it without providing a bindings (or something), we have no 
way to revert/disable it. Imagine we want to disable SMC in the future. 
How a guest will know that
	- until Xen 4.10 SMC was not existing,
	- between Xen 4.10 and Xen 4.x you can use them
	- after Xen 4.y they can be disabled.

All changes should be detected through the firmware tables (DT, ACPI) or 
another Xen method (i.e XENFEAT_*). For instance, the guest has to parse 
the firmware tables in order to know PSCI is available.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk]

On 10.08.17 19:11, Julien Grall wrote:
> 
> 
> On 10/08/17 16:33, Volodymyr Babchuk wrote:
>> Hi Julien,
>>
>> On 09.08.17 13:10, Julien Grall wrote:
>>> Hi Volodymyr,
>>>
>>> CC "THE REST" maintainers to get an opinion on the public headers.
>>>
>>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>>> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
>>>> SMCCC states that both HVC and SMC are valid conduits to call to a
>>>> different
>>>> firmware functions. Thus, for example PSCI calls can be made both by
>>>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>>>> Besides functional calls there are query calls, which allows underling
>>>> OS determine version, UID and number of functions provided by service
>>>> provider.
>>>>
>>>> This patch adds new file `vsmc.c`, which handles both generic SMCs
>>>> and HVC according to SMC. At this moment it implements only one
>>>> service: Standard Hypervisor Service.
>>>>
>>>> Standard Hypervisor Service only supports query calls, so caller can
>>>> ask about hypervisor UID and determine that it is XEN running.
>>>>
>>>> This change allows more generic handling for SMCs and HVCs and it can
>>>> be easily extended to support new services and functions.
>>>>
>>>> But, before SMC is forwarded to standard SMCCC handler, it can be 
>>>> routed
>>>> to a domain monitor, if one is installed.
>>>>
>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>> ---
>>>>  - Updated description to indicate that this patch affects only SMC
>>>> call path.
>>>>  - added "xen_" prefix to definitions in include/public/arch-arm/smc.h
>>>>  - moved do_trap_smc() into vsmc.c from traps.c
>>>>  - replaced all tabs with spaces
>>>
>>> I would have really appreciated a summary of the discussion we had on
>>> the previous version regarding the bindings. This is a real blocker
>>> for this series and should not be ignored.
>>>
>>
>> While I agree that question about bindings is important, I can't see how
>> it affects this patch series. This patch series does not break anything.
>> Because
>>
>> 1. This series add only new feature: generic hypervisor service with no
>> immediate use. All ARM guests are already aware that they are running on
>> XEN. All ARM guests know that they call *only* PSCI.
>>
>> 2. I see importance of this patch series for embedded platforms, where
>> developer exactly knows what software of which version he/she will run.
>> I doubt that server platforms will need something beyond PSCI, which I
>> preserved as is.
> 
> I disagree here. SMCCC could be used to provide Dom0 a way to interact 
> with the firmware if necessary.
AFAIK, Dom0 usually is built with particular version of XEN in mind (or 
at least minimal XEN version).

>>
>> A I'm not denying importance of SMC bindings, but I think it is not
>> blocker for my patches. We can add bindings later, when there will be
>> consensus on how they should look. In meantime SMC handler can be used
>> by anyone who knows that is available.
> 
> I am not in favor on getting something merged in Xen until we agree on 
> the way for the guest to know it is there. 
I think that SMC implementation will be the same, regardless the way we 
can tell guest that it is available. At this time guests can safely 
assume that SMCCC is not implemented in XEN. This wouldn't break anything.

> It means you have to carry 
> hack in your kernel in order to use SMC. Maybe this is fine for you, but 
> I don't want to make this assumption on Xen upstream today.
Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both 
cases it reads DT to get conduit method (SMC/HVC). So there are already 
bindings for generic uses. Other uses are platform-specific (okay, 
probably there can be a problem).

> This is a change in the interface that should be notified to the guest. 
> If we expose it without providing a bindings (or something), we have no 
> way to revert/disable it. Imagine we want to disable SMC in the future. 
Natural way was to disable Security Extensions in PFR1 register. This 
was not done and now we have curious situation: guest thinks that SMC is 
available, but then it gets Undefined Instruction exception when it 
tries to invoke SMC.

> How a guest will know that
>      - until Xen 4.10 SMC was not existing,
>      - between Xen 4.10 and Xen 4.x you can use them
>      - after Xen 4.y they can be disabled.
It is a broader question: how software can know that SMCCC is available 
on a platform? Not SMC, but SMCCC as a protocol. Probably, there should 
be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc 
that they can rely on SMCCC spec. I think that it is question to ARM 
guys (including you), because this affects all ARM machines.

> All changes should be detected through the firmware tables (DT, ACPI) or 
> another Xen method (i.e XENFEAT_*). For instance, the guest has to parse 
> the firmware tables in order to know PSCI is available.
Yep. The same does OP-TEE code. It parses DT to get conduit. Probably, 
this is wrong approach. Should all SMCCC calls use the same conduit?. 
Should platform provide conduit for each SMCCC service owner?

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Julien Grall]

Hi,

On 10/08/17 18:40, Volodymyr Babchuk wrote:
>
>
> On 10.08.17 19:11, Julien Grall wrote:
>>
>>
>> On 10/08/17 16:33, Volodymyr Babchuk wrote:
>>> Hi Julien,
>>>
>>> On 09.08.17 13:10, Julien Grall wrote:
>>>> Hi Volodymyr,
>>>>
>>>> CC "THE REST" maintainers to get an opinion on the public headers.
>>>>
>>>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>>>> SMCCC (SMC Call Convention) describes how to handle both HVCs and
>>>>> SMCs.
>>>>> SMCCC states that both HVC and SMC are valid conduits to call to a
>>>>> different
>>>>> firmware functions. Thus, for example PSCI calls can be made both by
>>>>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>>>>> Besides functional calls there are query calls, which allows underling
>>>>> OS determine version, UID and number of functions provided by service
>>>>> provider.
>>>>>
>>>>> This patch adds new file `vsmc.c`, which handles both generic SMCs
>>>>> and HVC according to SMC. At this moment it implements only one
>>>>> service: Standard Hypervisor Service.
>>>>>
>>>>> Standard Hypervisor Service only supports query calls, so caller can
>>>>> ask about hypervisor UID and determine that it is XEN running.
>>>>>
>>>>> This change allows more generic handling for SMCs and HVCs and it can
>>>>> be easily extended to support new services and functions.
>>>>>
>>>>> But, before SMC is forwarded to standard SMCCC handler, it can be
>>>>> routed
>>>>> to a domain monitor, if one is installed.
>>>>>
>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>> Reviewed-by: Oleksandr Andrushchenko
>>>>> <oleksandr_andrushchenko@epam.com>
>>>>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>> ---
>>>>>  - Updated description to indicate that this patch affects only SMC
>>>>> call path.
>>>>>  - added "xen_" prefix to definitions in include/public/arch-arm/smc.h
>>>>>  - moved do_trap_smc() into vsmc.c from traps.c
>>>>>  - replaced all tabs with spaces
>>>>
>>>> I would have really appreciated a summary of the discussion we had on
>>>> the previous version regarding the bindings. This is a real blocker
>>>> for this series and should not be ignored.
>>>>
>>>
>>> While I agree that question about bindings is important, I can't see how
>>> it affects this patch series. This patch series does not break anything.
>>> Because
>>>
>>> 1. This series add only new feature: generic hypervisor service with no
>>> immediate use. All ARM guests are already aware that they are running on
>>> XEN. All ARM guests know that they call *only* PSCI.
>>>
>>> 2. I see importance of this patch series for embedded platforms, where
>>> developer exactly knows what software of which version he/she will run.
>>> I doubt that server platforms will need something beyond PSCI, which I
>>> preserved as is.
>>
>> I disagree here. SMCCC could be used to provide Dom0 a way to interact
>> with the firmware if necessary.
> AFAIK, Dom0 usually is built with particular version of XEN in mind (or
> at least minimal XEN version).

That's not true. Dom0 is a generic kernel able to probe everything from 
the firmware tables and Xen interface. I use the exact Linux kernel on 
multiple platforms with no issue.

The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with 
different release cadence. We provide a stable interface that is 
backward compatible (returning error code for non-existent hypercalls). 
So a Linux released in 10 years should still work on Xen 4.10 and adapt 
to the features available.

>
>>>
>>> A I'm not denying importance of SMC bindings, but I think it is not
>>> blocker for my patches. We can add bindings later, when there will be
>>> consensus on how they should look. In meantime SMC handler can be used
>>> by anyone who knows that is available.
>>
>> I am not in favor on getting something merged in Xen until we agree on
>> the way for the guest to know it is there.
> I think that SMC implementation will be the same, regardless the way we
> can tell guest that it is available. At this time guests can safely
> assume that SMCCC is not implemented in XEN. This wouldn't break anything.

So you suggest to merge this series but says "The guest should not 
assume the presence of SMC"? This is rather a bit odd...

>
>> It means you have to carry hack in your kernel in order to use SMC.
>> Maybe this is fine for you, but I don't want to make this assumption
>> on Xen upstream today.
> Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both
> cases it reads DT to get conduit method (SMC/HVC). So there are already
> bindings for generic uses. Other uses are platform-specific (okay,
> probably there can be a problem).
>
>> This is a change in the interface that should be notified to the
>> guest. If we expose it without providing a bindings (or something), we
>> have no way to revert/disable it. Imagine we want to disable SMC in
>> the future.
> Natural way was to disable Security Extensions in PFR1 register. This
> was not done and now we have curious situation: guest thinks that SMC is
> available, but then it gets Undefined Instruction exception when it
> tries to invoke SMC.

Security extensions does not mean that SMCCC is present... Even if we 
had returned an error, you don't know if it was from the SMCCC protocol 
or my foo protocol.

It would be valid (though a bit odd) for a firmware to inject an 
undefined instruction if it is unable to understand the SMC.

The main problem today is neither HVC #0 nor SMC #0 are SMCCC compliant. 
They are let's call it XEN Protocol compliant. KVM has the same problem.

So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant, 
you can go ahead probing the different components".

As I said, I am not asking you to write the binding. I am asking you to 
have in mind other use cases. If the binding takes to much time, then a 
solution would be to add a XENFEAT_*.

>
>> How a guest will know that
>>      - until Xen 4.10 SMC was not existing,
>>      - between Xen 4.10 and Xen 4.x you can use them
>>      - after Xen 4.y they can be disabled.
> It is a broader question: how software can know that SMCCC is available
> on a platform? Not SMC, but SMCCC as a protocol. Probably, there should
> be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc
> that they can rely on SMCCC spec. I think that it is question to ARM
> guys (including you), because this affects all ARM machines.

I spent quite some time to explain that in an e-mail to the previous 
version... See the thread [1].

>
>> All changes should be detected through the firmware tables (DT, ACPI)
>> or another Xen method (i.e XENFEAT_*). For instance, the guest has to
>> parse the firmware tables in order to know PSCI is available.
> Yep. The same does OP-TEE code. It parses DT to get conduit. Probably,
> this is wrong approach. Should all SMCCC calls use the same conduit?.
> Should platform provide conduit for each SMCCC service owner?

Technically a guest should only use HVC. I am happy to allow the SMC to 
support unmodified OS that don't use DT.

Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html

-- 
Julien Grall

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk]

Hi,

On 10.08.17 21:18, Julien Grall wrote:
> Hi,
> 
> On 10/08/17 18:40, Volodymyr Babchuk wrote:
>>
>>
>> On 10.08.17 19:11, Julien Grall wrote:
>>>
>>>
>>> On 10/08/17 16:33, Volodymyr Babchuk wrote:
>>>> Hi Julien,
>>>>
>>>> On 09.08.17 13:10, Julien Grall wrote:
>>>>> Hi Volodymyr,
>>>>>
>>>>> CC "THE REST" maintainers to get an opinion on the public headers.
>>>>>
>>>>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>>>>> SMCCC (SMC Call Convention) describes how to handle both HVCs and
>>>>>> SMCs.
>>>>>> SMCCC states that both HVC and SMC are valid conduits to call to a
>>>>>> different
>>>>>> firmware functions. Thus, for example PSCI calls can be made both by
>>>>>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>>>>>> Besides functional calls there are query calls, which allows 
>>>>>> underling
>>>>>> OS determine version, UID and number of functions provided by service
>>>>>> provider.
>>>>>>
>>>>>> This patch adds new file `vsmc.c`, which handles both generic SMCs
>>>>>> and HVC according to SMC. At this moment it implements only one
>>>>>> service: Standard Hypervisor Service.
>>>>>>
>>>>>> Standard Hypervisor Service only supports query calls, so caller can
>>>>>> ask about hypervisor UID and determine that it is XEN running.
>>>>>>
>>>>>> This change allows more generic handling for SMCs and HVCs and it can
>>>>>> be easily extended to support new services and functions.
>>>>>>
>>>>>> But, before SMC is forwarded to standard SMCCC handler, it can be
>>>>>> routed
>>>>>> to a domain monitor, if one is installed.
>>>>>>
>>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>>> Reviewed-by: Oleksandr Andrushchenko
>>>>>> <oleksandr_andrushchenko@epam.com>
>>>>>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>> ---
>>>>>>  - Updated description to indicate that this patch affects only SMC
>>>>>> call path.
>>>>>>  - added "xen_" prefix to definitions in 
>>>>>> include/public/arch-arm/smc.h
>>>>>>  - moved do_trap_smc() into vsmc.c from traps.c
>>>>>>  - replaced all tabs with spaces
>>>>>
>>>>> I would have really appreciated a summary of the discussion we had on
>>>>> the previous version regarding the bindings. This is a real blocker
>>>>> for this series and should not be ignored.
>>>>>
>>>>
>>>> While I agree that question about bindings is important, I can't see 
>>>> how
>>>> it affects this patch series. This patch series does not break 
>>>> anything.
>>>> Because
>>>>
>>>> 1. This series add only new feature: generic hypervisor service with no
>>>> immediate use. All ARM guests are already aware that they are 
>>>> running on
>>>> XEN. All ARM guests know that they call *only* PSCI.
>>>>
>>>> 2. I see importance of this patch series for embedded platforms, where
>>>> developer exactly knows what software of which version he/she will run.
>>>> I doubt that server platforms will need something beyond PSCI, which I
>>>> preserved as is.
>>>
>>> I disagree here. SMCCC could be used to provide Dom0 a way to interact
>>> with the firmware if necessary.
>> AFAIK, Dom0 usually is built with particular version of XEN in mind (or
>> at least minimal XEN version).
> 
> That's not true. Dom0 is a generic kernel able to probe everything from 
> the firmware tables and Xen interface. I use the exact Linux kernel on 
> multiple platforms with no issue.
Okay, I was speaking about embedded use case.

> The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with 
> different release cadence. We provide a stable interface that is 
> backward compatible (returning error code for non-existent hypercalls). 
> So a Linux released in 10 years should still work on Xen 4.10 and adapt 
> to the features available.
I just want to clarify this. At least four hypercalls are not backward 
compatible: platform_op, sysctl, domctl and flask_op. I had a problem 
with this, when played with MiniOS-based monitor. Sure, your kernel will 
boot up, but some well known XEN services will be absent.
This have nothing with SMC bindings problem, though.

>>
>>>>
>>>> A I'm not denying importance of SMC bindings, but I think it is not
>>>> blocker for my patches. We can add bindings later, when there will be
>>>> consensus on how they should look. In meantime SMC handler can be used
>>>> by anyone who knows that is available.
>>>
>>> I am not in favor on getting something merged in Xen until we agree on
>>> the way for the guest to know it is there.
>> I think that SMC implementation will be the same, regardless the way we
>> can tell guest that it is available. At this time guests can safely
>> assume that SMCCC is not implemented in XEN. This wouldn't break 
>> anything.
> 
> So you suggest to merge this series but says "The guest should not 
> assume the presence of SMC"? This is rather a bit odd...
Basically yes. This is one of the options. If guest knowns for sure that 
SMCCC is available - it can use it. If it is unsure - then it does not 
use it.
In meantime we can develop a generic way to tell guest that SMCCC is 
present on a platform.
I just don't want to bloat up this patch series. There are already 7 
patches and looks like there will be more.
But I can add another patch/two with bindings if you insist. This is not 
a big deal. I just have concerns about this right now.

>>
>>> It means you have to carry hack in your kernel in order to use SMC.
>>> Maybe this is fine for you, but I don't want to make this assumption
>>> on Xen upstream today.
>> Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both
>> cases it reads DT to get conduit method (SMC/HVC). So there are already
>> bindings for generic uses. Other uses are platform-specific (okay,
>> probably there can be a problem).
>>
>>> This is a change in the interface that should be notified to the
>>> guest. If we expose it without providing a bindings (or something), we
>>> have no way to revert/disable it. Imagine we want to disable SMC in
>>> the future.
>> Natural way was to disable Security Extensions in PFR1 register. This
>> was not done and now we have curious situation: guest thinks that SMC is
>> available, but then it gets Undefined Instruction exception when it
>> tries to invoke SMC.
> 
> Security extensions does not mean that SMCCC is present... Even if we 
> had returned an error, you don't know if it was from the SMCCC protocol 
> or my foo protocol.

> It would be valid (though a bit odd) for a firmware to inject an 
> undefined instruction if it is unable to understand the SMC.
Okay, I can see logic there. This is a strange way to return an error, 
but it is not prohibited in reference manual at least.

> The main problem today is neither HVC #0 nor SMC #0 are SMCCC compliant. 
> They are let's call it XEN Protocol compliant. KVM has the same problem.

> So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant, 
> you can go ahead probing the different components".
Yes, I agree with this. I just trying to say, that it is not only XEN 
problem, it is platform-wide.

> As I said, I am not asking you to write the binding. I am asking you to 
> have in mind other use cases. If the binding takes to much time, then a 
> solution would be to add a XENFEAT_*.
I can write DT binding. Problem is that this binding should be adopted 
by all OSes, hypervisors and firmwares. More on this below.

>>
>>> How a guest will know that
>>>      - until Xen 4.10 SMC was not existing,
>>>      - between Xen 4.10 and Xen 4.x you can use them
>>>      - after Xen 4.y they can be disabled.
>> It is a broader question: how software can know that SMCCC is available
>> on a platform? Not SMC, but SMCCC as a protocol. Probably, there should
>> be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc
>> that they can rely on SMCCC spec. I think that it is question to ARM
>> guys (including you), because this affects all ARM machines.
> 
> I spent quite some time to explain that in an e-mail to the previous 
> version... See the thread [1].
Yes, I've seen this thread, naturally. But I can't find there answer to 
my question.
As I said, this binding (or any other way to tell software about SMCCC) 
should not be XEN-specific. As you mentioned, KVM have the exactly same 
problem. I think, bare-metal platforms suffer from similar issue. So, in 
my opinion, it will be great to have agreement on SMCCC discovery from 
all players. So, when I boot my kernel on any ARM machine, it can tell 
for sure if SMCCC is supported. What to you think?

We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT. 
But is this right?
>>
>>> All changes should be detected through the firmware tables (DT, ACPI)
>>> or another Xen method (i.e XENFEAT_*). For instance, the guest has to
>>> parse the firmware tables in order to know PSCI is available.
>> Yep. The same does OP-TEE code. It parses DT to get conduit. Probably,
>> this is wrong approach. Should all SMCCC calls use the same conduit?.
>> Should platform provide conduit for each SMCCC service owner?
> 
> Technically a guest should only use HVC. I am happy to allow the SMC to 
> support unmodified OS that don't use DT.
> Cheers,
> 
> [1] 
> https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html
> 

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Julien Grall]

On 10/08/2017 21:09, Volodymyr Babchuk wrote:
> Hi,
>
> On 10.08.17 21:18, Julien Grall wrote:
>> Hi,
>>
>> On 10/08/17 18:40, Volodymyr Babchuk wrote:
>>>
>>>
>>> On 10.08.17 19:11, Julien Grall wrote:
>>>>
>>>>
>>>> On 10/08/17 16:33, Volodymyr Babchuk wrote:
>>>>> Hi Julien,
>>>>>
>>>>> On 09.08.17 13:10, Julien Grall wrote:
>>>>>> Hi Volodymyr,
>>>>>>
>>>>>> CC "THE REST" maintainers to get an opinion on the public headers.
>>>>>>
>>>>>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>>>>>> SMCCC (SMC Call Convention) describes how to handle both HVCs and
>>>>>>> SMCs.
>>>>>>> SMCCC states that both HVC and SMC are valid conduits to call to a
>>>>>>> different
>>>>>>> firmware functions. Thus, for example PSCI calls can be made both by
>>>>>>> SMC or HVC. Also SMCCC defines function number coding for such
>>>>>>> calls.
>>>>>>> Besides functional calls there are query calls, which allows
>>>>>>> underling
>>>>>>> OS determine version, UID and number of functions provided by
>>>>>>> service
>>>>>>> provider.
>>>>>>>
>>>>>>> This patch adds new file `vsmc.c`, which handles both generic SMCs
>>>>>>> and HVC according to SMC. At this moment it implements only one
>>>>>>> service: Standard Hypervisor Service.
>>>>>>>
>>>>>>> Standard Hypervisor Service only supports query calls, so caller can
>>>>>>> ask about hypervisor UID and determine that it is XEN running.
>>>>>>>
>>>>>>> This change allows more generic handling for SMCs and HVCs and it
>>>>>>> can
>>>>>>> be easily extended to support new services and functions.
>>>>>>>
>>>>>>> But, before SMC is forwarded to standard SMCCC handler, it can be
>>>>>>> routed
>>>>>>> to a domain monitor, if one is installed.
>>>>>>>
>>>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>>>> Reviewed-by: Oleksandr Andrushchenko
>>>>>>> <oleksandr_andrushchenko@epam.com>
>>>>>>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>> ---
>>>>>>>  - Updated description to indicate that this patch affects only SMC
>>>>>>> call path.
>>>>>>>  - added "xen_" prefix to definitions in
>>>>>>> include/public/arch-arm/smc.h
>>>>>>>  - moved do_trap_smc() into vsmc.c from traps.c
>>>>>>>  - replaced all tabs with spaces
>>>>>>
>>>>>> I would have really appreciated a summary of the discussion we had on
>>>>>> the previous version regarding the bindings. This is a real blocker
>>>>>> for this series and should not be ignored.
>>>>>>
>>>>>
>>>>> While I agree that question about bindings is important, I can't
>>>>> see how
>>>>> it affects this patch series. This patch series does not break
>>>>> anything.
>>>>> Because
>>>>>
>>>>> 1. This series add only new feature: generic hypervisor service
>>>>> with no
>>>>> immediate use. All ARM guests are already aware that they are
>>>>> running on
>>>>> XEN. All ARM guests know that they call *only* PSCI.
>>>>>
>>>>> 2. I see importance of this patch series for embedded platforms, where
>>>>> developer exactly knows what software of which version he/she will
>>>>> run.
>>>>> I doubt that server platforms will need something beyond PSCI, which I
>>>>> preserved as is.
>>>>
>>>> I disagree here. SMCCC could be used to provide Dom0 a way to interact
>>>> with the firmware if necessary.
>>> AFAIK, Dom0 usually is built with particular version of XEN in mind (or
>>> at least minimal XEN version).
>>
>> That's not true. Dom0 is a generic kernel able to probe everything
>> from the firmware tables and Xen interface. I use the exact Linux
>> kernel on multiple platforms with no issue.
> Okay, I was speaking about embedded use case.

Bear in mind that Xen is not only embedded. When you upstream a new 
feature you have to think about how this could be used by anyone.

>
>> The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with
>> different release cadence. We provide a stable interface that is
>> backward compatible (returning error code for non-existent
>> hypercalls). So a Linux released in 10 years should still work on Xen
>> 4.10 and adapt to the features available.
> I just want to clarify this. At least four hypercalls are not backward
> compatible: platform_op, sysctl, domctl and flask_op. I had a problem
> with this, when played with MiniOS-based monitor. Sure, your kernel will
> boot up, but some well known XEN services will be absent.
> This have nothing with SMC bindings problem, though.

I am not sure to follow here... Were they ever be supported on ARM? If 
not, then it is no a backward compatibility issue. You cannot assume 
that all the hypercalls existing on x86 will be available on ARM.

For a list of known available/supported hypercalls for ARM see:
public/include/arch-arm.h.

Also bear in mind that some hypercalls are not part of the stable ABI. 
For instance domctls are not stable and it is well-known that you have 
to rebuild your app for every new Xen versions...

But domctls will never be used in Linux Kernel as they only meant to be 
used to control a domain.

>
>>>
>>>>>
>>>>> A I'm not denying importance of SMC bindings, but I think it is not
>>>>> blocker for my patches. We can add bindings later, when there will be
>>>>> consensus on how they should look. In meantime SMC handler can be used
>>>>> by anyone who knows that is available.
>>>>
>>>> I am not in favor on getting something merged in Xen until we agree on
>>>> the way for the guest to know it is there.
>>> I think that SMC implementation will be the same, regardless the way we
>>> can tell guest that it is available. At this time guests can safely
>>> assume that SMCCC is not implemented in XEN. This wouldn't break
>>> anything.
>>
>> So you suggest to merge this series but says "The guest should not
>> assume the presence of SMC"? This is rather a bit odd...
> Basically yes. This is one of the options. If guest knowns for sure that
> SMCCC is available - it can use it. If it is unsure - then it does not
> use it.
> In meantime we can develop a generic way to tell guest that SMCCC is
> present on a platform.
> I just don't want to bloat up this patch series. There are already 7
> patches and looks like there will be more.

Let's be honest, a 7 patches series is quite small :P.

> But I can add another patch/two with bindings if you insist. This is not
> a big deal. I just have concerns about this right now.
>
>>>
>>>> It means you have to carry hack in your kernel in order to use SMC.
>>>> Maybe this is fine for you, but I don't want to make this assumption
>>>> on Xen upstream today.
>>> Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both
>>> cases it reads DT to get conduit method (SMC/HVC). So there are already
>>> bindings for generic uses. Other uses are platform-specific (okay,
>>> probably there can be a problem).
>>>
>>>> This is a change in the interface that should be notified to the
>>>> guest. If we expose it without providing a bindings (or something), we
>>>> have no way to revert/disable it. Imagine we want to disable SMC in
>>>> the future.
>>> Natural way was to disable Security Extensions in PFR1 register. This
>>> was not done and now we have curious situation: guest thinks that SMC is
>>> available, but then it gets Undefined Instruction exception when it
>>> tries to invoke SMC.
>>
>> Security extensions does not mean that SMCCC is present... Even if we
>> had returned an error, you don't know if it was from the SMCCC
>> protocol or my foo protocol.
>
>> It would be valid (though a bit odd) for a firmware to inject an
>> undefined instruction if it is unable to understand the SMC.
> Okay, I can see logic there. This is a strange way to return an error,
> but it is not prohibited in reference manual at least.
>
>> The main problem today is neither HVC #0 nor SMC #0 are SMCCC
>> compliant. They are let's call it XEN Protocol compliant. KVM has the
>> same problem.
>
>> So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant,
>> you can go ahead probing the different components".
> Yes, I agree with this. I just trying to say, that it is not only XEN
> problem, it is platform-wide.

I am well aware about it... and kept saying that from the beginning. 
Except that nobody really thought about it because embedded are mostly 
focused to get things working for a specific set of configuration.

>
>> As I said, I am not asking you to write the binding. I am asking you
>> to have in mind other use cases. If the binding takes to much time,
>> then a solution would be to add a XENFEAT_*.
> I can write DT binding. Problem is that this binding should be adopted
> by all OSes, hypervisors and firmwares. More on this below.
>
>>>
>>>> How a guest will know that
>>>>      - until Xen 4.10 SMC was not existing,
>>>>      - between Xen 4.10 and Xen 4.x you can use them
>>>>      - after Xen 4.y they can be disabled.
>>> It is a broader question: how software can know that SMCCC is available
>>> on a platform? Not SMC, but SMCCC as a protocol. Probably, there should
>>> be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc
>>> that they can rely on SMCCC spec. I think that it is question to ARM
>>> guys (including you), because this affects all ARM machines.
>>
>> I spent quite some time to explain that in an e-mail to the previous
>> version... See the thread [1].
> Yes, I've seen this thread, naturally. But I can't find there answer to
> my question.

Because there is none of the moment and it is still in discussion...

> As I said, this binding (or any other way to tell software about SMCCC)
> should not be XEN-specific. As you mentioned, KVM have the exactly same
> problem. I think, bare-metal platforms suffer from similar issue. So, in
> my opinion, it will be great to have agreement on SMCCC discovery from
> all players. So, when I boot my kernel on any ARM machine, it can tell
> for sure if SMCCC is supported. What to you think?

There are already discussion going-on. It is known and taken care.

>
> We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT.
> But is this right?
>>>
>>>> All changes should be detected through the firmware tables (DT, ACPI)
>>>> or another Xen method (i.e XENFEAT_*). For instance, the guest has to
>>>> parse the firmware tables in order to know PSCI is available.
>>> Yep. The same does OP-TEE code. It parses DT to get conduit. Probably,
>>> this is wrong approach. Should all SMCCC calls use the same conduit?.
>>> Should platform provide conduit for each SMCCC service owner?
>>
>> Technically a guest should only use HVC. I am happy to allow the SMC
>> to support unmodified OS that don't use DT.
>> Cheers,
>>
>> [1]
>> https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html
>>
>>

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Julien Grall]

Hi Volodymyr,

On 10/08/17 22:09, Julien Grall wrote:
>>
>> We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT.
>> But is this right?

Answering to this question after some thoughts and discussion around.

The generic approach is indeed the best solution, however I am afraid it 
will take some times to get ready and might not be ready for Xen 4.10.

As I mentioned earlier, I don't want to merge this without any way to 
discover the existence of SMCCC in the hypervisor. I also understand 
that you would like to get this merged in Xen 4.10.

So the alternative we have is to provide a XEN-only approach for the 
time being. It has to work for both ACPI and DT, this likely means we 
want a XENFEAT_* flag.

That alternative would be supersede when the generic approach will be 
available. Though we would have to keep both around, but it is it not a 
big deal.

Does it make sense?

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk]

Hi Julien,

On 11.08.17 00:09, Julien Grall wrote:
> 
> 
> On 10/08/2017 21:09, Volodymyr Babchuk wrote:
>> Hi,
>>
>> On 10.08.17 21:18, Julien Grall wrote:
>>> Hi,
>>>
>>> On 10/08/17 18:40, Volodymyr Babchuk wrote:
>>>>
>>>>
>>>> On 10.08.17 19:11, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 10/08/17 16:33, Volodymyr Babchuk wrote:
>>>>>> Hi Julien,
>>>>>>
>>>>>> On 09.08.17 13:10, Julien Grall wrote:
>>>>>>> Hi Volodymyr,
>>>>>>>
>>>>>>> CC "THE REST" maintainers to get an opinion on the public headers.
>>>>>>>
>>>>>>> On 08/08/17 21:08, Volodymyr Babchuk wrote:
>>>>>>>> SMCCC (SMC Call Convention) describes how to handle both HVCs and
>>>>>>>> SMCs.
>>>>>>>> SMCCC states that both HVC and SMC are valid conduits to call to a
>>>>>>>> different
>>>>>>>> firmware functions. Thus, for example PSCI calls can be made 
>>>>>>>> both by
>>>>>>>> SMC or HVC. Also SMCCC defines function number coding for such
>>>>>>>> calls.
>>>>>>>> Besides functional calls there are query calls, which allows
>>>>>>>> underling
>>>>>>>> OS determine version, UID and number of functions provided by
>>>>>>>> service
>>>>>>>> provider.
>>>>>>>>
>>>>>>>> This patch adds new file `vsmc.c`, which handles both generic SMCs
>>>>>>>> and HVC according to SMC. At this moment it implements only one
>>>>>>>> service: Standard Hypervisor Service.
>>>>>>>>
>>>>>>>> Standard Hypervisor Service only supports query calls, so caller 
>>>>>>>> can
>>>>>>>> ask about hypervisor UID and determine that it is XEN running.
>>>>>>>>
>>>>>>>> This change allows more generic handling for SMCs and HVCs and it
>>>>>>>> can
>>>>>>>> be easily extended to support new services and functions.
>>>>>>>>
>>>>>>>> But, before SMC is forwarded to standard SMCCC handler, it can be
>>>>>>>> routed
>>>>>>>> to a domain monitor, if one is installed.
>>>>>>>>
>>>>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>>>>> Reviewed-by: Oleksandr Andrushchenko
>>>>>>>> <oleksandr_andrushchenko@epam.com>
>>>>>>>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>>> ---
>>>>>>>>  - Updated description to indicate that this patch affects only SMC
>>>>>>>> call path.
>>>>>>>>  - added "xen_" prefix to definitions in
>>>>>>>> include/public/arch-arm/smc.h
>>>>>>>>  - moved do_trap_smc() into vsmc.c from traps.c
>>>>>>>>  - replaced all tabs with spaces
>>>>>>>
>>>>>>> I would have really appreciated a summary of the discussion we 
>>>>>>> had on
>>>>>>> the previous version regarding the bindings. This is a real blocker
>>>>>>> for this series and should not be ignored.
>>>>>>>
>>>>>>
>>>>>> While I agree that question about bindings is important, I can't
>>>>>> see how
>>>>>> it affects this patch series. This patch series does not break
>>>>>> anything.
>>>>>> Because
>>>>>>
>>>>>> 1. This series add only new feature: generic hypervisor service
>>>>>> with no
>>>>>> immediate use. All ARM guests are already aware that they are
>>>>>> running on
>>>>>> XEN. All ARM guests know that they call *only* PSCI.
>>>>>>
>>>>>> 2. I see importance of this patch series for embedded platforms, 
>>>>>> where
>>>>>> developer exactly knows what software of which version he/she will
>>>>>> run.
>>>>>> I doubt that server platforms will need something beyond PSCI, 
>>>>>> which I
>>>>>> preserved as is.
>>>>>
>>>>> I disagree here. SMCCC could be used to provide Dom0 a way to interact
>>>>> with the firmware if necessary.
>>>> AFAIK, Dom0 usually is built with particular version of XEN in mind (or
>>>> at least minimal XEN version).
>>>
>>> That's not true. Dom0 is a generic kernel able to probe everything
>>> from the firmware tables and Xen interface. I use the exact Linux
>>> kernel on multiple platforms with no issue.
>> Okay, I was speaking about embedded use case.
> 
> Bear in mind that Xen is not only embedded. When you upstream a new 
> feature you have to think about how this could be used by anyone.
>>
>>> The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with
>>> different release cadence. We provide a stable interface that is
>>> backward compatible (returning error code for non-existent
>>> hypercalls). So a Linux released in 10 years should still work on Xen
>>> 4.10 and adapt to the features available.
>> I just want to clarify this. At least four hypercalls are not backward
>> compatible: platform_op, sysctl, domctl and flask_op. I had a problem
>> with this, when played with MiniOS-based monitor. Sure, your kernel will
>> boot up, but some well known XEN services will be absent.
>> This have nothing with SMC bindings problem, though.
> 
> I am not sure to follow here... Were they ever be supported on ARM? If 
> not, then it is no a backward compatibility issue. You cannot assume 
> that all the hypercalls existing on x86 will be available on ARM.
> 
> For a list of known available/supported hypercalls for ARM see:
> public/include/arch-arm.h.
> 
> Also bear in mind that some hypercalls are not part of the stable ABI. 
> For instance domctls are not stable and it is well-known that you have 
> to rebuild your app for every new Xen versions...
> 
> But domctls will never be used in Linux Kernel as they only meant to be 
> used to control a domain.
> 
>>
>>>>
>>>>>>
>>>>>> A I'm not denying importance of SMC bindings, but I think it is not
>>>>>> blocker for my patches. We can add bindings later, when there will be
>>>>>> consensus on how they should look. In meantime SMC handler can be 
>>>>>> used
>>>>>> by anyone who knows that is available.
>>>>>
>>>>> I am not in favor on getting something merged in Xen until we agree on
>>>>> the way for the guest to know it is there.
>>>> I think that SMC implementation will be the same, regardless the way we
>>>> can tell guest that it is available. At this time guests can safely
>>>> assume that SMCCC is not implemented in XEN. This wouldn't break
>>>> anything.
>>>
>>> So you suggest to merge this series but says "The guest should not
>>> assume the presence of SMC"? This is rather a bit odd...
>> Basically yes. This is one of the options. If guest knowns for sure that
>> SMCCC is available - it can use it. If it is unsure - then it does not
>> use it.
>> In meantime we can develop a generic way to tell guest that SMCCC is
>> present on a platform.
>> I just don't want to bloat up this patch series. There are already 7
>> patches and looks like there will be more.
> 
> Let's be honest, a 7 patches series is quite small :P.
Aaaah, okay :)

>> But I can add another patch/two with bindings if you insist. This is not
>> a big deal. I just have concerns about this right now.
>>
>>>>
>>>>> It means you have to carry hack in your kernel in order to use SMC.
>>>>> Maybe this is fine for you, but I don't want to make this assumption
>>>>> on Xen upstream today.
>>>> Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both
>>>> cases it reads DT to get conduit method (SMC/HVC). So there are already
>>>> bindings for generic uses. Other uses are platform-specific (okay,
>>>> probably there can be a problem).
>>>>
>>>>> This is a change in the interface that should be notified to the
>>>>> guest. If we expose it without providing a bindings (or something), we
>>>>> have no way to revert/disable it. Imagine we want to disable SMC in
>>>>> the future.
>>>> Natural way was to disable Security Extensions in PFR1 register. This
>>>> was not done and now we have curious situation: guest thinks that 
>>>> SMC is
>>>> available, but then it gets Undefined Instruction exception when it
>>>> tries to invoke SMC.
>>>
>>> Security extensions does not mean that SMCCC is present... Even if we
>>> had returned an error, you don't know if it was from the SMCCC
>>> protocol or my foo protocol.
>>
>>> It would be valid (though a bit odd) for a firmware to inject an
>>> undefined instruction if it is unable to understand the SMC.
>> Okay, I can see logic there. This is a strange way to return an error,
>> but it is not prohibited in reference manual at least.
>>
>>> The main problem today is neither HVC #0 nor SMC #0 are SMCCC
>>> compliant. They are let's call it XEN Protocol compliant. KVM has the
>>> same problem.
>>
>>> So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant,
>>> you can go ahead probing the different components".
>> Yes, I agree with this. I just trying to say, that it is not only XEN
>> problem, it is platform-wide.
> 
> I am well aware about it... and kept saying that from the beginning. 
> Except that nobody really thought about it because embedded are mostly 
> focused to get things working for a specific set of configuration.
Yes, I see. Okay, I'm curious on how such thinks should be worked out. 
E.g. XEN ML is not the best place to discuss platform-wide questions.

>>
>>> As I said, I am not asking you to write the binding. I am asking you
>>> to have in mind other use cases. If the binding takes to much time,
>>> then a solution would be to add a XENFEAT_*.
>> I can write DT binding. Problem is that this binding should be adopted
>> by all OSes, hypervisors and firmwares. More on this below.
>>
>>>>
>>>>> How a guest will know that
>>>>>      - until Xen 4.10 SMC was not existing,
>>>>>      - between Xen 4.10 and Xen 4.x you can use them
>>>>>      - after Xen 4.y they can be disabled.
>>>> It is a broader question: how software can know that SMCCC is available
>>>> on a platform? Not SMC, but SMCCC as a protocol. Probably, there should
>>>> be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc
>>>> that they can rely on SMCCC spec. I think that it is question to ARM
>>>> guys (including you), because this affects all ARM machines.
>>>
>>> I spent quite some time to explain that in an e-mail to the previous
>>> version... See the thread [1].
>> Yes, I've seen this thread, naturally. But I can't find there answer to
>> my question.
> 
> Because there is none of the moment and it is still in discussion...
> 
>> As I said, this binding (or any other way to tell software about SMCCC)
>> should not be XEN-specific. As you mentioned, KVM have the exactly same
>> problem. I think, bare-metal platforms suffer from similar issue. So, in
>> my opinion, it will be great to have agreement on SMCCC discovery from
>> all players. So, when I boot my kernel on any ARM machine, it can tell
>> for sure if SMCCC is supported. What to you think?
> 
> There are already discussion going-on. It is known and taken care.
Are you referring to that thread at [1]? Or to some internal discussion?

>>
>> We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT.
>> But is this right?
>>>>
>>>>> All changes should be detected through the firmware tables (DT, ACPI)
>>>>> or another Xen method (i.e XENFEAT_*). For instance, the guest has to
>>>>> parse the firmware tables in order to know PSCI is available.
>>>> Yep. The same does OP-TEE code. It parses DT to get conduit. Probably,
>>>> this is wrong approach. Should all SMCCC calls use the same conduit?.
>>>> Should platform provide conduit for each SMCCC service owner?
>>>
>>> Technically a guest should only use HVC. I am happy to allow the SMC
>>> to support unmodified OS that don't use DT.
>>> Cheers,
>>>
>>> [1]
>>> https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html 
>>>
>>>
>>>
> 
> Cheers,
> 

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

[PATCH 5/7] arm: traps: handle PSCI calls inside `smccc.c`

[Volodymyr Babchuk]

PSCI is part of HVC/SMC interface, so it should be handled in
appropriate place: `vsmc.c`. This patch just moves PSCI
handler calls from `traps.c` to `vsmc.c`.

PSCI is considered as two different "services" in terms of SMCCC.
Older PSCI 1.0 is treated as "architecture service", while never
PSCI 2.0 is defined as "standard secure service".

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 - Added do_trap_hvc() handler in vsmc.c


---
 xen/arch/arm/traps.c              | 124 ++------------------------------
 xen/arch/arm/vsmc.c               | 144 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vsmc.h        |   1 +
 xen/include/public/arch-arm/smc.h |  10 ++-
 4 files changed, 160 insertions(+), 119 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index b119dc0..7b296da 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -39,7 +39,6 @@
 #include <asm/event.h>
 #include <asm/regs.h>
 #include <asm/cpregs.h>
-#include <asm/psci.h>
 #include <asm/mmio.h>
 #include <asm/cpufeature.h>
 #include <asm/flushtlb.h>
@@ -1446,119 +1445,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 }
 #endif
 
-#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
-#define PSCI_ARG(reg,n) get_user_reg(reg, n)
-
-#ifdef CONFIG_ARM_64
-#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
-#else
-#define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
-#endif
-
-/* 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 & PSCI_0_2_64BIT) >> 30 ) );
-}
-
-static void do_trap_psci(struct cpu_user_regs *regs)
-{
-    register_t fid = PSCI_ARG(regs,0);
-
-    /* preloading in case psci_mode_check fails */
-    PSCI_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
-    switch( fid )
-    {
-    case PSCI_cpu_off:
-        {
-            uint32_t pstate = PSCI_ARG32(regs,1);
-            perfc_incr(vpsci_cpu_off);
-            PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
-        }
-        break;
-    case PSCI_cpu_on:
-        {
-            uint32_t vcpuid = PSCI_ARG32(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            perfc_incr(vpsci_cpu_on);
-            PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
-        }
-        break;
-    case PSCI_0_2_FN_PSCI_VERSION:
-        perfc_incr(vpsci_version);
-        PSCI_SET_RESULT(regs, do_psci_0_2_version());
-        break;
-    case PSCI_0_2_FN_CPU_OFF:
-        perfc_incr(vpsci_cpu_off);
-        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
-        break;
-    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
-        perfc_incr(vpsci_migrate_info_type);
-        PSCI_SET_RESULT(regs, do_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:
-        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());
-        break;
-    case PSCI_0_2_FN_SYSTEM_OFF:
-        perfc_incr(vpsci_system_off);
-        do_psci_0_2_system_off();
-        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
-        break;
-    case PSCI_0_2_FN_SYSTEM_RESET:
-        perfc_incr(vpsci_system_reset);
-        do_psci_0_2_system_reset();
-        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
-        break;
-    case PSCI_0_2_FN_CPU_ON:
-    case PSCI_0_2_FN64_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));
-        }
-        break;
-    case PSCI_0_2_FN_CPU_SUSPEND:
-    case PSCI_0_2_FN64_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));
-        }
-        break;
-    case PSCI_0_2_FN_AFFINITY_INFO:
-    case PSCI_0_2_FN64_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));
-        }
-        break;
-    case PSCI_0_2_FN_MIGRATE:
-    case PSCI_0_2_FN64_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));
-        }
-        break;
-    default:
-        domain_crash_synchronous();
-        return;
-    }
-}
-
 #ifdef CONFIG_ARM_64
 #define HYPERCALL_RESULT_REG(r) (r)->x0
 #define HYPERCALL_ARG1(r) (r)->x0
@@ -2865,8 +2751,9 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
             return do_debug_trap(regs, hsr.iss & 0x00ff);
 #endif
         if ( hsr.iss == 0 )
-            return do_trap_psci(regs);
-        do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
+            do_trap_hvc_smccc(regs);
+        else
+            do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
         break;
 #ifdef CONFIG_ARM_64
     case HSR_EC_HVC64:
@@ -2877,8 +2764,9 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
             return do_debug_trap(regs, hsr.iss & 0x00ff);
 #endif
         if ( hsr.iss == 0 )
-            return do_trap_psci(regs);
-        do_trap_hypercall(regs, &regs->x16, hsr.iss);
+            do_trap_hvc_smccc(regs);
+        else
+            do_trap_hypercall(regs, &regs->x16, hsr.iss);
         break;
     case HSR_EC_SMC64:
         /*
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 5ef6167..718b30f 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -19,12 +19,16 @@
 #include <xen/types.h>
 #include <public/arch-arm/smc.h>
 #include <asm/monitor.h>
+#include <asm/psci.h>
 #include <asm/vsmc.h>
 #include <asm/regs.h>
 
 /* Number of functions currently supported by Hypervisor Service. */
 #define XEN_SMCCC_FUNCTION_COUNT 3
 
+/* Number of functions currently supported by Standard Service Service. */
+#define SSC_SMCCC_FUNCTION_COUNT 13
+
 /* SMCCC interface for hypervisor. Tell about itself. */
 static bool handle_hypervisor(struct cpu_user_regs *regs)
 {
@@ -47,6 +51,133 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
     return false;
 }
 
+#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
+#define PSCI_ARG(reg,n) get_user_reg(reg, n)
+
+#ifdef CONFIG_ARM_64
+#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
+#else
+#define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
+#endif
+
+/* old (arvm7) PSCI interface */
+static bool handle_arch(struct cpu_user_regs *regs)
+{
+    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
+    {
+    case PSCI_cpu_off:
+    {
+        uint32_t pstate = PSCI_ARG32(regs,1);
+        perfc_incr(vpsci_cpu_off);
+        PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
+    }
+    return true;
+    case PSCI_cpu_on:
+    {
+        uint32_t vcpuid = PSCI_ARG32(regs,1);
+        register_t epoint = PSCI_ARG(regs,2);
+        perfc_incr(vpsci_cpu_on);
+        PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
+    }
+    return true;
+    }
+    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 & PSCI_0_2_64BIT) >> 30 ) );
+}
+
+/* PSCI 2.0 interface */
+static bool handle_ssc(struct cpu_user_regs *regs)
+{
+    register_t fid = PSCI_ARG(regs, 0);
+
+    switch ( ARM_SMCCC_FUNC_NUM(fid) )
+    {
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
+        perfc_incr(vpsci_version);
+        PSCI_SET_RESULT(regs, do_psci_0_2_version());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
+        perfc_incr(vpsci_cpu_off);
+        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
+        perfc_incr(vpsci_migrate_info_type);
+        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
+        perfc_incr(vpsci_system_off);
+        do_psci_0_2_system_off();
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
+        perfc_incr(vpsci_system_reset);
+        do_psci_0_2_system_reset();
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_CALL_COUNT:
+        set_user_reg(regs, 0, SSC_SMCCC_FUNCTION_COUNT);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_UID:
+        set_user_reg(regs, 0, SSC_SMCCC_UID.a[0]);
+        set_user_reg(regs, 1, SSC_SMCCC_UID.a[1]);
+        set_user_reg(regs, 2, SSC_SMCCC_UID.a[2]);
+        set_user_reg(regs, 3, SSC_SMCCC_UID.a[3]);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_REVISION:
+        set_user_reg(regs, 0, SSC_SMCCC_MAJOR_REVISION);
+        set_user_reg(regs, 1, SSC_SMCCC_MINOR_REVISION);
+        return true;
+    }
+    return false;
+}
+
 /*
  * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
  * returns true if that was valid SMCCC call (even if function number
@@ -91,6 +222,12 @@ static bool vsmc_handle_call(struct cpu_user_regs *regs)
     case ARM_SMCCC_OWNER_HYPERVISOR:
         handled = handle_hypervisor(regs);
         break;
+    case ARM_SMCCC_OWNER_ARCH:
+        handled = handle_arch(regs);
+        break;
+    case ARM_SMCCC_OWNER_STANDARD:
+        handled = handle_ssc(regs);
+        break;
     }
 
     if ( !handled )
@@ -129,6 +266,13 @@ void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
         inject_undef_exception(regs, hsr);
 }
 
+/* This function will be called from traps.c */
+void do_trap_hvc_smccc(struct cpu_user_regs *regs)
+{
+    if ( !vsmc_handle_call(regs) )
+        domain_crash_synchronous();
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
index 7baabef..d1e7b01 100644
--- a/xen/include/asm-arm/vsmc.h
+++ b/xen/include/asm-arm/vsmc.h
@@ -81,6 +81,7 @@
 #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
 
 void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
+void do_trap_hvc_smccc(struct cpu_user_regs *regs);
 
 #endif  /* __ASM_ARM_VSMC_H__ */
 
diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
index 42f3165..11b5ef7 100644
--- a/xen/include/public/arch-arm/smc.h
+++ b/xen/include/public/arch-arm/smc.h
@@ -56,8 +56,16 @@ typedef struct {
                                         0x9a, 0xcf, 0x79, 0xd1, \
                                         0x8d, 0xde, 0xe6, 0x67)
 
-#endif  /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
+/* Standard Service version. Check comment for Hypervisor Service for rules. */
+#define SSC_SMCCC_MAJOR_REVISION 0
+#define SSC_SMCCC_MINOR_REVISION 1
+
+/* Standard Service Call UID. Randomly generated with 3rd party tool. */
+#define SSC_SMCCC_UID XEN_ARM_SMCCC_UID(0xf863386f, 0x4b39, 0x4cbd, \
+                                        0x92, 0x20, 0xce, 0x16, \
+                                        0x41, 0xe5, 0x9f, 0x6f)
 
+#endif  /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
 /*
  * Local variables:
  * mode: C
-- 
2.7.4


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

Re: [PATCH 5/7] arm: traps: handle PSCI calls inside `smccc.c`

[Julien Grall]

Hi Volodymyr,

The title is wrong.

On 08/08/17 21:08, Volodymyr Babchuk wrote:
> PSCI is part of HVC/SMC interface, so it should be handled in
> appropriate place: `vsmc.c`. This patch just moves PSCI
> handler calls from `traps.c` to `vsmc.c`.
>
> PSCI is considered as two different "services" in terms of SMCCC.
> Older PSCI 1.0 is treated as "architecture service", while never

s/never/newer/

> PSCI 2.0 is defined as "standard secure service".

Hmmm. PSCI 2.0 does not exist. I am aware of 4 versions for PSCI:
	- 0.1
	- 0.2
	- 1.0
	- 1.1

We support all of them but 1.1.

For 0.1 the PSCI id is 1 (cpu_off) and 2 (cpu_on). So I think they are 
not part of the "architecture service" but "existing APIs". This mean 
you broke support of PSCI 0.1 on Xen.

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  - Added do_trap_hvc() handler in vsmc.c
>
>
> ---
>  xen/arch/arm/traps.c              | 124 ++------------------------------
>  xen/arch/arm/vsmc.c               | 144 ++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vsmc.h        |   1 +
>  xen/include/public/arch-arm/smc.h |  10 ++-
>  4 files changed, 160 insertions(+), 119 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index b119dc0..7b296da 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -39,7 +39,6 @@
>  #include <asm/event.h>
>  #include <asm/regs.h>
>  #include <asm/cpregs.h>
> -#include <asm/psci.h>
>  #include <asm/mmio.h>
>  #include <asm/cpufeature.h>
>  #include <asm/flushtlb.h>
> @@ -1446,119 +1445,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  }
>  #endif
>
> -#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
> -#define PSCI_ARG(reg,n) get_user_reg(reg, n)
> -
> -#ifdef CONFIG_ARM_64
> -#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
> -#else
> -#define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
> -#endif
> -
> -/* 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 & PSCI_0_2_64BIT) >> 30 ) );
> -}
> -
> -static void do_trap_psci(struct cpu_user_regs *regs)
> -{
> -    register_t fid = PSCI_ARG(regs,0);
> -
> -    /* preloading in case psci_mode_check fails */
> -    PSCI_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
> -    switch( fid )
> -    {
> -    case PSCI_cpu_off:
> -        {
> -            uint32_t pstate = PSCI_ARG32(regs,1);
> -            perfc_incr(vpsci_cpu_off);
> -            PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
> -        }
> -        break;
> -    case PSCI_cpu_on:
> -        {
> -            uint32_t vcpuid = PSCI_ARG32(regs,1);
> -            register_t epoint = PSCI_ARG(regs,2);
> -            perfc_incr(vpsci_cpu_on);
> -            PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
> -        }
> -        break;
> -    case PSCI_0_2_FN_PSCI_VERSION:
> -        perfc_incr(vpsci_version);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_version());
> -        break;
> -    case PSCI_0_2_FN_CPU_OFF:
> -        perfc_incr(vpsci_cpu_off);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
> -        break;
> -    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> -        perfc_incr(vpsci_migrate_info_type);
> -        PSCI_SET_RESULT(regs, do_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:
> -        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());
> -        break;
> -    case PSCI_0_2_FN_SYSTEM_OFF:
> -        perfc_incr(vpsci_system_off);
> -        do_psci_0_2_system_off();
> -        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> -        break;
> -    case PSCI_0_2_FN_SYSTEM_RESET:
> -        perfc_incr(vpsci_system_reset);
> -        do_psci_0_2_system_reset();
> -        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> -        break;
> -    case PSCI_0_2_FN_CPU_ON:
> -    case PSCI_0_2_FN64_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));
> -        }
> -        break;
> -    case PSCI_0_2_FN_CPU_SUSPEND:
> -    case PSCI_0_2_FN64_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));
> -        }
> -        break;
> -    case PSCI_0_2_FN_AFFINITY_INFO:
> -    case PSCI_0_2_FN64_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));
> -        }
> -        break;
> -    case PSCI_0_2_FN_MIGRATE:
> -    case PSCI_0_2_FN64_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));
> -        }
> -        break;
> -    default:
> -        domain_crash_synchronous();
> -        return;
> -    }
> -}
> -
>  #ifdef CONFIG_ARM_64
>  #define HYPERCALL_RESULT_REG(r) (r)->x0
>  #define HYPERCALL_ARG1(r) (r)->x0
> @@ -2865,8 +2751,9 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
>              return do_debug_trap(regs, hsr.iss & 0x00ff);
>  #endif
>          if ( hsr.iss == 0 )
> -            return do_trap_psci(regs);
> -        do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
> +            do_trap_hvc_smccc(regs);
> +        else
> +            do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);

If you do return do_trap_hvc_smcc(regs); you would avoid to modify the 
rest of the function.

>          break;
>  #ifdef CONFIG_ARM_64
>      case HSR_EC_HVC64:
> @@ -2877,8 +2764,9 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
>              return do_debug_trap(regs, hsr.iss & 0x00ff);
>  #endif
>          if ( hsr.iss == 0 )
> -            return do_trap_psci(regs);
> -        do_trap_hypercall(regs, &regs->x16, hsr.iss);
> +            do_trap_hvc_smccc(regs);
> +        else
> +            do_trap_hypercall(regs, &regs->x16, hsr.iss);

Ditto.

>          break;
>      case HSR_EC_SMC64:
>          /*
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 5ef6167..718b30f 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -19,12 +19,16 @@
>  #include <xen/types.h>
>  #include <public/arch-arm/smc.h>
>  #include <asm/monitor.h>
> +#include <asm/psci.h>
>  #include <asm/vsmc.h>
>  #include <asm/regs.h>
>
>  /* Number of functions currently supported by Hypervisor Service. */
>  #define XEN_SMCCC_FUNCTION_COUNT 3
>
> +/* Number of functions currently supported by Standard Service Service. */
> +#define SSC_SMCCC_FUNCTION_COUNT 13
> +
>  /* SMCCC interface for hypervisor. Tell about itself. */
>  static bool handle_hypervisor(struct cpu_user_regs *regs)
>  {
> @@ -47,6 +51,133 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
>      return false;
>  }
>
> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
> +#define PSCI_ARG(reg,n) get_user_reg(reg, n)
> +
> +#ifdef CONFIG_ARM_64
> +#define PSCI_ARG32(reg,n) (uint32_t)(get_user_reg(reg, n) & 0x00000000FFFFFFFF)
> +#else
> +#define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
> +#endif
> +
> +/* old (arvm7) PSCI interface */

I don't think this is true. PSCI 0.1 is also supported by ARM64.

> +static bool handle_arch(struct cpu_user_regs *regs)
> +{
> +    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
> +    {
> +    case PSCI_cpu_off:
> +    {
> +        uint32_t pstate = PSCI_ARG32(regs,1);
> +        perfc_incr(vpsci_cpu_off);
> +        PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
> +    }
> +    return true;

Please address the comment made in on v2.

> +    case PSCI_cpu_on:
> +    {
> +        uint32_t vcpuid = PSCI_ARG32(regs,1);
> +        register_t epoint = PSCI_ARG(regs,2);
> +        perfc_incr(vpsci_cpu_on);
> +        PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
> +    }
> +    return true;

Ditto.

> +    }
> +    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 & PSCI_0_2_64BIT) >> 30 ) );
> +}
> +
> +/* PSCI 2.0 interface */
> +static bool handle_ssc(struct cpu_user_regs *regs)

ssc stands for?

> +{
> +    register_t fid = PSCI_ARG(regs, 0);
> +
> +    switch ( ARM_SMCCC_FUNC_NUM(fid) )
> +    {
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
> +        perfc_incr(vpsci_version);
> +        PSCI_SET_RESULT(regs, do_psci_0_2_version());
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
> +        perfc_incr(vpsci_cpu_off);
> +        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
> +        perfc_incr(vpsci_migrate_info_type);
> +        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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());
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
> +        perfc_incr(vpsci_system_off);
> +        do_psci_0_2_system_off();
> +        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
> +        perfc_incr(vpsci_system_reset);
> +        do_psci_0_2_system_reset();
> +        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_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));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_COUNT:
> +        set_user_reg(regs, 0, SSC_SMCCC_FUNCTION_COUNT);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_UID:
> +        set_user_reg(regs, 0, SSC_SMCCC_UID.a[0]);
> +        set_user_reg(regs, 1, SSC_SMCCC_UID.a[1]);
> +        set_user_reg(regs, 2, SSC_SMCCC_UID.a[2]);
> +        set_user_reg(regs, 3, SSC_SMCCC_UID.a[3]);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_REVISION:
> +        set_user_reg(regs, 0, SSC_SMCCC_MAJOR_REVISION);
> +        set_user_reg(regs, 1, SSC_SMCCC_MINOR_REVISION);
> +        return true;
> +    }
> +    return false;
> +}
> +
>  /*
>   * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
>   * returns true if that was valid SMCCC call (even if function number
> @@ -91,6 +222,12 @@ static bool vsmc_handle_call(struct cpu_user_regs *regs)
>      case ARM_SMCCC_OWNER_HYPERVISOR:
>          handled = handle_hypervisor(regs);
>          break;
> +    case ARM_SMCCC_OWNER_ARCH:
> +        handled = handle_arch(regs);
> +        break;
> +    case ARM_SMCCC_OWNER_STANDARD:
> +        handled = handle_ssc(regs);
> +        break;
>      }
>
>      if ( !handled )
> @@ -129,6 +266,13 @@ void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
>          inject_undef_exception(regs, hsr);
>  }
>
> +/* This function will be called from traps.c */
> +void do_trap_hvc_smccc(struct cpu_user_regs *regs)
> +{
> +    if ( !vsmc_handle_call(regs) )

It is a bit weird to call a function name "smc" in fro HVC.

> +        domain_crash_synchronous();

Hmmm that's not compliant to the SMCCC.

> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
> index 7baabef..d1e7b01 100644
> --- a/xen/include/asm-arm/vsmc.h
> +++ b/xen/include/asm-arm/vsmc.h
> @@ -81,6 +81,7 @@
>  #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>
>  void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
> +void do_trap_hvc_smccc(struct cpu_user_regs *regs);
>
>  #endif  /* __ASM_ARM_VSMC_H__ */
>
> diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
> index 42f3165..11b5ef7 100644
> --- a/xen/include/public/arch-arm/smc.h
> +++ b/xen/include/public/arch-arm/smc.h
> @@ -56,8 +56,16 @@ typedef struct {
>                                          0x9a, 0xcf, 0x79, 0xd1, \
>                                          0x8d, 0xde, 0xe6, 0x67)
>
> -#endif  /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
> +/* Standard Service version. Check comment for Hypervisor Service for rules. */
> +#define SSC_SMCCC_MAJOR_REVISION 0
> +#define SSC_SMCCC_MINOR_REVISION 1
> +
> +/* Standard Service Call UID. Randomly generated with 3rd party tool. */
> +#define SSC_SMCCC_UID XEN_ARM_SMCCC_UID(0xf863386f, 0x4b39, 0x4cbd, \
> +                                        0x92, 0x20, 0xce, 0x16, \
> +                                        0x41, 0xe5, 0x9f, 0x6f)
>
> +#endif  /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */

This change is here because you dropped a newline between the #endif and 
the emacs magic.

>  /*
>   * Local variables:
>   * mode: C
>

Cheers,

-- 
Julien Grall

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

[PATCH 6/7] arm: psci: use definitions provided by vsmc.h

[Volodymyr Babchuk]

vsmc.h provides definitions to construct SMC call function number according
to SMCCC. We don't need multiple definitions for one thing, and definitions
in vsmc.h are more generic than ones used in psci.h.

So psci.h will only provide function codes and whole SMC function number
will be constructed using generic macroses from vsmc.h.

This change also affects vsmc.c and seattle.c, because they both use PSCI
function numbers.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
This is new patch, suggested by Julien Grail.

---
 xen/arch/arm/platforms/seattle.c |  5 +++--
 xen/arch/arm/psci.c              | 12 ++++++------
 xen/arch/arm/vsmc.c              | 26 +++++++++++++------------
 xen/include/asm-arm/psci.h       | 41 ++++++++++++++++++----------------------
 4 files changed, 41 insertions(+), 43 deletions(-)

diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
index 86dce91..679632d 100644
--- a/xen/arch/arm/platforms/seattle.c
+++ b/xen/arch/arm/platforms/seattle.c
@@ -19,6 +19,7 @@
 
 #include <asm/platform.h>
 #include <asm/psci.h>
+#include <asm/vsmc.h>
 
 static const char * const seattle_dt_compat[] __initconst =
 {
@@ -33,12 +34,12 @@ static const char * const seattle_dt_compat[] __initconst =
  */
 static void seattle_system_reset(void)
 {
-    call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+    call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_SYSTEM_RESET), 0, 0, 0);
 }
 
 static void seattle_system_off(void)
 {
-    call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
+    call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_SYSTEM_OFF), 0, 0, 0);
 }
 
 PLATFORM_START(seattle, "SEATTLE")
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 34ee97e..4b01131 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -31,9 +31,9 @@
  * (native-width) function ID.
  */
 #ifdef CONFIG_ARM_64
-#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN64_##name
+#define PSCI_0_2_FN_NATIVE(n)   PSCI_0_2_FN64(n)
 #else
-#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN_##name
+#define PSCI_0_2_FN_NATIVE(n)   PSCI_0_2_FN32(n)
 #endif
 
 uint32_t psci_ver;
@@ -48,13 +48,13 @@ int call_psci_cpu_on(int cpu)
 void call_psci_system_off(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
-        call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
+        call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_SYSTEM_OFF), 0, 0, 0);
 }
 
 void call_psci_system_reset(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
-        call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+        call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_SYSTEM_RESET), 0, 0, 0);
 }
 
 int __init psci_is_smc_method(const struct dt_device_node *psci)
@@ -144,7 +144,7 @@ int __init psci_init_0_2(void)
         }
     }
 
-    psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
+    psci_ver = call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_PSCI_VERSION), 0, 0, 0);
 
     /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
     if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 )
@@ -154,7 +154,7 @@ int __init psci_init_0_2(void)
         return -EOPNOTSUPP;
     }
 
-    psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
+    psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(PSCI_0_2_FUNC_CPU_ON);
 
     printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
            PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 718b30f..ea86eea 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -87,7 +87,9 @@ static bool handle_arch(struct cpu_user_regs *regs)
 /* 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 & PSCI_0_2_64BIT) >> 30 ) );
+    return !( is_64bit_domain(d)^
+              ((fid & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT)) >>
+               ARM_SMCCC_CALL_CONV_SHIFT) );
 }
 
 /* PSCI 2.0 interface */
@@ -97,34 +99,34 @@ static bool handle_ssc(struct cpu_user_regs *regs)
 
     switch ( ARM_SMCCC_FUNC_NUM(fid) )
     {
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
+    case PSCI_0_2_FUNC_PSCI_VERSION:
         perfc_incr(vpsci_version);
         PSCI_SET_RESULT(regs, do_psci_0_2_version());
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
+    case PSCI_0_2_FUNC_CPU_OFF:
         perfc_incr(vpsci_cpu_off);
         PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
+    case PSCI_0_2_FUNC_MIGRATE_INFO_TYPE:
         perfc_incr(vpsci_migrate_info_type);
         PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
+    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());
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
+    case PSCI_0_2_FUNC_SYSTEM_OFF:
         perfc_incr(vpsci_system_off);
         do_psci_0_2_system_off();
         PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
-        return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
+       return true;
+    case PSCI_0_2_FUNC_SYSTEM_RESET:
         perfc_incr(vpsci_system_reset);
         do_psci_0_2_system_reset();
         PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
+    case PSCI_0_2_FUNC_CPU_ON:
         perfc_incr(vpsci_cpu_on);
         if ( psci_mode_check(current->domain, fid) )
         {
@@ -134,7 +136,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
             PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
         }
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
+    case PSCI_0_2_FUNC_CPU_SUSPEND:
         perfc_incr(vpsci_cpu_suspend);
         if ( psci_mode_check(current->domain, fid) )
         {
@@ -144,7 +146,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
             PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
         }
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
+    case PSCI_0_2_FUNC_AFFINITY_INFO:
         perfc_incr(vpsci_cpu_affinity_info);
         if ( psci_mode_check(current->domain, fid) )
         {
@@ -153,7 +155,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
             PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
         }
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
+    case PSCI_0_2_FUNC_MIGRATE:
         perfc_incr(vpsci_cpu_migrate);
         if ( psci_mode_check(current->domain, fid) )
         {
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index be2458a..af6edf8 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_PSCI_H__
 #define __ASM_PSCI_H__
 
+#include <asm/vsmc.h>
+
 /* PSCI return values (inclusive of all PSCI versions) */
 #define PSCI_SUCCESS                 0
 #define PSCI_NOT_SUPPORTED          -1
@@ -41,30 +43,23 @@ register_t 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);
 
-/* 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_FN32(n) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                \
+                                            ARM_SMCCC_SMC_32,                   \
+                                            ARM_SMCCC_OWNER_STANDARD, n)
+#define PSCI_0_2_FN64(n) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                \
+                                            ARM_SMCCC_SMC_64,                   \
+                                            ARM_SMCCC_OWNER_STANDARD, n)
 
-#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)
+#define PSCI_0_2_FUNC_PSCI_VERSION        0
+#define PSCI_0_2_FUNC_CPU_SUSPEND         1
+#define PSCI_0_2_FUNC_CPU_OFF             2
+#define PSCI_0_2_FUNC_CPU_ON              3
+#define PSCI_0_2_FUNC_AFFINITY_INFO       4
+#define PSCI_0_2_FUNC_MIGRATE             5
+#define PSCI_0_2_FUNC_MIGRATE_INFO_TYPE   6
+#define PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU 7
+#define PSCI_0_2_FUNC_SYSTEM_OFF          8
+#define PSCI_0_2_FUNC_SYSTEM_RESET        9
 
 /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
 #define PSCI_0_2_AFFINITY_LEVEL_ON      0
-- 
2.7.4


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

Re: [PATCH 6/7] arm: psci: use definitions provided by vsmc.h

[Julien Grall]

title: s/psci/PSCI/

On 08/08/17 21:08, Volodymyr Babchuk wrote:
> vsmc.h provides definitions to construct SMC call function number according
> to SMCCC. We don't need multiple definitions for one thing, and definitions
> in vsmc.h are more generic than ones used in psci.h.
>
> So psci.h will only provide function codes and whole SMC function number
> will be constructed using generic macroses from vsmc.h.

s/macroses/macros/

>
> This change also affects vsmc.c and seattle.c, because they both use PSCI
> function numbers.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> This is new patch, suggested by Julien Grail.
>
> ---
>  xen/arch/arm/platforms/seattle.c |  5 +++--
>  xen/arch/arm/psci.c              | 12 ++++++------
>  xen/arch/arm/vsmc.c              | 26 +++++++++++++------------
>  xen/include/asm-arm/psci.h       | 41 ++++++++++++++++++----------------------
>  4 files changed, 41 insertions(+), 43 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
> index 86dce91..679632d 100644
> --- a/xen/arch/arm/platforms/seattle.c
> +++ b/xen/arch/arm/platforms/seattle.c
> @@ -19,6 +19,7 @@
>
>  #include <asm/platform.h>
>  #include <asm/psci.h>
> +#include <asm/vsmc.h>

vsmc.h stands for "virtual SMC". Here you use for the "physical SMC". So 
please don't include vsmc.h here. Likely the correct solution is to move 
out any generic smccc code in a separate header and use it everywhere.

Furthermore, psci.h already include the header so I don't see any reason 
to do it another time here.

>
>  static const char * const seattle_dt_compat[] __initconst =
>  {
> @@ -33,12 +34,12 @@ static const char * const seattle_dt_compat[] __initconst =
>   */
>  static void seattle_system_reset(void)
>  {
> -    call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +    call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_SYSTEM_RESET), 0, 0, 0);

Can we please define PSCI_0_2_FN32_SYSTEM_RESET & co? This would 
simplify the code a bit and avoid duplicating the name twice.

Or you could just pass SYSTEM_RESET to PSCI_0_2_FN32(...) and do the 
concatenation in it.

>  }
>
>  static void seattle_system_off(void)
>  {
> -    call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +    call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_SYSTEM_OFF), 0, 0, 0);
>  }
>
>  PLATFORM_START(seattle, "SEATTLE")
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 34ee97e..4b01131 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -31,9 +31,9 @@
>   * (native-width) function ID.
>   */
>  #ifdef CONFIG_ARM_64
> -#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN64_##name
> +#define PSCI_0_2_FN_NATIVE(n)   PSCI_0_2_FN64(n)
>  #else
> -#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN_##name
> +#define PSCI_0_2_FN_NATIVE(n)   PSCI_0_2_FN32(n)
>  #endif

You could also have done

#define PSCI_0_2_FN_NATIVE(name) PSCI_0_2_FN32(PSCI_0_2_FN_##name)

Avoiding modifying all the caller below and duplicating the name.

>
>  uint32_t psci_ver;
> @@ -48,13 +48,13 @@ int call_psci_cpu_on(int cpu)
>  void call_psci_system_off(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
> -        call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +        call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_SYSTEM_OFF), 0, 0, 0);
>  }
>
>  void call_psci_system_reset(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
> -        call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +        call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_SYSTEM_RESET), 0, 0, 0);
>  }
>
>  int __init psci_is_smc_method(const struct dt_device_node *psci)
> @@ -144,7 +144,7 @@ int __init psci_init_0_2(void)
>          }
>      }
>
> -    psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> +    psci_ver = call_smc(PSCI_0_2_FN32(PSCI_0_2_FUNC_PSCI_VERSION), 0, 0, 0);
>
>      /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
>      if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 )
> @@ -154,7 +154,7 @@ int __init psci_init_0_2(void)
>          return -EOPNOTSUPP;
>      }
>
> -    psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
> +    psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(PSCI_0_2_FUNC_CPU_ON);
>
>      printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
>             PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 718b30f..ea86eea 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -87,7 +87,9 @@ static bool handle_arch(struct cpu_user_regs *regs)
>  /* 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 & PSCI_0_2_64BIT) >> 30 ) );
> +    return !( is_64bit_domain(d)^
> +              ((fid & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT)) >>
> +               ARM_SMCCC_CALL_CONV_SHIFT) );

This is not really obvious to read. Can we please abstract this?

>  }
>
>  /* PSCI 2.0 interface */
> @@ -97,34 +99,34 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>
>      switch ( ARM_SMCCC_FUNC_NUM(fid) )
>      {
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
> +    case PSCI_0_2_FUNC_PSCI_VERSION:
>          perfc_incr(vpsci_version);
>          PSCI_SET_RESULT(regs, do_psci_0_2_version());
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
> +    case PSCI_0_2_FUNC_CPU_OFF:
>          perfc_incr(vpsci_cpu_off);
>          PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
> +    case PSCI_0_2_FUNC_MIGRATE_INFO_TYPE:
>          perfc_incr(vpsci_migrate_info_type);
>          PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
> +    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());
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
> +    case PSCI_0_2_FUNC_SYSTEM_OFF:
>          perfc_incr(vpsci_system_off);
>          do_psci_0_2_system_off();
>          PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> -        return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
> +       return true;
> +    case PSCI_0_2_FUNC_SYSTEM_RESET:
>          perfc_incr(vpsci_system_reset);
>          do_psci_0_2_system_reset();
>          PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
> +    case PSCI_0_2_FUNC_CPU_ON:
>          perfc_incr(vpsci_cpu_on);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -134,7 +136,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>              PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
>          }
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
> +    case PSCI_0_2_FUNC_CPU_SUSPEND:
>          perfc_incr(vpsci_cpu_suspend);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -144,7 +146,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>              PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
>          }
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
> +    case PSCI_0_2_FUNC_AFFINITY_INFO:
>          perfc_incr(vpsci_cpu_affinity_info);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -153,7 +155,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>              PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
>          }
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
> +    case PSCI_0_2_FUNC_MIGRATE:
>          perfc_incr(vpsci_cpu_migrate);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index be2458a..af6edf8 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -1,6 +1,8 @@
>  #ifndef __ASM_PSCI_H__
>  #define __ASM_PSCI_H__
>
> +#include <asm/vsmc.h>
> +
>  /* PSCI return values (inclusive of all PSCI versions) */
>  #define PSCI_SUCCESS                 0
>  #define PSCI_NOT_SUPPORTED          -1
> @@ -41,30 +43,23 @@ register_t 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);
>
> -/* 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_FN32(n) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                \
> +                                            ARM_SMCCC_SMC_32,                   \
> +                                            ARM_SMCCC_OWNER_STANDARD, n)
> +#define PSCI_0_2_FN64(n) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,                \
> +                                            ARM_SMCCC_SMC_64,                   \
> +                                            ARM_SMCCC_OWNER_STANDARD, n)
>
> -#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)
> +#define PSCI_0_2_FUNC_PSCI_VERSION        0
> +#define PSCI_0_2_FUNC_CPU_SUSPEND         1
> +#define PSCI_0_2_FUNC_CPU_OFF             2
> +#define PSCI_0_2_FUNC_CPU_ON              3
> +#define PSCI_0_2_FUNC_AFFINITY_INFO       4
> +#define PSCI_0_2_FUNC_MIGRATE             5
> +#define PSCI_0_2_FUNC_MIGRATE_INFO_TYPE   6
> +#define PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU 7
> +#define PSCI_0_2_FUNC_SYSTEM_OFF          8
> +#define PSCI_0_2_FUNC_SYSTEM_RESET        9

Any reason to move from FN to FUNC?

>
>  /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
>  #define PSCI_0_2_AFFINITY_LEVEL_ON      0
>

Cheers,

-- 
Julien Grall

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

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

[Volodymyr Babchuk]

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.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
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);
-- 
2.7.4


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

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

[Julien Grall]

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@epam.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> 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@lists.xen.org
https://lists.xen.org/xen-devel