* [PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-24 14:41 ` Julien Grall
2017-08-21 20:27 ` [PATCH v4 02/11] arm: traps: check if SMC was conditional before handling it Volodymyr Babchuk
` (9 subsequent siblings)
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
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 <volodymyr_babchuk@epam.com>
---
* Added space into reg,n
* Used 32-bit constant tin PSCI_ARG32
---
xen/arch/arm/traps.c | 40 ++++++++++++++++++----------------------
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 13efb58..66f12cb 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1450,14 +1450,13 @@ 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) & 0xFFFFFFFF)
#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)
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
#endif
/* helper function for checking arm mode 32/64 bit */
@@ -1471,14 +1470,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:
@@ -1486,36 +1485,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:
@@ -1525,8 +1524,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:
@@ -1537,8 +1535,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:
@@ -1548,8 +1545,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:
@@ -1558,7 +1554,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
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code
2017-08-21 20:27 ` [PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
@ 2017-08-24 14:41 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-08-24 14:41 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
On 21/08/17 21:27, Volodymyr Babchuk wrote:
> 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
s/macroses/macros/ I think.
> CONFIG_ARM_64 definition.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
> * Added space into reg,n
> * Used 32-bit constant tin PSCI_ARG32
>
> ---
> xen/arch/arm/traps.c | 40 ++++++++++++++++++----------------------
> 1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 13efb58..66f12cb 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1450,14 +1450,13 @@ 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) & 0xFFFFFFFF)
Please drop the mask, the cast is sufficient.
> #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)
> +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
Please mention in the commit message that you also modify the coding style.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 02/11] arm: traps: check if SMC was conditional before handling it
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
2017-08-21 20:27 ` [PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-24 14:42 ` Julien Grall
2017-08-21 20:27 ` [PATCH v4 03/11] public: xen.h: add definitions for UUID handling Volodymyr Babchuk
` (8 subsequent siblings)
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Trapped SMC instruction can fail condition check on ARMv8 arhcitecture
(ARM DDI 0487B.a page D7-2271). So we need to check if condition was meet.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
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 66f12cb..82cd2b1 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2159,6 +2159,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
^ permalink raw reply related [flat|nested] 48+ messages in thread* [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
2017-08-21 20:27 ` [PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
2017-08-21 20:27 ` [PATCH v4 02/11] arm: traps: check if SMC was conditional before handling it Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-22 7:26 ` Jan Beulich
2017-08-21 20:27 ` [PATCH v4 04/11] arm: processor.h: add definition for immediate value mask Volodymyr Babchuk
` (7 subsequent siblings)
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, Volodymyr Babchuk
Added type xen_uuid_t. This type represents UUID as an array of 16
bytes in big endian format.
Added macro XEN_DEFINE_UUID that constructs UUID in the usual way:
XEN_DEFINE_UUID(00112233, 4455, 6677, 8899, aabbccddeeff)
will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as
{0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88,
0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}
NB: This is compatible with Linux kernel and with libuuid, but it is not
compatible with Microsoft, as they use mixed-endian encoding (some
components are little-endian, some are big-endian).
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
xen/include/public/xen.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 2ac6b1e..d1a4765 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
__DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
__DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
+typedef uint8_t xen_uuid_t[16];
+#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6) \
+ {((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF, \
+ ((a) >> 8) & 0xFF, ((a) >> 0) & 0xFF, \
+ ((b) >> 8) & 0xFF, ((b) >> 0) & 0xFF, \
+ ((c) >> 8) & 0xFF, ((c) >> 0) & 0xFF, \
+ ((d) >> 8) & 0xFF, ((d) >> 0) & 0xFF, \
+ e1, e2, e3, e4, e5, e6}
+
#endif /* !__ASSEMBLY__ */
/* Default definitions for macros used by domctl/sysctl. */
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-21 20:27 ` [PATCH v4 03/11] public: xen.h: add definitions for UUID handling Volodymyr Babchuk
@ 2017-08-22 7:26 ` Jan Beulich
2017-08-22 14:37 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-08-22 7:26 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
>>> On 21.08.17 at 22:27, <volodymyr_babchuk@epam.com> wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
> __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
> __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
>
> +typedef uint8_t xen_uuid_t[16];
As expressed before, I'm opposed to this being a plain array. I've
pointed you at the EFI representation as an example; at the very
least I'd expect a wrapper structure around the array (which is
_not_ to say that I would ack such a patch, but at least I wouldn't
nak it).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-22 7:26 ` Jan Beulich
@ 2017-08-22 14:37 ` Volodymyr Babchuk
2017-08-23 8:10 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-22 14:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
Hi Jan,
On 22.08.17 10:26, Jan Beulich wrote:
>>>> On 21.08.17 at 22:27, <volodymyr_babchuk@epam.com> wrote:
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
>> __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
>> __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
>>
>> +typedef uint8_t xen_uuid_t[16];
>
> As expressed before, I'm opposed to this being a plain array. I've
> pointed you at the EFI representation as an example; at the very
> least I'd expect a wrapper structure around the array (which is
> _not_ to say that I would ack such a patch, but at least I wouldn't
> nak it).
EFI code uses GUID, which is product of Microsoft's NIH syndrome.
Let me cite some parts of RFC 4122:
4.1. Format
*The UUID format is 16 octets*; some bits of the eight octet variant
field specified below determine finer structure.
.....
4.1.2. Layout and Byte Order
.....
In the absence of explicit application or presentation protocol
specification to the contrary, a UUID is encoded as a 128-bit object,
as follows:
The fields are encoded as 16 octets, with the sizes and order of the
fields defined above, and with each field encoded with the Most
Significant Byte first (known as network byte order). Note that the
field names, particularly for multiplexed fields, follow historical
practice.
0 1 2 3
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| time_low |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| time_mid | time_hi_and_version |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|clk_seq_hi_res | clk_seq_low | node (0-1) |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| node (2-5) |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
.....
BTW, GUID handling is incompatible with this RFC, because in GUID
first three fields are stored in LE format, while other fields are
stored in BE format.
I can't see why you want to map UUID to a certain structure. I can
create such wrapper, but it will be just dead code, because there are no
users for it. Frankly, I can't imagine why someone will want to read,
say, clk_seq_hi_res field of UUID.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-22 14:37 ` Volodymyr Babchuk
@ 2017-08-23 8:10 ` Jan Beulich
2017-08-23 11:08 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-08-23 8:10 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
>>> On 22.08.17 at 16:37, <volodymyr_babchuk@epam.com> wrote:
> I can't see why you want to map UUID to a certain structure.
This is so that the type cannot mistakenly be passed to a function
taking unsigned char *, or be assigned to a variable of that type.
Please see our TYPE_SAFE() macro which we use to specifically
enclose certain scalar types in a structure to that they won't be
compatible with other types deriving from the same scalar base type.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-23 8:10 ` Jan Beulich
@ 2017-08-23 11:08 ` Volodymyr Babchuk
2017-08-23 11:29 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-23 11:08 UTC (permalink / raw)
To: Jan Beulich
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
Hello Jan
On 23.08.17 11:10, Jan Beulich wrote:
>>>> On 22.08.17 at 16:37, <volodymyr_babchuk@epam.com> wrote:
>> I can't see why you want to map UUID to a certain structure.
>
> This is so that the type cannot mistakenly be passed to a function
> taking unsigned char *, or be assigned to a variable of that type.
Right, I see the point there.
> Please see our TYPE_SAFE() macro which we use to specifically
> enclose certain scalar types in a structure to that they won't be
> compatible with other types deriving from the same scalar base type.
I see. So what about
struct xen_uuid_t
{
uint8_t a[16];
};
then?
One can convert it to union with different representations (array,
RFC4122 struct, etc) later if there will be need for this.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-23 11:08 ` Volodymyr Babchuk
@ 2017-08-23 11:29 ` Jan Beulich
2017-08-30 16:20 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-08-23 11:29 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
>>> On 23.08.17 at 13:08, <volodymyr_babchuk@epam.com> wrote:
> On 23.08.17 11:10, Jan Beulich wrote:
>>>>> On 22.08.17 at 16:37, <volodymyr_babchuk@epam.com> wrote:
>>> I can't see why you want to map UUID to a certain structure.
>>
>> This is so that the type cannot mistakenly be passed to a function
>> taking unsigned char *, or be assigned to a variable of that type.
> Right, I see the point there.
>
>> Please see our TYPE_SAFE() macro which we use to specifically
>> enclose certain scalar types in a structure to that they won't be
>> compatible with other types deriving from the same scalar base type.
> I see. So what about
>
> struct xen_uuid_t
> {
> uint8_t a[16];
> };
>
> then?
Yes, that's what I had asked for as the minimal solution. That
would be in line with (but better than) xen_domain_handle_t,
which I've just realized we also have.
> One can convert it to union with different representations (array,
> RFC4122 struct, etc) later if there will be need for this.
Well, why don't you make it a union but stick to just the array
for now if you dislike making it similar to the EFI one? That way
we can add further representations if needed/desired without
breaking existing consumers.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-23 11:29 ` Jan Beulich
@ 2017-08-30 16:20 ` Volodymyr Babchuk
2017-08-31 7:34 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-30 16:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
Hi Jan,
On 23.08.17 14:29, Jan Beulich wrote:
>>>> On 23.08.17 at 13:08, <volodymyr_babchuk@epam.com> wrote:
>> On 23.08.17 11:10, Jan Beulich wrote:
>>>>>> On 22.08.17 at 16:37, <volodymyr_babchuk@epam.com> wrote:
>>>> I can't see why you want to map UUID to a certain structure.
>>>
>>> This is so that the type cannot mistakenly be passed to a function
>>> taking unsigned char *, or be assigned to a variable of that type.
>> Right, I see the point there.
>>
>>> Please see our TYPE_SAFE() macro which we use to specifically
>>> enclose certain scalar types in a structure to that they won't be
>>> compatible with other types deriving from the same scalar base type.
>> I see. So what about
>>
>> struct xen_uuid_t
>> {
>> uint8_t a[16];
>> };
>>
>> then?
>
> Yes, that's what I had asked for as the minimal solution. That
> would be in line with (but better than) xen_domain_handle_t,
> which I've just realized we also have.
>
>> One can convert it to union with different representations (array,
>> RFC4122 struct, etc) later if there will be need for this.
>
> Well, why don't you make it a union but stick to just the array
> for now if you dislike making it similar to the EFI one? That way
> we can add further representations if needed/desired without
> breaking existing consumers.
My first intention was to declare union with all possible
representations, so it would be possible to access the same UUID as an
array of bytes or, for example, as Microsoft GUID. Like this:
typedef union {
/* UUID represented as a 128-bit object */
uint8_t obj[16];
/* Representation according to RFC 4122 */
struct {
__be32 time_low;
__be16 time_mid;
__be16 time_hi_and_version;
__u8 clock_seq_hi_and_reserved;
__u8 clock_seq_low;
__u8 node[6];
} rfc4122;
/* Microsoft/Intel style GUID representation */
struct {
__le32 Data1;
__le16 Data2;
__le16 Data3;
__u8 Data4[8];
} guid;
/* SMCCC compatible format */
struct {
__le32 r0;
__le32 r1;
__le32 r2;
__le32 r3;
} smccc;
} xen_uuid_t;
But looks like we can't use something like __packed or
__attribute__((__packed__)) in the public header. This means that we
can't rely on right overlapping and users of this union should take care
to read and write only to one chosen substructure. I think, this is
error-prone, so it is better to stick to
typedef struct
{
uint8_t a[16];
} xen_uuid_t;
BTW, I'm very interested how it can be guaranteed that structures
defined in xen.h will have the same size and alignment on both sides of
communication channel, taking into account, then we rely only on C89
standard.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-30 16:20 ` Volodymyr Babchuk
@ 2017-08-31 7:34 ` Jan Beulich
2017-08-31 12:24 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-08-31 7:34 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
>>> On 30.08.17 at 18:20, <volodymyr_babchuk@epam.com> wrote:
> My first intention was to declare union with all possible
> representations, so it would be possible to access the same UUID as an
> array of bytes or, for example, as Microsoft GUID. Like this:
>
> typedef union {
> /* UUID represented as a 128-bit object */
> uint8_t obj[16];
>
> /* Representation according to RFC 4122 */
> struct {
> __be32 time_low;
> __be16 time_mid;
> __be16 time_hi_and_version;
> __u8 clock_seq_hi_and_reserved;
> __u8 clock_seq_low;
> __u8 node[6];
> } rfc4122;
>
> /* Microsoft/Intel style GUID representation */
> struct {
> __le32 Data1;
> __le16 Data2;
> __le16 Data3;
> __u8 Data4[8];
> } guid;
>
> /* SMCCC compatible format */
> struct {
> __le32 r0;
> __le32 r1;
> __le32 r2;
> __le32 r3;
> } smccc;
> } xen_uuid_t;
>
>
> But looks like we can't use something like __packed or
> __attribute__((__packed__)) in the public header. This means that we
> can't rely on right overlapping and users of this union should take care
> to read and write only to one chosen substructure.
I don't see any use of that attribute in the structure definition
above, nor any need to add one - all fields are suitably aligned
anyway. You can't use __be* and __le* types in the public
headers, though - these will need to be uint*_t.
> BTW, I'm very interested how it can be guaranteed that structures
> defined in xen.h will have the same size and alignment on both sides of
> communication channel, taking into account, then we rely only on C89
> standard.
I don't understand this comment.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-31 7:34 ` Jan Beulich
@ 2017-08-31 12:24 ` Volodymyr Babchuk
2017-08-31 12:53 ` Jan Beulich
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 12:24 UTC (permalink / raw)
To: Jan Beulich
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
Hi Jan,
On 31.08.17 10:34, Jan Beulich wrote:
>>>> On 30.08.17 at 18:20, <volodymyr_babchuk@epam.com> wrote:
>> My first intention was to declare union with all possible
>> representations, so it would be possible to access the same UUID as an
>> array of bytes or, for example, as Microsoft GUID. Like this:
>>
>> typedef union {
>> /* UUID represented as a 128-bit object */
>> uint8_t obj[16];
>>
>> /* Representation according to RFC 4122 */
>> struct {
>> __be32 time_low;
>> __be16 time_mid;
>> __be16 time_hi_and_version;
>> __u8 clock_seq_hi_and_reserved;
>> __u8 clock_seq_low;
>> __u8 node[6];
>> } rfc4122;
>>
>> /* Microsoft/Intel style GUID representation */
>> struct {
>> __le32 Data1;
>> __le16 Data2;
>> __le16 Data3;
>> __u8 Data4[8];
>> } guid;
>>
>> /* SMCCC compatible format */
>> struct {
>> __le32 r0;
>> __le32 r1;
>> __le32 r2;
>> __le32 r3;
>> } smccc;
>> } xen_uuid_t;
>>
>>
>> But looks like we can't use something like __packed or
>> __attribute__((__packed__)) in the public header. This means that we
>> can't rely on right overlapping and users of this union should take care
>> to read and write only to one chosen substructure.
>
> I don't see any use of that attribute in the structure definition
> above, nor any need to add one - all fields are suitably aligned
> anyway. You can't use __be* and __le* types in the public
> headers, though - these will need to be uint*_t.
This is a public header. As I understand it can be used by different
compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to
padding or alignment of fields in structures. No one can guarantee that
sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) ==
sizeof(xen_uuid_t.smccc) == 16
On all platforms. Using any compiler. With any compiler options.
This is implementation defined ([1]). Standard says "This should present
no problem unless binary data written by one implementation are read by
another.". But in case of public headers, this structures can be written
by one implementation and read by another.
>> BTW, I'm very interested how it can be guaranteed that structures
>> defined in xen.h will have the same size and alignment on both sides of
>> communication channel, taking into account, then we rely only on C89
>> standard.
>
> I don't understand this comment.
See my reply above. There absolutely no guarantees, that MSVC compiler
will not add extra padding somewhere. This will impair interoperability.
I think, this is why (amongst other reasons) WinAPI structures has
dwSize as the first field. And this is why _IO* macros in Linux kernel
use sizeof() to create ioctl number.
But, as I can see, XEN code checks only magic or version.
[1] http://port70.net/~nsz/c/c89/c89-draft.html#A.6.3.8
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-31 12:24 ` Volodymyr Babchuk
@ 2017-08-31 12:53 ` Jan Beulich
2017-08-31 13:21 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-08-31 12:53 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
>>> On 31.08.17 at 14:24, <volodymyr_babchuk@epam.com> wrote:
> Hi Jan,
>
> On 31.08.17 10:34, Jan Beulich wrote:
>>>>> On 30.08.17 at 18:20, <volodymyr_babchuk@epam.com> wrote:
>>> My first intention was to declare union with all possible
>>> representations, so it would be possible to access the same UUID as an
>>> array of bytes or, for example, as Microsoft GUID. Like this:
>>>
>>> typedef union {
>>> /* UUID represented as a 128-bit object */
>>> uint8_t obj[16];
>>>
>>> /* Representation according to RFC 4122 */
>>> struct {
>>> __be32 time_low;
>>> __be16 time_mid;
>>> __be16 time_hi_and_version;
>>> __u8 clock_seq_hi_and_reserved;
>>> __u8 clock_seq_low;
>>> __u8 node[6];
>>> } rfc4122;
>>>
>>> /* Microsoft/Intel style GUID representation */
>>> struct {
>>> __le32 Data1;
>>> __le16 Data2;
>>> __le16 Data3;
>>> __u8 Data4[8];
>>> } guid;
>>>
>>> /* SMCCC compatible format */
>>> struct {
>>> __le32 r0;
>>> __le32 r1;
>>> __le32 r2;
>>> __le32 r3;
>>> } smccc;
>>> } xen_uuid_t;
>>>
>>>
>>> But looks like we can't use something like __packed or
>>> __attribute__((__packed__)) in the public header. This means that we
>>> can't rely on right overlapping and users of this union should take care
>>> to read and write only to one chosen substructure.
>>
>> I don't see any use of that attribute in the structure definition
>> above, nor any need to add one - all fields are suitably aligned
>> anyway. You can't use __be* and __le* types in the public
>> headers, though - these will need to be uint*_t.
> This is a public header. As I understand it can be used by different
> compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to
> padding or alignment of fields in structures. No one can guarantee that
>
> sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) ==
> sizeof(xen_uuid_t.smccc) == 16
>
> On all platforms. Using any compiler. With any compiler options.
>
> This is implementation defined ([1]). Standard says "This should present
> no problem unless binary data written by one implementation are read by
> another.". But in case of public headers, this structures can be written
> by one implementation and read by another.
My reference to C89 was to tell you what language constructs
you're allowed to use. For binary layout, conventions also
matter (like gABI and processor specific ABIs). Without that we
wouldn't be able to write any C header in compatible manner.
What helps us greatly is that we're not needing interfaces for
cross-host communication - the entire public interface assumes
that producer and consumer run on the same physical system
(or, for the parts concerning migration, on similar ones).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-31 12:53 ` Jan Beulich
@ 2017-08-31 13:21 ` Volodymyr Babchuk
2017-08-31 14:34 ` Ian Jackson
2017-08-31 15:12 ` Jan Beulich
0 siblings, 2 replies; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 13:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
On 31.08.17 15:53, Jan Beulich wrote:
>>>> On 31.08.17 at 14:24, <volodymyr_babchuk@epam.com> wrote:
>> Hi Jan,
>>
>> On 31.08.17 10:34, Jan Beulich wrote:
>>>>>> On 30.08.17 at 18:20, <volodymyr_babchuk@epam.com> wrote:
>>>> My first intention was to declare union with all possible
>>>> representations, so it would be possible to access the same UUID as an
>>>> array of bytes or, for example, as Microsoft GUID. Like this:
>>>>
>>>> typedef union {
>>>> /* UUID represented as a 128-bit object */
>>>> uint8_t obj[16];
>>>>
>>>> /* Representation according to RFC 4122 */
>>>> struct {
>>>> __be32 time_low;
>>>> __be16 time_mid;
>>>> __be16 time_hi_and_version;
>>>> __u8 clock_seq_hi_and_reserved;
>>>> __u8 clock_seq_low;
>>>> __u8 node[6];
>>>> } rfc4122;
>>>>
>>>> /* Microsoft/Intel style GUID representation */
>>>> struct {
>>>> __le32 Data1;
>>>> __le16 Data2;
>>>> __le16 Data3;
>>>> __u8 Data4[8];
>>>> } guid;
>>>>
>>>> /* SMCCC compatible format */
>>>> struct {
>>>> __le32 r0;
>>>> __le32 r1;
>>>> __le32 r2;
>>>> __le32 r3;
>>>> } smccc;
>>>> } xen_uuid_t;
>>>>
>>>>
>>>> But looks like we can't use something like __packed or
>>>> __attribute__((__packed__)) in the public header. This means that we
>>>> can't rely on right overlapping and users of this union should take care
>>>> to read and write only to one chosen substructure.
>>>
>>> I don't see any use of that attribute in the structure definition
>>> above, nor any need to add one - all fields are suitably aligned
>>> anyway. You can't use __be* and __le* types in the public
>>> headers, though - these will need to be uint*_t.
>> This is a public header. As I understand it can be used by different
>> compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to
>> padding or alignment of fields in structures. No one can guarantee that
>>
>> sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) ==
>> sizeof(xen_uuid_t.smccc) == 16
>>
>> On all platforms. Using any compiler. With any compiler options.
>>
>> This is implementation defined ([1]). Standard says "This should present
>> no problem unless binary data written by one implementation are read by
>> another.". But in case of public headers, this structures can be written
>> by one implementation and read by another.
>
> My reference to C89 was to tell you what language constructs
> you're allowed to use. For binary layout, conventions also
> matter (like gABI and processor specific ABIs). Without that we
> wouldn't be able to write any C header in compatible manner.
> What helps us greatly is that we're not needing interfaces for
> cross-host communication - the entire public interface assumes
> that producer and consumer run on the same physical system
> (or, for the parts concerning migration, on similar ones).
So, will it be acceptable to use my approach with that union?
Do you have any ideas how to indicate endianess of the fields, then? I
can just write it in the comments. But I fear of misuse.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-31 13:21 ` Volodymyr Babchuk
@ 2017-08-31 14:34 ` Ian Jackson
2017-08-31 15:12 ` Jan Beulich
1 sibling, 0 replies; 48+ messages in thread
From: Ian Jackson @ 2017-08-31 14:34 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Tim Deegan, xen-devel, Julien Grall, Jan Beulich
Volodymyr Babchuk writes ("Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling"):
> Do you have any ideas how to indicate endianess of the fields, then? I
> can just write it in the comments. But I fear of misuse.
I definitely prefer your approach of providing only an array. (It
needs to be in a struct for typesafety, as discussed.) At least that
way the semantics are clear.
No-one is likely to want to access these individual fields. If they
do so they are doing something very wrong.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling
2017-08-31 13:21 ` Volodymyr Babchuk
2017-08-31 14:34 ` Ian Jackson
@ 2017-08-31 15:12 ` Jan Beulich
1 sibling, 0 replies; 48+ messages in thread
From: Jan Beulich @ 2017-08-31 15:12 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
>>> On 31.08.17 at 15:21, <volodymyr_babchuk@epam.com> wrote:
> So, will it be acceptable to use my approach with that union?
As per Ian's reply, go with just the containerized uint8_t[].
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 04/11] arm: processor.h: add definition for immediate value mask
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (2 preceding siblings ...)
2017-08-21 20:27 ` [PATCH v4 03/11] public: xen.h: add definitions for UUID handling Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-24 14:45 ` Julien Grall
2017-08-21 20:27 ` [PATCH v4 05/11] arm: add SMCCC protocol definitions Volodymyr Babchuk
` (6 subsequent siblings)
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
This patch adds definition HSR_XXC_IMM_MASK. It can be used to extract
immediate value for trapped HVC32, HVC64, SMC64, SVC32, SVC64 instructions,
as described at ARM ARM (ARM DDI 0487B.a pages D7-2270, D7-2272).
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
xen/include/asm-arm/processor.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 51ce802..89752a7 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -580,6 +580,9 @@ union hsr {
HSR_SYSREG_CRN_MASK|HSR_SYSREG_CRM_MASK|\
HSR_SYSREG_OP2_MASK)
+/* HSR.EC == HSR_{HVC32, HVC64, SMC64, SVC32, SVC64} */
+#define HSR_XXC_IMM_MASK (0xffff)
+
/* Physical Address Register */
#define PAR_F (_AC(1,U)<<0)
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 04/11] arm: processor.h: add definition for immediate value mask
2017-08-21 20:27 ` [PATCH v4 04/11] arm: processor.h: add definition for immediate value mask Volodymyr Babchuk
@ 2017-08-24 14:45 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-08-24 14:45 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
Title: It is a too generic, you may want to rename to: "Define HVC/SMC
immediate mask"
On 21/08/17 21:27, Volodymyr Babchuk wrote:
> This patch adds definition HSR_XXC_IMM_MASK. It can be used to extract
s/adds definition/define/
> immediate value for trapped HVC32, HVC64, SMC64, SVC32, SVC64 instructions,
> as described at ARM ARM (ARM DDI 0487B.a pages D7-2270, D7-2272).
s/at/in the/
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> xen/include/asm-arm/processor.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 51ce802..89752a7 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -580,6 +580,9 @@ union hsr {
> HSR_SYSREG_CRN_MASK|HSR_SYSREG_CRM_MASK|\
> HSR_SYSREG_OP2_MASK)
>
> +/* HSR.EC == HSR_{HVC32, HVC64, SMC64, SVC32, SVC64} */
> +#define HSR_XXC_IMM_MASK (0xffff)
> +
> /* Physical Address Register */
> #define PAR_F (_AC(1,U)<<0)
>
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 05/11] arm: add SMCCC protocol definitions.
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (3 preceding siblings ...)
2017-08-21 20:27 ` [PATCH v4 04/11] arm: processor.h: add definition for immediate value mask Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-24 15:00 ` Julien Grall
2017-08-21 20:27 ` [PATCH v4 06/11] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
` (5 subsequent siblings)
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
This patch adds generic definitions used in ARM SMC call convention.
Those definitions was taken from linux header arm-smccc.h, extended
and formatted according to XEN coding style.
They can be used by both SMCCC clients (like PSCI) and by SMCCC
servers (like vPSCI or upcoming generic SMCCC handler).
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
xen/include/asm-arm/smccc.h | 92 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 92 insertions(+)
create mode 100644 xen/include/asm-arm/smccc.h
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
new file mode 100644
index 0000000..67da3fb
--- /dev/null
+++ b/xen/include/asm-arm/smccc.h
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2015, Linaro Limited
+ * 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_SMCCC_H__
+#define __ASM_ARM_SMCCC_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)
+
+#endif /* __ASM_ARM_SMCCC_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
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 05/11] arm: add SMCCC protocol definitions.
2017-08-21 20:27 ` [PATCH v4 05/11] arm: add SMCCC protocol definitions Volodymyr Babchuk
@ 2017-08-24 15:00 ` Julien Grall
2017-08-28 20:28 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2017-08-24 15:00 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
Title: No need for the full stop.
On 21/08/17 21:27, Volodymyr Babchuk wrote:
> This patch adds generic definitions used in ARM SMC call convention.
> Those definitions was taken from linux header arm-smccc.h, extended
> and formatted according to XEN coding style.
>
> They can be used by both SMCCC clients (like PSCI) and by SMCCC
> servers (like vPSCI or upcoming generic SMCCC handler).
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> xen/include/asm-arm/smccc.h | 92 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 92 insertions(+)
> create mode 100644 xen/include/asm-arm/smccc.h
>
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> new file mode 100644
> index 0000000..67da3fb
> --- /dev/null
> +++ b/xen/include/asm-arm/smccc.h
> @@ -0,0 +1,92 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited
Linaro? Where does this code come from?
> + * 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_SMCCC_H__
> +#define __ASM_ARM_SMCCC_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
I am not sure to understand why you embed SMC in the name? The
convention is SMC32/HVC32. So I would name
ARM_SMCCC_CONV_{32,64}
> +#define ARM_SMCCC_CALL_CONV_SHIFT 30
Also, I would rename to ARM_SMCCC_CONV to fit with the value above.
> +
> +#define ARM_SMCCC_OWNER_MASK 0x3F
Missing U.
> +#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)
Can we please use static inline helper for the 4 macros above?
> +
> +/*
> + * 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))
The indentation looks wrong here. Also I think the (& *MASK) is not
necessary here. It would be wrong by the user to pass a wrong value and
even with the masking you would end up to wrong result.
If you are really worry of wrong result, then you should use
BUILD_BUG_ON()/BUG_ON().
> +
> +/* 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)
> +
> +#endif /* __ASM_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
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 05/11] arm: add SMCCC protocol definitions.
2017-08-24 15:00 ` Julien Grall
@ 2017-08-28 20:28 ` Volodymyr Babchuk
2017-09-13 10:04 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-28 20:28 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Julien,
On 24.08.17 18:00, Julien Grall wrote:
> Hi Volodymyr,
>
> Title: No need for the full stop.
>
> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>> This patch adds generic definitions used in ARM SMC call convention.
>> Those definitions was taken from linux header arm-smccc.h, extended
>> and formatted according to XEN coding style.
>>
>> They can be used by both SMCCC clients (like PSCI) and by SMCCC
>> servers (like vPSCI or upcoming generic SMCCC handler).
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>> xen/include/asm-arm/smccc.h | 92
>> +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 92 insertions(+)
>> create mode 100644 xen/include/asm-arm/smccc.h
>>
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> new file mode 100644
>> index 0000000..67da3fb
>> --- /dev/null
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -0,0 +1,92 @@
>> +/*
>> + * Copyright (c) 2015, Linaro Limited
>
> Linaro? Where does this code come from?
From the linux kernel. I think, I mentioned that in previous comments.
But this code was extended by me. And now there will be more changes.
Should I add remark about code origin somewhere?
>> + * 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_SMCCC_H__
>> +#define __ASM_ARM_SMCCC_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
>
> I am not sure to understand why you embed SMC in the name? The
> convention is SMC32/HVC32. So I would name
>
> ARM_SMCCC_CONV_{32,64}
>
>> +#define ARM_SMCCC_CALL_CONV_SHIFT 30
>
> Also, I would rename to ARM_SMCCC_CONV to fit with the value above.
>
>> +
>> +#define ARM_SMCCC_OWNER_MASK 0x3F
>
> Missing U.
>
>> +#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)
>
> Can we please use static inline helper for the 4 macros above?
>
>> +
>> +/*
>> + * 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))
>
> The indentation looks wrong here. Also I think the (& *MASK) is not
> necessary here. It would be wrong by the user to pass a wrong value and
> even with the masking you would end up to wrong result.
>
> If you are really worry of wrong result, then you should use
> BUILD_BUG_ON()/BUG_ON().
>
>> +
>> +/* 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)
>> +
>> +#endif /* __ASM_ARM_SMCCC_H__ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:b
>> + */
>>
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 05/11] arm: add SMCCC protocol definitions.
2017-08-28 20:28 ` Volodymyr Babchuk
@ 2017-09-13 10:04 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-09-13 10:04 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
On 08/28/2017 09:28 PM, Volodymyr Babchuk wrote:
> Hi Julien,
Hi,
>
> On 24.08.17 18:00, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> Title: No need for the full stop.
>>
>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>> This patch adds generic definitions used in ARM SMC call convention.
>>> Those definitions was taken from linux header arm-smccc.h, extended
>>> and formatted according to XEN coding style.
>>>
>>> They can be used by both SMCCC clients (like PSCI) and by SMCCC
>>> servers (like vPSCI or upcoming generic SMCCC handler).
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>> ---
>>> xen/include/asm-arm/smccc.h | 92
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 92 insertions(+)
>>> create mode 100644 xen/include/asm-arm/smccc.h
>>>
>>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>>> new file mode 100644
>>> index 0000000..67da3fb
>>> --- /dev/null
>>> +++ b/xen/include/asm-arm/smccc.h
>>> @@ -0,0 +1,92 @@
>>> +/*
>>> + * Copyright (c) 2015, Linaro Limited
>>
>> Linaro? Where does this code come from?
> From the linux kernel. I think, I mentioned that in previous comments.
> But this code was extended by me. And now there will be more changes.
> Should I add remark about code origin somewhere?
Yes, the origin of code (repo + commit ID) + changes you made at least
in the commit message.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 06/11] arm: smccc: handle SMCs according to SMCCC
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (4 preceding siblings ...)
2017-08-21 20:27 ` [PATCH v4 05/11] arm: add SMCCC protocol definitions Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-24 16:40 ` Julien Grall
2017-08-21 20:27 ` [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c` Volodymyr Babchuk
` (4 subsequent siblings)
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, 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 <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
* Reworked UUID handling (due to new UUID type definition)
* Renamed vsmc_handle_call() to vsmccc_handle_call() to emphasis
that it handles both SMC and HVC
* Added comment for inject_undef_exception() usage
* Used HSR_XXC_IMM_MASK insted of 0xFF
* Added full stops to comments
---
xen/arch/arm/Makefile | 1 +
xen/arch/arm/traps.c | 18 +----
xen/arch/arm/vsmc.c | 160 ++++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/vsmc.h | 30 +++++++
xen/include/public/arch-arm/smc.h | 58 ++++++++++++++
5 files changed, 250 insertions(+), 17 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 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 82cd2b1..4141a89 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -50,6 +50,7 @@
#include <asm/regs.h>
#include <asm/traps.h>
#include <asm/vgic.h>
+#include <asm/vsmc.h>
#include <asm/vtimer.h>
#include "decode.h"
@@ -2155,23 +2156,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..0a81294
--- /dev/null
+++ b/xen/arch/arm/vsmc.c
@@ -0,0 +1,160 @@
+/*
+ * 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/regs.h>
+#include <asm/smccc.h>
+#include <asm/traps.h>
+#include <asm/vsmc.h>
+
+/* 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)
+{
+#define FILL_UUID(n) \
+ set_user_reg(regs, n, (register_t) u[n * 4 + 0] << 24 | \
+ u[n * 4 + 1] << 16 | \
+ u[n * 4 + 2] << 8 | \
+ u[n * 4 + 3] << 0 )
+
+ FILL_UUID(0);
+ FILL_UUID(1);
+ FILL_UUID(2);
+ FILL_UUID(3);
+#undef FILL_UUID
+}
+
+/* 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 ( 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:
+ fill_uuid(regs, xen_uuid);
+ 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;
+}
+
+/*
+ * vsmccc_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 vsmccc_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 disassemble 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 & HSR_XXC_IMM_MASK) != 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.
+ * vsmccc_handle_call() will return false if this call is not
+ * SMCCC compatbile (i.e. immediate value != 0). As it is not
+ * compatible, we can't be sure that guest will understand
+ * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
+ */
+ if ( vsmccc_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..31aaa55
--- /dev/null
+++ b/xen/include/asm-arm/vsmc.h
@@ -0,0 +1,30 @@
+/*
+ * 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>
+
+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..3d3cd90
--- /dev/null
+++ b/xen/include/public/arch-arm/smc.h
@@ -0,0 +1,58 @@
+/*
+ * 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__
+
+#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)
+
+#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
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 06/11] arm: smccc: handle SMCs according to SMCCC
2017-08-21 20:27 ` [PATCH v4 06/11] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
@ 2017-08-24 16:40 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-08-24 16:40 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
On 21/08/17 21:27, 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 <volodymyr_babchuk@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>
> * Reworked UUID handling (due to new UUID type definition)
> * Renamed vsmc_handle_call() to vsmccc_handle_call() to emphasis
> that it handles both SMC and HVC
> * Added comment for inject_undef_exception() usage
> * Used HSR_XXC_IMM_MASK insted of 0xFF
> * Added full stops to comments
>
> ---
>
> xen/arch/arm/Makefile | 1 +
> xen/arch/arm/traps.c | 18 +----
> xen/arch/arm/vsmc.c | 160 ++++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/vsmc.h | 30 +++++++
> xen/include/public/arch-arm/smc.h | 58 ++++++++++++++
> 5 files changed, 250 insertions(+), 17 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 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 82cd2b1..4141a89 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -50,6 +50,7 @@
> #include <asm/regs.h>
> #include <asm/traps.h>
> #include <asm/vgic.h>
> +#include <asm/vsmc.h>
> #include <asm/vtimer.h>
>
> #include "decode.h"
> @@ -2155,23 +2156,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);
> -}
> -
In general we try to avoid code movement in the same patch as new code.
I am ok with that one, but please avoid this in the future.
> 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..0a81294
> --- /dev/null
> +++ b/xen/arch/arm/vsmc.c
> @@ -0,0 +1,160 @@
> +/*
> + * 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/regs.h>
> +#include <asm/smccc.h>
> +#include <asm/traps.h>
> +#include <asm/vsmc.h>
> +
> +/* 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)
> +{
> +#define FILL_UUID(n) \
> + set_user_reg(regs, n, (register_t) u[n * 4 + 0] << 24 | \
> + u[n * 4 + 1] << 16 | \
> + u[n * 4 + 2] << 8 | \
> + u[n * 4 + 3] << 0 )
This does not seem to match the spec: "Bytes 0...3 with byte 0 in the
low-order bits".
Which I understand as byte 0 should be in [0...7], byte 1 [8..15]...
> +
> + FILL_UUID(0);
> + FILL_UUID(1);
> + FILL_UUID(2);
> + FILL_UUID(3);
> +#undef FILL_UUID
This function is really hard to parse and I don't think the macro is
worth it.
I would do something like:
for (i = 0; i < 4)
{
uint8_t *bytes = &u[n * 4];
uint32_t r;
r = bytes[0];
r |= bytes[1] << 8;
r |= bytes[2] << 16;
r |= bytes[3] << 24;
set_user_reg(...);
}
Plus some comment on top explaining the layout. Yes, it takes more line,
but at least it is easier to read it.
> +}
> +
> +/* 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;
Missing newline here.
> + 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:
> + fill_uuid(regs, xen_uuid);
> + 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;
default:
return false;
> + }
> + return false;
And drop that one.
> +}
> +
> +/*
> + * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
> + * returns true if that was valid SMCCC call (even if function number
> + * was unkown).
s/unkown/unknown/
> + */
> +static bool vsmccc_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 disassemble 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 & HSR_XXC_IMM_MASK) != 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. */
I don't think this is comment is necessary.
> +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.
> + * vsmccc_handle_call() will return false if this call is not
> + * SMCCC compatbile (i.e. immediate value != 0). As it is not
> + * compatible, we can't be sure that guest will understand
> + * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
> + */
> + if ( vsmccc_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..31aaa55
> --- /dev/null
> +++ b/xen/include/asm-arm/vsmc.h
> @@ -0,0 +1,30 @@
> +/*
> + * 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>
> +
> +void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
I don't see any benefits of having a new header with just two prototype.
It would make sense to move them in traps.h.
> +
> +#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..3d3cd90
> --- /dev/null
> +++ b/xen/include/public/arch-arm/smc.h
Likely, this should be called smccc.h.
> @@ -0,0 +1,58 @@
> +/*
> + * 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__
> +
> +#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)
> +
> +#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
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (5 preceding siblings ...)
2017-08-21 20:27 ` [PATCH v4 06/11] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-24 16:58 ` Julien Grall
2017-08-21 20:27 ` [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h Volodymyr Babchuk
` (3 subsequent siblings)
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, 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`.
Older PSCI 0.1 uses SMC function identifiers in range that is
resereved for exisings APIs (ARM DEN 0028B, page 16), while newer
PSCI 0.2 is defined as "standard secure service" with own ranges
(ARM DEN 0028B, page 18).
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>
---
* Fixed mistakes about non-existant PSCI 2.0
* Added special SMC function number handling for PSCI 0.1
* Fixed coding style in handle_psci_0_1()
* Changed how return do_trap_hvc_smccc() is called from traps.c
* Renamed SSC to SSSC (Standard Secure Service Calls)
---
xen/arch/arm/traps.c | 117 +------------------------
xen/arch/arm/vsmc.c | 175 ++++++++++++++++++++++++++++++++++++--
xen/include/asm-arm/smccc.h | 4 +
xen/include/asm-arm/vsmc.h | 1 +
xen/include/public/arch-arm/smc.h | 8 ++
5 files changed, 183 insertions(+), 122 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 4141a89..517e013 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1451,119 +1451,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) & 0xFFFFFFFF)
-#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
@@ -2252,7 +2139,7 @@ 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);
+ return do_trap_hvc_smccc(regs);
do_trap_hypercall(regs, (register_t *)®s->r12, hsr.iss);
break;
#ifdef CONFIG_ARM_64
@@ -2264,7 +2151,7 @@ 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);
+ return do_trap_hvc_smccc(regs);
do_trap_hypercall(regs, ®s->x16, hsr.iss);
break;
case HSR_EC_SMC64:
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 0a81294..956d4ef 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -19,6 +19,7 @@
#include <xen/types.h>
#include <public/arch-arm/smc.h>
#include <asm/monitor.h>
+#include <asm/psci.h>
#include <asm/regs.h>
#include <asm/smccc.h>
#include <asm/traps.h>
@@ -27,6 +28,9 @@
/* Number of functions currently supported by Hypervisor Service. */
#define XEN_SMCCC_FUNCTION_COUNT 3
+/* Number of functions currently supported by Standard Service Service Calls. */
+#define SSSC_SMCCC_FUNCTION_COUNT 13
+
static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
{
#define FILL_UUID(n) \
@@ -62,6 +66,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) & 0xFFFFFFFF)
+#else
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
+#endif
+
+/* PSCI 0.1 interface */
+static bool handle_psci_0_1(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 0.2 interface and other Standard Secure Calls */
+static bool handle_sssc(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, SSSC_SMCCC_FUNCTION_COUNT);
+ return true;
+ case ARM_SMCCC_FUNC_CALL_UID:
+ {
+ static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
+ fill_uuid(regs, psci_uuid);
+ return true;
+ }
+ case ARM_SMCCC_FUNC_CALL_REVISION:
+ set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
+ set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
+ return true;
+ }
+ return false;
+}
+
/*
* vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
* returns true if that was valid SMCCC call (even if function number
@@ -71,6 +202,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
{
bool handled = false;
const union hsr hsr = { .bits = regs->hsr };
+ register_t func_id = get_user_reg(regs, 0);
/*
* Check immediate value for HVC32, HVC64 and SMC64.
@@ -94,24 +226,38 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
}
/* 64 bit calls are allowed only from 64 bit domains. */
- if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
+ if ( ARM_SMCCC_IS_64(func_id) &&
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)) )
+ /*
+ * Special case: identifier range for existing APIs.
+ * This range is described in SMCCC (ARM DEN 0028B, page 16),
+ * but it does not conforms to standard function identifier
+ * encoding.
+ */
+ if ( func_id >= ARM_SMCCC_RESERVED_RANGE_START &&
+ func_id <= ARM_SMCCC_RESERVED_RANGE_END )
+ handled = handle_psci_0_1(regs);
+ else
{
- case ARM_SMCCC_OWNER_HYPERVISOR:
- handled = handle_hypervisor(regs);
- break;
+ switch ( ARM_SMCCC_OWNER_NUM(func_id) )
+ {
+ case ARM_SMCCC_OWNER_HYPERVISOR:
+ handled = handle_hypervisor(regs);
+ break;
+ case ARM_SMCCC_OWNER_STANDARD:
+ handled = handle_sssc(regs);
+ break;
+ }
}
if ( !handled )
{
- gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
- get_user_reg(regs, 0));
+ gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", func_id);
/* Inform caller that function is not supported. */
set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
}
@@ -150,6 +296,21 @@ 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)
+{
+ const union hsr hsr = { .bits = regs->hsr };
+
+ /*
+ * vsmccc_handle_call() will return false if this call is not
+ * SMCCC compatbile (i.e. immediate value != 0). As it is not
+ * compatible, we can't be sure that guest will understand
+ * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
+ */
+ if ( !vsmccc_handle_call(regs) )
+ inject_undef_exception(regs, hsr);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 67da3fb..7c0003c 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -80,6 +80,10 @@
/* Only one error code defined in SMCCC */
#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1)
+/* SMCCC function identifier range which is reserved for existing APIs */
+#define ARM_SMCCC_RESERVED_RANGE_START 0x0
+#define ARM_SMCCC_RESERVED_RANGE_END 0x0100FFFF
+
#endif /* __ASM_ARM_SMCCC_H__ */
/*
diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
index 31aaa55..90ff610 100644
--- a/xen/include/asm-arm/vsmc.h
+++ b/xen/include/asm-arm/vsmc.h
@@ -17,6 +17,7 @@
#include <xen/types.h>
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 3d3cd90..c5327e3 100644
--- a/xen/include/public/arch-arm/smc.h
+++ b/xen/include/public/arch-arm/smc.h
@@ -46,6 +46,14 @@
#define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \
0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
+/* Standard Service Service Call version. */
+#define SSSC_SMCCC_MAJOR_REVISION 0
+#define SSSC_SMCCC_MINOR_REVISION 1
+
+/* Standard Service Call UID. Randomly generated with uuidgen. */
+#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd, 0x9220,\
+ 0xce, 0x16, 0x41, 0xe5, 0x9f, 0x6f)
+
#endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
/*
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`
2017-08-21 20:27 ` [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c` Volodymyr Babchuk
@ 2017-08-24 16:58 ` Julien Grall
2017-08-25 10:56 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2017-08-24 16:58 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
On 21/08/17 21:27, 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`.
>
> Older PSCI 0.1 uses SMC function identifiers in range that is
> resereved for exisings APIs (ARM DEN 0028B, page 16), while newer
s/resereved/reserved/
s/exisings/existing/
> PSCI 0.2 is defined as "standard secure service" with own ranges
0.2 and later
"with its own ranges" I think.
> (ARM DEN 0028B, page 18).
>
> 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>
> ---
>
> * Fixed mistakes about non-existant PSCI 2.0
> * Added special SMC function number handling for PSCI 0.1
> * Fixed coding style in handle_psci_0_1()
> * Changed how return do_trap_hvc_smccc() is called from traps.c
> * Renamed SSC to SSSC (Standard Secure Service Calls)
>
> ---
> xen/arch/arm/traps.c | 117 +------------------------
> xen/arch/arm/vsmc.c | 175 ++++++++++++++++++++++++++++++++++++--
> xen/include/asm-arm/smccc.h | 4 +
> xen/include/asm-arm/vsmc.h | 1 +
> xen/include/public/arch-arm/smc.h | 8 ++
> 5 files changed, 183 insertions(+), 122 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 4141a89..517e013 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1451,119 +1451,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) & 0xFFFFFFFF)
> -#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
> @@ -2252,7 +2139,7 @@ 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);
> + return do_trap_hvc_smccc(regs);
> do_trap_hypercall(regs, (register_t *)®s->r12, hsr.iss);
> break;
> #ifdef CONFIG_ARM_64
> @@ -2264,7 +2151,7 @@ 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);
> + return do_trap_hvc_smccc(regs);
> do_trap_hypercall(regs, ®s->x16, hsr.iss);
> break;
> case HSR_EC_SMC64:
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 0a81294..956d4ef 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -19,6 +19,7 @@
> #include <xen/types.h>
> #include <public/arch-arm/smc.h>
> #include <asm/monitor.h>
> +#include <asm/psci.h>
> #include <asm/regs.h>
> #include <asm/smccc.h>
> #include <asm/traps.h>
> @@ -27,6 +28,9 @@
> /* Number of functions currently supported by Hypervisor Service. */
> #define XEN_SMCCC_FUNCTION_COUNT 3
>
> +/* Number of functions currently supported by Standard Service Service Calls. */
> +#define SSSC_SMCCC_FUNCTION_COUNT 13
> +
> static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
> {
> #define FILL_UUID(n) \
> @@ -62,6 +66,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) & 0xFFFFFFFF)
> +#else
> +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
> +#endif
> +
> +/* PSCI 0.1 interface */
> +static bool handle_psci_0_1(struct cpu_user_regs *regs)
> +{
> + switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
I am not sure to understand the mask here.
> + {
> + case PSCI_cpu_off:
> + {
> + uint32_t pstate = PSCI_ARG32(regs, 1);
Missing newline here. I am ok with you do the clean-up in this patch
rather separately.
> + 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);
Ditto.
> + 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 0.2 interface and other Standard Secure Calls */
> +static bool handle_sssc(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;
It is a bit hard to read. Some newline between each "case" would help
for clarity I think.
> + 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, SSSC_SMCCC_FUNCTION_COUNT);
> + return true;
> + case ARM_SMCCC_FUNC_CALL_UID:
> + {
> + static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
Newline here please. But can't we just do:
return fill_uuid(regs, SSC_SMCCC_UID);
This would make the code simpler.
> + fill_uuid(regs, psci_uuid);
> + return true;
> + }
> + case ARM_SMCCC_FUNC_CALL_REVISION:
> + set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
> + set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
Same here.
> + return true;
> + }
> + return false;
> +}
> +
> /*
> * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
> * returns true if that was valid SMCCC call (even if function number
> @@ -71,6 +202,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
> {
> bool handled = false;
> const union hsr hsr = { .bits = regs->hsr };
> + register_t func_id = get_user_reg(regs, 0);
Please introduce func_id in patch #6 rather than doing this rework here.
>
> /*
> * Check immediate value for HVC32, HVC64 and SMC64.
> @@ -94,24 +226,38 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
> }
>
> /* 64 bit calls are allowed only from 64 bit domains. */
> - if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
> + if ( ARM_SMCCC_IS_64(func_id) &&
> 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)) )
> + /*
> + * Special case: identifier range for existing APIs.
> + * This range is described in SMCCC (ARM DEN 0028B, page 16),
> + * but it does not conforms to standard function identifier
> + * encoding.
> + */
> + if ( func_id >= ARM_SMCCC_RESERVED_RANGE_START &&
> + func_id <= ARM_SMCCC_RESERVED_RANGE_END )
> + handled = handle_psci_0_1(regs);
The region "reserved for existing APIs" is not only for psci 0.1. You
should make it clear in the name.
> + else
> {
> - case ARM_SMCCC_OWNER_HYPERVISOR:
> - handled = handle_hypervisor(regs);
> - break;
> + switch ( ARM_SMCCC_OWNER_NUM(func_id) )
> + {
> + case ARM_SMCCC_OWNER_HYPERVISOR:
> + handled = handle_hypervisor(regs);
> + break;
> + case ARM_SMCCC_OWNER_STANDARD:
> + handled = handle_sssc(regs);
> + break;
> + }
> }
>
> if ( !handled )
> {
> - gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
> - get_user_reg(regs, 0));
> + gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", func_id);
> /* Inform caller that function is not supported. */
> set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
> }
> @@ -150,6 +296,21 @@ 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)
> +{
> + const union hsr hsr = { .bits = regs->hsr };
> +
> + /*
> + * vsmccc_handle_call() will return false if this call is not
> + * SMCCC compatbile (i.e. immediate value != 0). As it is not
s/compatbile/compatible/. And you want to use e.g instead of i.e because
"immediate value != 0" is not the only way to return false.
> + * compatible, we can't be sure that guest will understand
> + * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
> + */
> + if ( !vsmccc_handle_call(regs) )
> + inject_undef_exception(regs, hsr);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 67da3fb..7c0003c 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -80,6 +80,10 @@
> /* Only one error code defined in SMCCC */
> #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1)
>
> +/* SMCCC function identifier range which is reserved for existing APIs */
> +#define ARM_SMCCC_RESERVED_RANGE_START 0x0
> +#define ARM_SMCCC_RESERVED_RANGE_END 0x0100FFFF
> +
> #endif /* __ASM_ARM_SMCCC_H__ */
>
> /*
> diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
> index 31aaa55..90ff610 100644
> --- a/xen/include/asm-arm/vsmc.h
> +++ b/xen/include/asm-arm/vsmc.h
> @@ -17,6 +17,7 @@
> #include <xen/types.h>
>
> 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 3d3cd90..c5327e3 100644
> --- a/xen/include/public/arch-arm/smc.h
> +++ b/xen/include/public/arch-arm/smc.h
> @@ -46,6 +46,14 @@
> #define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \
> 0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
>
> +/* Standard Service Service Call version. */
> +#define SSSC_SMCCC_MAJOR_REVISION 0
> +#define SSSC_SMCCC_MINOR_REVISION 1
> +
> +/* Standard Service Call UID. Randomly generated with uuidgen. */
> +#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd, 0x9220,\
> + 0xce, 0x16, 0x41, 0xe5, 0x9f, 0x6f)
> +
> #endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
Cheers,
>
> /*
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`
2017-08-24 16:58 ` Julien Grall
@ 2017-08-25 10:56 ` Volodymyr Babchuk
2017-08-25 11:10 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-25 10:56 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Julien,
Thanks for the review.
On 24.08.17 19:58, Julien Grall wrote:
> Hi Volodymyr,
>
> On 21/08/17 21:27, 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`.
>>
>> Older PSCI 0.1 uses SMC function identifiers in range that is
>> resereved for exisings APIs (ARM DEN 0028B, page 16), while newer
>
> s/resereved/reserved/
> s/exisings/existing/
>
>> PSCI 0.2 is defined as "standard secure service" with own ranges
>
> 0.2 and later
>
> "with its own ranges" I think.
>
>> (ARM DEN 0028B, page 18).
>>
>> 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>
>> ---
>>
>> * Fixed mistakes about non-existant PSCI 2.0
>> * Added special SMC function number handling for PSCI 0.1
>> * Fixed coding style in handle_psci_0_1()
>> * Changed how return do_trap_hvc_smccc() is called from traps.c
>> * Renamed SSC to SSSC (Standard Secure Service Calls)
>>
>> ---
>> xen/arch/arm/traps.c | 117 +------------------------
>> xen/arch/arm/vsmc.c | 175
>> ++++++++++++++++++++++++++++++++++++--
>> xen/include/asm-arm/smccc.h | 4 +
>> xen/include/asm-arm/vsmc.h | 1 +
>> xen/include/public/arch-arm/smc.h | 8 ++
>> 5 files changed, 183 insertions(+), 122 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 4141a89..517e013 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1451,119 +1451,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) & 0xFFFFFFFF)
>> -#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
>> @@ -2252,7 +2139,7 @@ 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);
>> + return do_trap_hvc_smccc(regs);
>> do_trap_hypercall(regs, (register_t *)®s->r12, hsr.iss);
>> break;
>> #ifdef CONFIG_ARM_64
>> @@ -2264,7 +2151,7 @@ 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);
>> + return do_trap_hvc_smccc(regs);
>> do_trap_hypercall(regs, ®s->x16, hsr.iss);
>> break;
>> case HSR_EC_SMC64:
>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>> index 0a81294..956d4ef 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -19,6 +19,7 @@
>> #include <xen/types.h>
>> #include <public/arch-arm/smc.h>
>> #include <asm/monitor.h>
>> +#include <asm/psci.h>
>> #include <asm/regs.h>
>> #include <asm/smccc.h>
>> #include <asm/traps.h>
>> @@ -27,6 +28,9 @@
>> /* Number of functions currently supported by Hypervisor Service. */
>> #define XEN_SMCCC_FUNCTION_COUNT 3
>>
>> +/* Number of functions currently supported by Standard Service
>> Service Calls. */
>> +#define SSSC_SMCCC_FUNCTION_COUNT 13
>> +
>> static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
>> {
>> #define FILL_UUID(n) \
>> @@ -62,6 +66,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) & 0xFFFFFFFF)
>> +#else
>> +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
>> +#endif
>> +
>> +/* PSCI 0.1 interface */
>> +static bool handle_psci_0_1(struct cpu_user_regs *regs)
>> +{
>> + switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
>
> I am not sure to understand the mask here.
>
>> + {
>> + case PSCI_cpu_off:
>> + {
>> + uint32_t pstate = PSCI_ARG32(regs, 1);
>
> Missing newline here. I am ok with you do the clean-up in this patch
> rather separately.
>
>> + 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);
>
> Ditto.
>
>> + 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 0.2 interface and other Standard Secure Calls */
>> +static bool handle_sssc(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;
>
> It is a bit hard to read. Some newline between each "case" would help
> for clarity I think.
>
>> + 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, SSSC_SMCCC_FUNCTION_COUNT);
>> + return true;
>> + case ARM_SMCCC_FUNC_CALL_UID:
>> + {
>> + static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
>
> Newline here please. But can't we just do:
>
> return fill_uuid(regs, SSC_SMCCC_UID);
>
> This would make the code simpler.
I'm not sure that I got this. fill_uuid() returns void, while
handle_sssc() returns bool.
>> + fill_uuid(regs, psci_uuid);
>> + return true;
>> + }
>> + case ARM_SMCCC_FUNC_CALL_REVISION:
>> + set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
>> + set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
>
> Same here.
Under "same" you meant newline, correct?
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> /*
>> * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
>> * returns true if that was valid SMCCC call (even if function number
>> @@ -71,6 +202,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs
>> *regs)
>> {
>> bool handled = false;
>> const union hsr hsr = { .bits = regs->hsr };
>> + register_t func_id = get_user_reg(regs, 0);
>
> Please introduce func_id in patch #6 rather than doing this rework here.
>
>>
>> /*
>> * Check immediate value for HVC32, HVC64 and SMC64.
>> @@ -94,24 +226,38 @@ static bool vsmccc_handle_call(struct
>> cpu_user_regs *regs)
>> }
>>
>> /* 64 bit calls are allowed only from 64 bit domains. */
>> - if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
>> + if ( ARM_SMCCC_IS_64(func_id) &&
>> 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)) )
>> + /*
>> + * Special case: identifier range for existing APIs.
>> + * This range is described in SMCCC (ARM DEN 0028B, page 16),
>> + * but it does not conforms to standard function identifier
>> + * encoding.
>> + */
>> + if ( func_id >= ARM_SMCCC_RESERVED_RANGE_START &&
>> + func_id <= ARM_SMCCC_RESERVED_RANGE_END )
>> + handled = handle_psci_0_1(regs);
>
> The region "reserved for existing APIs" is not only for psci 0.1. You
> should make it clear in the name.
>
>> + else
>> {
>> - case ARM_SMCCC_OWNER_HYPERVISOR:
>> - handled = handle_hypervisor(regs);
>> - break;
>> + switch ( ARM_SMCCC_OWNER_NUM(func_id) )
>> + {
>> + case ARM_SMCCC_OWNER_HYPERVISOR:
>> + handled = handle_hypervisor(regs);
>> + break;
>> + case ARM_SMCCC_OWNER_STANDARD:
>> + handled = handle_sssc(regs);
>> + break;
>> + }
>> }
>>
>> if ( !handled )
>> {
>> - gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
>> - get_user_reg(regs, 0));
>> + gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
>> func_id);
>> /* Inform caller that function is not supported. */
>> set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
>> }
>> @@ -150,6 +296,21 @@ 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)
>> +{
>> + const union hsr hsr = { .bits = regs->hsr };
>> +
>> + /*
>> + * vsmccc_handle_call() will return false if this call is not
>> + * SMCCC compatbile (i.e. immediate value != 0). As it is not
>
> s/compatbile/compatible/. And you want to use e.g instead of i.e because
> "immediate value != 0" is not the only way to return false.
>
>> + * compatible, we can't be sure that guest will understand
>> + * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
>> + */
>> + if ( !vsmccc_handle_call(regs) )
>> + inject_undef_exception(regs, hsr);
>> +}
>> +
>> /*
>> * Local variables:
>> * mode: C
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> index 67da3fb..7c0003c 100644
>> --- a/xen/include/asm-arm/smccc.h
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -80,6 +80,10 @@
>> /* Only one error code defined in SMCCC */
>> #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION (-1)
>>
>> +/* SMCCC function identifier range which is reserved for existing
>> APIs */
>> +#define ARM_SMCCC_RESERVED_RANGE_START 0x0
>> +#define ARM_SMCCC_RESERVED_RANGE_END 0x0100FFFF
>> +
>> #endif /* __ASM_ARM_SMCCC_H__ */
>>
>> /*
>> diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
>> index 31aaa55..90ff610 100644
>> --- a/xen/include/asm-arm/vsmc.h
>> +++ b/xen/include/asm-arm/vsmc.h
>> @@ -17,6 +17,7 @@
>> #include <xen/types.h>
>>
>> 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 3d3cd90..c5327e3 100644
>> --- a/xen/include/public/arch-arm/smc.h
>> +++ b/xen/include/public/arch-arm/smc.h
>> @@ -46,6 +46,14 @@
>> #define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369,
>> 0x9acf, \
>> 0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
>>
>> +/* Standard Service Service Call version. */
>> +#define SSSC_SMCCC_MAJOR_REVISION 0
>> +#define SSSC_SMCCC_MINOR_REVISION 1
>> +
>> +/* Standard Service Call UID. Randomly generated with uuidgen. */
>> +#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd,
>> 0x9220,\
>> + 0xce, 0x16, 0x41, 0xe5, 0x9f, 0x6f)
>> +
>> #endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
>
> Cheers,
>
>>
>> /*
>>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`
2017-08-25 10:56 ` Volodymyr Babchuk
@ 2017-08-25 11:10 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-08-25 11:10 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
On 25/08/17 11:56, Volodymyr Babchuk wrote:
> Hi Julien,
Hi Volodymyr,
> On 24.08.17 19:58, Julien Grall wrote:
>>> + case ARM_SMCCC_FUNC_CALL_COUNT:
>>> + set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
>>> + return true;
>>> + case ARM_SMCCC_FUNC_CALL_UID:
>>> + {
>>> + static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
>>
>> Newline here please. But can't we just do:
>>
>> return fill_uuid(regs, SSC_SMCCC_UID);
>>
>> This would make the code simpler.
> I'm not sure that I got this. fill_uuid() returns void, while
> handle_sssc() returns bool.
You could make fill_uuid() returning a bool. Overall every caller of
fill_uuid will do:
fill_uuid(....);
return true;
>
>>> + fill_uuid(regs, psci_uuid);
>>> + return true;
>>> + }
>>> + case ARM_SMCCC_FUNC_CALL_REVISION:
>>> + set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
>>> + set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
>>
>> Same here.
> Under "same" you meant newline, correct?
Hmmm, I don't remember :/. Although, we may want to introduce a helper
to fill the revision. Those wrapper would simplify the implementation of
"generic" function.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (6 preceding siblings ...)
2017-08-21 20:27 ` [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c` Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-24 17:22 ` Julien Grall
2017-08-21 20:27 ` [PATCH v4 09/11] arm: vsmc: remove 64 bit mode check in PSCI handler Volodymyr Babchuk
` (2 subsequent siblings)
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
smccc.h provides definitions to construct SMC call function number according
to SMCCC. We don't need multiple definitions for one thing, and definitions
in smccc.h are more generic than ones used in psci.h.
So psci.h will only provide function codes, while whole SMC function
identifier will be constructed using generic macros from smccc.h.
PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, because this
is a new entity. It can lead to problems, if we'll just change value of
PSCI_0_2_FN_xxx without renaming it.
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>
---
* Reworked definitions to minimize their lenght
* Described why I replaced PSCI_0_2_FN_xxx with PSCI_0_2_FUNC_xxx
---
xen/arch/arm/platforms/seattle.c | 5 +++--
xen/arch/arm/psci.c | 10 ++++-----
xen/arch/arm/vsmc.c | 24 +++++++++++-----------
xen/include/asm-arm/psci.h | 44 ++++++++++++++++++----------------------
4 files changed, 40 insertions(+), 43 deletions(-)
diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
index 86dce91..fb2ad13 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(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(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..645fe58 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(name) PSCI_0_2_FN64(name)
#else
-#define PSCI_0_2_FN_NATIVE(name) PSCI_0_2_FN_##name
+#define PSCI_0_2_FN_NATIVE(name) PSCI_0_2_FN32(name)
#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(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(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_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 )
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 956d4ef..46a2fde 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -102,7 +102,7 @@ static bool handle_psci_0_x(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) || !ARM_SMCCC_IS_64(fid);
}
/* PSCI 0.2 interface and other Standard Secure Calls */
@@ -112,34 +112,34 @@ static bool handle_sssc(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) )
{
@@ -149,7 +149,7 @@ static bool handle_sssc(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) )
{
@@ -159,7 +159,7 @@ static bool handle_sssc(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) )
{
@@ -168,7 +168,7 @@ static bool handle_sssc(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..14fb98f 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/smccc.h>
+
/* PSCI return values (inclusive of all PSCI versions) */
#define PSCI_SUCCESS 0
#define PSCI_NOT_SUPPORTED -1
@@ -41,30 +43,24 @@ 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_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_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_32, \
+ ARM_SMCCC_OWNER_STANDARD, \
+ PSCI_0_2_FUNC_##name)
+#define PSCI_0_2_FN64(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_64, \
+ ARM_SMCCC_OWNER_STANDARD, \
+ PSCI_0_2_FUNC_##name)
+#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
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h
2017-08-21 20:27 ` [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h Volodymyr Babchuk
@ 2017-08-24 17:22 ` Julien Grall
2017-08-25 11:00 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2017-08-24 17:22 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
On 21/08/17 21:27, Volodymyr Babchuk wrote:
> smccc.h provides definitions to construct SMC call function number according
> to SMCCC. We don't need multiple definitions for one thing, and definitions
> in smccc.h are more generic than ones used in psci.h.
>
> So psci.h will only provide function codes, while whole SMC function
> identifier will be constructed using generic macros from smccc.h.
>
> PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, because this
> is a new entity. It can lead to problems, if we'll just change value of
> PSCI_0_2_FN_xxx without renaming it.
I don't think "new entity" is a good reason to rename them. And the
previous naming was kind of nice to read. More that you still use
PSCI_0_2_FN{32,64}. We should definitely stay consistent in naming.
So what is the exact problem? Is it because you are worry to miss some
of them?
>
> This change also affects vsmc.c and seattle.c, because they both use PSCI
> function numbers.
This paragraph could be dropped.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
> * Reworked definitions to minimize their lenght
> * Described why I replaced PSCI_0_2_FN_xxx with PSCI_0_2_FUNC_xxx
>
> ---
> xen/arch/arm/platforms/seattle.c | 5 +++--
> xen/arch/arm/psci.c | 10 ++++-----
> xen/arch/arm/vsmc.c | 24 +++++++++++-----------
> xen/include/asm-arm/psci.h | 44 ++++++++++++++++++----------------------
> 4 files changed, 40 insertions(+), 43 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
> index 86dce91..fb2ad13 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>
Again, vsmc.h stands for "virtual SMC". Here you use for the "physical
SMC". So please don't include vsmc.h 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(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(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..645fe58 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(name) PSCI_0_2_FN64(name)
> #else
> -#define PSCI_0_2_FN_NATIVE(name) PSCI_0_2_FN_##name
> +#define PSCI_0_2_FN_NATIVE(name) PSCI_0_2_FN32(name)
> #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(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(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_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 )
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 956d4ef..46a2fde 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -102,7 +102,7 @@ static bool handle_psci_0_x(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) || !ARM_SMCCC_IS_64(fid);
I don't see any reason to do the renaming in psci_mode_check given that
you will remove this function in the next patch.
> }
>
> /* PSCI 0.2 interface and other Standard Secure Calls */
> @@ -112,34 +112,34 @@ static bool handle_sssc(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) )
> {
> @@ -149,7 +149,7 @@ static bool handle_sssc(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) )
> {
> @@ -159,7 +159,7 @@ static bool handle_sssc(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) )
> {
> @@ -168,7 +168,7 @@ static bool handle_sssc(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..14fb98f 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/smccc.h>
> +
> /* PSCI return values (inclusive of all PSCI versions) */
> #define PSCI_SUCCESS 0
> #define PSCI_NOT_SUPPORTED -1
> @@ -41,30 +43,24 @@ 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 */
Why did you drop this comment?
> -#define PSCI_0_2_FN_BASE 0x84000000
> -#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE + (n))
> -#define PSCI_0_2_64BIT 0x40000000
> -#define PSCI_0_2_FN64_BASE \
> - (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT)
> -#define PSCI_0_2_FN64(n) (PSCI_0_2_FN64_BASE + (n))
> -
> -#define PSCI_0_2_FN_PSCI_VERSION PSCI_0_2_FN(0)
> -#define PSCI_0_2_FN_CPU_SUSPEND PSCI_0_2_FN(1)
> -#define PSCI_0_2_FN_CPU_OFF PSCI_0_2_FN(2)
> -#define PSCI_0_2_FN_CPU_ON PSCI_0_2_FN(3)
> -#define PSCI_0_2_FN_AFFINITY_INFO PSCI_0_2_FN(4)
> -#define PSCI_0_2_FN_MIGRATE PSCI_0_2_FN(5)
> -#define PSCI_0_2_FN_MIGRATE_INFO_TYPE PSCI_0_2_FN(6)
> -#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7)
> -#define PSCI_0_2_FN_SYSTEM_OFF PSCI_0_2_FN(8)
> -#define PSCI_0_2_FN_SYSTEM_RESET PSCI_0_2_FN(9)
> -
> -#define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1)
> -#define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3)
> -#define PSCI_0_2_FN64_AFFINITY_INFO PSCI_0_2_FN64(4)
> -#define PSCI_0_2_FN64_MIGRATE PSCI_0_2_FN64(5)
> -#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU PSCI_0_2_FN64(7)
> +#define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_SMC_32, \
> + ARM_SMCCC_OWNER_STANDARD, \
> + PSCI_0_2_FUNC_##name)
The indentation looks wrong here.
> +#define PSCI_0_2_FN64(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_SMC_64, \
> + ARM_SMCCC_OWNER_STANDARD, \
> + PSCI_0_2_FUNC_##name)
Ditto.
> +#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
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h
2017-08-24 17:22 ` Julien Grall
@ 2017-08-25 11:00 ` Volodymyr Babchuk
2017-08-25 11:13 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-25 11:00 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi,
On 24.08.17 20:22, Julien Grall wrote:
> Hi Volodymyr,
>
> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>> smccc.h provides definitions to construct SMC call function number
>> according
>> to SMCCC. We don't need multiple definitions for one thing, and
>> definitions
>> in smccc.h are more generic than ones used in psci.h.
>>
>> So psci.h will only provide function codes, while whole SMC function
>> identifier will be constructed using generic macros from smccc.h.
>>
>> PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, because
>> this
>> is a new entity. It can lead to problems, if we'll just change value of
>> PSCI_0_2_FN_xxx without renaming it.
>
> I don't think "new entity" is a good reason to rename them. And the
> previous naming was kind of nice to read. More that you still use
> PSCI_0_2_FN{32,64}. We should definitely stay consistent in naming.
>
> So what is the exact problem? Is it because you are worry to miss some
> of them?
Actually yes. That helped me to find references to PSCI code in seattle.c
>>
>> This change also affects vsmc.c and seattle.c, because they both use PSCI
>> function numbers.
>
> This paragraph could be dropped.
>
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>
>> * Reworked definitions to minimize their lenght
>> * Described why I replaced PSCI_0_2_FN_xxx with PSCI_0_2_FUNC_xxx
>>
>> ---
>> xen/arch/arm/platforms/seattle.c | 5 +++--
>> xen/arch/arm/psci.c | 10 ++++-----
>> xen/arch/arm/vsmc.c | 24 +++++++++++-----------
>> xen/include/asm-arm/psci.h | 44
>> ++++++++++++++++++----------------------
>> 4 files changed, 40 insertions(+), 43 deletions(-)
>>
>> diff --git a/xen/arch/arm/platforms/seattle.c
>> b/xen/arch/arm/platforms/seattle.c
>> index 86dce91..fb2ad13 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>
>
> Again, vsmc.h stands for "virtual SMC". Here you use for the "physical
> SMC". So please don't include vsmc.h 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(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(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..645fe58 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(name) PSCI_0_2_FN64(name)
>> #else
>> -#define PSCI_0_2_FN_NATIVE(name) PSCI_0_2_FN_##name
>> +#define PSCI_0_2_FN_NATIVE(name) PSCI_0_2_FN32(name)
>> #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(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(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_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 )
>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>> index 956d4ef..46a2fde 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -102,7 +102,7 @@ static bool handle_psci_0_x(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) || !ARM_SMCCC_IS_64(fid);
>
> I don't see any reason to do the renaming in psci_mode_check given that
> you will remove this function in the next patch.
Yep, but in comments for the previous series you asked me to make this code
easier to read :)
PSCI_0_2_64BIT is dropped now, so I had to use long definitions from smccc.h
>> }
>>
>> /* PSCI 0.2 interface and other Standard Secure Calls */
>> @@ -112,34 +112,34 @@ static bool handle_sssc(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) )
>> {
>> @@ -149,7 +149,7 @@ static bool handle_sssc(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) )
>> {
>> @@ -159,7 +159,7 @@ static bool handle_sssc(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) )
>> {
>> @@ -168,7 +168,7 @@ static bool handle_sssc(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..14fb98f 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/smccc.h>
>> +
>> /* PSCI return values (inclusive of all PSCI versions) */
>> #define PSCI_SUCCESS 0
>> #define PSCI_NOT_SUPPORTED -1
>> @@ -41,30 +43,24 @@ 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 */
>
> Why did you drop this comment?
>
>> -#define PSCI_0_2_FN_BASE 0x84000000
>> -#define PSCI_0_2_FN(n) (PSCI_0_2_FN_BASE + (n))
>> -#define PSCI_0_2_64BIT 0x40000000
>> -#define PSCI_0_2_FN64_BASE \
>> - (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT)
>> -#define PSCI_0_2_FN64(n) (PSCI_0_2_FN64_BASE + (n))
>> -
>> -#define PSCI_0_2_FN_PSCI_VERSION PSCI_0_2_FN(0)
>> -#define PSCI_0_2_FN_CPU_SUSPEND PSCI_0_2_FN(1)
>> -#define PSCI_0_2_FN_CPU_OFF PSCI_0_2_FN(2)
>> -#define PSCI_0_2_FN_CPU_ON PSCI_0_2_FN(3)
>> -#define PSCI_0_2_FN_AFFINITY_INFO PSCI_0_2_FN(4)
>> -#define PSCI_0_2_FN_MIGRATE PSCI_0_2_FN(5)
>> -#define PSCI_0_2_FN_MIGRATE_INFO_TYPE PSCI_0_2_FN(6)
>> -#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7)
>> -#define PSCI_0_2_FN_SYSTEM_OFF PSCI_0_2_FN(8)
>> -#define PSCI_0_2_FN_SYSTEM_RESET PSCI_0_2_FN(9)
>> -
>> -#define PSCI_0_2_FN64_CPU_SUSPEND PSCI_0_2_FN64(1)
>> -#define PSCI_0_2_FN64_CPU_ON PSCI_0_2_FN64(3)
>> -#define PSCI_0_2_FN64_AFFINITY_INFO PSCI_0_2_FN64(4)
>> -#define PSCI_0_2_FN64_MIGRATE PSCI_0_2_FN64(5)
>> -#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU PSCI_0_2_FN64(7)
>> +#define PSCI_0_2_FN32(name)
>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
>> +
>> ARM_SMCCC_SMC_32, \
>> +
>> ARM_SMCCC_OWNER_STANDARD, \
>> + PSCI_0_2_FUNC_##name)
>
> The indentation looks wrong here.
>
>> +#define PSCI_0_2_FN64(name)
>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
>> +
>> ARM_SMCCC_SMC_64, \
>> +
>> ARM_SMCCC_OWNER_STANDARD, \
>> + PSCI_0_2_FUNC_##name)
>
>
> Ditto.
>
>> +#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
>>
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h
2017-08-25 11:00 ` Volodymyr Babchuk
@ 2017-08-25 11:13 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-08-25 11:13 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
On 25/08/17 12:00, Volodymyr Babchuk wrote:
> Hi,
>
> On 24.08.17 20:22, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>> smccc.h provides definitions to construct SMC call function number
>>> according
>>> to SMCCC. We don't need multiple definitions for one thing, and
>>> definitions
>>> in smccc.h are more generic than ones used in psci.h.
>>>
>>> So psci.h will only provide function codes, while whole SMC function
>>> identifier will be constructed using generic macros from smccc.h.
>>>
>>> PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx,
>>> because this
>>> is a new entity. It can lead to problems, if we'll just change value of
>>> PSCI_0_2_FN_xxx without renaming it.
>>
>> I don't think "new entity" is a good reason to rename them. And the
>> previous naming was kind of nice to read. More that you still use
>> PSCI_0_2_FN{32,64}. We should definitely stay consistent in naming.
>>
>> So what is the exact problem? Is it because you are worry to miss some
>> of them?
> Actually yes. That helped me to find references to PSCI code in seattle.c
But now you have inconsistent naming which is not better. I would prefer
much prefer to keep the same name as before.
>>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>>> index 956d4ef..46a2fde 100644
>>> --- a/xen/arch/arm/vsmc.c
>>> +++ b/xen/arch/arm/vsmc.c
>>> @@ -102,7 +102,7 @@ static bool handle_psci_0_x(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) || !ARM_SMCCC_IS_64(fid);
>>
>> I don't see any reason to do the renaming in psci_mode_check given
>> that you will remove this function in the next patch.
> Yep, but in comments for the previous series you asked me to make this code
> easier to read :)
> PSCI_0_2_64BIT is dropped now, so I had to use long definitions from
> smccc.h
If you remove the code in this series, then there are no need to review
it. You can just mention in the commit message why you didn't clean-up
that function.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 09/11] arm: vsmc: remove 64 bit mode check in PSCI handler
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (7 preceding siblings ...)
2017-08-21 20:27 ` [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-21 20:27 ` [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature Volodymyr Babchuk
2017-08-21 20:27 ` [PATCH v4 11/11] arm: enable " Volodymyr Babchuk
10 siblings, 0 replies; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, 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.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
* Used case xxx:
{
}
formatting as Julien suggested
---
xen/arch/arm/vsmc.c | 63 +++++++++++++++++++++++++----------------------------
1 file changed, 30 insertions(+), 33 deletions(-)
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 46a2fde..673b23c 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -99,12 +99,6 @@ static bool handle_psci_0_x(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) || !ARM_SMCCC_IS_64(fid);
-}
-
/* PSCI 0.2 interface and other Standard Secure Calls */
static bool handle_sssc(struct cpu_user_regs *regs)
{
@@ -126,8 +120,7 @@ static bool handle_sssc(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);
@@ -140,42 +133,46 @@ static bool handle_sssc(struct cpu_user_regs *regs)
PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
return true;
case PSCI_0_2_FUNC_CPU_ON:
+ {
+ register_t vcpuid = PSCI_ARG(regs,1);
+ register_t epoint = PSCI_ARG(regs,2);
+ register_t cid = PSCI_ARG(regs,3);
+
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(vcpuid, epoint, cid));
return true;
+ }
case PSCI_0_2_FUNC_CPU_SUSPEND:
+ {
+ uint32_t pstate = PSCI_ARG32(regs,1);
+ register_t epoint = PSCI_ARG(regs,2);
+ register_t cid = PSCI_ARG(regs,3);
+
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(pstate, epoint, cid));
return true;
+ }
case PSCI_0_2_FUNC_AFFINITY_INFO:
+ {
+ register_t taff = PSCI_ARG(regs,1);
+ uint32_t laff = PSCI_ARG32(regs,2);
+
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(taff, laff));
return true;
+ }
case PSCI_0_2_FUNC_MIGRATE:
+ {
+ uint32_t tcpu = PSCI_ARG32(regs,1);
+
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(tcpu));
return true;
+ }
case ARM_SMCCC_FUNC_CALL_COUNT:
set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
return true;
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (8 preceding siblings ...)
2017-08-21 20:27 ` [PATCH v4 09/11] arm: vsmc: remove 64 bit mode check in PSCI handler Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-24 17:25 ` Julien Grall
2017-08-21 20:27 ` [PATCH v4 11/11] arm: enable " Volodymyr Babchuk
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, Volodymyr Babchuk
This feature indicates that hypervisor is compatible with ARM
SMC calling convention. Hypervisor will not inject an undefined
instruction exception if an invalid SMC function were called and
will not crash a domain if an invlalid HVC functions were called.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
xen/include/public/features.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index 2110b04..1a989b8 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -102,6 +102,9 @@
/* Guest can use XENMEMF_vnode to specify virtual node for memory op. */
#define XENFEAT_memory_op_vnode_supported 13
+/* arm: Hypervisor supports ARM SMC calling convention. */
+#define XENFEAT_ARM_SMCCC_supported 14
+
#define XENFEAT_NR_SUBMAPS 1
#endif /* __XEN_PUBLIC_FEATURES_H__ */
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-08-21 20:27 ` [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature Volodymyr Babchuk
@ 2017-08-24 17:25 ` Julien Grall
2017-08-31 12:20 ` Sergej Proskurin
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2017-08-24 17:25 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
On 21/08/17 21:27, Volodymyr Babchuk wrote:
> This feature indicates that hypervisor is compatible with ARM
> SMC calling convention. Hypervisor will not inject an undefined
> instruction exception if an invalid SMC function were called and
> will not crash a domain if an invlalid HVC functions were called.
s/invlalid/invalid/
The last sentence is misleading. Xen will still inject and undefined
instruction for some SMC/HVC. You may want to rework it to make it clear.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> xen/include/public/features.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/include/public/features.h b/xen/include/public/features.h
> index 2110b04..1a989b8 100644
> --- a/xen/include/public/features.h
> +++ b/xen/include/public/features.h
> @@ -102,6 +102,9 @@
> /* Guest can use XENMEMF_vnode to specify virtual node for memory op. */
> #define XENFEAT_memory_op_vnode_supported 13
>
> +/* arm: Hypervisor supports ARM SMC calling convention. */
> +#define XENFEAT_ARM_SMCCC_supported 14
> +
> #define XENFEAT_NR_SUBMAPS 1
>
> #endif /* __XEN_PUBLIC_FEATURES_H__ */
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-08-24 17:25 ` Julien Grall
@ 2017-08-31 12:20 ` Sergej Proskurin
2017-08-31 12:44 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Sergej Proskurin @ 2017-08-31 12:20 UTC (permalink / raw)
To: Julien Grall, Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Volodymyr, hi Julien,
On 08/24/2017 07:25 PM, Julien Grall wrote:
>
>
> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>> This feature indicates that hypervisor is compatible with ARM
>> SMC calling convention. Hypervisor will not inject an undefined
>> instruction exception if an invalid SMC function were called and
>> will not crash a domain if an invlalid HVC functions were called.
>
> s/invlalid/invalid/
>
> The last sentence is misleading. Xen will still inject and undefined
> instruction for some SMC/HVC. You may want to rework it to make it clear.
>
Now that you say that Xen will still inject an undefined instruction
exception for some SMCs, I have a to ask for which exactly?
I might be off topic here, so please tell me if you believe this is not
the right place for this question. In this case I will open an new
thread. Right now, I am working with the previous implementation of
do_trap_smc that was extended in this patch. Yet, as far as I
understand, the behavior should not change, which is why I am asking
this quesiton in this thread.
Currently, I am working on SMC guest injections and trying to understand
the resulting behavior. Every time, right after the execution of an
injected SMC instruction, the guest traps into the undefined instruction
exception handler in EL1 and I simply don't understand why. As far as I
understand, as soon an injected SMC instruction gets executed, it should
_transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is set).
As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
that now contains the original instruction instead of the injected SMC),
the guest should simply continue its execution.
Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC instructions
to EL2, that trap has priority over this disable". So this means that if
SMCs are disabled for NS EL1, the guest will trap into the hypervisor on
SMC execution. Yet, since SMCs are disabled from NS EL1, the guest will
execute an undefined instrcution exception. Which is what I was thinking
about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
the other hand I believe that it is highly unlikely that the EFI loader
explicitly disables SMC's for NS EL1. However, since I don't have access
to SCR_EL3.SMD from EL2, I can't tell whether this is the reason for the
behavior I am experiencing on my board or not.
It would be of great help if you would provide me with some more clarity
on my case. I am sure that I have missed something that simply needs
clarification. Thank you very much in advance.
Thanks,
~Sergej
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-08-31 12:20 ` Sergej Proskurin
@ 2017-08-31 12:44 ` Volodymyr Babchuk
2017-08-31 13:51 ` Sergej Proskurin
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 12:44 UTC (permalink / raw)
To: Sergej Proskurin, Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hello Sergej,
On 31.08.17 15:20, Sergej Proskurin wrote:
> Hi Volodymyr, hi Julien,
>
>
> On 08/24/2017 07:25 PM, Julien Grall wrote:
>>
>>
>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>> This feature indicates that hypervisor is compatible with ARM
>>> SMC calling convention. Hypervisor will not inject an undefined
>>> instruction exception if an invalid SMC function were called and
>>> will not crash a domain if an invlalid HVC functions were called.
>>
>> s/invlalid/invalid/
>>
>> The last sentence is misleading. Xen will still inject and undefined
>> instruction for some SMC/HVC. You may want to rework it to make it clear.
>>
>
> Now that you say that Xen will still inject an undefined instruction
> exception for some SMCs, I have a to ask for which exactly?
For ones that are compatible with ARM SMCCC [1]. E.g if you are running
SMCCC-compatible system and you are calling SMC/HVC with immediate value
0, then you are safe.
> I might be off topic here, so please tell me if you believe this is not
> the right place for this question. In this case I will open an new
> thread. Right now, I am working with the previous implementation of
> do_trap_smc that was extended in this patch. Yet, as far as I
> understand, the behavior should not change, which is why I am asking
> this quesiton in this thread.
If you are talking about forwarding SMC exception to VM monitor, then
yes, that should not change.
> Currently, I am working on SMC guest injections and trying to understand
> the resulting behavior. Every time, right after the execution of an
> injected SMC instruction, the guest traps into the undefined instruction
> exception handler in EL1 and I simply don't understand why. As far as I
> understand, as soon an injected SMC instruction gets executed, it should
> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is set).
> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
> that now contains the original instruction instead of the injected SMC),
> the guest should simply continue its execution.
Hm. What do you mean under "SMC instruction injection?".
Current code in hypervisor will always inject undefined instruction
exception when you call SMC (unless you installed VM monitor for the
guest). Also, it will not increase PC. So, if you'll try to remove
inject_undef_exception() call, you'll get into an infinite loop.
> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC instructions
> to EL2, that trap has priority over this disable". So this means that if
> SMCs are disabled for NS EL1, the guest will trap into the hypervisor on
> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest will
> execute an undefined instrcution exception. Which is what I was thinking
> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
> the other hand I believe that it is highly unlikely that the EFI loader
> explicitly disables SMC's for NS EL1. However, since I don't have access
> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason for the
> behavior I am experiencing on my board or not.
According to ARM ARM, hypervisor should trap SMC even if was disabled by
EL3. I think, that in your case the problem is in current implementation
of do_trap_smc()
> It would be of great help if you would provide me with some more clarity
> on my case. I am sure that I have missed something that simply needs
> clarification. Thank you very much in advance.
I don't quite understood, what you are trying to achieve. But I think
that pair of printk()s in do_trap_smc() will reveal much.
[1]
http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-08-31 12:44 ` Volodymyr Babchuk
@ 2017-08-31 13:51 ` Sergej Proskurin
2017-08-31 14:58 ` Volodymyr Babchuk
0 siblings, 1 reply; 48+ messages in thread
From: Sergej Proskurin @ 2017-08-31 13:51 UTC (permalink / raw)
To: Volodymyr Babchuk, Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Volodymyr,
On 08/31/2017 02:44 PM, Volodymyr Babchuk wrote:
> Hello Sergej,
>
> On 31.08.17 15:20, Sergej Proskurin wrote:
>> Hi Volodymyr, hi Julien,
>>
>>
>> On 08/24/2017 07:25 PM, Julien Grall wrote:
>>>
>>>
>>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>>> This feature indicates that hypervisor is compatible with ARM
>>>> SMC calling convention. Hypervisor will not inject an undefined
>>>> instruction exception if an invalid SMC function were called and
>>>> will not crash a domain if an invlalid HVC functions were called.
>>>
>>> s/invlalid/invalid/
>>>
>>> The last sentence is misleading. Xen will still inject and undefined
>>> instruction for some SMC/HVC. You may want to rework it to make it
>>> clear.
>>>
>>
>> Now that you say that Xen will still inject an undefined instruction
>> exception for some SMCs, I have a to ask for which exactly?
> For ones that are compatible with ARM SMCCC [1]. E.g if you are
> running SMCCC-compatible system and you are calling SMC/HVC with
> immediate value 0, then you are safe.
>
Alright, as far as I understand this is exactly what I do right now. I
inject an SMC that is encoded as 0xD4000003.
>> I might be off topic here, so please tell me if you believe this is not
>> the right place for this question. In this case I will open an new
>> thread. Right now, I am working with the previous implementation of
>> do_trap_smc that was extended in this patch. Yet, as far as I
>> understand, the behavior should not change, which is why I am asking
>> this quesiton in this thread.
> If you are talking about forwarding SMC exception to VM monitor, then
> yes, that should not change.
Yes, exactly. Sorry, I forgot to mention that I have a modified
xen-access version running in dom0 that registers an SMC monitor and
also increases the PC by 4 (or dependent on the case, simply leaves it
as it is) on every SMC trap.
>
>> Currently, I am working on SMC guest injections and trying to understand
>> the resulting behavior. Every time, right after the execution of an
>> injected SMC instruction, the guest traps into the undefined instruction
>> exception handler in EL1 and I simply don't understand why. As far as I
>> understand, as soon an injected SMC instruction gets executed, it should
>> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is set).
>> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
>> that now contains the original instruction instead of the injected SMC),
>> the guest should simply continue its execution.
> Hm. What do you mean under "SMC instruction injection?".
My code runs in dom0 and "injects" an SMC instruction to predefined
addresses inside the guest as to simulate software breakpoints. By this,
I mean that the code replaces the original guest instruction at a
certain address with an SMC. Think of a debugger that uses software
breakpoints. The idea is to put back the original instruction right
after the SMC gets called, so that the guest can continue with its
execution. You can find more information about that in [0], yet please
consider that I try to trap the SMC directly in Xen instead of TrustZone.
> Current code in hypervisor will always inject undefined instruction
> exception when you call SMC (unless you installed VM monitor for the
> guest). Also, it will not increase PC. So, if you'll try to remove
> inject_undef_exception() call, you'll get into an infinite loop.
>
I have a registered SMC monitor running in dom0 that does not reinject
the undefined instruction exception in do_trap_smc(). So there is no
indefinite loop at this point. What I see is that as soon as my code in
xen-access (dom0) increments the trapped guest PC by 4 (and also if it
doesn't) the next instruction inside the guest will be inside the undef
instruction handler (I can see that because I have implemented a single
stepping mechanism for AArch64 in Xen that gets activated right after
the guest executes the injected SMC instruction).
>> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
>> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC instructions
>> to EL2, that trap has priority over this disable". So this means that if
>> SMCs are disabled for NS EL1, the guest will trap into the hypervisor on
>> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest will
>> execute an undefined instrcution exception. Which is what I was thinking
>> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
>> the other hand I believe that it is highly unlikely that the EFI loader
>> explicitly disables SMC's for NS EL1. However, since I don't have access
>> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason for the
>> behavior I am experiencing on my board or not.
> According to ARM ARM, hypervisor should trap SMC even if was disabled
> by EL3. I think, that in your case the problem is in current
> implementation of do_trap_smc()
>
Unfortunately, I don't think that this is the problem of do_trap_smc()
(see above). But let me check one more time.
>> It would be of great help if you would provide me with some more clarity
>> on my case. I am sure that I have missed something that simply needs
>> clarification. Thank you very much in advance.
> I don't quite understood, what you are trying to achieve. But I think
> that pair of printk()s in do_trap_smc() will reveal much.
>
Yea, the idea is to inject SMC instructions into the guest to simulate
software breakpoints for guest analysis purposes. Please let me cleanup
my current printk output to better present my issue.
>
> [1]
> http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
Thank you,
~Sergej
[0] http://www.cse.psu.edu/~trj1/papers/most14.pdf
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-08-31 13:51 ` Sergej Proskurin
@ 2017-08-31 14:58 ` Volodymyr Babchuk
2017-08-31 20:16 ` Sergej Proskurin
0 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 14:58 UTC (permalink / raw)
To: Sergej Proskurin, Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Sergej
On 31.08.17 16:51, Sergej Proskurin wrote:
> Hi Volodymyr,
>
>
> On 08/31/2017 02:44 PM, Volodymyr Babchuk wrote:
>> Hello Sergej,
>>
>> On 31.08.17 15:20, Sergej Proskurin wrote:
>>> Hi Volodymyr, hi Julien,
>>>
>>>
>>> On 08/24/2017 07:25 PM, Julien Grall wrote:
>>>>
>>>>
>>>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>>>> This feature indicates that hypervisor is compatible with ARM
>>>>> SMC calling convention. Hypervisor will not inject an undefined
>>>>> instruction exception if an invalid SMC function were called and
>>>>> will not crash a domain if an invlalid HVC functions were called.
>>>>
>>>> s/invlalid/invalid/
>>>>
>>>> The last sentence is misleading. Xen will still inject and undefined
>>>> instruction for some SMC/HVC. You may want to rework it to make it
>>>> clear.
>>>>
>>>
>>> Now that you say that Xen will still inject an undefined instruction
>>> exception for some SMCs, I have a to ask for which exactly?
>> For ones that are compatible with ARM SMCCC [1]. E.g if you are
>> running SMCCC-compatible system and you are calling SMC/HVC with
>> immediate value 0, then you are safe.
>>
>
> Alright, as far as I understand this is exactly what I do right now. I
> inject an SMC that is encoded as 0xD4000003.
Actually, this patch series are not merged yet, so no SMCCC support
right. But this should not a problem in your case.
>>> I might be off topic here, so please tell me if you believe this is not
>>> the right place for this question. In this case I will open an new
>>> thread. Right now, I am working with the previous implementation of
>>> do_trap_smc that was extended in this patch. Yet, as far as I
>>> understand, the behavior should not change, which is why I am asking
>>> this quesiton in this thread.
>> If you are talking about forwarding SMC exception to VM monitor, then
>> yes, that should not change.
>
> Yes, exactly. Sorry, I forgot to mention that I have a modified
> xen-access version running in dom0 that registers an SMC monitor and
> also increases the PC by 4 (or dependent on the case, simply leaves it
> as it is) on every SMC trap.
Aha, I see. I never was able to test this feature fully. I played with
my own VM monitor, when I tried to offload SMC handling to another
domain. But I had to comment out most of the VM monitor code in XEN.
>>
>>> Currently, I am working on SMC guest injections and trying to understand
>>> the resulting behavior. Every time, right after the execution of an
>>> injected SMC instruction, the guest traps into the undefined instruction
>>> exception handler in EL1 and I simply don't understand why. As far as I
>>> understand, as soon an injected SMC instruction gets executed, it should
>>> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is set).
>>> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
>>> that now contains the original instruction instead of the injected SMC),
>>> the guest should simply continue its execution.
>> Hm. What do you mean under "SMC instruction injection?".
>
> My code runs in dom0 and "injects" an SMC instruction to predefined
> addresses inside the guest as to simulate software breakpoints. By this,
> I mean that the code replaces the original guest instruction at a
> certain address with an SMC. Think of a debugger that uses software
> breakpoints. The idea is to put back the original instruction right
> after the SMC gets called, so that the guest can continue with its
> execution. You can find more information about that in [0], yet please
> consider that I try to trap the SMC directly in Xen instead of TrustZone.
Yep, I see. Immediate question: do you flush icache after you put
original instruction back? Then I can't see, why this should not work.
If only VM monitor core in XEN is not broken. I don't know any users of
this.
I'm just curious, why are you using SMC, not BRK instruction?
>> Current code in hypervisor will always inject undefined instruction
>> exception when you call SMC (unless you installed VM monitor for the
>> guest). Also, it will not increase PC. So, if you'll try to remove
>> inject_undef_exception() call, you'll get into an infinite loop.
>>
>
> I have a registered SMC monitor running in dom0 that does not reinject
> the undefined instruction exception in do_trap_smc(). So there is no
> indefinite loop at this point. What I see is that as soon as my code in
> xen-access (dom0) increments the trapped guest PC by 4 (and also if it
> doesn't) the next instruction inside the guest will be inside the undef
> instruction handler (I can see that because I have implemented a single
> stepping mechanism for AArch64 in Xen that gets activated right after
> the guest executes the injected SMC instruction).
That's strange. Can you print whole vCPU state to determine that PC
points to the right place? Also you can check DFAR. Probably you can
even dump memory pointed by DFAR to make sure that you written back
correct instruction.
>>> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
>>> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC instructions
>>> to EL2, that trap has priority over this disable". So this means that if
>>> SMCs are disabled for NS EL1, the guest will trap into the hypervisor on
>>> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest will
>>> execute an undefined instrcution exception. Which is what I was thinking
>>> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
>>> the other hand I believe that it is highly unlikely that the EFI loader
>>> explicitly disables SMC's for NS EL1. However, since I don't have access
>>> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason for the
>>> behavior I am experiencing on my board or not.
>> According to ARM ARM, hypervisor should trap SMC even if was disabled
>> by EL3. I think, that in your case the problem is in current
>> implementation of do_trap_smc()
>>
>
> Unfortunately, I don't think that this is the problem of do_trap_smc()
> (see above). But let me check one more time.
As I said, I know no users for SMC monitor, and I'm not exactly sure
that it works properly.
>
>>> It would be of great help if you would provide me with some more clarity
>>> on my case. I am sure that I have missed something that simply needs
>>> clarification. Thank you very much in advance.
>> I don't quite understood, what you are trying to achieve. But I think
>> that pair of printk()s in do_trap_smc() will reveal much.
>>
>
> Yea, the idea is to inject SMC instructions into the guest to simulate
> software breakpoints for guest analysis purposes. Please let me cleanup
> my current printk output to better present my issue.
>
>>
>> [1]
>> http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
>
> Thank you,
> ~Sergej
>
> [0] http://www.cse.psu.edu/~trj1/papers/most14.pdf
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-08-31 14:58 ` Volodymyr Babchuk
@ 2017-08-31 20:16 ` Sergej Proskurin
2017-09-04 6:07 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Sergej Proskurin @ 2017-08-31 20:16 UTC (permalink / raw)
To: Volodymyr Babchuk, Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Volodymyr,
On 08/31/2017 04:58 PM, Volodymyr Babchuk wrote:
> Hi Sergej
>
> On 31.08.17 16:51, Sergej Proskurin wrote:
>> Hi Volodymyr,
>>
>>
>> On 08/31/2017 02:44 PM, Volodymyr Babchuk wrote:
>>> Hello Sergej,
>>>
>>> On 31.08.17 15:20, Sergej Proskurin wrote:
>>>> Hi Volodymyr, hi Julien,
>>>>
>>>>
>>>> On 08/24/2017 07:25 PM, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>>>>> This feature indicates that hypervisor is compatible with ARM
>>>>>> SMC calling convention. Hypervisor will not inject an undefined
>>>>>> instruction exception if an invalid SMC function were called and
>>>>>> will not crash a domain if an invlalid HVC functions were called.
>>>>>
>>>>> s/invlalid/invalid/
>>>>>
>>>>> The last sentence is misleading. Xen will still inject and undefined
>>>>> instruction for some SMC/HVC. You may want to rework it to make it
>>>>> clear.
>>>>>
>>>>
>>>> Now that you say that Xen will still inject an undefined instruction
>>>> exception for some SMCs, I have a to ask for which exactly?
>>> For ones that are compatible with ARM SMCCC [1]. E.g if you are
>>> running SMCCC-compatible system and you are calling SMC/HVC with
>>> immediate value 0, then you are safe.
>>>
>>
>> Alright, as far as I understand this is exactly what I do right now. I
>> inject an SMC that is encoded as 0xD4000003.
> Actually, this patch series are not merged yet, so no SMCCC support
> right. But this should not a problem in your case.
>
>>>> I might be off topic here, so please tell me if you believe this is
>>>> not
>>>> the right place for this question. In this case I will open an new
>>>> thread. Right now, I am working with the previous implementation of
>>>> do_trap_smc that was extended in this patch. Yet, as far as I
>>>> understand, the behavior should not change, which is why I am asking
>>>> this quesiton in this thread.
>>> If you are talking about forwarding SMC exception to VM monitor, then
>>> yes, that should not change.
>>
>> Yes, exactly. Sorry, I forgot to mention that I have a modified
>> xen-access version running in dom0 that registers an SMC monitor and
>> also increases the PC by 4 (or dependent on the case, simply leaves it
>> as it is) on every SMC trap.
> Aha, I see. I never was able to test this feature fully. I played with
> my own VM monitor, when I tried to offload SMC handling to another
> domain. But I had to comment out most of the VM monitor code in XEN.
>
>>>
>>>> Currently, I am working on SMC guest injections and trying to
>>>> understand
>>>> the resulting behavior. Every time, right after the execution of an
>>>> injected SMC instruction, the guest traps into the undefined
>>>> instruction
>>>> exception handler in EL1 and I simply don't understand why. As far
>>>> as I
>>>> understand, as soon an injected SMC instruction gets executed, it
>>>> should
>>>> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is
>>>> set).
>>>> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
>>>> that now contains the original instruction instead of the injected
>>>> SMC),
>>>> the guest should simply continue its execution.
>>> Hm. What do you mean under "SMC instruction injection?".
>>
>> My code runs in dom0 and "injects" an SMC instruction to predefined
>> addresses inside the guest as to simulate software breakpoints. By this,
>> I mean that the code replaces the original guest instruction at a
>> certain address with an SMC. Think of a debugger that uses software
>> breakpoints. The idea is to put back the original instruction right
>> after the SMC gets called, so that the guest can continue with its
>> execution. You can find more information about that in [0], yet please
>> consider that I try to trap the SMC directly in Xen instead of
>> TrustZone.
> Yep, I see. Immediate question: do you flush icache after you put
> original instruction back?
Yeap. But the current behavior does not let me to go this far, as I the
system jumps into the interrupt handler and single-steps the handler
instead of the instruction of interest.
> Then I can't see, why this should not work. If only VM monitor core in
> XEN is not broken. I don't know any users of this.
> I'm just curious, why are you using SMC, not BRK instruction?
>
I use SMC instructions as the guest can register for BRK events. The
guest cannot register for SMC events. So, in order stay stealthy towards
the guest and also not to cope with BRK re-injections, SMC's seemed to
be the right choice :)
>>> Current code in hypervisor will always inject undefined instruction
>>> exception when you call SMC (unless you installed VM monitor for the
>>> guest). Also, it will not increase PC. So, if you'll try to remove
>>> inject_undef_exception() call, you'll get into an infinite loop.
>>>
>>
>> I have a registered SMC monitor running in dom0 that does not reinject
>> the undefined instruction exception in do_trap_smc(). So there is no
>> indefinite loop at this point. What I see is that as soon as my code in
>> xen-access (dom0) increments the trapped guest PC by 4 (and also if it
>> doesn't) the next instruction inside the guest will be inside the undef
>> instruction handler (I can see that because I have implemented a single
>> stepping mechanism for AArch64 in Xen that gets activated right after
>> the guest executes the injected SMC instruction).
> That's strange. Can you print whole vCPU state to determine that PC
> points to the right place? Also you can check DFAR. Probably you can
> even dump memory pointed by DFAR to make sure that you written back
> correct instruction.
Yea, I do that. And both the SMC injection, as well as further vCPU
state seems to be correct at this point.
Today, I saw an interesting behavior in my single-stepping
implementation, which is the reason for my late reply. I can't explain
what is going wrong, yet. So I will need to further investigate this
behavior and post and RFC for the single-stepping mechanism as to put
more eyes on the issue. Maybe, this will help solve it.
But anyway, thank you very much for your help! I really appreciate it :)
>
>>>> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
>>>> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC
>>>> instructions
>>>> to EL2, that trap has priority over this disable". So this means
>>>> that if
>>>> SMCs are disabled for NS EL1, the guest will trap into the
>>>> hypervisor on
>>>> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest
>>>> will
>>>> execute an undefined instrcution exception. Which is what I was
>>>> thinking
>>>> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
>>>> the other hand I believe that it is highly unlikely that the EFI
>>>> loader
>>>> explicitly disables SMC's for NS EL1. However, since I don't have
>>>> access
>>>> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason
>>>> for the
>>>> behavior I am experiencing on my board or not.
>>> According to ARM ARM, hypervisor should trap SMC even if was disabled
>>> by EL3. I think, that in your case the problem is in current
>>> implementation of do_trap_smc()
>>>
>>
>> Unfortunately, I don't think that this is the problem of do_trap_smc()
>> (see above). But let me check one more time.
> As I said, I know no users for SMC monitor, and I'm not exactly sure
> that it works properly.
>
>>
>>>> It would be of great help if you would provide me with some more
>>>> clarity
>>>> on my case. I am sure that I have missed something that simply needs
>>>> clarification. Thank you very much in advance.
>>> I don't quite understood, what you are trying to achieve. But I think
>>> that pair of printk()s in do_trap_smc() will reveal much.
>>>
>>
>> Yea, the idea is to inject SMC instructions into the guest to simulate
>> software breakpoints for guest analysis purposes. Please let me cleanup
>> my current printk output to better present my issue.
>>
>>>
>>> [1]
>>> http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
>>>
>>
>> Thank you,
>> ~Sergej
>>
>> [0] http://www.cse.psu.edu/~trj1/papers/most14.pdf
>>
Thanks,
~Sergej
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-08-31 20:16 ` Sergej Proskurin
@ 2017-09-04 6:07 ` Julien Grall
2017-09-04 9:57 ` Sergej Proskurin
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2017-09-04 6:07 UTC (permalink / raw)
To: Sergej Proskurin, Volodymyr Babchuk, Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 9480 bytes --]
Hello,
Sorry for the formatting, writing from my phone. Ki
On Thu, 31 Aug 2017, 22:18 Sergej Proskurin <proskurin@sec.in.tum.de> wrote:
> Hi Volodymyr,
>
>
> On 08/31/2017 04:58 PM, Volodymyr Babchuk wrote:
> > Hi Sergej
> >
> > On 31.08.17 16:51, Sergej Proskurin wrote:
> >> Hi Volodymyr,
> >>
> >>
> >> On 08/31/2017 02:44 PM, Volodymyr Babchuk wrote:
> >>> Hello Sergej,
> >>>
> >>> On 31.08.17 15:20, Sergej Proskurin wrote:
> >>>> Hi Volodymyr, hi Julien,
> >>>>
> >>>>
> >>>> On 08/24/2017 07:25 PM, Julien Grall wrote:
> >>>>>
> >>>>>
> >>>>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
> >>>>>> This feature indicates that hypervisor is compatible with ARM
> >>>>>> SMC calling convention. Hypervisor will not inject an undefined
> >>>>>> instruction exception if an invalid SMC function were called and
> >>>>>> will not crash a domain if an invlalid HVC functions were called.
> >>>>>
> >>>>> s/invlalid/invalid/
> >>>>>
> >>>>> The last sentence is misleading. Xen will still inject and undefined
> >>>>> instruction for some SMC/HVC. You may want to rework it to make it
> >>>>> clear.
> >>>>>
> >>>>
> >>>> Now that you say that Xen will still inject an undefined instruction
> >>>> exception for some SMCs, I have a to ask for which exactly?
> >>> For ones that are compatible with ARM SMCCC [1]. E.g if you are
> >>> running SMCCC-compatible system and you are calling SMC/HVC with
> >>> immediate value 0, then you are safe.
> >>>
> >>
> >> Alright, as far as I understand this is exactly what I do right now. I
> >> inject an SMC that is encoded as 0xD4000003.
> > Actually, this patch series are not merged yet, so no SMCCC support
> > right. But this should not a problem in your case.
> >
> >>>> I might be off topic here, so please tell me if you believe this is
> >>>> not
> >>>> the right place for this question. In this case I will open an new
> >>>> thread. Right now, I am working with the previous implementation of
> >>>> do_trap_smc that was extended in this patch. Yet, as far as I
> >>>> understand, the behavior should not change, which is why I am asking
> >>>> this quesiton in this thread.
> >>> If you are talking about forwarding SMC exception to VM monitor, then
> >>> yes, that should not change.
> >>
> >> Yes, exactly. Sorry, I forgot to mention that I have a modified
> >> xen-access version running in dom0 that registers an SMC monitor and
> >> also increases the PC by 4 (or dependent on the case, simply leaves it
> >> as it is) on every SMC trap.
> > Aha, I see. I never was able to test this feature fully. I played with
> > my own VM monitor, when I tried to offload SMC handling to another
> > domain. But I had to comment out most of the VM monitor code in XEN.
> >
> >>>
> >>>> Currently, I am working on SMC guest injections and trying to
> >>>> understand
> >>>> the resulting behavior. Every time, right after the execution of an
> >>>> injected SMC instruction, the guest traps into the undefined
> >>>> instruction
> >>>> exception handler in EL1 and I simply don't understand why. As far
> >>>> as I
> >>>> understand, as soon an injected SMC instruction gets executed, it
> >>>> should
> >>>> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is
> >>>> set).
> >>>> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
> >>>> that now contains the original instruction instead of the injected
> >>>> SMC),
> >>>> the guest should simply continue its execution.
> >>> Hm. What do you mean under "SMC instruction injection?".
> >>
> >> My code runs in dom0 and "injects" an SMC instruction to predefined
> >> addresses inside the guest as to simulate software breakpoints. By this,
> >> I mean that the code replaces the original guest instruction at a
> >> certain address with an SMC. Think of a debugger that uses software
> >> breakpoints. The idea is to put back the original instruction right
> >> after the SMC gets called, so that the guest can continue with its
> >> execution. You can find more information about that in [0], yet please
> >> consider that I try to trap the SMC directly in Xen instead of
> >> TrustZone.
> > Yep, I see. Immediate question: do you flush icache after you put
> > original instruction back?
>
> Yeap. But the current behavior does not let me to go this far, as I the
> system jumps into the interrupt handler and single-steps the handler
> instead of the instruction of interest.
On your first mail, you started with "smc injection doesn't work", then "I
replace instruction" and now you mention about single-stepping.
This doesn't help at all to understand what you are doing and really not
related to this thread.
So can you please details exactly what you are doing rather than giving
bits by bits?
> > Then I can't see, why this should not work. If only VM monitor core in
> > XEN is not broken. I don't know any users of this.
> > I'm just curious, why are you using SMC, not BRK instruction?
> >
>
> I use SMC instructions as the guest can register for BRK events. The
> guest cannot register for SMC events. So, in order stay stealthy towards
> the guest and also not to cope with BRK re-injections, SMC's seemed to
> be the right choice :
I have already said that using SMC is a pretty bad idea when Tamas added
the trapping and you guys still seem to think it is a good idea...
> >>> Current code in hypervisor will always inject undefined instruction
> >>> exception when you call SMC (unless you installed VM monitor for the
> >>> guest). Also, it will not increase PC. So, if you'll try to remove
> >>> inject_undef_exception() call, you'll get into an infinite loop.
> >>>
> >>
> >> I have a registered SMC monitor running in dom0 that does not reinject
> >> the undefined instruction exception in do_trap_smc(). So there is no
> >> indefinite loop at this point. What I see is that as soon as my code in
> >> xen-access (dom0) increments the trapped guest PC by 4 (and also if it
> >> doesn't) the next instruction inside the guest will be inside the undef
> >> instruction handler (I can see that because I have implemented a single
> >> stepping mechanism for AArch64 in Xen that gets activated right after
> >> the guest executes the injected SMC instruction).
> > That's strange. Can you print whole vCPU state to determine that PC
> > points to the right place? Also you can check DFAR. Probably you can
> > even dump memory pointed by DFAR to make sure that you written back
> > correct instruction.
>
> Yea, I do that. And both the SMC injection, as well as further vCPU
> state seems to be correct at this point.
>
> Today, I saw an interesting behavior in my single-stepping
> implementation, which is the reason for my late reply. I can't explain
> what is going wrong, yet. So I will need to further investigate this
> behavior and post and RFC for the single-stepping mechanism as to put
> more eyes on the issue. Maybe, this will help solve it.
>
> But anyway, thank you very much for your help! I really appreciate it :)
>
You probably want to look at
https://lists.xen.org/archives/html/xen-devel/2017-08/msg00661.html and
maybe sync-up with this person if you are not working with him.
Cheers,
> >
> >>>> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
> >>>> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC
> >>>> instructions
> >>>> to EL2, that trap has priority over this disable". So this means
> >>>> that if
> >>>> SMCs are disabled for NS EL1, the guest will trap into the
> >>>> hypervisor on
> >>>> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest
> >>>> will
> >>>> execute an undefined instrcution exception. Which is what I was
> >>>> thinking
> >>>> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
> >>>> the other hand I believe that it is highly unlikely that the EFI
> >>>> loader
> >>>> explicitly disables SMC's for NS EL1. However, since I don't have
> >>>> access
> >>>> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason
> >>>> for the
> >>>> behavior I am experiencing on my board or not.
> >>> According to ARM ARM, hypervisor should trap SMC even if was disabled
> >>> by EL3. I think, that in your case the problem is in current
> >>> implementation of do_trap_smc()
> >>>
> >>
> >> Unfortunately, I don't think that this is the problem of do_trap_smc()
> >> (see above). But let me check one more time.
> > As I said, I know no users for SMC monitor, and I'm not exactly sure
> > that it works properly.
> >
> >>
> >>>> It would be of great help if you would provide me with some more
> >>>> clarity
> >>>> on my case. I am sure that I have missed something that simply needs
> >>>> clarification. Thank you very much in advance.
> >>> I don't quite understood, what you are trying to achieve. But I think
> >>> that pair of printk()s in do_trap_smc() will reveal much.
> >>>
> >>
> >> Yea, the idea is to inject SMC instructions into the guest to simulate
> >> software breakpoints for guest analysis purposes. Please let me cleanup
> >> my current printk output to better present my issue.
> >>
> >>>
> >>> [1]
> >>>
> http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
> >>>
> >>
> >> Thank you,
> >> ~Sergej
> >>
> >> [0] http://www.cse.psu.edu/~trj1/papers/most14.pdf
> >>
>
> Thanks,
> ~Sergej
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>
[-- Attachment #1.2: Type: text/html, Size: 13138 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-09-04 6:07 ` Julien Grall
@ 2017-09-04 9:57 ` Sergej Proskurin
2017-09-11 11:33 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Sergej Proskurin @ 2017-09-04 9:57 UTC (permalink / raw)
To: Julien Grall, Volodymyr Babchuk, Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Julien,
On 09/04/2017 08:07 AM, Julien Grall wrote:
> Hello,
>
> Sorry for the formatting, writing from my phone. Ki
>
> On Thu, 31 Aug 2017, 22:18 Sergej Proskurin <proskurin@sec.in.tum.de> wrote:
>
[...]
>
> On your first mail, you started with "smc injection doesn't work", then "I
> replace instruction" and now you mention about single-stepping.
>
> This doesn't help at all to understand what you are doing and really not
> related to this thread.
>
> So can you please details exactly what you are doing rather than giving
> bits by bits?
>
I will provide more information in a separate thread soon so that the
actual issue, hopefully, will become clearer. Thank you.
>> I use SMC instructions as the guest can register for BRK events. The
>> guest cannot register for SMC events. So, in order stay stealthy towards
>> the guest and also not to cope with BRK re-injections, SMC's seemed to
>> be the right choice :
>
> I have already said that using SMC is a pretty bad idea when Tamas added
> the trapping and you guys still seem to think it is a good idea...
I did not know about this conversation with Tamas. Why do you believe
that using SMC instructions is not a good idea? Could you please refer
me to the particular thread? Thank you.
>>>>> Current code in hypervisor will always inject undefined instruction
>>>>> exception when you call SMC (unless you installed VM monitor for the
>>>>> guest). Also, it will not increase PC. So, if you'll try to remove
>>>>> inject_undef_exception() call, you'll get into an infinite loop.
>>>>>
>>>> I have a registered SMC monitor running in dom0 that does not reinject
>>>> the undefined instruction exception in do_trap_smc(). So there is no
>>>> indefinite loop at this point. What I see is that as soon as my code in
>>>> xen-access (dom0) increments the trapped guest PC by 4 (and also if it
>>>> doesn't) the next instruction inside the guest will be inside the undef
>>>> instruction handler (I can see that because I have implemented a single
>>>> stepping mechanism for AArch64 in Xen that gets activated right after
>>>> the guest executes the injected SMC instruction).
>>> That's strange. Can you print whole vCPU state to determine that PC
>>> points to the right place? Also you can check DFAR. Probably you can
>>> even dump memory pointed by DFAR to make sure that you written back
>>> correct instruction.
>> Yea, I do that. And both the SMC injection, as well as further vCPU
>> state seems to be correct at this point.
>>
>> Today, I saw an interesting behavior in my single-stepping
>> implementation, which is the reason for my late reply. I can't explain
>> what is going wrong, yet. So I will need to further investigate this
>> behavior and post and RFC for the single-stepping mechanism as to put
>> more eyes on the issue. Maybe, this will help solve it.
>>
>> But anyway, thank you very much for your help! I really appreciate it :)
>>
> You probably want to look at
> https://lists.xen.org/archives/html/xen-devel/2017-08/msg00661.html and
> maybe sync-up with this person if you are not working with him.
Thanks, for mentioning that. Florian is a student of mine who has also
looked at single-stepping on ARMv8. We have collaborated on this topic
together. I will take over on that, as his work goes slightly into a
different direction.
Thanks,
~Sergej
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature
2017-09-04 9:57 ` Sergej Proskurin
@ 2017-09-11 11:33 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-09-11 11:33 UTC (permalink / raw)
To: Sergej Proskurin, Julien Grall, Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
On 04/09/17 10:57, Sergej Proskurin wrote:
> Hi Julien,
>
>
> On 09/04/2017 08:07 AM, Julien Grall wrote:
>> Hello,
>>
>> Sorry for the formatting, writing from my phone. Ki
>>
>> On Thu, 31 Aug 2017, 22:18 Sergej Proskurin <proskurin@sec.in.tum.de> wrote:
>>
>
> [...]
>
>>
>> On your first mail, you started with "smc injection doesn't work", then "I
>> replace instruction" and now you mention about single-stepping.
>>
>> This doesn't help at all to understand what you are doing and really not
>> related to this thread.
>>
>> So can you please details exactly what you are doing rather than giving
>> bits by bits?
>>
>
> I will provide more information in a separate thread soon so that the
> actual issue, hopefully, will become clearer. Thank you.
>
>>> I use SMC instructions as the guest can register for BRK events. The
>>> guest cannot register for SMC events. So, in order stay stealthy towards
>>> the guest and also not to cope with BRK re-injections, SMC's seemed to
>>> be the right choice :
>>
>> I have already said that using SMC is a pretty bad idea when Tamas added
>> the trapping and you guys still seem to think it is a good idea...
>
> I did not know about this conversation with Tamas. Why do you believe
> that using SMC instructions is not a good idea? Could you please refer
> me to the particular thread? Thank you.
I am not sure on which thread it was discussed.
https://lists.gt.net/xen/devel/427459 may contain some details.
By definition SMC is for Secure Monitor Call. It might be possible to
have a guest access the secure firmware (e.g imagine an Android guest
doing video decoding). Given that you don't identify the immediate of
the SMC (which is BTW not easily available on ARMv7), you cannot have
both interacting together. And even with that you don't know if the SMC
#imm might be used by the firmware...
Furthermore, SMC cannot be executed at EL0 so you can't monitor
application. They also won't be available if the platform that does not
have EL3.
So yes SMC is a pretty bad choice here if you want to get a generic
solution.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v4 11/11] arm: enable XENFEAT_ARM_SMCCC_supported feature
2017-08-21 20:27 [PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (9 preceding siblings ...)
2017-08-21 20:27 ` [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature Volodymyr Babchuk
@ 2017-08-21 20:27 ` Volodymyr Babchuk
2017-08-22 7:29 ` Jan Beulich
10 siblings, 1 reply; 48+ messages in thread
From: Volodymyr Babchuk @ 2017-08-21 20:27 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, Volodymyr Babchuk
As Xen now supports SMCCC, we can enable this feature to tell
guests that it is safe to call generic SMCs and HVCs.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
xen/common/kernel.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index ce7cb8a..18c4d51 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -332,6 +332,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
(1U << XENFEAT_auto_translated_physmap);
if ( is_hardware_domain(d) )
fi.submap |= 1U << XENFEAT_dom0;
+#ifdef CONFIG_ARM
+ fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported);
+#endif
#ifdef CONFIG_X86
switch ( d->guest_type )
{
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v4 11/11] arm: enable XENFEAT_ARM_SMCCC_supported feature
2017-08-21 20:27 ` [PATCH v4 11/11] arm: enable " Volodymyr Babchuk
@ 2017-08-22 7:29 ` Jan Beulich
2017-08-24 17:23 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Jan Beulich @ 2017-08-22 7:29 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel,
Julien Grall
>>> On 21.08.17 at 22:27, <volodymyr_babchuk@epam.com> wrote:
> As Xen now supports SMCCC, we can enable this feature to tell
> guests that it is safe to call generic SMCs and HVCs.
I think this and patch 10 should be folded.
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> xen/common/kernel.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index ce7cb8a..18c4d51 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -332,6 +332,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> (1U << XENFEAT_auto_translated_physmap);
> if ( is_hardware_domain(d) )
> fi.submap |= 1U << XENFEAT_dom0;
> +#ifdef CONFIG_ARM
> + fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported);
> +#endif
Pointless parentheses (as other code in context shows you).
With both aspects taken care of
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v4 11/11] arm: enable XENFEAT_ARM_SMCCC_supported feature
2017-08-22 7:29 ` Jan Beulich
@ 2017-08-24 17:23 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2017-08-24 17:23 UTC (permalink / raw)
To: Jan Beulich, Volodymyr Babchuk
Cc: Edgar E . Iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
George Dunlap, Andrew Cooper, Ian Jackson, xen-devel
On 22/08/17 08:29, Jan Beulich wrote:
>>>> On 21.08.17 at 22:27, <volodymyr_babchuk@epam.com> wrote:
>> As Xen now supports SMCCC, we can enable this feature to tell
>> guests that it is safe to call generic SMCs and HVCs.
>
> I think this and patch 10 should be folded.
+1.
>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>> xen/common/kernel.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
>> index ce7cb8a..18c4d51 100644
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -332,6 +332,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> (1U << XENFEAT_auto_translated_physmap);
>> if ( is_hardware_domain(d) )
>> fi.submap |= 1U << XENFEAT_dom0;
>> +#ifdef CONFIG_ARM
>> + fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported);
>> +#endif
>
> Pointless parentheses (as other code in context shows you).
>
> With both aspects taken care of
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 48+ messages in thread