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

[Jan Beulich <jbeulich@xxxxxxxx>]

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