Re: [Xen-devel] [PATCH RFC for-4.13 03/10] xen/arm: traps: Rework entry/exit from the guest path

[Julien Grall <julien.grall@xxxxxxx>]

On 27/09/2019 12:45, Volodymyr Babchuk wrote:
> Julien,
Hi...

> Julien Grall writes:
>
> > At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
> > used to deal with actions to be done before/after any guest request is
> > handled.
> >
> > While they are meant to work in pair, the former is called for most of
> > the traps, including traps from the same exception level (i.e.
> > hypervisor) whilst the latter will only be called when returning to the
> > guest.
> >
> > As pointed out, the enter_hypervisor_head() is not called from all the
> > traps, so this makes potentially difficult to extend it for the dealing
> > with same exception level.
> >
> > Furthermore, some assembly only path will require to call
> > enter_hypervisor_tail(). So the function is now directly call by
> > assembly in for guest vector only. This means that the check whether we
> > are called in a guest trap can now be removed.
> >
> > Take the opportunity to rename enter_hypervisor_tail() and
> > leave_hypervisor_tail() to something more meaningful and document them.
> > This should help everyone to understand the purpose of the two
> > functions.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> >
> > ---
> >
> > I haven't done the 32-bits part yet. I wanted to gather feedback before
> > looking in details how to integrate that with Arm32.
> I'm looking at patches one by one and it is looking okay so far.
>
>
> > ---
> >   xen/arch/arm/arm64/entry.S |  4 ++-
> >   xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
> >   2 files changed, 37 insertions(+), 38 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > index 40d9f3ec8c..9eafae516b 100644
> > --- a/xen/arch/arm/arm64/entry.S
> > +++ b/xen/arch/arm/arm64/entry.S
> > @@ -147,7 +147,7 @@
> >
> >           .if \hyp == 0         /* Guest mode */
> >
> > -        bl      leave_hypervisor_tail /* Disables interrupts on return */
> > +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
> >
> >           exit_guest \compat
> >
> > @@ -175,6 +175,8 @@
> >                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> >           msr     daifclr, \iflags
> >           mov     x0, sp
> Looks like this mov can be removed (see commend below).
>
> > +        bl      enter_hypervisor_from_guest
> > +        mov     x0, sp
> >           bl      do_trap_\trap
> >   1:
> >           exit    hyp=0, compat=\compat
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index a3b961bd06..20ba34ec91 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
> >                cpu_require_ssbd_mitigation();
> >   }
> >
> > -static void enter_hypervisor_head(struct cpu_user_regs *regs)
> > +/*
> > + * Actions that needs to be done after exiting the guest and before any
> > + * request from it is handled.
> Maybe it is me only, but the phrasing is confusing. I had to read it two
> times before I get it. What about "Actions that needs to be done when
> raising exception level"? Or maybe "Actions that needs to be done when
> switching from guest to hypervisor mode" ?
Is it a suggestion to replace the full sentence or just the first before (i.e. 
before 'and')?
> > + */
> > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> With the guest_mode(regs) check removal , this function does not use regs
> anymore.
I have nearly done it while working on the series, but then I thought that it 
would be better keep all the functions called from the entry path in assembly 
similar.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel