|
[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
On Tue, 1 Oct 2019, Stefano Stabellini wrote:
> On Tue, 1 Oct 2019, Julien Grall wrote:
> > Hi,
> >
> > On 01/10/2019 21:12, Stefano Stabellini wrote:
> > > On Thu, 26 Sep 2019, Julien Grall wrote:
> > >> 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.
> > >> ---
> > >> 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
> > >> + 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.
> > >> + */
> > >> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> > >> {
> > >> - 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);
> > >> }
> > >
> > > I am OK with the general approach but one thing to note is that the fiq
> > > handler doesn't use the guest_vector macro at the moment.
> >
> > do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode().
> > So I don't see an issue here.
> >
> > As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler
> > does not use guest_vector.
> >
> > That said, it is interesting to see that we don't deal the same way the
> > FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the
> > latter will crash the guest.
> >
> > It would be good if we can have the same behavior accross the two arch
> > if possible. I vaguely recall someone (Andre?) mentioning some changes
> > around FIQ in KVM recently. Andre, are FIQ meant to work with Guest?
> >
> > Also, a side effect of not calling enter_hypervisor_head() is workaround
> > are not re-enabled. We are going to panic soon after, so it may not be
> > that much an issue.
>
> Right, that is what I was thinking too, but I wanted to highlight it. At
> least it would be worth adding a sentence to the commit message about
> it.
Actually on second thought, maybe we have to apply the workaround anyway
to identify/spot that the guest somehow managed to trigger a serror? I
mean: maybe it is important enough that we should let the user know.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |