[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Julien Grall <julien.grall@xxxxxxx>
  • From: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Date: Fri, 27 Sep 2019 11:45:08 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=yaNeqG76s962cna1sObEXrq+6pdi/TkkjDGtzv7VM5Y=; b=enzxgjEG75s+6beMmS8bMPtip70DNd+v+p/qTeu85IWbWWI2qBeZukeNQcaY+jqOeZ3J1Kg0pR0GZicx+H/7WM2UUJ0h47G29h5XLr7RcjJ05MNMFi6WgMiaw8s6nCNnRHyurJsErqjl5sPmCSVvDZeY8T/T0ZqRP5pcBrlyf3G7jcBZGCpXGc4A+mHDlN1OBbAdTgvKN+cMsDa9c6mvpBNlrEqztQRDhm4zqa504EuO1SDjPiTw32F1eHU2bl5sHpBquuu/f2PchPtgEIZfNwZ0XC7nAum7D70O1p7d9Ls9/+CpsgXoy36dOMSqb9AQYhA+NoR9+1wsakiFErrD0Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=BqyxEFJorRKf7DhquQS+OwPqhe9TUAXiBQbqBnGMF4+zIldiVSy9CGaK3VLPVrf1oRl/aNOURwuq829x0rZu4A7et/0zxY8yRwLDxzo2ejNfi4tsGYyDMeeYOOwaBlD0LqI9RQNhmyqZMTg2kI6szEcP+40Jf1g1CE54rHNnG+MTdCaac7SWtmu15AVjP2mtXm2g10SA6VktasiweNyt48JScOMVBYdTfse/jzdLUTivOF4uQLDevvvoG140f8ZvSqiTM7zwCZt2RT+JJPr54Uqtr617fq4WNHSTqys94+6faxnVM+BIuwZ1QPL1QLCEu5AV53Lx6TIimtdOTGhN2w==
  • Authentication-results: spf=none (sender IP is ) smtp.mailfrom=Volodymyr_Babchuk@xxxxxxxx;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, "andrii.anisov@xxxxxxxxx" <andrii.anisov@xxxxxxxxx>
  • Delivery-date: Fri, 27 Sep 2019 11:45:13 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHVdJmSYNSsXwZ/UE+SD815sb8MBKc/aNGA
  • Thread-topic: [PATCH RFC for-4.13 03/10] xen/arm: traps: Rework entry/exit from the guest path

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.