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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Julien,

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" ?

> + */
> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
With the guest_mode(regs) check removal , this function does not use regs
anymore.

>  {
> -    if ( guest_mode(regs) )
> -    {
> -        struct vcpu *v = current;
> +    struct vcpu *v = current;
>
> -        /* If the guest has disabled the workaround, bring it back on. */
> -        if ( needs_ssbd_flip(v) )
> -            arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
> +    /* If the guest has disabled the workaround, bring it back on. */
> +    if ( needs_ssbd_flip(v) )
> +        arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
>
> -        /*
> -         * If we pended a virtual abort, preserve it until it gets cleared.
> -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> -         * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> -         * (alias of HCR.VA) is cleared to 0."
> -         */
> -        if ( v->arch.hcr_el2 & HCR_VA )
> -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> +    /*
> +     * If we pended a virtual abort, preserve it until it gets cleared.
> +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> +     * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> +     * (alias of HCR.VA) is cleared to 0."
> +     */
> +    if ( v->arch.hcr_el2 & HCR_VA )
> +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>
>  #ifdef CONFIG_NEW_VGIC
> -        /*
> -         * We need to update the state of our emulated devices using level
> -         * triggered interrupts before syncing back the VGIC state.
> -         *
> -         * TODO: Investigate whether this is necessary to do on every
> -         * trap and how it can be optimised.
> -         */
> -        vtimer_update_irqs(v);
> -        vcpu_update_evtchn_irq(v);
> +    /*
> +     * We need to update the state of our emulated devices using level
> +     * triggered interrupts before syncing back the VGIC state.
> +     *
> +     * TODO: Investigate whether this is necessary to do on every
> +     * trap and how it can be optimised.
> +     */
> +    vtimer_update_irqs(v);
> +    vcpu_update_evtchn_irq(v);
>  #endif
>
> -        vgic_sync_from_lrs(v);
> -    }
> +    vgic_sync_from_lrs(v);
>  }
>
>  void do_trap_guest_sync(struct cpu_user_regs *regs)
>  {
>      const union hsr hsr = { .bits = regs->hsr };
>
> -    enter_hypervisor_head(regs);
> -
>      switch ( hsr.ec )
>      {
>      case HSR_EC_WFI_WFE:
> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>  {
>      const union hsr hsr = { .bits = regs->hsr };
>
> -    enter_hypervisor_head(regs);
> -
>      switch ( hsr.ec )
>      {
>  #ifdef CONFIG_ARM_64
> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>
>  void do_trap_hyp_serror(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
> -
>      __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
>  }
>
>  void do_trap_guest_serror(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
> -
>      __do_trap_serror(regs, true);
>  }
>
>  void do_trap_irq(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
>      gic_interrupt(regs, 0);
>  }
>
>  void do_trap_fiq(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
>      gic_interrupt(regs, 1);
>  }
>
> @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void)
>      local_irq_disable();
>  }
>
> -void leave_hypervisor_tail(void)
> +/*
> + * Actions that needs to be done before entering the guest. This is the
> + * last thing executed before the guest context is fully restored.
> + *
> + * The function will return with interrupts disabled.
> + */
> +void leave_hypervisor_to_guest(void)
>  {
>      local_irq_disable();


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