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

[Michal Orzel <michal.orzel@xxxxxxx>]

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