Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-09-21 Thread Julien Grall 



On 20/09/17 21:26, Volodymyr Babchuk wrote:



On 20.09.17 23:02, Julien Grall wrote:



On 20/09/2017 19:11, Volodymyr Babchuk wrote:

On 20.09.17 20:21, Julien Grall wrote:



On 19/09/17 22:44, Volodymyr Babchuk wrote:

Hi Julien,


Hi Volodymyr,



On 13.09.17 14:11, Julien Grall wrote:

Hi,

On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:



+static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t

*u)


Actually why do you pass a pointer for u? This requires every caller
to introduce temporary variable because the UUID is usually a define.

Hmm, another way probably is to pass a whole structure as a parameter.
Are you suggesting this approach? Something like
fill_uuid(regs, (xen_uuid_t)MY_UUID)?


Something list that. But why do you need the cast? MY_UUID is supposed
to be a xen_uuid_t. No?

It have no type. It is just an initializer list like {1,2,3,4,5,6}. If
you remember that thread, there is a requirement to make public headers
compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.
Instead it is defined as a plain initializer list.


In that case why don't introduce a version for non-strict ansi? This 
would introduce a bit of safety and avoid cast a bit unexplained. (see 
how __DECL_REG(...,...) is done in include/public/asm-arm.h?

I believe you meant arch-arm.h.

Just to be clear, you are proposing to introduce one
#define XEN_DEFINE_UUID in a public header, and another one in a private 
header?


No the two in the public header. One version for strict-ansi compiler, 
the other for gcc-compatible one.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-09-20 Thread Volodymyr Babchuk 



On 20.09.17 23:02, Julien Grall wrote:



On 20/09/2017 19:11, Volodymyr Babchuk wrote:

On 20.09.17 20:21, Julien Grall wrote:



On 19/09/17 22:44, Volodymyr Babchuk wrote:

Hi Julien,


Hi Volodymyr,



On 13.09.17 14:11, Julien Grall wrote:

Hi,

On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:



+static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t

*u)


Actually why do you pass a pointer for u? This requires every caller
to introduce temporary variable because the UUID is usually a define.

Hmm, another way probably is to pass a whole structure as a parameter.
Are you suggesting this approach? Something like
fill_uuid(regs, (xen_uuid_t)MY_UUID)?


Something list that. But why do you need the cast? MY_UUID is supposed
to be a xen_uuid_t. No?

It have no type. It is just an initializer list like {1,2,3,4,5,6}. If
you remember that thread, there is a requirement to make public headers
compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.
Instead it is defined as a plain initializer list.


In that case why don't introduce a version for non-strict ansi? This 
would introduce a bit of safety and avoid cast a bit unexplained. (see 
how __DECL_REG(...,...) is done in include/public/asm-arm.h?

I believe you meant arch-arm.h.

Just to be clear, you are proposing to introduce one
#define XEN_DEFINE_UUID in a public header, and another one in a private 
header?
Public one will be strictly ANSI-compatible, while private one will be 
only gcc-compatible? That is doable, but it will require #ifdef magic 
across different headers.

If other maintainers are okay with that, I can do it in this way.






With your current solution each caller as to do:

xen_uuid_t foo = MY_UUID;

fill_uuid(regs, );

return true;

What I suggested in the previous version is to get fill_uuid return
true. So you make each caller simpler.

Yes, but it will not be correct semantically. There will arise many
questions:
1. Why helper function that only writes data returns bool?
2. If it returns true, can it return false?
3. Should we check its return value before passing it further?



I really don't see how

return fill_uuid(regs, MY_UUID);

would be semantically incorrect or even raise all those questions. It is
perfectly fine to always return true. We have place like that and it
helps to streamline the code.This is a somewhat arguable topic.

Yes, your variant produces less lines of code.
But it is harder to read, actually. `return fill_uid(regs, MY_UUID)`
implies that there are some logic in `fill_uid()` and it can return
different results, depending on its parameters. Which is not true and,
thus, misleading.
Just try to look at this from stranger's point of view. Usually you
don't declare function as a `bool` just to spare a line of code. If you
see boolean function, you expect that there are some reason, some logic
behind this.


You didn't get my point. When doing emulation, most of the time case in 
a switch have similar prototype. It gets an input and may (or may not) 
return an error. It is easier to think of each case with the same 
prototype rather than using different one for the sake of avoiding using 
bool because you would always return true. It can also help to switch to 
an array instead.
Aha, looks like  I'm beginning to see what you are mean. Okay I'll make 
those helpers to return bool.




Look, I have an idea how to resolve this.
fill_uid() will check that MY_UUID != {, f, f, ,
}.
If UUID is invalid, it will print warning to console and return false.
If UUID is valid, it will fill registers and return true.
Now it will be semantically correct to define it as `bool fill_uuid()`


Just no. UUID are coming from the hypervisor and most of the time (if 
not always) static. There are clearly no point to always check the UUID 
is valid when it is likely is.

Good point





This can work for `fill_uuid()`. But you also expressed the same idea
regarding code that return version. I can create helper function that
fills registers with version info. But there I don't see any excuse to
declare that helper as boolean.


The idea is you are going to abstract each case and make easier to 
potentially use an array rather than switch in some cases. So you want 
to return bool for every function.


Anyway, there are other bugs to fix and it seems unhelpful to argue 
here. I will write a follow-up because I don't like the idea of trying 
to adapt prototype just because "it does not make sense to return true...".

Okay, I think I got your idea. And, anyways, you are maintainer :)
Will do as you say.


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

Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-09-20 Thread Julien Grall 



