* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-07 8:37 ` Michal Orzel
@ 2021-12-07 9:05 ` Bertrand Marquis
2021-12-07 9:55 ` Jan Beulich
2021-12-07 19:11 ` Julien Grall
2 siblings, 0 replies; 26+ messages in thread
From: Bertrand Marquis @ 2021-12-07 9:05 UTC (permalink / raw)
To: Michal Orzel
Cc: Julien Grall, xen-devel, Stefano Stabellini, Volodymyr Babchuk
Hi Michal,
> On 7 Dec 2021, at 08:37, Michal Orzel <michal.orzel@arm.com> wrote:
>
> Hi Julien,
>
> On 06.12.2021 16:29, Julien Grall wrote:
>> Hi,
>>
>> On 06/12/2021 14:20, Michal Orzel wrote:
>>> to hypervisor when switching to AArch32 state.
>>>
> I will change to "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 IMPLEMENTATIONDEFINED"
>>
>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>
> Ok.
>>>
>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>> needs to be fixed.
>>
>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>
> I would say:
> "
> The reason why this is a problem 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,
> "
>>>
>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>> entry to hypervisor when switching to AArch32 state.
>>>
>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>> when not passed.
>>
>> Which error do you see otherwise? Is it a compilation error?
>>
> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
> So basically in all the places where entry is called with hyp=1.
> When taking the current patch and removing default value for compat you will get:
> ```
> entry.S:254: Error: ".endif" without ".if"
> entry.S:258: Error: symbol `.if' is already defined
> entry.S:258: Error: ".endif" without ".if"
> entry.S:262: Error: symbol `.if' is already defined
> entry.S:262: Error: ".endif" without ".if"
> entry.S:266: Error: symbol `.if' is already defined
> entry.S:266: Error: ".endif" without ".if"
> entry.S:278: Error: symbol `.if' is already defined
> entry.S:278: Error: ".endif" without ".if"
> entry.S:292: Error: symbol `.if' is already defined
> entry.S:292: Error: ".endif" without ".if"
> entry.S:317: Error: symbol `.if' is already defined
> entry.S:317: Error: ".endif" without ".if"
> ```
>
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>> ---
>>> xen/arch/arm/arm64/entry.S | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>> index fc3811ad0a..d364128175 100644
>>> --- a/xen/arch/arm/arm64/entry.S
>>> +++ b/xen/arch/arm/arm64/entry.S
>>> @@ -109,8 +109,16 @@
>>> * If 0, we rely on the on x0/x1 to have been saved at the correct
>>> * position on the stack before.
>>> */
>>> - .macro entry, hyp, compat, save_x0_x1=1
>>> + .macro entry, hyp, compat=0, save_x0_x1=1
>>> sub sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR */
>>> +
>>> + /* Zero the upper 32 bits of the registers when switching to AArch32 */
>>> + .if \compat == 1 /* AArch32 mode */
>>> + .irp nr,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
>>> + mov w\nr, w\nr
>>> + .endr
>>> + .endif
>>
>> So Jan mentioned, the x0/x1 may have already been saved. So you may need to fetch them from the stack and then clobber the top 32-bit.
>>
> So I would do the following:
> -fetch x0/x1 from the stack
> -clobber them
> -store them again on the stack
>
> /*
> * Zero the upper 32 bits of the gp registers when switching
> * from AArch32.
> */
> .if \compat == 1 /* AArch32 mode */
>
> /* x0/x1 have already been saved so fetch them to zero top 32 bits */
> .if \save_x0_x1 == 0
> ldp x0, x1, [sp], #(UREGS_kernel_sizeof - UREGS_X0)
> .endif
>
> .irp nr,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
> mov w\nr, w\nr
> .endr
>
> .if \save_x0_x1 == 0
> stp x0, x1, [sp, #-(UREGS_kernel_sizeof - UREGS_X0)]
> .endif
>
> .endif
This solution looks ok. X0 and x1 when they are used is as scratch register for x1 or using w0 for x0 so it is ok to clean them here and not earlier.
Cheers
Bertrand
>
>> Cheers,
>>
>
> Cheers,
> Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-07 8:37 ` Michal Orzel
2021-12-07 9:05 ` Bertrand Marquis
@ 2021-12-07 9:55 ` Jan Beulich
2021-12-07 19:25 ` Julien Grall
2021-12-07 19:11 ` Julien Grall
2 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2021-12-07 9:55 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
Julien Grall, xen-devel
On 07.12.2021 09:37, Michal Orzel wrote:
> On 06.12.2021 16:29, Julien Grall wrote:
>> On 06/12/2021 14:20, Michal Orzel wrote:
>>> to hypervisor when switching to AArch32 state.
>>>
> I will change to "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 IMPLEMENTATIONDEFINED"
>>
>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>
> Ok.
>>>
>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>> needs to be fixed.
>>
>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>
> I would say:
> "
> The reason why this is a problem 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,
> "
>>>
>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>> entry to hypervisor when switching to AArch32 state.
>>>
>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>> when not passed.
>>
>> Which error do you see otherwise? Is it a compilation error?
>>
> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
> So basically in all the places where entry is called with hyp=1.
> When taking the current patch and removing default value for compat you will get:
> ```
> entry.S:254: Error: ".endif" without ".if"
> entry.S:258: Error: symbol `.if' is already defined
> entry.S:258: Error: ".endif" without ".if"
> entry.S:262: Error: symbol `.if' is already defined
> entry.S:262: Error: ".endif" without ".if"
> entry.S:266: Error: symbol `.if' is already defined
> entry.S:266: Error: ".endif" without ".if"
> entry.S:278: Error: symbol `.if' is already defined
> entry.S:278: Error: ".endif" without ".if"
> entry.S:292: Error: symbol `.if' is already defined
> entry.S:292: Error: ".endif" without ".if"
> entry.S:317: Error: symbol `.if' is already defined
> entry.S:317: Error: ".endif" without ".if"
> ```
An alternative might be to use
.if 0\compat
>>> --- a/xen/arch/arm/arm64/entry.S
>>> +++ b/xen/arch/arm/arm64/entry.S
>>> @@ -109,8 +109,16 @@
>>> * If 0, we rely on the on x0/x1 to have been saved at the correct
>>> * position on the stack before.
>>> */
>>> - .macro entry, hyp, compat, save_x0_x1=1
>>> + .macro entry, hyp, compat=0, save_x0_x1=1
>>> sub sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR */
>>> +
>>> + /* Zero the upper 32 bits of the registers when switching to AArch32 */
>>> + .if \compat == 1 /* AArch32 mode */
>>> + .irp nr,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
>>> + mov w\nr, w\nr
>>> + .endr
>>> + .endif
>>
>> So Jan mentioned, the x0/x1 may have already been saved. So you may need to fetch them from the stack and then clobber the top 32-bit.
>>
> So I would do the following:
> -fetch x0/x1 from the stack
> -clobber them
> -store them again on the stack
>
> /*
> * Zero the upper 32 bits of the gp registers when switching
> * from AArch32.
> */
> .if \compat == 1 /* AArch32 mode */
>
> /* x0/x1 have already been saved so fetch them to zero top 32 bits */
> .if \save_x0_x1 == 0
> ldp x0, x1, [sp], #(UREGS_kernel_sizeof - UREGS_X0)
> .endif
>
> .irp nr,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
> mov w\nr, w\nr
> .endr
>
> .if \save_x0_x1 == 0
> stp x0, x1, [sp, #-(UREGS_kernel_sizeof - UREGS_X0)]
> .endif
>
> .endif
Wouldn't it be more efficient to store 32 bits of zero each into the
high halves of the respective stack slots? Afaict same code size, but
less memory / cache traffic. Plus it would avoid the latent issue of
a user of the macro actually expecting the two registers to retain
their values across the macro invocation.
I'm also puzzled by the two different memory addressing forms, but I
can easily see that I may be lacking enough Arm knowledge there.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-07 9:55 ` Jan Beulich
@ 2021-12-07 19:25 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2021-12-07 19:25 UTC (permalink / raw)
To: Jan Beulich, Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Hi Jan,
On 07/12/2021 09:55, Jan Beulich wrote:
>>>> --- a/xen/arch/arm/arm64/entry.S
>>>> +++ b/xen/arch/arm/arm64/entry.S
>>>> @@ -109,8 +109,16 @@
>>>> * If 0, we rely on the on x0/x1 to have been saved at the correct
>>>> * position on the stack before.
>>>> */
>>>> - .macro entry, hyp, compat, save_x0_x1=1
>>>> + .macro entry, hyp, compat=0, save_x0_x1=1
>>>> sub sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR */
>>>> +
>>>> + /* Zero the upper 32 bits of the registers when switching to AArch32 */
>>>> + .if \compat == 1 /* AArch32 mode */
>>>> + .irp nr,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
>>>> + mov w\nr, w\nr
>>>> + .endr
>>>> + .endif
>>>
>>> So Jan mentioned, the x0/x1 may have already been saved. So you may need to fetch them from the stack and then clobber the top 32-bit.
>>>
>> So I would do the following:
>> -fetch x0/x1 from the stack
>> -clobber them
>> -store them again on the stack
>>
>> /*
>> * Zero the upper 32 bits of the gp registers when switching
>> * from AArch32.
>> */
>> .if \compat == 1 /* AArch32 mode */
>>
>> /* x0/x1 have already been saved so fetch them to zero top 32 bits */
>> .if \save_x0_x1 == 0
>> ldp x0, x1, [sp], #(UREGS_kernel_sizeof - UREGS_X0)
>> .endif
>>
>> .irp nr,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
>> mov w\nr, w\nr
>> .endr
>>
>> .if \save_x0_x1 == 0
>> stp x0, x1, [sp, #-(UREGS_kernel_sizeof - UREGS_X0)]
>> .endif
>>
>> .endif
>
> Wouldn't it be more efficient to store 32 bits of zero each into the
> high halves of the respective stack slots? Afaict same code size, but
> less memory / cache traffic.
It would indeed be more efficient.
> Plus it would avoid the latent issue of
> a user of the macro actually expecting the two registers to retain
> their values across the macro invocation.
While this is not explicitely written, a caller cannot expect the
registers to be preserved by this macro.
>
> I'm also puzzled by the two different memory addressing forms, but I
> can easily see that I may be lacking enough Arm knowledge there.
I agree this is quite puzzling. The first one would update 'sp' after
loading the register but I don't quite understand why it is necessary.
Assuming the this is happening before the instruction 'sub sp, sp, ...',
then we should only need to load/store from sp with an offset (i.e. the
second form).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-07 8:37 ` Michal Orzel
2021-12-07 9:05 ` Bertrand Marquis
2021-12-07 9:55 ` Jan Beulich
@ 2021-12-07 19:11 ` Julien Grall
2021-12-08 7:20 ` Jan Beulich
2 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2021-12-07 19:11 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis
On 07/12/2021 08:37, Michal Orzel wrote:
> Hi Julien,
Hi,
> On 06.12.2021 16:29, Julien Grall wrote:
>> Hi,
>>
>> On 06/12/2021 14:20, Michal Orzel wrote:
>>> to hypervisor when switching to AArch32 state.
>>>
> I will change to "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 IMPLEMENTATIONDEFINED"
>>
>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>
> Ok.
>>>
>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>> needs to be fixed.
>>
>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>
> I would say:
> "
> The reason why this is a problem 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,
> "
>>>
>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>> entry to hypervisor when switching to AArch32 state.
>>>
>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>> when not passed.
>>
>> Which error do you see otherwise? Is it a compilation error?
>>
> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
> So basically in all the places where entry is called with hyp=1.
> When taking the current patch and removing default value for compat you will get:
> ```
> entry.S:254: Error: ".endif" without ".if"
> entry.S:258: Error: symbol `.if' is already defined
> entry.S:258: Error: ".endif" without ".if"
> entry.S:262: Error: symbol `.if' is already defined
> entry.S:262: Error: ".endif" without ".if"
> entry.S:266: Error: symbol `.if' is already defined
> entry.S:266: Error: ".endif" without ".if"
> entry.S:278: Error: symbol `.if' is already defined
> entry.S:278: Error: ".endif" without ".if"
> entry.S:292: Error: symbol `.if' is already defined
> entry.S:292: Error: ".endif" without ".if"
> entry.S:317: Error: symbol `.if' is already defined
> entry.S:317: Error: ".endif" without ".if"
> ```
Thanks for input. I am concerned with your suggested approach (or using
.if 0\compat as suggested by Jan) because they allow the caller to not
properly specify compat when hyp=0. The risk here is we may generate the
wrong entry.
compat should need to be specified when hyp=1 as we will always run in
aarch64 mode. So could we protect this code with hyp=0?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-07 19:11 ` Julien Grall
@ 2021-12-08 7:20 ` Jan Beulich
2021-12-08 9:55 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2021-12-08 7:20 UTC (permalink / raw)
To: Julien Grall, Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
On 07.12.2021 20:11, Julien Grall wrote:
>
>
> On 07/12/2021 08:37, Michal Orzel wrote:
>> Hi Julien,
>
> Hi,
>
>> On 06.12.2021 16:29, Julien Grall wrote:
>>> Hi,
>>>
>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>> to hypervisor when switching to AArch32 state.
>>>>
>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>
>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>
>> Ok.
>>>>
>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>> needs to be fixed.
>>>
>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>
>> I would say:
>> "
>> The reason why this is a problem 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,
>> "
>>>>
>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>> entry to hypervisor when switching to AArch32 state.
>>>>
>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>> when not passed.
>>>
>>> Which error do you see otherwise? Is it a compilation error?
>>>
>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>> So basically in all the places where entry is called with hyp=1.
>> When taking the current patch and removing default value for compat you will get:
>> ```
>> entry.S:254: Error: ".endif" without ".if"
>> entry.S:258: Error: symbol `.if' is already defined
>> entry.S:258: Error: ".endif" without ".if"
>> entry.S:262: Error: symbol `.if' is already defined
>> entry.S:262: Error: ".endif" without ".if"
>> entry.S:266: Error: symbol `.if' is already defined
>> entry.S:266: Error: ".endif" without ".if"
>> entry.S:278: Error: symbol `.if' is already defined
>> entry.S:278: Error: ".endif" without ".if"
>> entry.S:292: Error: symbol `.if' is already defined
>> entry.S:292: Error: ".endif" without ".if"
>> entry.S:317: Error: symbol `.if' is already defined
>> entry.S:317: Error: ".endif" without ".if"
>> ```
>
> Thanks for input. I am concerned with your suggested approach (or using
> .if 0\compat as suggested by Jan) because they allow the caller to not
> properly specify compat when hyp=0. The risk here is we may generate the
> wrong entry.
>
> compat should need to be specified when hyp=1 as we will always run in
> aarch64 mode. So could we protect this code with hyp=0?
Since my suggestion was only to avoid the need for specifying a default
for the parameter (which you didn't seem to be happy about), it would
then merely extend to
.if !0\hyp && 0\compat
or something along those lines.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-08 7:20 ` Jan Beulich
@ 2021-12-08 9:55 ` Julien Grall
2021-12-08 10:18 ` Jan Beulich
2021-12-14 9:17 ` Michal Orzel
0 siblings, 2 replies; 26+ messages in thread
From: Julien Grall @ 2021-12-08 9:55 UTC (permalink / raw)
To: Jan Beulich, Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Hi,
On 08/12/2021 07:20, Jan Beulich wrote:
> On 07.12.2021 20:11, Julien Grall wrote:
>>
>>
>> On 07/12/2021 08:37, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi,
>>
>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>> to hypervisor when switching to AArch32 state.
>>>>>
>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>
>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>
>>> Ok.
>>>>>
>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>> needs to be fixed.
>>>>
>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>
>>> I would say:
>>> "
>>> The reason why this is a problem 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,
>>> "
>>>>>
>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>
>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>> when not passed.
>>>>
>>>> Which error do you see otherwise? Is it a compilation error?
>>>>
>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>> So basically in all the places where entry is called with hyp=1.
>>> When taking the current patch and removing default value for compat you will get:
>>> ```
>>> entry.S:254: Error: ".endif" without ".if"
>>> entry.S:258: Error: symbol `.if' is already defined
>>> entry.S:258: Error: ".endif" without ".if"
>>> entry.S:262: Error: symbol `.if' is already defined
>>> entry.S:262: Error: ".endif" without ".if"
>>> entry.S:266: Error: symbol `.if' is already defined
>>> entry.S:266: Error: ".endif" without ".if"
>>> entry.S:278: Error: symbol `.if' is already defined
>>> entry.S:278: Error: ".endif" without ".if"
>>> entry.S:292: Error: symbol `.if' is already defined
>>> entry.S:292: Error: ".endif" without ".if"
>>> entry.S:317: Error: symbol `.if' is already defined
>>> entry.S:317: Error: ".endif" without ".if"
>>> ```
>>
>> Thanks for input. I am concerned with your suggested approach (or using
>> .if 0\compat as suggested by Jan) because they allow the caller to not
>> properly specify compat when hyp=0. The risk here is we may generate the
>> wrong entry.
>>
>> compat should need to be specified when hyp=1 as we will always run in
>> aarch64 mode. So could we protect this code with hyp=0?
>
> Since my suggestion was only to avoid the need for specifying a default
> for the parameter (which you didn't seem to be happy about), it would
> then merely extend to
>
> .if !0\hyp && 0\compat
Isn't it effectively the same as setting a default value?
The reason we seem to get away is because other part of the macro (e.g.
entry_guest) will need compat to be valid.
But that seems pretty fragile to me. So I would prefer if the new code
it added within a macro that is only called when hyp==0.
Cheers,
>
> or something along those lines.
>
> Jan
>
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-08 9:55 ` Julien Grall
@ 2021-12-08 10:18 ` Jan Beulich
2021-12-14 9:17 ` Michal Orzel
1 sibling, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2021-12-08 10:18 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
xen-devel, Michal Orzel
On 08.12.2021 10:55, Julien Grall wrote:
> Hi,
>
> On 08/12/2021 07:20, Jan Beulich wrote:
>> On 07.12.2021 20:11, Julien Grall wrote:
>>>
>>>
>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>> Hi Julien,
>>>
>>> Hi,
>>>
>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>
>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>
>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>
>>>> Ok.
>>>>>>
>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>> needs to be fixed.
>>>>>
>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>
>>>> I would say:
>>>> "
>>>> The reason why this is a problem 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,
>>>> "
>>>>>>
>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>
>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>> when not passed.
>>>>>
>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>
>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>> So basically in all the places where entry is called with hyp=1.
>>>> When taking the current patch and removing default value for compat you will get:
>>>> ```
>>>> entry.S:254: Error: ".endif" without ".if"
>>>> entry.S:258: Error: symbol `.if' is already defined
>>>> entry.S:258: Error: ".endif" without ".if"
>>>> entry.S:262: Error: symbol `.if' is already defined
>>>> entry.S:262: Error: ".endif" without ".if"
>>>> entry.S:266: Error: symbol `.if' is already defined
>>>> entry.S:266: Error: ".endif" without ".if"
>>>> entry.S:278: Error: symbol `.if' is already defined
>>>> entry.S:278: Error: ".endif" without ".if"
>>>> entry.S:292: Error: symbol `.if' is already defined
>>>> entry.S:292: Error: ".endif" without ".if"
>>>> entry.S:317: Error: symbol `.if' is already defined
>>>> entry.S:317: Error: ".endif" without ".if"
>>>> ```
>>>
>>> Thanks for input. I am concerned with your suggested approach (or using
>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>> properly specify compat when hyp=0. The risk here is we may generate the
>>> wrong entry.
>>>
>>> compat should need to be specified when hyp=1 as we will always run in
>>> aarch64 mode. So could we protect this code with hyp=0?
>>
>> Since my suggestion was only to avoid the need for specifying a default
>> for the parameter (which you didn't seem to be happy about), it would
>> then merely extend to
>>
>> .if !0\hyp && 0\compat
> Isn't it effectively the same as setting a default value?
Hmm, yes, it is.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-08 9:55 ` Julien Grall
2021-12-08 10:18 ` Jan Beulich
@ 2021-12-14 9:17 ` Michal Orzel
2021-12-14 9:33 ` Julien Grall
1 sibling, 1 reply; 26+ messages in thread
From: Michal Orzel @ 2021-12-14 9:17 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Hi Julien, Jan
On 08.12.2021 10:55, Julien Grall wrote:
> Hi,
>
> On 08/12/2021 07:20, Jan Beulich wrote:
>> On 07.12.2021 20:11, Julien Grall wrote:
>>>
>>>
>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>> Hi Julien,
>>>
>>> Hi,
>>>
>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>
>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>
>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>
>>>> Ok.
>>>>>>
>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>> needs to be fixed.
>>>>>
>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>
>>>> I would say:
>>>> "
>>>> The reason why this is a problem 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,
>>>> "
>>>>>>
>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>
>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>> when not passed.
>>>>>
>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>
>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>> So basically in all the places where entry is called with hyp=1.
>>>> When taking the current patch and removing default value for compat you will get:
>>>> ```
>>>> entry.S:254: Error: ".endif" without ".if"
>>>> entry.S:258: Error: symbol `.if' is already defined
>>>> entry.S:258: Error: ".endif" without ".if"
>>>> entry.S:262: Error: symbol `.if' is already defined
>>>> entry.S:262: Error: ".endif" without ".if"
>>>> entry.S:266: Error: symbol `.if' is already defined
>>>> entry.S:266: Error: ".endif" without ".if"
>>>> entry.S:278: Error: symbol `.if' is already defined
>>>> entry.S:278: Error: ".endif" without ".if"
>>>> entry.S:292: Error: symbol `.if' is already defined
>>>> entry.S:292: Error: ".endif" without ".if"
>>>> entry.S:317: Error: symbol `.if' is already defined
>>>> entry.S:317: Error: ".endif" without ".if"
>>>> ```
>>>
>>> Thanks for input. I am concerned with your suggested approach (or using
>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>> properly specify compat when hyp=0. The risk here is we may generate the
>>> wrong entry.
>>>
>>> compat should need to be specified when hyp=1 as we will always run in
>>> aarch64 mode. So could we protect this code with hyp=0?
>>
>> Since my suggestion was only to avoid the need for specifying a default
>> for the parameter (which you didn't seem to be happy about), it would
>> then merely extend to
>>
>> .if !0\hyp && 0\compat
> Isn't it effectively the same as setting a default value?
>
> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>
> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>
So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
> Cheers,
>
>>
>> or something along those lines.
>>
>> Jan
>>
>
So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
stp wzr, wzr, [sp #8 * 0]
stp wzr, wzr, [sp #8 * 1]
...
FWIK this would store 0 in the upper addresses. Am I correct?
Cheers,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-14 9:17 ` Michal Orzel
@ 2021-12-14 9:33 ` Julien Grall
2021-12-14 9:51 ` Michal Orzel
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2021-12-14 9:33 UTC (permalink / raw)
To: Michal Orzel, Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
On 14/12/2021 09:17, Michal Orzel wrote:
> Hi Julien, Jan
Hi,
> On 08.12.2021 10:55, Julien Grall wrote:
>> Hi,
>>
>> On 08/12/2021 07:20, Jan Beulich wrote:
>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>
>>>>
>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi,
>>>>
>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>
>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>
>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>
>>>>> Ok.
>>>>>>>
>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>>> needs to be fixed.
>>>>>>
>>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>>
>>>>> I would say:
>>>>> "
>>>>> The reason why this is a problem 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,
>>>>> "
>>>>>>>
>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>
>>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>>> when not passed.
>>>>>>
>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>
>>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>>> So basically in all the places where entry is called with hyp=1.
>>>>> When taking the current patch and removing default value for compat you will get:
>>>>> ```
>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>> ```
>>>>
>>>> Thanks for input. I am concerned with your suggested approach (or using
>>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>>> properly specify compat when hyp=0. The risk here is we may generate the
>>>> wrong entry.
>>>>
>>>> compat should need to be specified when hyp=1 as we will always run in
>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>
>>> Since my suggestion was only to avoid the need for specifying a default
>>> for the parameter (which you didn't seem to be happy about), it would
>>> then merely extend to
>>>
>>> .if !0\hyp && 0\compat
>> Isn't it effectively the same as setting a default value?
>>
>> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>>
>> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>>
> So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
Yes. This is the only way I could think to avoid making 'compat'optional.
>> Cheers,
>>
>>>
>>> or something along those lines.
>>>
>>> Jan
>>>
>>
> So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
> stp wzr, wzr, [sp #8 * 0]
> stp wzr, wzr, [sp #8 * 1]
> ...
I don't think you can use stp here because this would store two 32-bit
values consecutively. Instead, you would need to use ldr to store one
32-bit value at the time.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-14 9:33 ` Julien Grall
@ 2021-12-14 9:51 ` Michal Orzel
2021-12-14 10:01 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Michal Orzel @ 2021-12-14 9:51 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Hi Julien,
On 14.12.2021 10:33, Julien Grall wrote:
>
>
> On 14/12/2021 09:17, Michal Orzel wrote:
>> Hi Julien, Jan
>
> Hi,
>
>> On 08.12.2021 10:55, Julien Grall wrote:
>>> Hi,
>>>
>>> On 08/12/2021 07:20, Jan Beulich wrote:
>>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>>> Hi Julien,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>>
>>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>>
>>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>>
>>>>>> Ok.
>>>>>>>>
>>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>>>> needs to be fixed.
>>>>>>>
>>>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>>>
>>>>>> I would say:
>>>>>> "
>>>>>> The reason why this is a problem 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,
>>>>>> "
>>>>>>>>
>>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>>
>>>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>>>> when not passed.
>>>>>>>
>>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>>
>>>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>>>> So basically in all the places where entry is called with hyp=1.
>>>>>> When taking the current patch and removing default value for compat you will get:
>>>>>> ```
>>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>>> ```
>>>>>
>>>>> Thanks for input. I am concerned with your suggested approach (or using
>>>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>>>> properly specify compat when hyp=0. The risk here is we may generate the
>>>>> wrong entry.
>>>>>
>>>>> compat should need to be specified when hyp=1 as we will always run in
>>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>>
>>>> Since my suggestion was only to avoid the need for specifying a default
>>>> for the parameter (which you didn't seem to be happy about), it would
>>>> then merely extend to
>>>>
>>>> .if !0\hyp && 0\compat
>>> Isn't it effectively the same as setting a default value?
>>>
>>> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>>>
>>> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>>>
>> So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
>
> Yes. This is the only way I could think to avoid making 'compat'optional.
>
>>> Cheers,
>>>
>>>>
>>>> or something along those lines.
>>>>
>>>> Jan
>>>>
>>>
>> So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
>> stp wzr, wzr, [sp #8 * 0]
>> stp wzr, wzr, [sp #8 * 1]
>> ...
>
> I don't think you can use stp here because this would store two 32-bit values consecutively. Instead, you would need to use ldr to store one 32-bit value at the time.
>
I hope you meant str and not ldr.
So a loop would look like that:
str wzr, [sp, #8 * 1]
str wzr, [sp, #8 * 3]
...
> Cheers,
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-14 9:51 ` Michal Orzel
@ 2021-12-14 10:01 ` Jan Beulich
2021-12-14 10:10 ` Michal Orzel
2021-12-14 11:01 ` Julien Grall
0 siblings, 2 replies; 26+ messages in thread
From: Jan Beulich @ 2021-12-14 10:01 UTC (permalink / raw)
To: Michal Orzel, Julien Grall
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
On 14.12.2021 10:51, Michal Orzel wrote:
> Hi Julien,
>
> On 14.12.2021 10:33, Julien Grall wrote:
>>
>>
>> On 14/12/2021 09:17, Michal Orzel wrote:
>>> Hi Julien, Jan
>>
>> Hi,
>>
>>> On 08.12.2021 10:55, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 08/12/2021 07:20, Jan Beulich wrote:
>>>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>>>
>>>>>>
>>>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>>>> Hi Julien,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>>>
>>>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>>>
>>>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>>>
>>>>>>> Ok.
>>>>>>>>>
>>>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>>>>> needs to be fixed.
>>>>>>>>
>>>>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>>>>
>>>>>>> I would say:
>>>>>>> "
>>>>>>> The reason why this is a problem 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,
>>>>>>> "
>>>>>>>>>
>>>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>>>
>>>>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>>>>> when not passed.
>>>>>>>>
>>>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>>>
>>>>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>>>>> So basically in all the places where entry is called with hyp=1.
>>>>>>> When taking the current patch and removing default value for compat you will get:
>>>>>>> ```
>>>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>>>> ```
>>>>>>
>>>>>> Thanks for input. I am concerned with your suggested approach (or using
>>>>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>>>>> properly specify compat when hyp=0. The risk here is we may generate the
>>>>>> wrong entry.
>>>>>>
>>>>>> compat should need to be specified when hyp=1 as we will always run in
>>>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>>>
>>>>> Since my suggestion was only to avoid the need for specifying a default
>>>>> for the parameter (which you didn't seem to be happy about), it would
>>>>> then merely extend to
>>>>>
>>>>> .if !0\hyp && 0\compat
>>>> Isn't it effectively the same as setting a default value?
>>>>
>>>> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>>>>
>>>> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>>>>
>>> So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
>>
>> Yes. This is the only way I could think to avoid making 'compat'optional.
>>
>>>> Cheers,
>>>>
>>>>>
>>>>> or something along those lines.
>>>>>
>>>>> Jan
>>>>>
>>>>
>>> So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
>>> stp wzr, wzr, [sp #8 * 0]
>>> stp wzr, wzr, [sp #8 * 1]
>>> ...
>>
>> I don't think you can use stp here because this would store two 32-bit values consecutively. Instead, you would need to use ldr to store one 32-bit value at the time.
>>
> I hope you meant str and not ldr.
> So a loop would look like that:
> str wzr, [sp, #8 * 1]
> str wzr, [sp, #8 * 3]
> ...
Why "a loop" and why #8 (I'd have expected #4)?
There's another oddity which I'm noticing only now, but which also
may look odd to me only because I lack sufficient Arm details: On
x86, it would not be advisable to store anything below the stack
pointer (like is done here when storing x0 and x1 early), unless
it's absolutely certain that no further interruptions could clobber
that part of the stack.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-14 10:01 ` Jan Beulich
@ 2021-12-14 10:10 ` Michal Orzel
2021-12-14 11:01 ` Julien Grall
1 sibling, 0 replies; 26+ messages in thread
From: Michal Orzel @ 2021-12-14 10:10 UTC (permalink / raw)
To: Jan Beulich, Julien Grall
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
On 14.12.2021 11:01, Jan Beulich wrote:
> On 14.12.2021 10:51, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 14.12.2021 10:33, Julien Grall wrote:
>>>
>>>
>>> On 14/12/2021 09:17, Michal Orzel wrote:
>>>> Hi Julien, Jan
>>>
>>> Hi,
>>>
>>>> On 08.12.2021 10:55, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 08/12/2021 07:20, Jan Beulich wrote:
>>>>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>>>>> Hi Julien,
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>>>>
>>>>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>>>>
>>>>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>>>
>>>>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>>>>>> needs to be fixed.
>>>>>>>>>
>>>>>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>>>>>
>>>>>>>> I would say:
>>>>>>>> "
>>>>>>>> The reason why this is a problem 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,
>>>>>>>> "
>>>>>>>>>>
>>>>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>>>>
>>>>>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>>>>>> when not passed.
>>>>>>>>>
>>>>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>>>>
>>>>>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>>>>>> So basically in all the places where entry is called with hyp=1.
>>>>>>>> When taking the current patch and removing default value for compat you will get:
>>>>>>>> ```
>>>>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>>>>> ```
>>>>>>>
>>>>>>> Thanks for input. I am concerned with your suggested approach (or using
>>>>>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>>>>>> properly specify compat when hyp=0. The risk here is we may generate the
>>>>>>> wrong entry.
>>>>>>>
>>>>>>> compat should need to be specified when hyp=1 as we will always run in
>>>>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>>>>
>>>>>> Since my suggestion was only to avoid the need for specifying a default
>>>>>> for the parameter (which you didn't seem to be happy about), it would
>>>>>> then merely extend to
>>>>>>
>>>>>> .if !0\hyp && 0\compat
>>>>> Isn't it effectively the same as setting a default value?
>>>>>
>>>>> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>>>>>
>>>>> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>>>>>
>>>> So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
>>>
>>> Yes. This is the only way I could think to avoid making 'compat'optional.
>>>
>>>>> Cheers,
>>>>>
>>>>>>
>>>>>> or something along those lines.
>>>>>>
>>>>>> Jan
>>>>>>
>>>>>
>>>> So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
>>>> stp wzr, wzr, [sp #8 * 0]
>>>> stp wzr, wzr, [sp #8 * 1]
>>>> ...
>>>
>>> I don't think you can use stp here because this would store two 32-bit values consecutively. Instead, you would need to use ldr to store one 32-bit value at the time.
>>>
>> I hope you meant str and not ldr.
>> So a loop would look like that:
>> str wzr, [sp, #8 * 1]
>> str wzr, [sp, #8 * 3]
>> ...
>
> Why "a loop" and why #8 (I'd have expected #4)?
>
You are right. I confused it with stp. #4 is correct.
> There's another oddity which I'm noticing only now, but which also
> may look odd to me only because I lack sufficient Arm details: On
> x86, it would not be advisable to store anything below the stack
> pointer (like is done here when storing x0 and x1 early), unless
> it's absolutely certain that no further interruptions could clobber
> that part of the stack.
>
I cannot answer this question. Sorry.
> Jan
>
Cheers
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-14 10:01 ` Jan Beulich
2021-12-14 10:10 ` Michal Orzel
@ 2021-12-14 11:01 ` Julien Grall
2021-12-14 11:30 ` Julien Grall
1 sibling, 1 reply; 26+ messages in thread
From: Julien Grall @ 2021-12-14 11:01 UTC (permalink / raw)
To: Jan Beulich, Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Hi,
Replying in one e-mail the comments from Jan and Michal.
On 14/12/2021 10:01, Jan Beulich wrote:
> On 14.12.2021 10:51, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 14.12.2021 10:33, Julien Grall wrote:
>>>
>>>
>>> On 14/12/2021 09:17, Michal Orzel wrote:
>>>> Hi Julien, Jan
>>>
>>> Hi,
>>>
>>>> On 08.12.2021 10:55, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 08/12/2021 07:20, Jan Beulich wrote:
>>>>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>>>>> Hi Julien,
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>>>>
>>>>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>>>>
>>>>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>>>
>>>>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>>>>>> needs to be fixed.
>>>>>>>>>
>>>>>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>>>>>
>>>>>>>> I would say:
>>>>>>>> "
>>>>>>>> The reason why this is a problem 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,
>>>>>>>> "
>>>>>>>>>>
>>>>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>>>>
>>>>>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>>>>>> when not passed.
>>>>>>>>>
>>>>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>>>>
>>>>>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>>>>>> So basically in all the places where entry is called with hyp=1.
>>>>>>>> When taking the current patch and removing default value for compat you will get:
>>>>>>>> ```
>>>>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>>>>> ```
>>>>>>>
>>>>>>> Thanks for input. I am concerned with your suggested approach (or using
>>>>>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>>>>>> properly specify compat when hyp=0. The risk here is we may generate the
>>>>>>> wrong entry.
>>>>>>>
>>>>>>> compat should need to be specified when hyp=1 as we will always run in
>>>>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>>>>
>>>>>> Since my suggestion was only to avoid the need for specifying a default
>>>>>> for the parameter (which you didn't seem to be happy about), it would
>>>>>> then merely extend to
>>>>>>
>>>>>> .if !0\hyp && 0\compat
>>>>> Isn't it effectively the same as setting a default value?
>>>>>
>>>>> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>>>>>
>>>>> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>>>>>
>>>> So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
>>>
>>> Yes. This is the only way I could think to avoid making 'compat'optional.
>>>
>>>>> Cheers,
>>>>>
>>>>>>
>>>>>> or something along those lines.
>>>>>>
>>>>>> Jan
>>>>>>
>>>>>
>>>> So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
>>>> stp wzr, wzr, [sp #8 * 0]
>>>> stp wzr, wzr, [sp #8 * 1]
>>>> ...
>>>
>>> I don't think you can use stp here because this would store two 32-bit values consecutively. Instead, you would need to use ldr to store one 32-bit value at the time.
>>>
>> I hope you meant str and not ldr.
Yes. I am not sure why I wrote ldr.
>> So a loop would look like that:
>> str wzr, [sp, #8 * 1]
>> str wzr, [sp, #8 * 3]
>> ...
>
> Why "a loop" and why #8 (I'd have expected #4)?
>
> There's another oddity which I'm noticing only now, but which also
> may look odd to me only because I lack sufficient Arm details: On
> x86, it would not be advisable to store anything below the stack
> pointer (like is done here when storing x0 and x1 early), unless
> it's absolutely certain that no further interruptions could clobber
> that part of the stack.
We are entering the hypervisor with both Interrupts and SErrors masked.
They will only be unmasked after the guest registers have been saved on
the stack.
You may still receive a Data Abort before the macro 'entry' has
completed. But this is going to result to an hypervisor crash because
they are not meant to happen in those paths.
So I believe, we are safe to modify sp before.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-14 11:01 ` Julien Grall
@ 2021-12-14 11:30 ` Julien Grall
2021-12-15 9:27 ` Michal Orzel
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2021-12-14 11:30 UTC (permalink / raw)
To: Jan Beulich, Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
On 14/12/2021 11:01, Julien Grall wrote:
> Hi,
>
> Replying in one e-mail the comments from Jan and Michal.
>
> On 14/12/2021 10:01, Jan Beulich wrote:
>> On 14.12.2021 10:51, Michal Orzel wrote:
>>> Hi Julien,
>>>
>>> On 14.12.2021 10:33, Julien Grall wrote:
>>>>
>>>>
>>>> On 14/12/2021 09:17, Michal Orzel wrote:
>>>>> Hi Julien, Jan
>>>>
>>>> Hi,
>>>>
>>>>> On 08.12.2021 10:55, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 08/12/2021 07:20, Jan Beulich wrote:
>>>>>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>>>>>> Hi Julien,
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>>>>>
>>>>>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>>>>>
>>>>>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>>>>>
>>>>>>>>> Ok.
>>>>>>>>>>>
>>>>>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed
>>>>>>>>>>> and this
>>>>>>>>>>> needs to be fixed.
>>>>>>>>>>
>>>>>>>>>> Can you outline why this is a problem and why we need to
>>>>>>>>>> protect? IIRC, the main concern is Xen may misinterpret what
>>>>>>>>>> the guest requested but we are not concerned about Xen using
>>>>>>>>>> wrong value.
>>>>>>>>>>
>>>>>>>>> I would say:
>>>>>>>>> "
>>>>>>>>> The reason why this is a problem 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,
>>>>>>>>> "
>>>>>>>>>>>
>>>>>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers
>>>>>>>>>>> on an
>>>>>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>>>>>
>>>>>>>>>>> Set default value of parameter compat of macro entry to 0
>>>>>>>>>>> (AArch64 mode
>>>>>>>>>>> as we are on 64-bit hypervisor) to avoid checking if
>>>>>>>>>>> parameter is blank
>>>>>>>>>>> when not passed.
>>>>>>>>>>
>>>>>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>>>>>
>>>>>>>>> Yes, this is a compilation error. The errors appear at each
>>>>>>>>> line when "entry" is called without passing value for "compat".
>>>>>>>>> So basically in all the places where entry is called with hyp=1.
>>>>>>>>> When taking the current patch and removing default value for
>>>>>>>>> compat you will get:
>>>>>>>>> ```
>>>>>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>>>>>> ```
>>>>>>>>
>>>>>>>> Thanks for input. I am concerned with your suggested approach
>>>>>>>> (or using
>>>>>>>> .if 0\compat as suggested by Jan) because they allow the caller
>>>>>>>> to not
>>>>>>>> properly specify compat when hyp=0. The risk here is we may
>>>>>>>> generate the
>>>>>>>> wrong entry.
>>>>>>>>
>>>>>>>> compat should need to be specified when hyp=1 as we will always
>>>>>>>> run in
>>>>>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>>>>>
>>>>>>> Since my suggestion was only to avoid the need for specifying a
>>>>>>> default
>>>>>>> for the parameter (which you didn't seem to be happy about), it
>>>>>>> would
>>>>>>> then merely extend to
>>>>>>>
>>>>>>> .if !0\hyp && 0\compat
>>>>>> Isn't it effectively the same as setting a default value?
>>>>>>
>>>>>> The reason we seem to get away is because other part of the macro
>>>>>> (e.g. entry_guest) will need compat to be valid.
>>>>>>
>>>>>> But that seems pretty fragile to me. So I would prefer if the new
>>>>>> code it added within a macro that is only called when hyp==0.
>>>>>>
>>>>> So you would like to have a macro that is called if hyp=0 (which
>>>>> means compat had to be passed) and inside this macro additional
>>>>> check if compat is 1?
>>>>
>>>> Yes. This is the only way I could think to avoid making
>>>> 'compat'optional.
>>>>
>>>>>> Cheers,
>>>>>>
>>>>>>>
>>>>>>> or something along those lines.
>>>>>>>
>>>>>>> Jan
>>>>>>>
>>>>>>
>>>>> So when it comes to zeroing the top 32bits by pushing zero to
>>>>> higher halves of stack slots I would do in a loop:
>>>>> stp wzr, wzr, [sp #8 * 0]
>>>>> stp wzr, wzr, [sp #8 * 1]
>>>>> ...
>>>>
>>>> I don't think you can use stp here because this would store two
>>>> 32-bit values consecutively. Instead, you would need to use ldr to
>>>> store one 32-bit value at the time.
>>>>
>>> I hope you meant str and not ldr.
>
> Yes. I am not sure why I wrote ldr.
>
>>> So a loop would look like that:
>>> str wzr, [sp, #8 * 1]
>>> str wzr, [sp, #8 * 3]
>>> ...
>>
>> Why "a loop" and why #8 (I'd have expected #4)?
>>
>> There's another oddity which I'm noticing only now, but which also
>> may look odd to me only because I lack sufficient Arm details: On
>> x86, it would not be advisable to store anything below the stack
>> pointer (like is done here when storing x0 and x1 early), unless
>> it's absolutely certain that no further interruptions could clobber
>> that part of the stack.
>
> We are entering the hypervisor with both Interrupts and SErrors masked.
> They will only be unmasked after the guest registers have been saved on
> the stack.
>
> You may still receive a Data Abort before the macro 'entry' has
> completed. But this is going to result to an hypervisor crash because
> they are not meant to happen in those paths.
>
> So I believe, we are safe to modify sp before.
Hmmm... I meant to write on the stack before sp is modified.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-14 11:30 ` Julien Grall
@ 2021-12-15 9:27 ` Michal Orzel
2021-12-15 9:35 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Michal Orzel @ 2021-12-15 9:27 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Replying to both Julien and Jan,
On 14.12.2021 12:30, Julien Grall wrote:
>
>
> On 14/12/2021 11:01, Julien Grall wrote:
>> Hi,
>>
>> Replying in one e-mail the comments from Jan and Michal.
>>
>> On 14/12/2021 10:01, Jan Beulich wrote:
>>> On 14.12.2021 10:51, Michal Orzel wrote:
>>>> Hi Julien,
>>>>
>>>> On 14.12.2021 10:33, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 14/12/2021 09:17, Michal Orzel wrote:
>>>>>> Hi Julien, Jan
>>>>>
>>>>> Hi,
>>>>>
>>>>>> On 08.12.2021 10:55, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 08/12/2021 07:20, Jan Beulich wrote:
>>>>>>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>>>>>>> Hi Julien,
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>>>>>>
>>>>>>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>>>>>>
>>>>>>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>>>>>>
>>>>>>>>>> Ok.
>>>>>>>>>>>>
>>>>>>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>>>>>>>> needs to be fixed.
>>>>>>>>>>>
>>>>>>>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>>>>>>>
>>>>>>>>>> I would say:
>>>>>>>>>> "
>>>>>>>>>> The reason why this is a problem 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,
>>>>>>>>>> "
>>>>>>>>>>>>
>>>>>>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>>>>>>
>>>>>>>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>>>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>>>>>>>> when not passed.
>>>>>>>>>>>
>>>>>>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>>>>>>
>>>>>>>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>>>>>>>> So basically in all the places where entry is called with hyp=1.
>>>>>>>>>> When taking the current patch and removing default value for compat you will get:
>>>>>>>>>> ```
>>>>>>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>>>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>>>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>>>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>>>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>>>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>>>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>>>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>>>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>>>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>>>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>>>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>>>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>>>>>>> ```
>>>>>>>>>
>>>>>>>>> Thanks for input. I am concerned with your suggested approach (or using
>>>>>>>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>>>>>>>> properly specify compat when hyp=0. The risk here is we may generate the
>>>>>>>>> wrong entry.
>>>>>>>>>
>>>>>>>>> compat should need to be specified when hyp=1 as we will always run in
>>>>>>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>>>>>>
>>>>>>>> Since my suggestion was only to avoid the need for specifying a default
>>>>>>>> for the parameter (which you didn't seem to be happy about), it would
>>>>>>>> then merely extend to
>>>>>>>>
>>>>>>>> .if !0\hyp && 0\compat
>>>>>>> Isn't it effectively the same as setting a default value?
>>>>>>>
>>>>>>> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>>>>>>>
>>>>>>> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>>>>>>>
>>>>>> So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
>>>>>
>>>>> Yes. This is the only way I could think to avoid making 'compat'optional.
>>>>>
>>>>>>> Cheers,
>>>>>>>
>>>>>>>>
>>>>>>>> or something along those lines.
>>>>>>>>
>>>>>>>> Jan
>>>>>>>>
>>>>>>>
>>>>>> So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
>>>>>> stp wzr, wzr, [sp #8 * 0]
>>>>>> stp wzr, wzr, [sp #8 * 1]
>>>>>> ...
>>>>>
>>>>> I don't think you can use stp here because this would store two 32-bit values consecutively. Instead, you would need to use ldr to store one 32-bit value at the time.
>>>>>
>>>> I hope you meant str and not ldr.
>>
>> Yes. I am not sure why I wrote ldr.
>>
>>>> So a loop would look like that:
>>>> str wzr, [sp, #8 * 1]
>>>> str wzr, [sp, #8 * 3]
>>>> ...
>>>
>>> Why "a loop" and why #8 (I'd have expected #4)?
>>>
>>> There's another oddity which I'm noticing only now, but which also
>>> may look odd to me only because I lack sufficient Arm details: On
>>> x86, it would not be advisable to store anything below the stack
>>> pointer (like is done here when storing x0 and x1 early), unless
>>> it's absolutely certain that no further interruptions could clobber
>>> that part of the stack.
>>
>> We are entering the hypervisor with both Interrupts and SErrors masked. They will only be unmasked after the guest registers have been saved on the stack.
>>
>> You may still receive a Data Abort before the macro 'entry' has completed. But this is going to result to an hypervisor crash because they are not meant to happen in those paths.
>>
>> So I believe, we are safe to modify sp before.
>
> Hmmm... I meant to write on the stack before sp is modified.
>
> Cheers,
>
I would like to summarize what we discussed before pushing v2.
Changes since v1:
-update commit message adding information why do we need to zero top 32bits
-zero corresponding stack slots instead of zeroing directly gp registers
-create a macro called by entry, protected by if hyp=0. In macro add if compat=1
Now when it comes to implementation.
1. Regarding save_x0_x1, it is 0 only for guest_sync_slowpath, which is called by guest_sync.
So as we are dealing only with compat=1, save_x0_x1 cannot be 0.
The conclusion is that we do not need to worry about it.
2. Regarding clearing high halves of stack slots.
The new macro (called zero_stack_top_halves) will be called in entry before the first instruction sub sp,sp.
To avoid saving sp position/moving it, the simplest would be to execute 30 times:
str wzr, [sp, #-(UREGS_kernel_sizeof - 4)]
str wzr, [sp, #-(UREGS_kernel_sizeof - 12)]
...
I could also use .irp loop like (.irp n,1,3,5,7,...) and then:
str wzr, [sp, #-(UREGS_kernel_sizeof - (4 * n))]
but FWIK Jan does not like loops :)
Let me know what u think.
Cheers,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-15 9:27 ` Michal Orzel
@ 2021-12-15 9:35 ` Jan Beulich
2021-12-15 9:48 ` Michal Orzel
2021-12-15 18:20 ` Julien Grall
0 siblings, 2 replies; 26+ messages in thread
From: Jan Beulich @ 2021-12-15 9:35 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
xen-devel, Julien Grall
On 15.12.2021 10:27, Michal Orzel wrote:
> Replying to both Julien and Jan,
>
> On 14.12.2021 12:30, Julien Grall wrote:
>>
>>
>> On 14/12/2021 11:01, Julien Grall wrote:
>>> Hi,
>>>
>>> Replying in one e-mail the comments from Jan and Michal.
>>>
>>> On 14/12/2021 10:01, Jan Beulich wrote:
>>>> On 14.12.2021 10:51, Michal Orzel wrote:
>>>>> Hi Julien,
>>>>>
>>>>> On 14.12.2021 10:33, Julien Grall wrote:
>>>>>>
>>>>>>
>>>>>> On 14/12/2021 09:17, Michal Orzel wrote:
>>>>>>> Hi Julien, Jan
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> On 08.12.2021 10:55, Julien Grall wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 08/12/2021 07:20, Jan Beulich wrote:
>>>>>>>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>>>>>>>> Hi Julien,
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>>>>>>>
>>>>>>>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>>>>>>>
>>>>>>>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>>>>>>>
>>>>>>>>>>> Ok.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>>>>>>>>> needs to be fixed.
>>>>>>>>>>>>
>>>>>>>>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>>>>>>>>
>>>>>>>>>>> I would say:
>>>>>>>>>>> "
>>>>>>>>>>> The reason why this is a problem 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,
>>>>>>>>>>> "
>>>>>>>>>>>>>
>>>>>>>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>>>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>>>>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>>>>>>>>> when not passed.
>>>>>>>>>>>>
>>>>>>>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>>>>>>>
>>>>>>>>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>>>>>>>>> So basically in all the places where entry is called with hyp=1.
>>>>>>>>>>> When taking the current patch and removing default value for compat you will get:
>>>>>>>>>>> ```
>>>>>>>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>>>>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>>>>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>>>>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>>>>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>>>>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>>>>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>>>>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>>>>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>>>>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>>>>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>>>>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>>>>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>>>>>>>> ```
>>>>>>>>>>
>>>>>>>>>> Thanks for input. I am concerned with your suggested approach (or using
>>>>>>>>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>>>>>>>>> properly specify compat when hyp=0. The risk here is we may generate the
>>>>>>>>>> wrong entry.
>>>>>>>>>>
>>>>>>>>>> compat should need to be specified when hyp=1 as we will always run in
>>>>>>>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>>>>>>>
>>>>>>>>> Since my suggestion was only to avoid the need for specifying a default
>>>>>>>>> for the parameter (which you didn't seem to be happy about), it would
>>>>>>>>> then merely extend to
>>>>>>>>>
>>>>>>>>> .if !0\hyp && 0\compat
>>>>>>>> Isn't it effectively the same as setting a default value?
>>>>>>>>
>>>>>>>> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>>>>>>>>
>>>>>>>> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>>>>>>>>
>>>>>>> So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
>>>>>>
>>>>>> Yes. This is the only way I could think to avoid making 'compat'optional.
>>>>>>
>>>>>>>> Cheers,
>>>>>>>>
>>>>>>>>>
>>>>>>>>> or something along those lines.
>>>>>>>>>
>>>>>>>>> Jan
>>>>>>>>>
>>>>>>>>
>>>>>>> So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
>>>>>>> stp wzr, wzr, [sp #8 * 0]
>>>>>>> stp wzr, wzr, [sp #8 * 1]
>>>>>>> ...
>>>>>>
>>>>>> I don't think you can use stp here because this would store two 32-bit values consecutively. Instead, you would need to use ldr to store one 32-bit value at the time.
>>>>>>
>>>>> I hope you meant str and not ldr.
>>>
>>> Yes. I am not sure why I wrote ldr.
>>>
>>>>> So a loop would look like that:
>>>>> str wzr, [sp, #8 * 1]
>>>>> str wzr, [sp, #8 * 3]
>>>>> ...
>>>>
>>>> Why "a loop" and why #8 (I'd have expected #4)?
>>>>
>>>> There's another oddity which I'm noticing only now, but which also
>>>> may look odd to me only because I lack sufficient Arm details: On
>>>> x86, it would not be advisable to store anything below the stack
>>>> pointer (like is done here when storing x0 and x1 early), unless
>>>> it's absolutely certain that no further interruptions could clobber
>>>> that part of the stack.
>>>
>>> We are entering the hypervisor with both Interrupts and SErrors masked. They will only be unmasked after the guest registers have been saved on the stack.
>>>
>>> You may still receive a Data Abort before the macro 'entry' has completed. But this is going to result to an hypervisor crash because they are not meant to happen in those paths.
>>>
>>> So I believe, we are safe to modify sp before.
>>
>> Hmmm... I meant to write on the stack before sp is modified.
>>
>> Cheers,
>>
>
> I would like to summarize what we discussed before pushing v2.
> Changes since v1:
> -update commit message adding information why do we need to zero top 32bits
> -zero corresponding stack slots instead of zeroing directly gp registers
> -create a macro called by entry, protected by if hyp=0. In macro add if compat=1
>
> Now when it comes to implementation.
>
> 1. Regarding save_x0_x1, it is 0 only for guest_sync_slowpath, which is called by guest_sync.
> So as we are dealing only with compat=1, save_x0_x1 cannot be 0.
> The conclusion is that we do not need to worry about it.
Oh, good point. I guess you may want to add a build time check to
avoid silently introducing a user of the macro violating that
assumption.
> 2. Regarding clearing high halves of stack slots.
I don't think I understood earlier responses that way. I think
fiddling with the stack was meant solely for x0 and x1 when they
were saved earlier on (i.e. instead of re-loading, zero-extending,
and then storing them back). That's also why ...
> The new macro (called zero_stack_top_halves) will be called in entry before the first instruction sub sp,sp.
> To avoid saving sp position/moving it, the simplest would be to execute 30 times:
> str wzr, [sp, #-(UREGS_kernel_sizeof - 4)]
> str wzr, [sp, #-(UREGS_kernel_sizeof - 12)]
> ...
> I could also use .irp loop like (.irp n,1,3,5,7,...) and then:
> str wzr, [sp, #-(UREGS_kernel_sizeof - (4 * n))]
> but FWIK Jan does not like loops :)
... in an earlier reply I expressed my surprise of you mentioning
loops - I simply didn't see how a loop would come into play when
dealing with just x0 and x1.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-15 9:35 ` Jan Beulich
@ 2021-12-15 9:48 ` Michal Orzel
2021-12-15 10:32 ` Jan Beulich
2021-12-15 18:20 ` Julien Grall
1 sibling, 1 reply; 26+ messages in thread
From: Michal Orzel @ 2021-12-15 9:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
xen-devel, Julien Grall
On 15.12.2021 10:35, Jan Beulich wrote:
> On 15.12.2021 10:27, Michal Orzel wrote:
>> Replying to both Julien and Jan,
>>
>> On 14.12.2021 12:30, Julien Grall wrote:
>>>
>>>
>>> On 14/12/2021 11:01, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> Replying in one e-mail the comments from Jan and Michal.
>>>>
>>>> On 14/12/2021 10:01, Jan Beulich wrote:
>>>>> On 14.12.2021 10:51, Michal Orzel wrote:
>>>>>> Hi Julien,
>>>>>>
>>>>>> On 14.12.2021 10:33, Julien Grall wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 14/12/2021 09:17, Michal Orzel wrote:
>>>>>>>> Hi Julien, Jan
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>> On 08.12.2021 10:55, Julien Grall wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 08/12/2021 07:20, Jan Beulich wrote:
>>>>>>>>>> On 07.12.2021 20:11, Julien Grall wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 07/12/2021 08:37, Michal Orzel wrote:
>>>>>>>>>>>> Hi Julien,
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>>> On 06.12.2021 16:29, Julien Grall wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 06/12/2021 14:20, Michal Orzel wrote:
>>>>>>>>>>>>>> to hypervisor when switching to AArch32 state.
>>>>>>>>>>>>>>
>>>>>>>>>>>> I will change to "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 IMPLEMENTATIONDEFINED"
>>>>>>>>>>>>>
>>>>>>>>>>>>> Typo: Missing space between IMPLEMENTATION and DEFINED.
>>>>>>>>>>>>>
>>>>>>>>>>>> Ok.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Currently Xen does not ensure that the top 32 bits are zeroed and this
>>>>>>>>>>>>>> needs to be fixed.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Can you outline why this is a problem and why we need to protect? IIRC, the main concern is Xen may misinterpret what the guest requested but we are not concerned about Xen using wrong value.
>>>>>>>>>>>>>
>>>>>>>>>>>> I would say:
>>>>>>>>>>>> "
>>>>>>>>>>>> The reason why this is a problem 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,
>>>>>>>>>>>> "
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Fix this bug by zeroing the upper 32 bits of these registers on an
>>>>>>>>>>>>>> entry to hypervisor when switching to AArch32 state.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Set default value of parameter compat of macro entry to 0 (AArch64 mode
>>>>>>>>>>>>>> as we are on 64-bit hypervisor) to avoid checking if parameter is blank
>>>>>>>>>>>>>> when not passed.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Which error do you see otherwise? Is it a compilation error?
>>>>>>>>>>>>>
>>>>>>>>>>>> Yes, this is a compilation error. The errors appear at each line when "entry" is called without passing value for "compat".
>>>>>>>>>>>> So basically in all the places where entry is called with hyp=1.
>>>>>>>>>>>> When taking the current patch and removing default value for compat you will get:
>>>>>>>>>>>> ```
>>>>>>>>>>>> entry.S:254: Error: ".endif" without ".if"
>>>>>>>>>>>> entry.S:258: Error: symbol `.if' is already defined
>>>>>>>>>>>> entry.S:258: Error: ".endif" without ".if"
>>>>>>>>>>>> entry.S:262: Error: symbol `.if' is already defined
>>>>>>>>>>>> entry.S:262: Error: ".endif" without ".if"
>>>>>>>>>>>> entry.S:266: Error: symbol `.if' is already defined
>>>>>>>>>>>> entry.S:266: Error: ".endif" without ".if"
>>>>>>>>>>>> entry.S:278: Error: symbol `.if' is already defined
>>>>>>>>>>>> entry.S:278: Error: ".endif" without ".if"
>>>>>>>>>>>> entry.S:292: Error: symbol `.if' is already defined
>>>>>>>>>>>> entry.S:292: Error: ".endif" without ".if"
>>>>>>>>>>>> entry.S:317: Error: symbol `.if' is already defined
>>>>>>>>>>>> entry.S:317: Error: ".endif" without ".if"
>>>>>>>>>>>> ```
>>>>>>>>>>>
>>>>>>>>>>> Thanks for input. I am concerned with your suggested approach (or using
>>>>>>>>>>> .if 0\compat as suggested by Jan) because they allow the caller to not
>>>>>>>>>>> properly specify compat when hyp=0. The risk here is we may generate the
>>>>>>>>>>> wrong entry.
>>>>>>>>>>>
>>>>>>>>>>> compat should need to be specified when hyp=1 as we will always run in
>>>>>>>>>>> aarch64 mode. So could we protect this code with hyp=0?
>>>>>>>>>>
>>>>>>>>>> Since my suggestion was only to avoid the need for specifying a default
>>>>>>>>>> for the parameter (which you didn't seem to be happy about), it would
>>>>>>>>>> then merely extend to
>>>>>>>>>>
>>>>>>>>>> .if !0\hyp && 0\compat
>>>>>>>>> Isn't it effectively the same as setting a default value?
>>>>>>>>>
>>>>>>>>> The reason we seem to get away is because other part of the macro (e.g. entry_guest) will need compat to be valid.
>>>>>>>>>
>>>>>>>>> But that seems pretty fragile to me. So I would prefer if the new code it added within a macro that is only called when hyp==0.
>>>>>>>>>
>>>>>>>> So you would like to have a macro that is called if hyp=0 (which means compat had to be passed) and inside this macro additional check if compat is 1?
>>>>>>>
>>>>>>> Yes. This is the only way I could think to avoid making 'compat'optional.
>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> or something along those lines.
>>>>>>>>>>
>>>>>>>>>> Jan
>>>>>>>>>>
>>>>>>>>>
>>>>>>>> So when it comes to zeroing the top 32bits by pushing zero to higher halves of stack slots I would do in a loop:
>>>>>>>> stp wzr, wzr, [sp #8 * 0]
>>>>>>>> stp wzr, wzr, [sp #8 * 1]
>>>>>>>> ...
>>>>>>>
>>>>>>> I don't think you can use stp here because this would store two 32-bit values consecutively. Instead, you would need to use ldr to store one 32-bit value at the time.
>>>>>>>
>>>>>> I hope you meant str and not ldr.
>>>>
>>>> Yes. I am not sure why I wrote ldr.
>>>>
>>>>>> So a loop would look like that:
>>>>>> str wzr, [sp, #8 * 1]
>>>>>> str wzr, [sp, #8 * 3]
>>>>>> ...
>>>>>
>>>>> Why "a loop" and why #8 (I'd have expected #4)?
>>>>>
>>>>> There's another oddity which I'm noticing only now, but which also
>>>>> may look odd to me only because I lack sufficient Arm details: On
>>>>> x86, it would not be advisable to store anything below the stack
>>>>> pointer (like is done here when storing x0 and x1 early), unless
>>>>> it's absolutely certain that no further interruptions could clobber
>>>>> that part of the stack.
>>>>
>>>> We are entering the hypervisor with both Interrupts and SErrors masked. They will only be unmasked after the guest registers have been saved on the stack.
>>>>
>>>> You may still receive a Data Abort before the macro 'entry' has completed. But this is going to result to an hypervisor crash because they are not meant to happen in those paths.
>>>>
>>>> So I believe, we are safe to modify sp before.
>>>
>>> Hmmm... I meant to write on the stack before sp is modified.
>>>
>>> Cheers,
>>>
>>
>> I would like to summarize what we discussed before pushing v2.
>> Changes since v1:
>> -update commit message adding information why do we need to zero top 32bits
>> -zero corresponding stack slots instead of zeroing directly gp registers
>> -create a macro called by entry, protected by if hyp=0. In macro add if compat=1
>>
>> Now when it comes to implementation.
>>
>> 1. Regarding save_x0_x1, it is 0 only for guest_sync_slowpath, which is called by guest_sync.
>> So as we are dealing only with compat=1, save_x0_x1 cannot be 0.
>> The conclusion is that we do not need to worry about it.
>
> Oh, good point. I guess you may want to add a build time check to
> avoid silently introducing a user of the macro violating that
> assumption.
>
>> 2. Regarding clearing high halves of stack slots.
>
> I don't think I understood earlier responses that way. I think
> fiddling with the stack was meant solely for x0 and x1 when they
> were saved earlier on (i.e. instead of re-loading, zero-extending,
> and then storing them back). That's also why ...
>
This patch and the problem it solves is about clearing top 32bits of all gp registers so not only x0,x1.
>> The new macro (called zero_stack_top_halves) will be called in entry before the first instruction sub sp,sp.
>> To avoid saving sp position/moving it, the simplest would be to execute 30 times:
>> str wzr, [sp, #-(UREGS_kernel_sizeof - 4)]
>> str wzr, [sp, #-(UREGS_kernel_sizeof - 12)]
>> ...
>> I could also use .irp loop like (.irp n,1,3,5,7,...) and then:
>> str wzr, [sp, #-(UREGS_kernel_sizeof - (4 * n))]
>> but FWIK Jan does not like loops :)
>
> ... in an earlier reply I expressed my surprise of you mentioning
> loops - I simply didn't see how a loop would come into play when
> dealing with just x0 and x1.
>
> Jan
>
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-15 9:48 ` Michal Orzel
@ 2021-12-15 10:32 ` Jan Beulich
2021-12-15 10:40 ` Michal Orzel
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2021-12-15 10:32 UTC (permalink / raw)
To: Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
xen-devel, Julien Grall
(Re-sending an abridged version, as apparently spam filters didn't like
the original message with more retained context; I'll have to see whether
this one also isn't liked. Sorry.)
On 15.12.2021 10:48, Michal Orzel wrote:
> This patch and the problem it solves is about clearing top 32bits of all gp registers so not only x0,x1.
That's well understood. Yet for everything still in registers simply
using mov ahead of the respective push (as you had it) is still
preferable imo.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-15 10:32 ` Jan Beulich
@ 2021-12-15 10:40 ` Michal Orzel
2021-12-15 18:25 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Michal Orzel @ 2021-12-15 10:40 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis,
xen-devel, Julien Grall
On 15.12.2021 11:32, Jan Beulich wrote:
> (Re-sending an abridged version, as apparently spam filters didn't like
> the original message with more retained context; I'll have to see whether
> this one also isn't liked. Sorry.)
>
> On 15.12.2021 10:48, Michal Orzel wrote:
>> This patch and the problem it solves is about clearing top 32bits of all gp registers so not only x0,x1.
>
> That's well understood. Yet for everything still in registers simply
> using mov ahead of the respective push (as you had it) is still
> preferable imo.
>
> Jan
>
In that case let's wait for Julien's opinion to decide whether I should get back to the previous
solution with mov or to the stack solution.
Cheers,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-15 10:40 ` Michal Orzel
@ 2021-12-15 18:25 ` Julien Grall
2021-12-16 7:14 ` Michal Orzel
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2021-12-15 18:25 UTC (permalink / raw)
To: Michal Orzel, Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Hi Michal,
On 15/12/2021 10:40, Michal Orzel wrote:
> On 15.12.2021 11:32, Jan Beulich wrote:
>> (Re-sending an abridged version, as apparently spam filters didn't like
>> the original message with more retained context; I'll have to see whether
>> this one also isn't liked. Sorry.)
>>
>> On 15.12.2021 10:48, Michal Orzel wrote:
>>> This patch and the problem it solves is about clearing top 32bits of all gp registers so not only x0,x1.
>>
>> That's well understood. Yet for everything still in registers simply
>> using mov ahead of the respective push (as you had it) is still
>> preferable imo.
>>
>> Jan
>>
>
> In that case let's wait for Julien's opinion to decide whether I should get back to the previous
> solution with mov or to the stack solution.
IIUC, your proposal is to:
1) Push all the 64-bit registers
2) Zero the top 32-bit
Jan's suggestion is to:
1) clobber the top 32-bit using mov wX, wX
2) Push all the registers
My preference is for the latter because there will be less memory/cache
access.
So, this would be your original patch + a compile time check to ensure
save_x0_x1 is 0 when compat=1.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-15 18:25 ` Julien Grall
@ 2021-12-16 7:14 ` Michal Orzel
0 siblings, 0 replies; 26+ messages in thread
From: Michal Orzel @ 2021-12-16 7:14 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Hi Julien,
On 15.12.2021 19:25, Julien Grall wrote:
> Hi Michal,
>
> On 15/12/2021 10:40, Michal Orzel wrote:
>> On 15.12.2021 11:32, Jan Beulich wrote:
>>> (Re-sending an abridged version, as apparently spam filters didn't like
>>> the original message with more retained context; I'll have to see whether
>>> this one also isn't liked. Sorry.)
>>>
>>> On 15.12.2021 10:48, Michal Orzel wrote:
>>>> This patch and the problem it solves is about clearing top 32bits of all gp registers so not only x0,x1.
>>>
>>> That's well understood. Yet for everything still in registers simply
>>> using mov ahead of the respective push (as you had it) is still
>>> preferable imo.
>>>
>>> Jan
>>>
>>
>> In that case let's wait for Julien's opinion to decide whether I should get back to the previous
>> solution with mov or to the stack solution.
>
> IIUC, your proposal is to:
> 1) Push all the 64-bit registers
> 2) Zero the top 32-bit
>
> Jan's suggestion is to:
> 1) clobber the top 32-bit using mov wX, wX
> 2) Push all the registers
>
> My preference is for the latter because there will be less memory/cache access.
>
> So, this would be your original patch + a compile time check to ensure save_x0_x1 is 0 when compat=1.
>
That is exactly what I would like to do. I will send this as v2.
> Cheers,
>
Cheers,
Michal
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] xen/arm64: Zero the top 32 bits of gp registers on entry...
2021-12-15 9:35 ` Jan Beulich
2021-12-15 9:48 ` Michal Orzel
@ 2021-12-15 18:20 ` Julien Grall
1 sibling, 0 replies; 26+ messages in thread
From: Julien Grall @ 2021-12-15 18:20 UTC (permalink / raw)
To: Jan Beulich, Michal Orzel
Cc: Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, xen-devel
Hi,
On 15/12/2021 09:35, Jan Beulich wrote:
>> 1. Regarding save_x0_x1, it is 0 only for guest_sync_slowpath, which is called by guest_sync.
>> So as we are dealing only with compat=1, save_x0_x1 cannot be 0.
>> The conclusion is that we do not need to worry about it.
>
> Oh, good point. I guess you may want to add a build time check to
> avoid silently introducing a user of the macro violating that
> assumption.
+1
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread