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

[Julien Grall <julien@xxxxxxx>]

On 14/12/2021 11:01, Julien Grall wrote:
> Hi,
>
> Replying in one e-mail the comments from Jan and Michal.
>
> On 14/12/2021 10:01, Jan Beulich wrote:
> > On 14.12.2021 10:51, Michal Orzel wrote:
> > > 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.
> Yes. I am not sure why I wrote ldr.
>
> > > So a loop would look like that:
> > > str wzr, [sp, #8 * 1]
> > > str wzr, [sp, #8 * 3]
> > > ...
> > Why "a loop" and why #8 (I'd have expected #4)?
> >
> > There's another oddity which I'm noticing only now, but which also
> > may look odd to me only because I lack sufficient Arm details: On
> > x86, it would not be advisable to store anything below the stack
> > pointer (like is done here when storing x0 and x1 early), unless
> > it's absolutely certain that no further interruptions could clobber
> > that part of the stack.
> We are entering the hypervisor with both Interrupts and SErrors masked. 
> They will only be unmasked after the guest registers have been saved on 
> the stack.
> You may still receive a Data Abort before the macro 'entry' has 
> completed. But this is going to result to an hypervisor crash because 
> they are not meant to happen in those paths.
> So I believe, we are safe to modify sp before.
Hmmm... I meant to write on the stack before sp is modified.

Cheers,

--
Julien Grall