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

[Julien Grall <julien@xxxxxxx>]

Hi Michal,

On 17/12/2021 11:52, Michal Orzel wrote:
> On 17.12.2021 11:01, Julien Grall wrote:
> > On 17/12/2021 07:21, Michal Orzel wrote:
> > > to hypervisor when switching 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 IMPLEMENTATION DEFINED"
> > >
> > > Currently Xen does not ensure that the top 32 bits are zeroed and this
> > > needs to be fixed. The reason why 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.
> > >
> > > Create a macro clobber_gp_top_halves to clobber top 32 bits of gp
> > > registers when hyp == 0 (guest mode) and compat == 1 (AArch32 mode).
> > > Add a compile time check to ensure that save_x0_x1 == 1 if
> > > compat == 1.
> > >
> > > Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> > > ---
> > > Changes since v2:
> > > -add clobbering of w30
> > > Changes since v1:
> > > -put new code into macro
> > > -add compile time check for save_x0_x1
> > > ---
> > >    xen/arch/arm/arm64/entry.S | 29 +++++++++++++++++++++++++++++
> > >    1 file changed, 29 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > > index fc3811ad0a..e351ef8639 100644
> > > --- a/xen/arch/arm/arm64/entry.S
> > > +++ b/xen/arch/arm/arm64/entry.S
> > > @@ -102,6 +102,30 @@
> > >            .endif
> > >              .endm
> > > +
> > > +/*
> > > + * Clobber top 32 bits of gp registers when switching from AArch32
> > > + */
> > > +        .macro clobber_gp_top_halves, compat, save_x0_x1
> > > +
> > > +        .if \compat == 1      /* AArch32 mode */
> > > +
> > > +        /*
> > > +         * save_x0_x1 is equal to 0 only for guest_sync (compat == 0).
> > > +         * Add a compile time check to avoid violating this rule.
> > > +         */
> > We may want in the future to allow save_x0_x1 == 1 with compat == 1 if we need 
> > to create fastpath like we did when entering AArch64.
> >
> > So I would reword the comment to make clear this is an implementation decision. 
> > How about:
> >
> > "At the moment, no-one is using save_x0_x1 == 0 with compat == 1. So the code is not 
> > handling it to simplify the implementation."
> >
> > If you are happy with the new comment, I can update it on commit:
> Please do. The comment looks ok.
Done. It is now committed.

Thanks for the fix!

Cheers,

--
Julien Grall