On 20/09/2017 19:11, Volodymyr Babchuk wrote:

On 20.09.17 20:21, Julien Grall wrote:



On 19/09/17 22:44, Volodymyr Babchuk wrote:

Hi Julien,


Hi Volodymyr,



On 13.09.17 14:11, Julien Grall wrote:

Hi,

On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:



+static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t

*u)


Actually why do you pass a pointer for u? This requires every caller
to introduce temporary variable because the UUID is usually a define.

Hmm, another way probably is to pass a whole structure as a parameter.
Are you suggesting this approach? Something like
fill_uuid(regs, (xen_uuid_t)MY_UUID)?


Something list that. But why do you need the cast? MY_UUID is supposed
to be a xen_uuid_t. No?

It have no type. It is just an initializer list like {1,2,3,4,5,6}. If
you remember that thread, there is a requirement to make public headers
compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.
Instead it is defined as a plain initializer list.


In that case why don't introduce a version for non-strict ansi? This 
would introduce a bit of safety and avoid cast a bit unexplained. (see 
how __DECL_REG(...,...) is done in include/public/asm-arm.h?







With your current solution each caller as to do:

xen_uuid_t foo = MY_UUID;

fill_uuid(regs, );

return true;

What I suggested in the previous version is to get fill_uuid return
true. So you make each caller simpler.

Yes, but it will not be correct semantically. There will arise many
questions:
1. Why helper function that only writes data returns bool?
2. If it returns true, can it return false?
3. Should we check its return value before passing it further?



I really don't see how

return fill_uuid(regs, MY_UUID);

would be semantically incorrect or even raise all those questions. It is
perfectly fine to always return true. We have place like that and it
helps to streamline the code.This is a somewhat arguable topic.

Yes, your variant produces less lines of code.
But it is harder to read, actually. `return fill_uid(regs, MY_UUID)`
implies that there are some logic in `fill_uid()` and it can return
different results, depending on its parameters. Which is not true and,
thus, misleading.
Just try to look at this from stranger's point of view. Usually you
don't declare function as a `bool` just to spare a line of code. If you
see boolean function, you expect that there are some reason, some logic
behind this.


You didn't get my point. When doing emulation, most of the time case in 
a switch have similar prototype. It gets an input and may (or may not) 
return an error. It is easier to think of each case with the same 
prototype rather than using different one for the sake of avoiding using 
bool because you would always return true. It can also help to switch to 
an array instead.




Look, I have an idea how to resolve this.
fill_uid() will check that MY_UUID != {, f, f, ,
}.
If UUID is invalid, it will print warning to console and return false.
If UUID is valid, it will fill registers and return true.
Now it will be semantically correct to define it as `bool fill_uuid()`


Just no. UUID are coming from the hypervisor and most of the time (if 
not always) static. There are clearly no point to always check the UUID 
is valid when it is likely is.




This can work for `fill_uuid()`. But you also expressed the same idea
regarding code that return version. I can create helper function that
fills registers with version info. But there I don't see any excuse to
declare that helper as boolean.


The idea is you are going to abstract each case and make easier to 
potentially use an array rather than switch in some cases. So you want 
to return bool for every function.


Anyway, there are other bugs to fix and it seems unhelpful to argue 
here. I will write a follow-up because I don't like the idea of trying 
to adapt prototype just because "it does not make sense to return true...".


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-09-20 Thread Volodymyr Babchuk 

Hi Julien,

On 20.09.17 20:21, Julien Grall wrote:
>
>
> On 19/09/17 22:44, Volodymyr Babchuk wrote:
>> Hi Julien,
>
> Hi Volodymyr,
>
>>
>> On 13.09.17 14:11, Julien Grall wrote:
>>> Hi,
>>>
>>> On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
>>
 +static void fill_uuid(struct cpu_user_regs *regs, const 
xen_uuid_t *u)

>>>
>>> Actually why do you pass a pointer for u? This requires every caller
>>> to introduce temporary variable because the UUID is usually a define.
>> Hmm, another way probably is to pass a whole structure as a parameter.
>> Are you suggesting this approach? Something like
>> fill_uuid(regs, (xen_uuid_t)MY_UUID)?
>
> Something list that. But why do you need the cast? MY_UUID is supposed
> to be a xen_uuid_t. No?
It have no type. It is just an initializer list like {1,2,3,4,5,6}. If 
you remember that thread, there is a requirement to make public headers 
compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.

Instead it is defined as a plain initializer list.

>>
>>> With your current solution each caller as to do:
>>>
>>> xen_uuid_t foo = MY_UUID;
>>>
>>> fill_uuid(regs, );
>>>
>>> return true;
>>>
>>> What I suggested in the previous version is to get fill_uuid return
>>> true. So you make each caller simpler.
>> Yes, but it will not be correct semantically. There will arise many
>> questions:
>> 1. Why helper function that only writes data returns bool?
>> 2. If it returns true, can it return false?
>> 3. Should we check its return value before passing it further?
>
>
> I really don't see how
>
> return fill_uuid(regs, MY_UUID);
>
> would be semantically incorrect or even raise all those questions. It is
> perfectly fine to always return true. We have place like that and it
> helps to streamline the code.This is a somewhat arguable topic.
Yes, your variant produces less lines of code.
But it is harder to read, actually. `return fill_uid(regs, MY_UUID)`
implies that there are some logic in `fill_uid()` and it can return 
different results, depending on its parameters. Which is not true and, 
thus, misleading.
Just try to look at this from stranger's point of view. Usually you 
don't declare function as a `bool` just to spare a line of code. If you 
see boolean function, you expect that there are some reason, some logic 
behind this.


Look, I have an idea how to resolve this.
fill_uid() will check that MY_UUID != {, f, f, , 
}.

If UUID is invalid, it will print warning to console and return false.
If UUID is valid, it will fill registers and return true.
Now it will be semantically correct to define it as `bool fill_uuid()`

This can work for `fill_uuid()`. But you also expressed the same idea 
regarding code that return version. I can create helper function that 
fills registers with version info. But there I don't see any excuse to 
declare that helper as boolean.


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

Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-09-20 Thread Julien Grall 



On 19/09/17 22:44, Volodymyr Babchuk wrote:

Hi Julien,


Hi Volodymyr,



On 13.09.17 14:11, Julien Grall wrote:

Hi,

On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:



+static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t *u)


