> On 17 Sep 2020, at 18:21, Ralf Ramsauer <ralf.ramsa...@oth-regensburg.de>
> wrote:
>
>
> On 9/17/20 8:26 AM, Jan Kiszka wrote:
>> Subject tag should be "arm64". And the patch should go over next first.
>> I can fix up both.
>>
>> On 16.09.20 15:07, Oliver Schwartz wrote:
>>> SMC/HVC calls may modify registers x0 to x3. To make sure the compiler
>>> doesn't assume input registers to be constant, also mark these registers
>>> as output when used as input.
>>>
>>> Signed-off-by: Oliver Schwartz <oliver.schwa...@gmx.de>
>>> ---
>>> hypervisor/arch/arm64/include/asm/smc.h | 13 ++++++-------
>>> include/arch/arm64/asm/jailhouse_hypercall.h | 20 +++++++++++---------
>>> 2 files changed, 17 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hypervisor/arch/arm64/include/asm/smc.h
>>> b/hypervisor/arch/arm64/include/asm/smc.h
>>> index 1a5d5c8..c80fe15 100644
>>> --- a/hypervisor/arch/arm64/include/asm/smc.h
>>> +++ b/hypervisor/arch/arm64/include/asm/smc.h
>>> @@ -28,8 +28,8 @@ static inline long smc_arg1(unsigned long id,
>>> unsigned long par1)
>>> register unsigned long __par1 asm("r1") = par1;
>>> asm volatile ("smc #0\n\t"
>>> - : "=r" (__id)
>>> - : "r"(__id), "r"(__par1)
>>> + : "+r" (__id), "+r"(__par1)
>>> + :
>>> : "memory", "x2", "x3");
>>>
>>
>> For SMCCC, I'm considering to align fully with Linux, i.e. convert the
>> remaining register clobberings into early ones, but I also have no
>> strong argument for it.
>>
>> Ralf, thoughts?
>
> Just had a look at Linux's implementation. And now I recall why I didn't
> c&p it from Linux: it's an unreadable macro hell [1] that would benefit
> from being open-coded. But trying to follow Linux's __constraint macro,
> shouldn't we protect r2 and r3 as well?
>
> Linux would unroll __constraint_read_1 and __constraint_write_1, and
> __constraint_read_1 would protect r2 and r3 as well. We protect x2 and
> x3 via the clobber list -- is that enough?
I’m by no means an expert on arm assembler. However, I’m quite positive that
there are no physical registers r2 and r3 that can be protected in arm64. These
are just placeholders that are translated by the compiler to x2 and x3. So
protecting r2/r3 as dummy arguments or x2/x3 via clobber list is in effect the
same, just more explicit.
Oliver
> Anyway, I think we can trust Linux's implementation, but Linux's clobber
> list only consists of "memory" and protects registers via operand lists.
> If anything would have blown up there, someone would have probably noticed.
>
> Ralf
>
> [1]
> https://elixir.bootlin.com/linux/latest/source/include/linux/arm-smccc.h#L293
> <https://elixir.bootlin.com/linux/latest/source/include/linux/arm-smccc.h#L293>
>
>>
>> Jan
>>
>>> return __id;
>>> @@ -43,8 +43,8 @@ static inline long smc_arg2(unsigned long id,
>>> unsigned long par1,
>>> register unsigned long __par2 asm("r2") = par2;
>>> asm volatile ("smc #0\n\t"
>>> - : "=r" (__id)
>>> - : "r"(__id), "r"(__par1), "r"(__par2)
>>> + : "+r" (__id), "+r"(__par1), "+r"(__par2)
>>> + :
>>> : "memory", "x3");
>>> return __id;
>>> @@ -62,9 +62,8 @@ static inline long smc_arg5(unsigned long id,
>>> unsigned long par1,
>>> register unsigned long __par5 asm("r5") = par5;
>>> asm volatile ("smc #0\n\t"
>>> - : "=r" (__id)
>>> - : "r"(__id), "r"(__par1), "r"(__par2), "r"(__par3),
>>> - "r"(__par4), "r"(__par5)
>>> + : "+r" (__id), "+r"(__par1), "+r"(__par2), "+r"(__par3)
>>> + : "r"(__par4), "r"(__par5)
>>> : "memory");
>>> return __id;
>>> diff --git a/include/arch/arm64/asm/jailhouse_hypercall.h
>>> b/include/arch/arm64/asm/jailhouse_hypercall.h
>>> index 108d052..a9d13ee 100644
>>> --- a/include/arch/arm64/asm/jailhouse_hypercall.h
>>> +++ b/include/arch/arm64/asm/jailhouse_hypercall.h
>>> @@ -42,6 +42,7 @@
>>> #define JAILHOUSE_CALL_NUM_RESULT "x0"
>>> #define JAILHOUSE_CALL_ARG1 "x1"
>>> #define JAILHOUSE_CALL_ARG2 "x2"
>>> +#define JAILHOUSE_CALL_CLOBBERED "x3"
>>> /* CPU statistics, arm64-specific part */
>>> #define JAILHOUSE_NUM_CPU_STATS
>>> JAILHOUSE_GENERIC_CPU_STATS + 5
>>> @@ -54,9 +55,10 @@ static inline __u64 jailhouse_call(__u64 num)
>>> asm volatile(
>>> JAILHOUSE_CALL_INS
>>> - : "=r" (num_result)
>>> - : "r" (num_result)
>>> - : "memory");
>>> + : "+r" (num_result)
>>> + :
>>> + : "memory", JAILHOUSE_CALL_ARG1, JAILHOUSE_CALL_ARG2,
>>> + JAILHOUSE_CALL_CLOBBERED);
>>> return num_result;
>>> }
>>> @@ -67,9 +69,9 @@ static inline __u64 jailhouse_call_arg1(__u64 num,
>>> __u64 arg1)
>>> asm volatile(
>>> JAILHOUSE_CALL_INS
>>> - : "=r" (num_result)
>>> - : "r" (num_result), "r" (__arg1)
>>> - : "memory");
>>> + : "+r" (num_result), "+r" (__arg1)
>>> + :
>>> + : "memory", JAILHOUSE_CALL_ARG2, JAILHOUSE_CALL_CLOBBERED);
>>> return num_result;
>>> }
>>> @@ -81,9 +83,9 @@ static inline __u64 jailhouse_call_arg2(__u64 num,
>>> __u64 arg1, __u64 arg2)
>>> asm volatile(
>>> JAILHOUSE_CALL_INS
>>> - : "=r" (num_result)
>>> - : "r" (num_result), "r" (__arg1), "r" (__arg2)
>>> - : "memory");
>>> + : "+r" (num_result), "+r" (__arg1), "+r" (__arg2)
>>> + :
>>> + : "memory", JAILHOUSE_CALL_CLOBBERED);
>>> return num_result;
>>> }
>>>
>>
>
> --
> You received this message because you are subscribed to the Google Groups
> "Jailhouse" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to jailhouse-dev+unsubscr...@googlegroups.com
> <mailto:jailhouse-dev+unsubscr...@googlegroups.com>.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/jailhouse-dev/7ecaf35b-6669-143d-60fb-f7b63fb27d28%40oth-regensburg.de
>
> <https://groups.google.com/d/msgid/jailhouse-dev/7ecaf35b-6669-143d-60fb-f7b63fb27d28%40oth-regensburg.de>.
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/EADE8BFE-2CA2-4942-80CA-CB40FDEEB71C%40gmx.de.
