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

[Julien Grall <julien@xxxxxxx>]

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?
Cheers,

--
Julien Grall