Actually why do you pass a pointer for u? This requires every caller 
to introduce temporary variable because the UUID is usually a define.

Hmm, another way probably is to pass a whole structure as a parameter.
Are you suggesting this approach? Something like
fill_uuid(regs, (xen_uuid_t)MY_UUID)?


Something list that. But why do you need the cast? MY_UUID is supposed 
to be a xen_uuid_t. No?





With your current solution each caller as to do:

xen_uuid_t foo = MY_UUID;

fill_uuid(regs, );

return true;

What I suggested in the previous version is to get fill_uuid return 
true. So you make each caller simpler.
Yes, but it will not be correct semantically. There will arise many 
questions:

1. Why helper function that only writes data returns bool?
2. If it returns true, can it return false?
3. Should we check its return value before passing it further?



I really don't see how

return fill_uuid(regs, MY_UUID);

would be semantically incorrect or even raise all those questions. It is 
perfectly fine to always return true. We have place like that and it 
helps to streamline the code.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-09-19 Thread Volodymyr Babchuk 

Hi Julien,

On 13.09.17 14:11, Julien Grall wrote:

Hi,

On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:



+static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t *u)


Actually why do you pass a pointer for u? This requires every caller to 
introduce temporary variable because the UUID is usually a define.

Hmm, another way probably is to pass a whole structure as a parameter.
Are you suggesting this approach? Something like
fill_uuid(regs, (xen_uuid_t)MY_UUID)?


With your current solution each caller as to do:

xen_uuid_t foo = MY_UUID;

fill_uuid(regs, );

return true;

What I suggested in the previous version is to get fill_uuid return 
true. So you make each caller simpler.
Yes, but it will not be correct semantically. There will arise many 
questions:

1. Why helper function that only writes data returns bool?
2. If it returns true, can it return false?
3. Should we check its return value before passing it further?


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

Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-09-13 Thread Julien Grall 

Hi,

On 08/31/2017 09:09 PM, 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 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, UUID and number of functions provided by service
provider.

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

