[PATCH v3] xen/arm64: Zero the top 32 bits of gp registers on entry...

[PATCH v3] xen/arm64: Zero the top 32 bits of gp registers on entry...

[Michal Orzel]

to hypervisor when switching from AArch32 state.

According to section D1.20.2 of Arm Arm(DDI 0487A.j):
"If the general-purpose register was accessible from AArch32 state the
upper 32 bits either become zero, or hold the value that the same
architectural register held before any AArch32 execution.
The choice between these two options is IMPLEMENTATION DEFINED"

Currently Xen does not ensure that the top 32 bits are zeroed and this
needs to be fixed. The reason why is that there are places in Xen
where we assume that top 32bits are zero for AArch32 guests.
If they are not, this can lead to misinterpretation of Xen regarding
what the guest requested. For example hypercalls returning an error
encoded in a signed long like do_sched_op, do_hmv_op, do_memory_op
would return -ENOSYS if the command passed as the first argument was
clobbered.

Create a macro clobber_gp_top_halves to clobber top 32 bits of gp
registers when hyp == 0 (guest mode) and compat == 1 (AArch32 mode).
Add a compile time check to ensure that save_x0_x1 == 1 if
compat == 1.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v2:
-add clobbering of w30
Changes since v1:
-put new code into macro
-add compile time check for save_x0_x1
---
 xen/arch/arm/arm64/entry.S | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index fc3811ad0a..e351ef8639 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -102,6 +102,30 @@
         .endif
 
         .endm
+
+/*
+ * Clobber top 32 bits of gp registers when switching from AArch32
+ */
+        .macro clobber_gp_top_halves, compat, save_x0_x1
+
+        .if \compat == 1      /* AArch32 mode */
+
+        /*
+         * save_x0_x1 is equal to 0 only for guest_sync (compat == 0).
+         * Add a compile time check to avoid violating this rule.
+         */
+        .if \save_x0_x1 == 0
+        .error "save_x0_x1 is 0 but compat is 1"
+        .endif
+
+        .irp n,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30
+        mov w\n, w\n
+        .endr
+
+        .endif
+
+        .endm
+
 /*
  * Save state on entry to hypervisor, restore on exit
  *
@@ -111,6 +135,11 @@
  */
         .macro  entry, hyp, compat, save_x0_x1=1
         sub     sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR */
+
+        .if \hyp == 0         /* Guest mode */
+        clobber_gp_top_halves compat=\compat, save_x0_x1=\save_x0_x1
+        .endif
+
         push    x28, x29
         push    x26, x27
         push    x24, x25
-- 
2.29.0

Re: [PATCH v3] xen/arm64: Zero the top 32 bits of gp registers on entry...

[Julien Grall]

Hi,

On 17/12/2021 07:21, Michal Orzel wrote:
> to hypervisor when switching from AArch32 state.
> 
> According to section D1.20.2 of Arm Arm(DDI 0487A.j):
> "If the general-purpose register was accessible from AArch32 state the
> upper 32 bits either become zero, or hold the value that the same
> architectural register held before any AArch32 execution.
> The choice between these two options is IMPLEMENTATION DEFINED"
> 
> Currently Xen does not ensure that the top 32 bits are zeroed and this
> needs to be fixed. The reason why is that there are places in Xen
> where we assume that top 32bits are zero for AArch32 guests.
> If they are not, this can lead to misinterpretation of Xen regarding
> what the guest requested. For example hypercalls returning an error
> encoded in a signed long like do_sched_op, do_hmv_op, do_memory_op
> would return -ENOSYS if the command passed as the first argument was
> clobbered.
> 
> Create a macro clobber_gp_top_halves to clobber top 32 bits of gp
> registers when hyp == 0 (guest mode) and compat == 1 (AArch32 mode).
> Add a compile time check to ensure that save_x0_x1 == 1 if
> compat == 1.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> Changes since v2:
> -add clobbering of w30
> Changes since v1:
> -put new code into macro
> -add compile time check for save_x0_x1
> ---
>   xen/arch/arm/arm64/entry.S | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index fc3811ad0a..e351ef8639 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -102,6 +102,30 @@
>           .endif
>   
>           .endm
> +
> +/*
> + * Clobber top 32 bits of gp registers when switching from AArch32
> + */
> +        .macro clobber_gp_top_halves, compat, save_x0_x1
> +
> +        .if \compat == 1      /* AArch32 mode */
> +
> +        /*
> +         * save_x0_x1 is equal to 0 only for guest_sync (compat == 0).
> +         * Add a compile time check to avoid violating this rule.
> +         */

We may want in the future to allow save_x0_x1 == 1 with compat == 1 if 
we need to create fastpath like we did when entering AArch64.

So I would reword the comment to make clear this is an implementation 
decision. How about:

"At the moment, no-one is using save_x0_x1 == 0 with compat == 1. So the 
code is not handling it to simplify the implementation."

If you are happy with the new comment, I can update it on commit:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3] xen/arm64: Zero the top 32 bits of gp registers on entry...

[Michal Orzel]

Hi Julien,

