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

Re: [Xen-devel] [PATCH v5] xen/arm: trap guest WFI



On Fri, 2013-04-05 at 14:19 +0100, Stefano Stabellini wrote:
> Trap guest WFI, block the guest VCPU unless it has pending interrupts.
> Awake the guest vcpu when a new interrupt for it arrrives.

"arrives"

> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> 
> Changes in v5:

it'd be useful to incorporate some of the details from these updates
into the changelog proper to describe the resulting solution, since some
of the specifics are a bit subtle.

> - implement local_event_delivery_disable/enable;
> - introduce gic_events_need_delivery, that takes into account the
> current status of the lr registers;
> - check whether evtchn_upcall_mask is set in
> local_events_need_delivery;
> - local_events_need_delivery: return false if the guest disabled irqs;
> - on guest WFI we should not block the guest vcpu if any interrupts need
> to be injected, even if the guest disabled interrupts.
> 
> Changes in v4:
> - local_events_need_delivery: no need to return true if
> evtchn_upcall_pending is set and the VGIC_IRQ_EVTCHN_CALLBACK irq is
> currently inflight: it just means that events are being handled.
> 
> Changes in v3:
> - check on the lr_pending list rather the inflight list in
> local_events_need_delivery;
> - if evtchn_upcall_pending also check the status of the
> VGIC_IRQ_EVTCHN_CALLBACK irq.
> 
> Changes in v2:
> - rebased;
> - implement local_events_need_delivery;
> - move the case HSR_EC_WFI_WFE to do_trap_hypervisor.
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e9c84c7..e2a072b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -457,7 +457,7 @@ int construct_dom0(struct domain *d)
>  
>      v->arch.sctlr = SCTLR_BASE;
>  
> -    WRITE_SYSREG(HCR_PTW|HCR_BSU_OUTER|HCR_AMO|HCR_IMO|HCR_VM, HCR_EL2);
> +    WRITE_SYSREG(HCR_PTW|HCR_BSU_OUTER|HCR_AMO|HCR_IMO|HCR_VM|HCR_TWI, 
> HCR_EL2);
>      isb();
>  
>      local_abort_enable();
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 41abdfb..ec828c8 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -544,9 +544,15 @@ static void gic_inject_irq_stop(void)
>      }
>  }
>  
> +int gic_events_need_delivery(void)
> +{
> +    return (!list_empty(&current->arch.vgic.lr_pending) ||
> +            this_cpu(lr_mask));
> +}
> +
>  void gic_inject(void)
>  {
> -    if (!this_cpu(lr_mask))
> +    if (!gic_events_need_delivery())
>          gic_inject_irq_stop();
>      else
>          gic_inject_irq_start();
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 600113c..af165ca 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -29,7 +29,9 @@
>  #include <xen/hypercall.h>
>  #include <xen/softirq.h>
>  #include <xen/domain_page.h>
> +#include <public/sched.h>
>  #include <public/xen.h>
> +#include <asm/event.h>
>  #include <asm/regs.h>
>  #include <asm/cpregs.h>
>  
> @@ -920,6 +922,19 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs 
> *regs)
>      union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
>  
>      switch (hsr.ec) {
> +    /* at the moment we only trap WFI */

Putting this after the case ...: would be more logical I think.

> +    case HSR_EC_WFI_WFE:
> +        do_sched_op_compat(SCHEDOP_block, 0);

I think it would be better to export xen/common/schedule.c:do_block
(perhaps as vcpu_block to match the unblock) rather than "abusing" the
hypercall entry points like this.

> +        /* The ARM spec declares that even if local irqs are masked in
> +         * the CPSR register, an irq should wake up a cpu from WFI anyway.
> +         * For this reason we need to check for irqs that need delivery,
> +         * ignoring the CPSR register, *after* calling SCHEDOP_block to
> +         * avoid races with vgic_vcpu_inject_irq.
> +         */
> +        if ( _local_events_need_delivery(0) )
> +            vcpu_unblock(current);
> +        regs->pc += hsr.len ? 4 : 2;
> +        break;
>      case HSR_EC_CP15_32:
>          if ( ! is_pv32_domain(current->domain) )
>              goto bad_trap;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 0d24df0..8efcefc 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -608,12 +608,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
> irq, int virtual)
>          {
>              list_add_tail(&n->inflight, &iter->inflight);
>              spin_unlock_irqrestore(&v->arch.vgic.lock, flags);

For symmetry of lock/unlock I'd prefer to remove this unlock and move
the out label up before the other unlock.

> -            return;
> +            goto out;
>          }
>      }
>      list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>      /* we have a new higher priority irq, inject it into the guest */
> +out:
> +    vcpu_unblock(v);
>  }
>  
>  /*
> diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
> index 10f58af..99a2b13 100644
> --- a/xen/include/asm-arm/event.h
> +++ b/xen/include/asm-arm/event.h
> @@ -1,27 +1,55 @@
>  #ifndef __ASM_EVENT_H__
>  #define __ASM_EVENT_H__
>  
> +#include <asm/gic.h>
> +#include <asm/domain.h>
> +
>  void vcpu_kick(struct vcpu *v);
>  void vcpu_mark_events_pending(struct vcpu *v);
>  
> -static inline int local_events_need_delivery(void)
> +static inline int _local_events_need_delivery(int check_masked)
>  {
> -    /* TODO
> -     * return (vcpu_info(v, evtchn_upcall_pending) &&
> -                        !vcpu_info(v, evtchn_upcall_mask)); */
> +    struct pending_irq *p = irq_to_pending(current, 
> VGIC_IRQ_EVTCHN_CALLBACK);
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +
> +    /* guest IRQs are masked */
> +    if ( check_masked && (regs->cpsr & PSR_IRQ_MASK) )
>          return 0;
> +
> +    /* XXX: if the first interrupt has already been delivered, we should

XXX usually means "TODO", whereas this is just a normal comment I think?

> +     * check whether any higher priority interrupts are in the
> +     * lr_pending queue or in the LR registers and return 1 only in that
> +     * case.
> +     * In practice the guest interrupt handler should run with
> +     * interrupts disabled so this shouldn't be a problem in the general
> +     * case.
> +     */
> +    if ( gic_events_need_delivery() )
> +        return 1;
> +
> +    if ( vcpu_info(current, evtchn_upcall_pending) &&
> +        !vcpu_info(current, evtchn_upcall_mask) &&
> +        list_empty(&p->inflight) )
> +        return 1;
> +
> +    return 0;
> +}
> +
> +static inline int local_events_need_delivery(void)
> +{
> +    return _local_events_need_delivery(1);
>  }
>  
>  int local_event_delivery_is_enabled(void);
>  
>  static inline void local_event_delivery_disable(void)
>  {
> -    /* TODO current->vcpu_info->evtchn_upcall_mask = 1; */
> +    current->vcpu_info->evtchn_upcall_mask = 1;

You use vcpu_info in the above, I think you should use it here too for
consistency.

However a bigger concern is that Xen on ARM doesn't really use
evtchn_upcall_mask on the guest side, so I'm not sure the hypervisor
should use it internally either.

I don't see any callers of this disable function outside of arch/x86.

>  }
>  
>  static inline void local_event_delivery_enable(void)

This one is called from do_block in common code but if the guest never
sets this then it seems a bit pointless. Perhaps this should be
manipulating CPSR_I instead?

>  {
> -    /* TODO current->vcpu_info->evtchn_upcall_mask = 0; */
> +    current->vcpu_info->evtchn_upcall_mask = 0;
>  }
>  
>  /* No arch specific virq definition now. Default to global. */
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index c6535d1..c3e0229 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -135,6 +135,7 @@ extern void gic_route_ppis(void);
>  extern void gic_route_spis(void);
>  
>  extern void gic_inject(void);
> +extern int gic_events_need_delivery(void);
>  
>  extern void __cpuinit init_maintenance_interrupt(void);
>  extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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