[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.