On 17.12.2021 11:01, Julien Grall wrote:
> Hi,
> 
> On 17/12/2021 07:21, Michal Orzel wrote:
>> to hypervisor when switching from AArch32 state.
>>
>> According to section D1.20.2 of Arm Arm(DDI 0487A.j):
>> "If the general-purpose register was accessible from AArch32 state the
>> upper 32 bits either become zero, or hold the value that the same
>> architectural register held before any AArch32 execution.
>> The choice between these two options is IMPLEMENTATION DEFINED"
>>
>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>> needs to be fixed. The reason why is that there are places in Xen
>> where we assume that top 32bits are zero for AArch32 guests.
>> If they are not, this can lead to misinterpretation of Xen regarding
>> what the guest requested. For example hypercalls returning an error
>> encoded in a signed long like do_sched_op, do_hmv_op, do_memory_op
>> would return -ENOSYS if the command passed as the first argument was
>> clobbered.
>>
>> Create a macro clobber_gp_top_halves to clobber top 32 bits of gp
>> registers when hyp == 0 (guest mode) and compat == 1 (AArch32 mode).
>> Add a compile time check to ensure that save_x0_x1 == 1 if
>> compat == 1.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> Changes since v2:
>> -add clobbering of w30
>> Changes since v1:
>> -put new code into macro
>> -add compile time check for save_x0_x1
>> ---
>>   xen/arch/arm/arm64/entry.S | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>> index fc3811ad0a..e351ef8639 100644
>> --- a/xen/arch/arm/arm64/entry.S
>> +++ b/xen/arch/arm/arm64/entry.S
>> @@ -102,6 +102,30 @@
>>           .endif
>>             .endm
>> +
>> +/*
>> + * Clobber top 32 bits of gp registers when switching from AArch32
>> + */
>> +        .macro clobber_gp_top_halves, compat, save_x0_x1
>> +
>> +        .if \compat == 1      /* AArch32 mode */
>> +
>> +        /*
>> +         * save_x0_x1 is equal to 0 only for guest_sync (compat == 0).
>> +         * Add a compile time check to avoid violating this rule.
>> +         */
> 
> We may want in the future to allow save_x0_x1 == 1 with compat == 1 if we need to create fastpath like we did when entering AArch64.
> 
> So I would reword the comment to make clear this is an implementation decision. How about:
> 
> "At the moment, no-one is using save_x0_x1 == 0 with compat == 1. So the code is not handling it to simplify the implementation."
> 
> If you are happy with the new comment, I can update it on commit:
> 
Please do. The comment looks ok.

> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,
> 

Cheers,
Michal

Re: [PATCH v3] xen/arm64: Zero the top 32 bits of gp registers on entry...

[Julien Grall]

Hi Michal,

On 17/12/2021 11:52, Michal Orzel wrote:
> On 17.12.2021 11:01, Julien Grall wrote:
>> On 17/12/2021 07:21, Michal Orzel wrote:
>>> to hypervisor when switching from AArch32 state.
>>>
>>> According to section D1.20.2 of Arm Arm(DDI 0487A.j):
>>> "If the general-purpose register was accessible from AArch32 state the
>>> upper 32 bits either become zero, or hold the value that the same
>>> architectural register held before any AArch32 execution.
>>> The choice between these two options is IMPLEMENTATION DEFINED"
>>>
>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>> needs to be fixed. The reason why is that there are places in Xen
>>> where we assume that top 32bits are zero for AArch32 guests.
>>> If they are not, this can lead to misinterpretation of Xen regarding
>>> what the guest requested. For example hypercalls returning an error
>>> encoded in a signed long like do_sched_op, do_hmv_op, do_memory_op
>>> would return -ENOSYS if the command passed as the first argument was
>>> clobbered.
>>>
>>> Create a macro clobber_gp_top_halves to clobber top 32 bits of gp
>>> registers when hyp == 0 (guest mode) and compat == 1 (AArch32 mode).
>>> Add a compile time check to ensure that save_x0_x1 == 1 if
>>> compat == 1.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>> ---
>>> Changes since v2:
>>> -add clobbering of w30
>>> Changes since v1:
>>> -put new code into macro
>>> -add compile time check for save_x0_x1
>>> ---
>>>    xen/arch/arm/arm64/entry.S | 29 +++++++++++++++++++++++++++++
>>>    1 file changed, 29 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>> index fc3811ad0a..e351ef8639 100644
>>> --- a/xen/arch/arm/arm64/entry.S
>>> +++ b/xen/arch/arm/arm64/entry.S
>>> @@ -102,6 +102,30 @@
>>>            .endif
>>>              .endm
>>> +
>>> +/*
>>> + * Clobber top 32 bits of gp registers when switching from AArch32
>>> + */
>>> +        .macro clobber_gp_top_halves, compat, save_x0_x1
>>> +
>>> +        .if \compat == 1      /* AArch32 mode */
>>> +
>>> +        /*
>>> +         * save_x0_x1 is equal to 0 only for guest_sync (compat == 0).
>>> +         * Add a compile time check to avoid violating this rule.
>>> +         */
>>
>> We may want in the future to allow save_x0_x1 == 1 with compat == 1 if we need to create fastpath like we did when entering AArch64.
>>
>> So I would reword the comment to make clear this is an implementation decision. How about:
>>
>> "At the moment, no-one is using save_x0_x1 == 0 with compat == 1. So the code is not handling it to simplify the implementation."
>>
>> If you are happy with the new comment, I can update it on commit:
>>
> Please do. The comment looks ok.

Done. It is now committed.

Thanks for the fix!

Cheers,

-- 
Julien Grall