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

[Julien Grall <julien@xxxxxxx>]

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