At this time 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 
Reviewed-by: Oleksandr Andrushchenko 
Reviewed-by: Oleksandr Tyshchenko 
---

  * reworked fill_uuid() function
  * dropped vsmc.h header. Function prototypes moved to traps.h
  * public/arch-arm/smc.h header renamed to smccc.h
  * introduced `register_t funcid` in vsmccc_handle_call()x
  * spelling fixes
  * coding style fixes

---
xen/arch/arm/Makefile   |   1 +
  xen/arch/arm/traps.c|  17 
  xen/arch/arm/vsmc.c | 168 
  xen/include/asm-arm/traps.h |   3 +
  xen/include/public/arch-arm/smccc.h |  58 +
  5 files changed, 230 insertions(+), 17 deletions(-)
  create mode 100644 xen/arch/arm/vsmc.c
  create mode 100644 xen/include/public/arch-arm/smccc.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index de00c5e..3d7dde9 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -51,6 +51,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 9132fe1..f3b64b4 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2155,23 +2155,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 000..97a6be3
--- /dev/null
+++ b/xen/arch/arm/vsmc.c
@@ -0,0 +1,168 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Number of functions currently supported by Hypervisor Service. */
+#define XEN_SMCCC_FUNCTION_COUNT 3
+
+static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t *u)


Actually why do you pass a pointer for u? This requires every caller to 
introduce temporary variable because the UUID is usually a define.


With your current solution each caller as to do:

xen_uuid_t foo = MY_UUID;

fill_uuid(regs, );

return true;

What I suggested in the previous version is to get fill_uuid return 
true. So you make each caller simpler.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-09-13 Thread Julien Grall 

Hi Volodymyr,

On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:

diff --git a/xen/include/public/arch-arm/smccc.h 
b/xen/include/public/arch-arm/smccc.h
new file mode 100644
index 000..a1d00ae
--- /dev/null
+++ b/xen/include/public/arch-arm/smccc.h
@@ -0,0 +1,58 @@
+/*
+ * smccc.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_SMCCC_H__
+#define __XEN_PUBLIC_ARCH_ARM_SMCCC_H__
+
+#include "public/xen.h"
+
+/*
+ * 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.
+ */
+#define XEN_SMCCC_MAJOR_REVISION 0
+#define XEN_SMCCC_MINOR_REVISION 1
+
+/* Hypervisor Service UID. Randomly generated with uuidgen. */
+#define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \
+ 0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)


The indentation looks wrong here.


+
+#endif /* __XEN_PUBLIC_ARCH_ARM_SMCCC_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

[Xen-devel] [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC

2017-08-31 Thread 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 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, UUID and number of functions provided by service
provider.

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

At this time 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 
Reviewed-by: Oleksandr Andrushchenko 
Reviewed-by: Oleksandr Tyshchenko 
---

 * reworked fill_uuid() function
 * dropped vsmc.h header. Function prototypes moved to traps.h
 * public/arch-arm/smc.h header renamed to smccc.h
 * introduced `register_t funcid` in vsmccc_handle_call()x
 * spelling fixes
 * coding style fixes

---
xen/arch/arm/Makefile   |   1 +
 xen/arch/arm/traps.c|  17 
 xen/arch/arm/vsmc.c | 168 
 xen/include/asm-arm/traps.h |   3 +
 xen/include/public/arch-arm/smccc.h |  58 +
 5 files changed, 230 insertions(+), 17 deletions(-)
 create mode 100644 xen/arch/arm/vsmc.c
 create mode 100644 xen/include/public/arch-arm/smccc.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index de00c5e..3d7dde9 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -51,6 +51,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 9132fe1..f3b64b4 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2155,23 +2155,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 000..97a6be3
--- /dev/null
+++ b/xen/arch/arm/vsmc.c
@@ -0,0 +1,168 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Number of functions currently supported by Hypervisor Service. */
+#define XEN_SMCCC_FUNCTION_COUNT 3
+
+static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t *u)
+{
+int n;
+
+/*
+ * UUIDs are returned in registers r0..r3, four bytes per register,
+ * first byte is stored in low-order bits of a register.
+ * (ARM DEN 0028B page 14)
+ */
+for (n = 0; n < 4; n++)
+{
+const uint8_t *bytes = u->a + n * 4;
+uint32_t r;
+
+r = bytes[0];
+r |= bytes[1] << 8;
+r |= bytes[2] << 16;
+r |= bytes[3] << 24;
+
+set_user_reg(regs, n, r);
+}
+}
+
+/* SMCCC interface for hypervisor. Tell about itself. */
+static bool handle_hypervisor(struct cpu_user_regs *regs)
+{
+static const xen_uuid_t xen_uuid = XEN_SMCCC_UID;
+
+switch ( smccc_get_fn(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:
+fill_uuid(regs, _uuid);
+return true;
+case ARM_SMCCC_FUNC_CALL_REVISION:
+set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
+