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

[Michal Orzel <michal.orzel@xxxxxxx>]

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