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

[Michal Orzel <michal.orzel@xxxxxxx>]

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