Re: [Xen-devel] ARM Generic Timer interrupt

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 26 May 2014, Oleksandr Tyshchenko wrote:
> Hello, all.
> 
> I am trying to run QNX as domU under XEN (4.4.0) on OMAP5 platform (ARM32).
> For this purposes I have made some changes to origin TI OMAP5 BSP.
> Also I have created QNX IFS loader for Xen domain builder.
> 
> Currently, dom0 (Linux Kernel) loads QNX as domU. The QNX boots without 
> crashes,
> and I have console (I left only one hw block in QNX - UART,
> I need it for debug output while HVC is not implemented).
> 
> During bringing up I have encountered with next problem.
> 1. QNX (our BSP) uses ARM Generic Timer as system timer:
> QNX doesn't mask/unmask timer interrupt. Of course the QNX doesn't know that 
> interrupt mask may be set by someone else
> and that it should be reset for timer interrupt to occur again.
> 
> 2. XEN handles the firing ARM Generic Timer:
> Before injecting irq to the guest the XEN sets interrupt mask for the virtual 
> timer.
> 
> .../xen/arch/arm/time.c
> 
> static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs 
> *regs)
> {
> ÂÂÂ current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
> ÂÂÂ WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, 
> CNTV_CTL_EL0);
> ÂÂÂ vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq, 1);
> }
> 
> And as result of run under XEN we don't have timer interrupts in QNX.
> I have changed interrupt handler in QNX to unmask timer interrupt before 
> return.

Hi Oleksandr,
thanks for reporting this issue and for the good analysis of the
problem, as always.


> But, I have question:
> Should the Hypervisor masks virtual timer IRQ on his own?
> It is a guest's resource and the guest itself should decide what to do.
> For example, I see that Linux Kernel (3.8) sets and clears timer interrupt 
> mask by itself.

In principle I agree with you that the vtimer is a guest resource.
However in practice if we don't mask the irq we can easily get into an
interrupt storm situation: if the guest doesn't handle the interrupt
immediately we could keep receiving the vtimer irq in the hypervisor and
busy loop around it. We really need to mask it somehow.

So the real question is: how are we going to mask the vtimer irq?
We could:
1) write CNTx_CTL_MASK to the ctl register, masking it at the timer
level
2) write to GICD_ICENABLER, deactivating it at the GICD level

Given that 1) didn't sound right to me, I tried 2) first but I had
issues with the ARM emulator at the time.  And as an ulterior
confirmation that deactivating it is not how ARM thought that the vtimer
should be used, Linux and KVM do 1) too.

But I don't like the idea of having to modify the vtimer handler in QNX,
so I have hacked together this patch, that would do 2) on top of my
maintenance interrupt series. Unfortunately it needs to ask for a
maintenance interrupt for the vtimer interrupt, so I can't say I am
completely convinced that it is the right thing to do.

What do people think about this?


diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 08ae23b..36e2ec0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -559,6 +559,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
         ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
     if ( p->desc != NULL )
         lr_val |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT);
+    else if ( p->irq == timer_get_irq(TIMER_VIRT_PPI) )
+        lr_val |= GICH_LR_MAINTENANCE_IRQ;
 
     GICH[GICH_LR + lr] = lr_val;
 
@@ -636,6 +638,16 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int 
virtual_irq,
     gic_add_to_lr_pending(v, irq_to_pending(v, virtual_irq));
 }
 
+void gic_disable_vtimer(void)
+{
+        GICD[GICD_ICENABLER] = (1 << timer_get_irq(TIMER_VIRT_PPI));
+}
+
+void gic_enable_vtimer(void)
+{
+        GICD[GICD_ISENABLER] = (1 << timer_get_irq(TIMER_VIRT_PPI));
+}
+
 static void gic_update_one_lr(struct vcpu *v, int i)
 {
     struct pending_irq *p;
@@ -676,6 +688,8 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
         clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         p->lr = GIC_INVALID_LR;
+        if ( timer_get_irq(TIMER_VIRT_PPI) == irq )
+            gic_enable_vtimer();
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
              test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
             gic_raise_guest_irq(v, irq, p->priority);
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 6e8d1f3..f9a6e7e 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -214,7 +214,6 @@ static void timer_interrupt(int irq, void *dev_id, struct 
cpu_user_regs *regs)
 static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
     current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
-    WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
     vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq);
 }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 0f0ba1d..0624aa9 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -757,6 +757,11 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     if ( !list_empty(&n->inflight) )
     {
+        if ( timer_get_irq(TIMER_VIRT_PPI) == irq )
+        {
+            gic_disable_vtimer();
+            goto out;
+        }
         set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
         gic_raise_inflight_irq(v, irq);
         goto out;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index bf6fb1e..08f3c08 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -227,6 +227,9 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
                   unsigned int *out_hwirq, unsigned int *out_type);
 void gic_clear_lrs(struct vcpu *v);
 
+extern void gic_disable_vtimer(void);
+extern void gic_enable_vtimer(void);
+
 #endif /* __ASSEMBLY__ */
 #endif

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