Trap guest WFI, block the guest VCPU unless it has pending interrupts.
Awake the guest vcpu when a new interrupt for it arrrives.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Changes in v5:
- 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(¤t->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 */
+ case HSR_EC_WFI_WFE:
+ do_sched_op_compat(SCHEDOP_block, 0);
+ /* 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);
- 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
+ * 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;
}
static inline void local_event_delivery_enable(void)
{
- /* 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,
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@eu.citrix.com> > > > 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(¤t->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 shouldXXX 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,
On Wed, 10 Apr 2013, Ian Campbell wrote:> > 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?It is something that we should implement at some point, that''s why it is marked as XXX or TODO.> > + * 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.OK.> 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.Good point.> I don''t see any callers of this disable function outside of arch/x86.I should just remove the function then.> > } > > > > 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?I think it''s fine to keep it as it is because in the future a guest on ARM might decide to use it and I think it''s an interface that we should support. Clearing CPSR_I might be the right thing to do in response to a SCHEDOP_block hypercall, but it is not something we can do in the WFI trap. Given that the SCHEDOP_block hypercall is never called and there is no reason to call it thanks to the existance of WFI, I say we should keep local_event_delivery_enable as it is in this patch. Also PV on HVM x86 guests have the same issue and at the moment they don''t clear the interrupt flag in local_event_delivery_enable either. Maybe we should find a way to return an error if a guest calls SCHEDOP_block on ARM?
On Thu, 18 Apr 2013, Stefano Stabellini wrote:> On Wed, 10 Apr 2013, Ian Campbell wrote: > > > 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? > > It is something that we should implement at some point, that''s why it is > marked as XXX or TODO. > > > > > + * 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. > > OK. > > > > 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. > > Good point. > > > > I don''t see any callers of this disable function outside of arch/x86. > > I should just remove the function then. > > > > > } > > > > > > 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? > > I think it''s fine to keep it as it is because in the future a guest on > ARM might decide to use it and I think it''s an interface that we should > support.Actually a better way to put it is that evtchn_upcall_mask is already part of an interface that we support.
On Thu, 2013-04-18 at 18:59 +0100, Stefano Stabellini wrote:> On Wed, 10 Apr 2013, Ian Campbell wrote: > > > 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? > > It is something that we should implement at some point, that''s why it is > marked as XXX or TODO.Oh, I read this as describing the reason for the following gic_events_need_delivery, not describing something which is missing there.> > > } > > > > > > 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? > > I think it''s fine to keep it as it is because in the future a guest on > ARM might decide to use it and I think it''s an interface that we should > support.No it isn''t, it is a piece of PV legacy which introduces difficult to solve races (all that critical section stuff) on the event re-enable path. There is no reason at all to use it when you have proper virtualised interrupt mask functionality in the hardware.> Clearing CPSR_I might be the right thing to do in response to a > SCHEDOP_block hypercall, but it is not something we can do in the WFI > trap.That''s fine, we can make both behave in an appropriate manner, assuming we need to keep both at all.> Given that the SCHEDOP_block hypercall is never called and there is no > reason to call it thanks to the existance of WFI, I say we should keep > local_event_delivery_enable as it is in this patch.It may well be appropriate for local_event_delivery_enable to do nothing or to clear the interrupt flag, that is something we can decide, and you make a good argument that in the WFI case it should do nothing. I expect the right answer is that your new do_block function should only optionally reenable interrupts, the SCHEDOP_block and WFI can both have their correct semantics. However it is completely clear to me is that this function shouldn''t be touching the s/w event channel mask on ARM because it conceptually simply does not exist.> Also PV on HVM x86 guests have the same issue and at the moment they > don''t clear the interrupt flag in local_event_delivery_enable either.PV on HVM x86 doesn''t call SCHEDOP_block because it doesn''t use the PV IRQ ops. I expect it will use the hlt instruction instead. evtchn_upcall_mask is also meaningless and unused for such guests as well. So SCHEDOP_block is probably equivalently buggy on x86 PVHVM, but can be forgiven somewhat because at least the bit has meaning in some x86 domain modes.> Maybe we should find a way to return an error if a guest calls > SCHEDOP_block on ARM?That would work also. Ian.
On Thu, 2013-04-18 at 19:03 +0100, Stefano Stabellini wrote:> > I think it''s fine to keep it as it is because in the future a guest on > > ARM might decide to use it and I think it''s an interface that we should > > support. > > Actually a better way to put it is that evtchn_upcall_mask is already > part of an interface that we support.No, it isn''t. Ian.
On Fri, 19 Apr 2013, Ian Campbell wrote:> > > > } > > > > > > > > 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? > > > > I think it''s fine to keep it as it is because in the future a guest on > > ARM might decide to use it and I think it''s an interface that we should > > support. > > No it isn''t, it is a piece of PV legacy which introduces difficult to > solve races (all that critical section stuff) on the event re-enable > path. There is no reason at all to use it when you have proper > virtualised interrupt mask functionality in the hardware.evtchn_upcall_mask is part of struct vcpu_info that is defined in xen/include/public/xen.h, that is an arch-independent public header. So yes, it is part of the interface that we support. I fail to see an easy way to remove it from it, we can''t just add a comment say "please don''t use it". I wouldn''t be happy with ifdef''ing it out because I would rather keep this interface arch-independent. However I do see an easy way to support it on ARM.
On Fri, 2013-04-19 at 11:01 +0100, Stefano Stabellini wrote:> On Fri, 19 Apr 2013, Ian Campbell wrote: > > > > > } > > > > > > > > > > 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? > > > > > > I think it''s fine to keep it as it is because in the future a guest on > > > ARM might decide to use it and I think it''s an interface that we should > > > support. > > > > No it isn''t, it is a piece of PV legacy which introduces difficult to > > solve races (all that critical section stuff) on the event re-enable > > path. There is no reason at all to use it when you have proper > > virtualised interrupt mask functionality in the hardware. > > evtchn_upcall_mask is part of struct vcpu_info that is defined in > xen/include/public/xen.h, that is an arch-independent public header. > > So yes, it is part of the interface that we support.There is all sorts of stuff under xen/include/public which is not part of the interface on ARM, PVMMU, start_info, etc. You wouldn''t say we support those, would you?> I fail to see an easy way to remove it from it, we can''t just add a > comment say "please don''t use it".We should take the same approach as with the startinfo, see <1363178714-5085-1-git-send-email-ian.campbell@citrix.com> which is acked but not yet applied. which is to add a sementic (not arch specific) define which the archs can opt in or out of.> I wouldn''t be happy with ifdef''ing it out because I would rather keep > this interface arch-independent.Right, this shouldn''t be an ifdef __arm__ or anything like that. It should be ifdef XEN_HAVE_PV_INTERRUPT_MASK or some name along those lines.> However I do see an easy way to support it on ARM.I don''t think it is actually usable at all even with this patch. But more importantly the interface is redundant, worse than the other alternative and actually implementing support for it in the guest is complex and error prone. Ian.
On Fri, 19 Apr 2013, Ian Campbell wrote:> On Fri, 2013-04-19 at 11:01 +0100, Stefano Stabellini wrote: > > On Fri, 19 Apr 2013, Ian Campbell wrote: > > > > > > } > > > > > > > > > > > > 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? > > > > > > > > I think it''s fine to keep it as it is because in the future a guest on > > > > ARM might decide to use it and I think it''s an interface that we should > > > > support. > > > > > > No it isn''t, it is a piece of PV legacy which introduces difficult to > > > solve races (all that critical section stuff) on the event re-enable > > > path. There is no reason at all to use it when you have proper > > > virtualised interrupt mask functionality in the hardware. > > > > evtchn_upcall_mask is part of struct vcpu_info that is defined in > > xen/include/public/xen.h, that is an arch-independent public header. > > > > So yes, it is part of the interface that we support. > > There is all sorts of stuff under xen/include/public which is not part > of the interface on ARM, PVMMU, start_info, etc. You wouldn''t say we > support those, would you?PVMMU hypercalls fail if they are called so they are not a problem; start_info is just an initial set of information and there is no way to map it in the guest so it is also not an issue from my POV. A single field of a supported struct is another matter though. There are books out there that describe that interface, I would rather not change it.> > I fail to see an easy way to remove it from it, we can''t just add a > > comment say "please don''t use it". > > We should take the same approach as with the startinfo, see > <1363178714-5085-1-git-send-email-ian.campbell@citrix.com> which is > acked but not yet applied. which is to add a sementic (not arch > specific) define which the archs can opt in or out of. > > > I wouldn''t be happy with ifdef''ing it out because I would rather keep > > this interface arch-independent. > > Right, this shouldn''t be an ifdef __arm__ or anything like that. It > should be ifdef XEN_HAVE_PV_INTERRUPT_MASK or some name along those > lines.It is still an ifdef.> > However I do see an easy way to support it on ARM. > > I don''t think it is actually usable at all even with this patch. But > more importantly the interface is redundant, worse than the other > alternative and actually implementing support for it in the guest is > complex and error prone.Yes, it is redundant but we are not asking the guest to implement it. We only need to provide support for it in Xen, and we have plenty of examples from Xen x86.
On Fri, 2013-04-19 at 14:03 +0100, Stefano Stabellini wrote:> On Fri, 19 Apr 2013, Ian Campbell wrote: > > On Fri, 2013-04-19 at 11:01 +0100, Stefano Stabellini wrote: > > > On Fri, 19 Apr 2013, Ian Campbell wrote: > > > > > > > } > > > > > > > > > > > > > > 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? > > > > > > > > > > I think it''s fine to keep it as it is because in the future a guest on > > > > > ARM might decide to use it and I think it''s an interface that we should > > > > > support. > > > > > > > > No it isn''t, it is a piece of PV legacy which introduces difficult to > > > > solve races (all that critical section stuff) on the event re-enable > > > > path. There is no reason at all to use it when you have proper > > > > virtualised interrupt mask functionality in the hardware. > > > > > > evtchn_upcall_mask is part of struct vcpu_info that is defined in > > > xen/include/public/xen.h, that is an arch-independent public header. > > > > > > So yes, it is part of the interface that we support. > > > > There is all sorts of stuff under xen/include/public which is not part > > of the interface on ARM, PVMMU, start_info, etc. You wouldn''t say we > > support those, would you? > > PVMMU hypercalls fail if they are called so they are not a problem; > start_info is just an initial set of information and there is no way to > map it in the guest so it is also not an issue from my POV. > > A single field of a supported struct is another matter though. > > There are books out there that describe that interface, I would rather > not change it. > > > > > I fail to see an easy way to remove it from it, we can''t just add a > > > comment say "please don''t use it". > > > > We should take the same approach as with the startinfo, see > > <1363178714-5085-1-git-send-email-ian.campbell@citrix.com> which is > > acked but not yet applied. which is to add a sementic (not arch > > specific) define which the archs can opt in or out of. > > > > > I wouldn''t be happy with ifdef''ing it out because I would rather keep > > > this interface arch-independent. > > > > Right, this shouldn''t be an ifdef __arm__ or anything like that. It > > should be ifdef XEN_HAVE_PV_INTERRUPT_MASK or some name along those > > lines. > > It is still an ifdef.That''s fine, and not just fine, it is the correct thing to do.> > > However I do see an easy way to support it on ARM. > > > > I don''t think it is actually usable at all even with this patch. But > > more importantly the interface is redundant, worse than the other > > alternative and actually implementing support for it in the guest is > > complex and error prone. > > Yes, it is redundant but we are not asking the guest to implement it.So you want to implement support for something which you don''t believe the guest will ever need? Why? Especially when the guest side software implementation of interrupt masking is known to have short comings compared with the excellent hardware support which the ARM platform gives us. We shouldn''t do things just because they are in theory possible. You don''t have any actual use case for this functionality. It is a waste of time and effort to implement it *and* it is needlessly introducing yet another interface to support. For absolutely no gain whatsoever.> We only need to provide support for it in Xen, and we have plenty of > examples from Xen x86.Ian.
On Fri, 19 Apr 2013, Ian Campbell wrote:> On Fri, 2013-04-19 at 14:03 +0100, Stefano Stabellini wrote: > > On Fri, 19 Apr 2013, Ian Campbell wrote: > > > On Fri, 2013-04-19 at 11:01 +0100, Stefano Stabellini wrote: > > > > On Fri, 19 Apr 2013, Ian Campbell wrote: > > > > > > > > } > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > > I think it''s fine to keep it as it is because in the future a guest on > > > > > > ARM might decide to use it and I think it''s an interface that we should > > > > > > support. > > > > > > > > > > No it isn''t, it is a piece of PV legacy which introduces difficult to > > > > > solve races (all that critical section stuff) on the event re-enable > > > > > path. There is no reason at all to use it when you have proper > > > > > virtualised interrupt mask functionality in the hardware. > > > > > > > > evtchn_upcall_mask is part of struct vcpu_info that is defined in > > > > xen/include/public/xen.h, that is an arch-independent public header. > > > > > > > > So yes, it is part of the interface that we support. > > > > > > There is all sorts of stuff under xen/include/public which is not part > > > of the interface on ARM, PVMMU, start_info, etc. You wouldn''t say we > > > support those, would you? > > > > PVMMU hypercalls fail if they are called so they are not a problem; > > start_info is just an initial set of information and there is no way to > > map it in the guest so it is also not an issue from my POV. > > > > A single field of a supported struct is another matter though. > > > > There are books out there that describe that interface, I would rather > > not change it. > > > > > > > > I fail to see an easy way to remove it from it, we can''t just add a > > > > comment say "please don''t use it". > > > > > > We should take the same approach as with the startinfo, see > > > <1363178714-5085-1-git-send-email-ian.campbell@citrix.com> which is > > > acked but not yet applied. which is to add a sementic (not arch > > > specific) define which the archs can opt in or out of. > > > > > > > I wouldn''t be happy with ifdef''ing it out because I would rather keep > > > > this interface arch-independent. > > > > > > Right, this shouldn''t be an ifdef __arm__ or anything like that. It > > > should be ifdef XEN_HAVE_PV_INTERRUPT_MASK or some name along those > > > lines. > > > > It is still an ifdef. > > That''s fine, and not just fine, it is the correct thing to do. > > > > > However I do see an easy way to support it on ARM. > > > > > > I don''t think it is actually usable at all even with this patch. But > > > more importantly the interface is redundant, worse than the other > > > alternative and actually implementing support for it in the guest is > > > complex and error prone. > > > > Yes, it is redundant but we are not asking the guest to implement it. > > So you want to implement support for something which you don''t believe > the guest will ever need? Why? > > Especially when the guest side software implementation of interrupt > masking is known to have short comings compared with the excellent > hardware support which the ARM platform gives us. > > We shouldn''t do things just because they are in theory possible. You > don''t have any actual use case for this functionality. > > It is a waste of time and effort to implement it *and* it is needlessly > introducing yet another interface to support. For absolutely no gain > whatsoever.Given that getting rid of evtchn_upcall_mask should not be part of the WFI patch anyway, I am just going to ignore it for now.