[PATCH v2] xen: arm: context switch vtimer PPI state.

[PATCH v2] xen: arm: context switch vtimer PPI state.

[Ian Campbell]

... instead of artificially masking the timer interrupt in the timer
state and relying on the guest to unmask which it isn't required to
do per the h/w spec, although Linux does, other OSes (FreeRTOS?) needed
changes. Making the vtimer follow the h/w spec is the main driving force
here although it may also enable us to do direct injection in the future
(GIC >=v4). It's also something I wanted to sort out before starting to
think about how to migrate a vtimer.

This series introduces the concept of routing a PPI to the currently
running VCPU and of context switching its state using the GICD I[SC]ACTIVER
registers to save and restore the active state of the interrupt. In the
case of the vtimer this prevents the nested invocations which the current
masking works around.

For edge triggered interrupts we also need to context switch the
pending bit via I[SC]PENDR. Note that for level triggered interrupts
SPENDR sets a latch which is only cleared by ICPENDR (and not by h/w
state changes), therefore we do not want to context switch the pending
state for level PPIs -- instead we rely on the context switch of the
peripheral to restore the correct level.

This is an update of a much older RFC[0]. Lots has changed, the hooks are
now at a different level with more common code (although maybe not as much
as I would like) and lots of refactoring, It also now handles non-1:1
configurations (tested with a patch to force GUEST_TIMER_VIRT_PPI to
something whic didn't match the h/w platform).

I also switched from using struct irq_guest->d (domain) == NULL to signal
this type of interrupt to using desc->state containing both IRQ_GUEST and
IRQ_PER_CPU as the trigger instead as discussed in[1].

I think I've addressed all the comments which remained relevant after the
reworking, although so much has changed I'm not completely sure, sorry if
I've missed anything.

Ian.

[0] http://lists.xen.org/archives/html/xen-devel/2015-01/msg03161.html
[1] http://lists.xen.org/archives/html/xen-devel/2015-11/msg00842.html

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

[PATCH v2 1/8] xen: arm: fix indendation of struct vtimer

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/domain.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index e7e40da..c56f06e 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -35,11 +35,11 @@ extern int dom0_11_mapping;
 #define is_domain_direct_mapped(d) ((d) == hardware_domain && dom0_11_mapping)
 
 struct vtimer {
-        struct vcpu *v;
-        int irq;
-        struct timer timer;
-        uint32_t ctl;
-        uint64_t cval;
+    struct vcpu *v;
+    int irq;
+    struct timer timer;
+    uint32_t ctl;
+    uint64_t cval;
 };
 
 struct arch_domain
-- 
2.1.4

[PATCH v2 2/8] xen: arm: fix typo in the description of struct pending_irq->desc

[Ian Campbell]

s/it/if/ makes more sense.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/vgic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index cb51a9e..7d580cc 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -66,7 +66,7 @@ struct pending_irq
 #define GIC_IRQ_GUEST_ENABLED  3
 #define GIC_IRQ_GUEST_MIGRATING   4
     unsigned long status;
-    struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
+    struct irq_desc *desc; /* only set if the irq corresponds to a physical irq */
     unsigned int irq;
 #define GIC_INVALID_LR         ~(uint8_t)0
     uint8_t lr;
-- 
2.1.4

[PATCH v2 3/8] xen: arm: Refactor route_irq_to_guest

[Ian Campbell]

Split out the bit which allocates the struct irqaction and calls
__setup_irq into a new function (setup_guest_irq). I'm going to want
to call this a second time in a subsequent patch.

Note that the action is now allocated and initialised with the desc
lock held (since it is taken by the caller). I don't think this is an
issue (and avoiding this would make things more complex)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: New patch (maybe, it's been a while...)
---
 xen/arch/arm/irq.c | 104 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 61 insertions(+), 43 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 5031777..6918438 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -394,61 +394,24 @@ bool_t is_assignable_irq(unsigned int irq)
     return ((irq >= NR_LOCAL_IRQS) && (irq < gic_number_lines()));
 }
 
-/*
- * Route an IRQ to a specific guest.
- * For now only SPIs are assignable to the guest.
- */
-int route_irq_to_guest(struct domain *d, unsigned int virq,
-                       unsigned int irq, const char * devname)
+static int setup_guest_irq(struct irq_desc *desc, unsigned int virq,
+                           unsigned int irqflags,
+                           struct irq_guest *info, const char *devname)
 {
+    const unsigned irq = desc->irq;
     struct irqaction *action;
-    struct irq_guest *info;
-    struct irq_desc *desc;
-    unsigned long flags;
     int retval = 0;
 
-    if ( virq >= vgic_num_irqs(d) )
-    {
-        printk(XENLOG_G_ERR
-               "the vIRQ number %u is too high for domain %u (max = %u)\n",
-               irq, d->domain_id, vgic_num_irqs(d));
-        return -EINVAL;
-    }
-
-    /* Only routing to virtual SPIs is supported */
-    if ( virq < NR_LOCAL_IRQS )
-    {
-        printk(XENLOG_G_ERR "IRQ can only be routed to an SPI\n");
-        return -EINVAL;
-    }
-
-    if ( !is_assignable_irq(irq) )
-    {
-        printk(XENLOG_G_ERR "the IRQ%u is not routable\n", irq);
-        return -EINVAL;
-    }
-    desc = irq_to_desc(irq);
+    ASSERT(spin_is_locked(&desc->lock));
 
     action = xmalloc(struct irqaction);
     if ( !action )
         return -ENOMEM;
 
-    info = xmalloc(struct irq_guest);
-    if ( !info )
-    {
-        xfree(action);
-        return -ENOMEM;
-    }
-
-    info->d = d;
-    info->virq = virq;
-
     action->dev_id = info;
     action->name = devname;
     action->free_on_release = 1;
 
-    spin_lock_irqsave(&desc->lock, flags);
-
     if ( desc->arch.type == DT_IRQ_TYPE_INVALID )
     {
         printk(XENLOG_G_ERR "IRQ %u has not been configured\n", irq);
@@ -466,6 +429,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
      */
     if ( desc->action != NULL )
     {
+        struct domain *d = info->d;
         struct domain *ad = irq_get_domain(desc);
 
         if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad )
@@ -493,6 +457,61 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
     if ( retval )
         goto out;
 
+    return 0;
+
+out:
+    xfree(action);
+    return retval;
+}
+
+/*
+ * Route an IRQ to a specific guest.
+ * For now only SPIs are assignable to the guest.
+ */
+int route_irq_to_guest(struct domain *d, unsigned int virq,
+                       unsigned int irq, const char * devname)
+{
+    struct irq_guest *info;
+    struct irq_desc *desc;
+    unsigned long flags;
+    int retval;
+
+    if ( virq >= vgic_num_irqs(d) )
+    {
+        printk(XENLOG_G_ERR
+               "the vIRQ number %u is too high for domain %u (max = %u)\n",
+               irq, d->domain_id, vgic_num_irqs(d));
+        return -EINVAL;
+    }
+
+    /* Only routing to virtual SPIs is supported */
+    if ( virq < NR_LOCAL_IRQS )
+    {
+        printk(XENLOG_G_ERR "IRQ can only be routed to an SPI\n");
+        return -EINVAL;
+    }
+
+    if ( !is_assignable_irq(irq) )
+    {
+        printk(XENLOG_G_ERR "the IRQ%u is not routable\n", irq);
+        return -EINVAL;
+    }
+
+    desc = irq_to_desc(irq);
+
+    info = xmalloc(struct irq_guest);
+    if ( !info )
+        return -ENOMEM;
+
+    info->d = d;
+    info->virq = virq;
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    retval = setup_guest_irq(desc, virq, flags, info, devname);
+    if ( retval )
+        goto out;
+
     retval = gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
 
     spin_unlock_irqrestore(&desc->lock, flags);
@@ -507,7 +526,6 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
 
 out:
     spin_unlock_irqrestore(&desc->lock, flags);
-    xfree(action);
 free_info:
     xfree(info);
 
-- 
2.1.4

[PATCH v2 4/8] xen: arm: remove vgic_vcpu_inject_spi()

[Ian Campbell]

Currently this has only a single caller, which I intend to teach about
injecting PPIs shortly. This helper doesn't add much so remove it.

Drop a stray tab from the comment immediately preceding this change.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/irq.c         |  7 +++++--
 xen/arch/arm/vgic.c        | 11 -----------
 xen/include/asm-arm/vgic.h |  1 -
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 6918438..5b073d1 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -214,6 +214,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
     if ( test_bit(_IRQ_GUEST, &desc->status) )
     {
         struct irq_guest *info = irq_get_guest_info(desc);
+        struct vcpu *v;
 
         perfc_incr(guest_irqs);
         desc->handler->end(desc);
@@ -223,8 +224,10 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         /*
          * The irq cannot be a PPI, we only support delivery of SPIs to
          * guests.
-	 */
-        vgic_vcpu_inject_spi(info->d, info->virq);
+         */
+        v = vgic_get_target_vcpu(info->d->vcpu[0], info->virq);
+        vgic_vcpu_inject_irq(v, info->virq);
+
         goto out_no_end;
     }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 7bb4570..7a76f00 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -477,17 +477,6 @@ out:
     }
 }
 
-void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq)
-{
-    struct vcpu *v;
-
-    /* the IRQ needs to be an SPI */
-    ASSERT(virq >= 32 && virq <= vgic_num_irqs(d));
-
-    v = vgic_get_target_vcpu(d->vcpu[0], virq);
-    vgic_vcpu_inject_irq(v, virq);
-}
-
 void arch_evtchn_inject(struct vcpu *v)
 {
     vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 7d580cc..aa675cb 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -207,7 +207,6 @@ extern void domain_vgic_free(struct domain *d);
 extern int vcpu_vgic_init(struct vcpu *v);
 extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
 extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
-extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq);
-- 
2.1.4

[PATCH v2 5/8] xen: arm: add interfaces to save/restore the state of a PPI.

[Ian Campbell]

Make use of the GICD I[SC]ACTIVER registers to save and
restore the active state of the interrupt.

For edge triggered interrupts we also need to context switch the
pending bit via I[SC]PENDR. Note that for level triggered interrupts
SPENDR sets a latch which is only cleared by ICPENDR (and not by h/w
state changes), therefore we do not want to context switch the pending
state for level PPIs -- instead we rely on the context switch of the
peripheral to restore the correct level.

Tested on GIC v2 only.

Unused as yet, will be used by the vtimer code shortly.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/gic-v2.c        | 67 ++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic-v3.c        | 67 ++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic.c           | 54 +++++++++++++++++++++++++++++++++++
 xen/arch/arm/irq.c           |  7 +++++
 xen/include/asm-arm/domain.h | 11 ++++++++
 xen/include/asm-arm/gic.h    | 16 +++++++++++
 xen/include/asm-arm/irq.h    |  3 ++
 7 files changed, 225 insertions(+)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 01e36b5..5308c35 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -81,6 +81,9 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 #define NR_GIC_CPU_IF 8
 
+static void gicv2_irq_enable(struct irq_desc *desc);
+static void gicv2_irq_disable(struct irq_desc *desc);
+
 static inline void writeb_gicd(uint8_t val, unsigned int offset)
 {
     writeb_relaxed(val, gicv2.map_dbase + offset);
@@ -149,6 +152,37 @@ static void gicv2_save_state(struct vcpu *v)
     writel_gich(0, GICH_HCR);
 }
 
+static void gicv2_save_and_mask_hwppi(struct irq_desc *desc,
+                                      struct hwppi_state *s)
+{
+    const unsigned int mask = (1u << desc->irq);
+    const unsigned int pendingr = readl_gicd(GICD_ISPENDR);
+    const unsigned int activer = readl_gicd(GICD_ISACTIVER);
+    const unsigned int enabler = readl_gicd(GICD_ISENABLER);
+    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
+
+    s->active = !!(activer & mask);
+    s->enabled = !!(enabler & mask);
+    s->pending = !!(pendingr & mask);
+
+    /* Write a 1 to IC...R to clear the corresponding bit of state */
+    if ( s->active )
+        writel_gicd(mask, GICD_ICACTIVER);
+    /*
+     * For an edge interrupt clear the pending state, for a level interrupt
+     * this clears the latch there is no need since saving the peripheral state
+     * (and/or restoring the next VCPU) will cause the correct action.
+     */
+    if ( is_edge && s->pending )
+        writel_gicd(mask, GICD_ICPENDR);
+
+    if ( s->enabled )
+        gicv2_irq_disable(desc);
+
+    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
+    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
+}
+
 static void gicv2_restore_state(const struct vcpu *v)
 {
     int i;
@@ -161,6 +195,37 @@ static void gicv2_restore_state(const struct vcpu *v)
     writel_gich(GICH_HCR_EN, GICH_HCR);
 }
 
+static void gicv2_restore_hwppi(struct irq_desc *desc,
+                                const struct hwppi_state *s)
+{
+    const unsigned int mask = (1u << desc->irq);
+    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
+
+    /*
+     * The IRQ must always have been set inactive and masked etc by
+     * the saving of the previous state via save_and_mask_hwppi.
+     */
+    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
+    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
+
+    if ( s->active )
+        writel_gicd(mask, GICD_ICACTIVER);
+
+    /*
+     * Restore pending state for edge triggered interrupts only. For
+     * level triggered interrupts the level will be restored as
+     * necessary by restoring the state of the relevant peripheral.
+     *
+     * For a level triggered interrupt ISPENDR acts as a *latch* which
+     * is only cleared by ICPENDR (i.e. the input level is no longer
+     * relevant). We certainly do not want that here.
+     */
+    if ( is_edge && s->pending )
+        writel_gicd(mask, GICD_ISPENDR);
+    if ( s->enabled )
+        gicv2_irq_enable(desc);
+}
+
 static void gicv2_dump_state(const struct vcpu *v)
 {
     int i;
@@ -744,7 +809,9 @@ const static struct gic_hw_operations gicv2_ops = {
     .init                = gicv2_init,
     .secondary_init      = gicv2_secondary_cpu_init,
     .save_state          = gicv2_save_state,
+    .save_and_mask_hwppi = gicv2_save_and_mask_hwppi,
     .restore_state       = gicv2_restore_state,
+    .restore_hwppi       = gicv2_restore_hwppi,
     .dump_state          = gicv2_dump_state,
     .gic_host_irq_type   = &gicv2_host_irq_type,
     .gic_guest_irq_type  = &gicv2_guest_irq_type,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index 4fe0c37..cfe705a 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -61,6 +61,9 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
 #define GICD_RDIST_BASE        (this_cpu(rbase))
 #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
 
+void gicv3_irq_enable(struct irq_desc *desc);
+void gicv3_irq_disable(struct irq_desc *desc);
+
 /*
  * Saves all 16(Max) LR registers. Though number of LRs implemented
  * is implementation specific.
@@ -373,6 +376,37 @@ static void gicv3_save_state(struct vcpu *v)
     v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
 }
 
+static void gicv3_save_and_mask_hwppi(struct irq_desc *desc,
+                                      struct hwppi_state *s)
+{
+    const unsigned int mask = (1u << desc->irq);
+    const unsigned int pendingr = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISPENDR);
+    const unsigned int activer = readl_gicd(GICD_RDIST_SGI_BASE + GICR_ISACTIVER);
+    const unsigned int enabler = readl_gicd(GICD_RDIST_SGI_BASE + GICR_ISENABLER);
+    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
+
+    s->active = !!(activer & mask);
+    s->enabled = !!(enabler & mask);
+    s->pending = !!(pendingr & mask);
+
+    /* Write a 1 to IC...R to clear the corresponding bit of state */
+    if ( s->active )
+        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ICACTIVER);
+    /*
+     * For an edge interrupt clear the pending state, for a level interrupt
+     * this clears the latch there is no need since saving the peripheral state
+     * (and/or restoring the next VCPU) will cause the correct action.
+     */
+    if ( is_edge && s->pending )
+        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ICPENDR);
+
+    if ( s->enabled )
+        gicv3_irq_disable(desc);
+
+    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER) & mask));
+    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISENABLER) & mask));
+}
+
 static void gicv3_restore_state(const struct vcpu *v)
 {
     uint32_t val;
@@ -399,6 +433,37 @@ static void gicv3_restore_state(const struct vcpu *v)
     dsb(sy);
 }
 
+static void gicv3_restore_hwppi(struct irq_desc *desc,
+                                const struct hwppi_state *s)
+{
+    const unsigned int mask = (1u << desc->irq);
+    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
+
+    /*
+     * The IRQ must always have been set inactive and masked etc by
+     * the saving of the previous state via save_and_mask_hwppi.
+     */
+    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER_ISACTIVER) & mask));
+    ASSERT(!(readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISACTIVER_ISENABLER) & mask));
+
+    if ( s->active )
+        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ISACTIVER_ICACTIVER);
+
+    /*
+     * Restore pending state for edge triggered interrupts only. For
+     * level triggered interrupts the level will be restored as
+     * necessary by restoring the state of the relevant peripheral.
+     *
+     * For a level triggered interrupt ISPENDR acts as a *latch* which
+     * is only cleared by ICPENDR (i.e. the input level is no longer
+     * relevant). We certainly do not want that here.
+     */
+    if ( is_edge && s->pending )
+        writel_relaxed(mask, GICD_RDIST_SGI_BASE + GICR_ISACTIVER_ISPENDR);
+    if ( s->enabled )
+        gicv3_irq_enable(desc);
+}
+
 static void gicv3_dump_state(const struct vcpu *v)
 {
     int i;
@@ -1296,7 +1361,9 @@ static const struct gic_hw_operations gicv3_ops = {
     .info                = &gicv3_info,
     .init                = gicv3_init,
     .save_state          = gicv3_save_state,
+    .save_and_mask_hwppi = gicv3_save_and_mask_hwppi,
     .restore_state       = gicv3_restore_state,
+    .restore_hwppi       = gicv3_restore_hwppi,
     .dump_state          = gicv3_dump_state,
     .gic_host_irq_type   = &gicv3_host_irq_type,
     .gic_guest_irq_type  = &gicv3_guest_irq_type,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index b0b0999..e294e89 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -67,6 +67,17 @@ unsigned int gic_number_lines(void)
     return gic_hw_ops->info->nr_lines;
 }
 
+void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq)
+{
+    memset(s, 0, sizeof(*s));
+    s->irq = irq;
+}
+
+void gic_hwppi_set_pending(struct hwppi_state *s)
+{
+    s->pending = true;
+}
+
 void gic_save_state(struct vcpu *v)
 {
     ASSERT(!local_irq_is_enabled());
@@ -81,6 +92,25 @@ void gic_save_state(struct vcpu *v)
     isb();
 }
 
+void gic_save_and_mask_hwppi(struct vcpu *v, const unsigned virq,
+                             struct hwppi_state *s)
+{
+    struct pending_irq *p = irq_to_pending(v, virq);
+    struct irq_desc *desc = p->desc;
+
+    spin_lock(&desc->lock);
+
+    ASSERT(virq >= 16 && virq < 32);
+    ASSERT(desc->irq >= 16 && desc->irq < 32);
+    ASSERT(!is_idle_vcpu(v));
+
+    s->inprogress = test_and_clear_bit(_IRQ_INPROGRESS, &desc->status);
+
+    gic_hw_ops->save_and_mask_hwppi(desc, s);
+
+    spin_unlock(&desc->lock);
+}
+
 void gic_restore_state(struct vcpu *v)
 {
     ASSERT(!local_irq_is_enabled());
@@ -94,6 +124,30 @@ void gic_restore_state(struct vcpu *v)
     gic_restore_pending_irqs(v);
 }
 
+void gic_restore_hwppi(struct vcpu *v,
+                       const unsigned virq,
+                       const struct hwppi_state *s)
+{
+    struct pending_irq *p = irq_to_pending(v, virq);
+    struct irq_desc *desc = irq_to_desc(s->irq);
+
+    spin_lock(&desc->lock);
+
+    ASSERT(virq >= 16 && virq < 32);
+    ASSERT(!is_idle_vcpu(v));
+
+    p->desc = desc; /* Migrate to new physical processor */
+
+    irq_set_virq(desc, virq);
+
+    gic_hw_ops->restore_hwppi(desc, s);
+
+    if ( s->inprogress )
+        set_bit(_IRQ_INPROGRESS, &desc->status);
+
+    spin_unlock(&desc->lock);
+}
+
 /*
  * needs to be called with a valid cpu_mask, ie each cpu in the mask has
  * already called gic_cpu_init
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 5b073d1..95be10e 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -138,6 +138,13 @@ static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
     return desc->action->dev_id;
 }
 
+void irq_set_virq(struct irq_desc *desc, unsigned virq)
+{
+    struct irq_guest *info = irq_get_guest_info(desc);
+    ASSERT(test_bit(_IRQ_PER_CPU, &desc->status));
+    info->virq = virq;
+}
+
 static inline struct domain *irq_get_domain(struct irq_desc *desc)
 {
     return irq_get_guest_info(desc)->d;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c56f06e..550e28b 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -34,6 +34,17 @@ enum domain_type {
 extern int dom0_11_mapping;
 #define is_domain_direct_mapped(d) ((d) == hardware_domain && dom0_11_mapping)
 
+struct hwppi_state {
+    /* h/w state */
+    unsigned irq;
+    unsigned long enabled:1;
+    unsigned long pending:1;
+    unsigned long active:1;
+
+    /* Xen s/w state */
+    unsigned long inprogress:1;
+};
+
 struct vtimer {
     struct vcpu *v;
     int irq;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 0116481..fe9af3e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -258,6 +258,20 @@ extern int gicv_setup(struct domain *d);
 extern void gic_save_state(struct vcpu *v);
 extern void gic_restore_state(struct vcpu *v);
 
+/*
+ * Save/restore the state of a single PPI which must be routed to
+ * <current-vcpu> (that is, is defined to be injected to the current
+ * vcpu).
+ */
+struct hwppi_state;
+extern void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq);
+extern void gic_hwppi_set_pending(struct hwppi_state *s);
+extern void gic_save_and_mask_hwppi(struct vcpu *v, unsigned irq,
+                                    struct hwppi_state *s);
+extern void gic_restore_hwppi(struct vcpu *v,
+                              const unsigned virq,
+                              const struct hwppi_state *s);
+
 /* SGI (AKA IPIs) */
 enum gic_sgi {
     GIC_SGI_EVENT_CHECK = 0,
@@ -308,8 +322,10 @@ struct gic_hw_operations {
     int (*init)(void);
     /* Save GIC registers */
     void (*save_state)(struct vcpu *);
+    void (*save_and_mask_hwppi)(struct irq_desc *desc, struct hwppi_state *s);
     /* Restore GIC registers */
     void (*restore_state)(const struct vcpu *);
+    void (*restore_hwppi)(struct irq_desc *desc, const struct hwppi_state *s);
     /* Dump GIC LR register information */
     void (*dump_state)(const struct vcpu *);
 
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index f33c331..d354e3b 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -43,6 +43,7 @@ void init_secondary_IRQ(void);
 
 int route_irq_to_guest(struct domain *d, unsigned int virq,
                        unsigned int irq, const char *devname);
+int route_irq_to_current_vcpu(unsigned int irq, const char *devname);
 int release_guest_irq(struct domain *d, unsigned int irq);
 
 void arch_move_irqs(struct vcpu *v);
@@ -56,6 +57,8 @@ int platform_get_irq(const struct dt_device_node *device, int index);
 
 void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
 
+void irq_set_virq(struct irq_desc *desc, unsigned virq);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
-- 
2.1.4

[PATCH v2 6/8] xen: arm: supporting routing a PPI to the current vcpu.

[Ian Campbell]

That is whichever vcpu is resident when the interrupt fires. An
interrupt is in this state when both IRQ_GUEST and IRQ_PER_CPU are set
in the descriptor status. Only PPIs can be in this mode.

This requires some peripheral specific code to make use of the
previously introduced functionality to save and restore the PPI state.
The vtimer driver will do so shortly.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/gic.c        | 24 ++++++++++++++
 xen/arch/arm/irq.c        | 84 +++++++++++++++++++++++++++++++++++++++++------
 xen/include/asm-arm/gic.h |  2 ++
 xen/include/asm-arm/irq.h |  2 +-
 4 files changed, 101 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e294e89..aea913b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -177,6 +177,30 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
     gic_set_irq_properties(desc, cpu_mask, priority);
 }
 
+/* Program the GIC to route an interrupt to the current guest
+ *
+ * That is the IRQ is delivered to whichever VCPU happens to be
+ * resident on the PCPU when the interrupt arrives.
+ *
+ * Currently the interrupt *must* be a PPI and the code responsible
+ * for the related hardware must save and restore the PPI with
+ * gic_save_and_mask_hwppi/gic_restore_hwppi.
+ */
+int gic_route_irq_to_current_guest(struct irq_desc *desc,
+                                   unsigned int priority)
+{
+    ASSERT(spin_is_locked(&desc->lock));
+    ASSERT(desc->irq >= 16 && desc->irq < 32);
+
+    desc->handler = gic_hw_ops->gic_guest_irq_type;
+    set_bit(_IRQ_GUEST, &desc->status);
+    set_bit(_IRQ_PER_CPU, &desc->status);
+
+    gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
+
+    return 0;
+}
+
 /* Program the GIC to route an interrupt to a guest
  *   - desc.lock must be held
  */
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 95be10e..ca18403 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -229,12 +229,15 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         set_bit(_IRQ_INPROGRESS, &desc->status);
 
         /*
-         * The irq cannot be a PPI, we only support delivery of SPIs to
-         * guests.
+         * A PPI exposed to a guest must always be in IRQ_GUEST|IRQ_PER_CPU
+         * mode ("route to active VCPU"), so we use current.
+         *
+         * For SPI we look up the required target as normal.
          */
-        v = vgic_get_target_vcpu(info->d->vcpu[0], info->virq);
-        vgic_vcpu_inject_irq(v, info->virq);
+        v = test_bit(_IRQ_PER_CPU, &desc->status) ? current :
+            vgic_get_target_vcpu(info->d->vcpu[0], info->virq);
 
+        vgic_vcpu_inject_irq(v, info->virq);
         goto out_no_end;
     }
 
@@ -363,11 +366,15 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
 
     if ( test_bit(_IRQ_GUEST, &desc->status) )
     {
-        struct domain *d = irq_get_domain(desc);
+        struct irq_guest *info = irq_get_guest_info(desc);
 
         spin_unlock_irqrestore(&desc->lock, flags);
-        printk(XENLOG_ERR "ERROR: IRQ %u is already in use by the domain %u\n",
-               irq, d->domain_id);
+        if ( !test_bit(_IRQ_PER_CPU, &desc->status) )
+            printk(XENLOG_ERR "ERROR: IRQ %u is already in use by domain %u\n",
+                   irq, info->d->domain_id);
+        else
+            printk(XENLOG_ERR
+                   "ERROR: IRQ %u is already in use by <current-vcpu>\n", irq);
         return -EBUSY;
     }
 
@@ -410,7 +417,7 @@ static int setup_guest_irq(struct irq_desc *desc, unsigned int virq,
 {
     const unsigned irq = desc->irq;
     struct irqaction *action;
-    int retval = 0;
+    int retval;
 
     ASSERT(spin_is_locked(&desc->lock));
 
@@ -451,12 +458,21 @@ static int setup_guest_irq(struct irq_desc *desc, unsigned int virq,
                        d->domain_id, irq, irq_get_guest_info(desc)->virq);
                 retval = -EBUSY;
             }
+            else
+                retval = 0;
             goto out;
         }
 
         if ( test_bit(_IRQ_GUEST, &desc->status) )
-            printk(XENLOG_G_ERR "IRQ %u is already used by domain %u\n",
-                   irq, ad->domain_id);
+        {
+            if ( !test_bit(_IRQ_PER_CPU, &desc->status) )
+                printk(XENLOG_ERR
+                       "ERROR: IRQ %u is already used by domain %u\n",
+                       irq, ad->domain_id);
+            else
+                printk(XENLOG_ERR
+                       "ERROR: IRQ %u is already used by <current-vcpu>\n", irq);
+        }
         else
             printk(XENLOG_G_ERR "IRQ %u is already used by Xen\n", irq);
         retval = -EBUSY;
@@ -542,6 +558,54 @@ free_info:
     return retval;
 }
 
+/*
+ * Route a PPI such that it is always delivered to the current vcpu on
+ * the pcpu. The driver for the peripheral must use
+ * gic_{save_and_mask,restore}_hwppi as part of the context switch.
+ */
+int route_hwppi_to_current_vcpu(unsigned int irq, const char *devname)
+{
+    struct irq_guest *info;
+    struct irq_desc *desc;
+    unsigned long flags;
+    int retval = 0;
+
+    /* Can only route PPIs to current VCPU */
+    if ( irq < 16 || irq >= 32 )
+        return -EINVAL;
+
+    desc = irq_to_desc(irq);
+
+    info = xmalloc(struct irq_guest);
+    if ( !info )
+        return -ENOMEM;
+
+    info->d = NULL; /* Routed to current vcpu, so no specific domain */
+    /* info->virq is set by gic_restore_hwppi. */
+
+    spin_lock_irqsave(&desc->lock, flags);
+
+    retval = setup_guest_irq(desc, irq, flags, info, devname);
+    if ( retval )
+    {
+        xfree(info);
+        return retval;
+    }
+
+    retval = gic_route_irq_to_current_guest(desc, GIC_PRI_IRQ);
+
+    spin_unlock_irqrestore(&desc->lock, flags);
+
+    if ( retval )
+    {
+        release_irq(desc->irq, info);
+        xfree(info);
+        return retval;
+    }
+
+    return 0;
+}
+
 int release_guest_irq(struct domain *d, unsigned int virq)
 {
     struct irq_desc *desc;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index fe9af3e..1e611fa 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -224,6 +224,8 @@ extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mas
 extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
                                   struct irq_desc *desc,
                                   unsigned int priority);
+int gic_route_irq_to_current_guest(struct irq_desc *desc,
+                                   unsigned int priority);
 
 /* Remove an IRQ passthrough to a guest */
 int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index d354e3b..b302e3e 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -43,7 +43,7 @@ void init_secondary_IRQ(void);
 
 int route_irq_to_guest(struct domain *d, unsigned int virq,
                        unsigned int irq, const char *devname);
-int route_irq_to_current_vcpu(unsigned int irq, const char *devname);
+int route_hwppi_to_current_vcpu(unsigned int irq, const char *devname);
 int release_guest_irq(struct domain *d, unsigned int irq);
 
 void arch_move_irqs(struct vcpu *v);
-- 
2.1.4

[PATCH v2 7/8] xen: arm: context switch vtimer PPI state.

[Ian Campbell]

... instead of artificially masking the timer interrupt in the timer
state and relying on the guest to unmask (which it isn't required to
do per the h/w spec, although Linux does).

By using the newly added hwppi save/restore functionality we make use
of the GICD I[SC]ACTIVER registers to save and restore the active
state of the interrupt, which prevents the nested invocations which
the current masking works around.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Rebased, in particular over Julien's passthrough stuff which
    reworked a bunch of IRQ related stuff.
    Also largely rewritten since precursor patches now lay very
    different groundwork.
---
 xen/arch/arm/time.c          | 26 ++------------------------
 xen/arch/arm/vtimer.c        | 41 +++++++++++++++++++++++++++++++++++++----
 xen/include/asm-arm/domain.h |  1 +
 3 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 5ded30c..2a1cdba 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -181,28 +181,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)
-{
-    /*
-     * Edge-triggered interrupts can be used for the virtual timer. Even
-     * if the timer output signal is masked in the context switch, the
-     * GIC will keep track that of any interrupts raised while IRQS are
-     * disabled. As soon as IRQs are re-enabled, the virtual interrupt
-     * will be injected to Xen.
-     *
-     * If an IDLE vCPU was scheduled next then we should ignore the
-     * interrupt.
-     */
-    if ( unlikely(is_idle_vcpu(current)) )
-        return;
-
-    perfc_incr(virt_timer_irqs);
-
-    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);
-}
-
 /*
  * Arch timer interrupt really ought to be level triggered, since the
  * design of the timer/comparator mechanism is based around that
@@ -242,8 +220,8 @@ void __cpuinit init_timer_interrupt(void)
 
     request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
                 "hyptimer", NULL);
-    request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
-                   "virtimer", NULL);
+    route_hwppi_to_current_vcpu(timer_irq[TIMER_VIRT_PPI], "virtimer");
+
     request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
                 "phytimer", NULL);
 
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 1418092..82e2b88 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -55,9 +55,19 @@ static void phys_timer_expired(void *data)
 static void virt_timer_expired(void *data)
 {
     struct vtimer *t = data;
-    t->ctl |= CNTx_CTL_MASK;
-    vgic_vcpu_inject_irq(t->v, t->irq);
-    perfc_incr(vtimer_virt_inject);
+    t->ctl |= CNTx_CTL_PENDING;
+    if ( !(t->ctl & CNTx_CTL_MASK) )
+    {
+        /*
+         * An edge triggered interrupt should now be pending. Since
+         * this timer can never expire while the domain is scheduled
+         * we know that the gic_restore_hwppi in virt_timer_restore
+         * will cause the real hwppi to occur and be routed.
+         */
+        gic_hwppi_set_pending(&t->ppi_state);
+        vcpu_unblock(t->v);
+        perfc_incr(vtimer_virt_inject);
+    }
 }
 
 int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
@@ -96,9 +106,12 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
 
 int vcpu_vtimer_init(struct vcpu *v)
 {
+    struct pending_irq *p;
     struct vtimer *t = &v->arch.phys_timer;
     bool_t d0 = is_hardware_domain(v->domain);
 
+    const unsigned host_vtimer_irq_ppi = timer_get_irq(TIMER_VIRT_PPI);
+
     /*
      * Hardware domain uses the hardware interrupts, guests get the virtual
      * platform.
@@ -116,10 +129,16 @@ int vcpu_vtimer_init(struct vcpu *v)
     init_timer(&t->timer, virt_timer_expired, t, v->processor);
     t->ctl = 0;
     t->irq = d0
-        ? timer_get_irq(TIMER_VIRT_PPI)
+        ? host_vtimer_irq_ppi
         : GUEST_TIMER_VIRT_PPI;
     t->v = v;
 
+    p = irq_to_pending(v, t->irq);
+    p->irq = t->irq;
+
+    gic_hwppi_state_init(&v->arch.virt_timer.ppi_state,
+                         host_vtimer_irq_ppi);
+
     v->arch.vtimer_initialized = 1;
 
     return 0;
@@ -147,6 +166,15 @@ int virt_timer_save(struct vcpu *v)
         set_timer(&v->arch.virt_timer.timer, ticks_to_ns(v->arch.virt_timer.cval +
                   v->domain->arch.virt_timer_base.offset - boot_count));
     }
+
+    /*
+     * Since the vtimer irq is a PPI we don't need to worry about
+     * racing against it becoming active while we are saving the
+     * state, since that requires the guest to be reading the IAR.
+     */
+    gic_save_and_mask_hwppi(v, v->arch.virt_timer.irq,
+                            &v->arch.virt_timer.ppi_state);
+
     return 0;
 }
 
@@ -161,6 +189,11 @@ int virt_timer_restore(struct vcpu *v)
     WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2);
     WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0);
     WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
+
+    gic_restore_hwppi(v,
+                      v->arch.virt_timer.irq,
+                      &v->arch.virt_timer.ppi_state);
+
     return 0;
 }
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 550e28b..aff21dd 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -51,6 +51,7 @@ struct vtimer {
     struct timer timer;
     uint32_t ctl;
     uint64_t cval;
+    struct hwppi_state ppi_state;
 };
 
 struct arch_domain
-- 
2.1.4

[PATCH v2 8/8] HACK: Force virt timer to PPI0 (IRQ16)

[Ian Campbell]

Just for testing so the guest vtimer ppi it isn't the same as the
physical virt timer PPI on my platform, and therefore allows to
exercise the non-1:1 bits of the code.
---
 xen/include/public/arch-arm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 6322548..01dbb41 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -432,7 +432,7 @@ typedef uint64_t xen_callback_t;
 #define GUEST_RAM_BANK_SIZES   { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
 
 /* Interrupts */
-#define GUEST_TIMER_VIRT_PPI    27
+#define GUEST_TIMER_VIRT_PPI    16
 #define GUEST_TIMER_PHYS_S_PPI  29
 #define GUEST_TIMER_PHYS_NS_PPI 30
 #define GUEST_EVTCHN_PPI        31
-- 
2.1.4

Re: [PATCH v2 1/8] xen: arm: fix indendation of struct vtimer

[Julien Grall]

Hi Ian,

On 10/11/2015 16:21, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

> ---
>   xen/include/asm-arm/domain.h | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index e7e40da..c56f06e 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -35,11 +35,11 @@ extern int dom0_11_mapping;
>   #define is_domain_direct_mapped(d) ((d) == hardware_domain && dom0_11_mapping)
>
>   struct vtimer {
> -        struct vcpu *v;
> -        int irq;
> -        struct timer timer;
> -        uint32_t ctl;
> -        uint64_t cval;
> +    struct vcpu *v;
> +    int irq;
> +    struct timer timer;
> +    uint32_t ctl;
> +    uint64_t cval;
>   };
>
>   struct arch_domain
>

-- 
Julien Grall

Re: [PATCH v2 2/8] xen: arm: fix typo in the description of struct pending_irq->desc

[Julien Grall]

Hi Ian,

On 10/11/2015 16:21, Ian Campbell wrote:
> s/it/if/ makes more sense.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

> ---
>   xen/include/asm-arm/vgic.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index cb51a9e..7d580cc 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -66,7 +66,7 @@ struct pending_irq
>   #define GIC_IRQ_GUEST_ENABLED  3
>   #define GIC_IRQ_GUEST_MIGRATING   4
>       unsigned long status;
> -    struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
> +    struct irq_desc *desc; /* only set if the irq corresponds to a physical irq */
>       unsigned int irq;
>   #define GIC_INVALID_LR         ~(uint8_t)0
>       uint8_t lr;
>

-- 
Julien Grall

Re: [PATCH v2 3/8] xen: arm: Refactor route_irq_to_guest

[Julien Grall]

Hi Ian,

On 10/11/15 16:21, Ian Campbell wrote:
> Split out the bit which allocates the struct irqaction and calls
> __setup_irq into a new function (setup_guest_irq). I'm going to want
> to call this a second time in a subsequent patch.
> 
> Note that the action is now allocated and initialised with the desc
> lock held (since it is taken by the caller). I don't think this is an
> issue (and avoiding this would make things more complex)

There is one potential issue, the desc->lock has to be taken with
interrupt disabled.

With this change, the interrupts are disabled far more longer than
previously.

I'm mostly worry about the xmalloc and the printk within this section.

Regards,

-- 
Julien Grall

Re: [PATCH v2] xen: arm: context switch vtimer PPI state.

[Ian Campbell]

On Tue, 2015-11-10 at 16:20 +0000, Ian Campbell wrote:
> ... instead of artificially masking the timer interrupt in the timer
> state

Julien pointed out IRL that this now causes vgic_{enable,disable}_irqs to
call into the h/w driver for that irq, which is new for a PPI. The GICv3
hook for enable/disable will write to the local redistributor, but a cpu is
entitled to do write to another vcpus GICR, for the banked registers
associated with PPIs. The upshot is that we will enable/disable the local
PPI instead of the remote one as expected.

This will require some thought (or if I'm really lucky some careful reading
of the spec to turn this into the expected behaviour for such banked
registers, doubtful...)...

Ian.



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

Re: [PATCH v2 5/8] xen: arm: add interfaces to save/restore the state of a PPI.

[Julien Grall]

Hi Ian,

On 10/11/15 16:21, Ian Campbell wrote:
> Make use of the GICD I[SC]ACTIVER registers to save and
> restore the active state of the interrupt.
> 
> For edge triggered interrupts we also need to context switch the
> pending bit via I[SC]PENDR. Note that for level triggered interrupts
> SPENDR sets a latch which is only cleared by ICPENDR (and not by h/w
> state changes), therefore we do not want to context switch the pending
> state for level PPIs -- instead we rely on the context switch of the
> peripheral to restore the correct level.
> 
> Tested on GIC v2 only.

I will give a try on the foundation model with GICv3.

> Unused as yet, will be used by the vtimer code shortly.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/gic-v2.c        | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c        | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic.c           | 54 +++++++++++++++++++++++++++++++++++
>  xen/arch/arm/irq.c           |  7 +++++
>  xen/include/asm-arm/domain.h | 11 ++++++++
>  xen/include/asm-arm/gic.h    | 16 +++++++++++
>  xen/include/asm-arm/irq.h    |  3 ++
>  7 files changed, 225 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 01e36b5..5308c35 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -81,6 +81,9 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>  /* Maximum cpu interface per GIC */
>  #define NR_GIC_CPU_IF 8
>  
> +static void gicv2_irq_enable(struct irq_desc *desc);
> +static void gicv2_irq_disable(struct irq_desc *desc);
> +
>  static inline void writeb_gicd(uint8_t val, unsigned int offset)
>  {
>      writeb_relaxed(val, gicv2.map_dbase + offset);
> @@ -149,6 +152,37 @@ static void gicv2_save_state(struct vcpu *v)
>      writel_gich(0, GICH_HCR);
>  }
>  
> +static void gicv2_save_and_mask_hwppi(struct irq_desc *desc,
> +                                      struct hwppi_state *s)
> +{
> +    const unsigned int mask = (1u << desc->irq);

I would add a comment to explain that PPI are always below 31. Otherwise
this construction would be invalid.

> +    const unsigned int pendingr = readl_gicd(GICD_ISPENDR);
> +    const unsigned int activer = readl_gicd(GICD_ISACTIVER);
> +    const unsigned int enabler = readl_gicd(GICD_ISENABLER);

pendingr, activer, enabler are 32-bit register. Please use uint32_t here
to make it clear.

> +    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> +
> +    s->active = !!(activer & mask);
> +    s->enabled = !!(enabler & mask);
> +    s->pending = !!(pendingr & mask);
> +
> +    /* Write a 1 to IC...R to clear the corresponding bit of state */
> +    if ( s->active )
> +        writel_gicd(mask, GICD_ICACTIVER);

NIT: Newline here for clarity.

> +    /*
> +     * For an edge interrupt clear the pending state, for a level interrupt
> +     * this clears the latch there is no need since saving the peripheral state
> +     * (and/or restoring the next VCPU) will cause the correct action.
> +     */
> +    if ( is_edge && s->pending )
> +        writel_gicd(mask, GICD_ICPENDR);
> +
> +    if ( s->enabled )
> +        gicv2_irq_disable(desc);

Don't we want to disable the IRQ first and then saving the state?

> +
> +    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
> +    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
> +}
> +
>  static void gicv2_restore_state(const struct vcpu *v)
>  {
>      int i;
> @@ -161,6 +195,37 @@ static void gicv2_restore_state(const struct vcpu *v)
>      writel_gich(GICH_HCR_EN, GICH_HCR);
>  }
>  
> +static void gicv2_restore_hwppi(struct irq_desc *desc,
> +                                const struct hwppi_state *s)
> +{
> +    const unsigned int mask = (1u << desc->irq);
> +    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> +
> +    /*
> +     * The IRQ must always have been set inactive and masked etc by
> +     * the saving of the previous state via save_and_mask_hwppi.
> +     */
> +    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
> +    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
> +
> +    if ( s->active )
> +        writel_gicd(mask, GICD_ICACTIVER);
> +
> +    /*
> +     * Restore pending state for edge triggered interrupts only. For
> +     * level triggered interrupts the level will be restored as
> +     * necessary by restoring the state of the relevant peripheral.
> +     *
> +     * For a level triggered interrupt ISPENDR acts as a *latch* which
> +     * is only cleared by ICPENDR (i.e. the input level is no longer
> +     * relevant). We certainly do not want that here.
> +     */
> +    if ( is_edge && s->pending )
> +        writel_gicd(mask, GICD_ISPENDR);

NIT: New line for clarity.

> +    if ( s->enabled )
> +        gicv2_irq_enable(desc);
> +}
> +
>  static void gicv2_dump_state(const struct vcpu *v)
>  {
>      int i;
> @@ -744,7 +809,9 @@ const static struct gic_hw_operations gicv2_ops = {
>      .init                = gicv2_init,
>      .secondary_init      = gicv2_secondary_cpu_init,
>      .save_state          = gicv2_save_state,
> +    .save_and_mask_hwppi = gicv2_save_and_mask_hwppi,
>      .restore_state       = gicv2_restore_state,
> +    .restore_hwppi       = gicv2_restore_hwppi,
>      .dump_state          = gicv2_dump_state,
>      .gic_host_irq_type   = &gicv2_host_irq_type,
>      .gic_guest_irq_type  = &gicv2_guest_irq_type,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 4fe0c37..cfe705a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -61,6 +61,9 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
>  #define GICD_RDIST_BASE        (this_cpu(rbase))
>  #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
>  
> +void gicv3_irq_enable(struct irq_desc *desc);
> +void gicv3_irq_disable(struct irq_desc *desc);
> +
>  /*
>   * Saves all 16(Max) LR registers. Though number of LRs implemented
>   * is implementation specific.
> @@ -373,6 +376,37 @@ static void gicv3_save_state(struct vcpu *v)
>      v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
>  }
>  
> +static void gicv3_save_and_mask_hwppi(struct irq_desc *desc,
> +                                      struct hwppi_state *s)
> +{
> +    const unsigned int mask = (1u << desc->irq);
> +    const unsigned int pendingr = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISPENDR);
> +    const unsigned int activer = readl_gicd(GICD_RDIST_SGI_BASE + GICR_ISACTIVER);
> +    const unsigned int enabler = readl_gicd(GICD_RDIST_SGI_BASE + GICR_ISENABLER);

Those registers don't exists. Did you try to build the GICv3 code?

[...]

>  void gic_restore_state(struct vcpu *v)
>  {
>      ASSERT(!local_irq_is_enabled());
> @@ -94,6 +124,30 @@ void gic_restore_state(struct vcpu *v)
>      gic_restore_pending_irqs(v);
>  }
>  
> +void gic_restore_hwppi(struct vcpu *v,
> +                       const unsigned virq,
> +                       const struct hwppi_state *s)
> +{
> +    struct pending_irq *p = irq_to_pending(v, virq);
> +    struct irq_desc *desc = irq_to_desc(s->irq);
> +
> +    spin_lock(&desc->lock);
> +
> +    ASSERT(virq >= 16 && virq < 32);
> +    ASSERT(!is_idle_vcpu(v));
> +
> +    p->desc = desc; /* Migrate to new physical processor */

Is it safe?

For GICv3, the re-distributor of a VCPU A could be access by VCPU B
which means that all the operations (disable/enable...) could be done
while we restore the PPI.

> +
> +    irq_set_virq(desc, virq);
> +
> +    gic_hw_ops->restore_hwppi(desc, s);
> +
> +    if ( s->inprogress )
> +        set_bit(_IRQ_INPROGRESS, &desc->status);
> +
> +    spin_unlock(&desc->lock);
> +}
> +
>  /*
>   * needs to be called with a valid cpu_mask, ie each cpu in the mask has
>   * already called gic_cpu_init
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 5b073d1..95be10e 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -138,6 +138,13 @@ static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
>      return desc->action->dev_id;
>  }
>  
> +void irq_set_virq(struct irq_desc *desc, unsigned virq)
> +{
> +    struct irq_guest *info = irq_get_guest_info(desc);
> +    ASSERT(test_bit(_IRQ_PER_CPU, &desc->status));
> +    info->virq = virq;
> +}
> +
>  static inline struct domain *irq_get_domain(struct irq_desc *desc)
>  {
>      return irq_get_guest_info(desc)->d;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index c56f06e..550e28b 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -34,6 +34,17 @@ enum domain_type {
>  extern int dom0_11_mapping;
>  #define is_domain_direct_mapped(d) ((d) == hardware_domain && dom0_11_mapping)
>  
> +struct hwppi_state {
> +    /* h/w state */
> +    unsigned irq;
> +    unsigned long enabled:1;
> +    unsigned long pending:1;
> +    unsigned long active:1;
> +
> +    /* Xen s/w state */
> +    unsigned long inprogress:1;
> +};
> +
>  struct vtimer {
>      struct vcpu *v;
>      int irq;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 0116481..fe9af3e 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -258,6 +258,20 @@ extern int gicv_setup(struct domain *d);
>  extern void gic_save_state(struct vcpu *v);
>  extern void gic_restore_state(struct vcpu *v);
>  
> +/*
> + * Save/restore the state of a single PPI which must be routed to
> + * <current-vcpu> (that is, is defined to be injected to the current
> + * vcpu).

I would add a comment here to explain that we expect the device which
use this PPI to be quiet while we save/restore.

For instance we want to disable the timer before saving the state.
Otherwise we will mess up the state.

> + */
> +struct hwppi_state;
> +extern void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq);
> +extern void gic_hwppi_set_pending(struct hwppi_state *s);
> +extern void gic_save_and_mask_hwppi(struct vcpu *v, unsigned irq,
> +                                    struct hwppi_state *s);
> +extern void gic_restore_hwppi(struct vcpu *v,
> +                              const unsigned virq,
> +                              const struct hwppi_state *s);
> +
>  /* SGI (AKA IPIs) */
>  enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
> @@ -308,8 +322,10 @@ struct gic_hw_operations {
>      int (*init)(void);
>      /* Save GIC registers */
>      void (*save_state)(struct vcpu *);
> +    void (*save_and_mask_hwppi)(struct irq_desc *desc, struct hwppi_state *s);
>      /* Restore GIC registers */
>      void (*restore_state)(const struct vcpu *);
> +    void (*restore_hwppi)(struct irq_desc *desc, const struct hwppi_state *s);
>      /* Dump GIC LR register information */
>      void (*dump_state)(const struct vcpu *);
>  
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index f33c331..d354e3b 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -43,6 +43,7 @@ void init_secondary_IRQ(void);
>  
>  int route_irq_to_guest(struct domain *d, unsigned int virq,
>                         unsigned int irq, const char *devname);
> +int route_irq_to_current_vcpu(unsigned int irq, const char *devname);

The prototype belongs to patch #6.

Regards,

-- 
Julien Grall

Re: [PATCH v2 5/8] xen: arm: add interfaces to save/restore the state of a PPI.

[Ian Campbell]

On Wed, 2015-11-11 at 12:49 +0000, Julien Grall wrote:
> 
> > +    if ( s->enabled )
> > +        gicv2_irq_disable(desc);
> 
> Don't we want to disable the IRQ first and then saving the state?

We need at least the enable/disable state first. Perhaps some of the other
stuff might be better done afterwards, I'll have a think.


> >  
> > +static void gicv3_save_and_mask_hwppi(struct irq_desc *desc,
> > +                                      struct hwppi_state *s)
> > +{
> > +    const unsigned int mask = (1u << desc->irq);
> > +    const unsigned int pendingr = readl_relaxed(GICD_RDIST_SGI_BASE +
> > GICR_ISPENDR);
> > +    const unsigned int activer = readl_gicd(GICD_RDIST_SGI_BASE +
> > GICR_ISACTIVER);
> > +    const unsigned int enabler = readl_gicd(GICD_RDIST_SGI_BASE +
> > GICR_ISENABLER);
> 
> Those registers don't exists. Did you try to build the GICv3 code?

I thought so (and was a bit surprised there were apparently all the right
defines in place). But actually I was building for arm32 and had forgotten
that doesn't build gicv3.

Ian.

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

Re: [PATCH v2 6/8] xen: arm: supporting routing a PPI to the current vcpu.

[Julien Grall]

Hi Ian,

On 10/11/15 16:21, Ian Campbell wrote:
> That is whichever vcpu is resident when the interrupt fires. An
> interrupt is in this state when both IRQ_GUEST and IRQ_PER_CPU are set
> in the descriptor status. Only PPIs can be in this mode.
> 
> This requires some peripheral specific code to make use of the
> previously introduced functionality to save and restore the PPI state.
> The vtimer driver will do so shortly.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/gic.c        | 24 ++++++++++++++
>  xen/arch/arm/irq.c        | 84 +++++++++++++++++++++++++++++++++++++++++------
>  xen/include/asm-arm/gic.h |  2 ++
>  xen/include/asm-arm/irq.h |  2 +-
>  4 files changed, 101 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index e294e89..aea913b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -177,6 +177,30 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>      gic_set_irq_properties(desc, cpu_mask, priority);
>  }
>  
> +/* Program the GIC to route an interrupt to the current guest

/*
 * Program ...

And missing full stop.

> + *
> + * That is the IRQ is delivered to whichever VCPU happens to be
> + * resident on the PCPU when the interrupt arrives.

s/That is// ?

> + *
> + * Currently the interrupt *must* be a PPI and the code responsible
> + * for the related hardware must save and restore the PPI with
> + * gic_save_and_mask_hwppi/gic_restore_hwppi.
> + */
> +int gic_route_irq_to_current_guest(struct irq_desc *desc,
> +                                   unsigned int priority)
> +{
> +    ASSERT(spin_is_locked(&desc->lock));
> +    ASSERT(desc->irq >= 16 && desc->irq < 32);
> +
> +    desc->handler = gic_hw_ops->gic_guest_irq_type;
> +    set_bit(_IRQ_GUEST, &desc->status);
> +    set_bit(_IRQ_PER_CPU, &desc->status);
> +
> +    gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
> +
> +    return 0;
> +}
> +
>  /* Program the GIC to route an interrupt to a guest
>   *   - desc.lock must be held
>   */
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 95be10e..ca18403 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -229,12 +229,15 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>          set_bit(_IRQ_INPROGRESS, &desc->status);
>  
>          /*
> -         * The irq cannot be a PPI, we only support delivery of SPIs to
> -         * guests.
> +         * A PPI exposed to a guest must always be in IRQ_GUEST|IRQ_PER_CPU
> +         * mode ("route to active VCPU"), so we use current.
> +         *
> +         * For SPI we look up the required target as normal.

I would mention that the SPI are delivered to a specific guest.

>           */
> -        v = vgic_get_target_vcpu(info->d->vcpu[0], info->virq);
> -        vgic_vcpu_inject_irq(v, info->virq);
> +        v = test_bit(_IRQ_PER_CPU, &desc->status) ? current :
> +            vgic_get_target_vcpu(info->d->vcpu[0], info->virq);
>  
> +        vgic_vcpu_inject_irq(v, info->virq);

Nit: Newline

>          goto out_no_end;
>      }
>  
> @@ -363,11 +366,15 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
>  
>      if ( test_bit(_IRQ_GUEST, &desc->status) )
>      {
> -        struct domain *d = irq_get_domain(desc);
> +        struct irq_guest *info = irq_get_guest_info(desc);
>  
>          spin_unlock_irqrestore(&desc->lock, flags);
> -        printk(XENLOG_ERR "ERROR: IRQ %u is already in use by the domain %u\n",
> -               irq, d->domain_id);
> +        if ( !test_bit(_IRQ_PER_CPU, &desc->status) )
> +            printk(XENLOG_ERR "ERROR: IRQ %u is already in use by domain %u\n",
> +                   irq, info->d->domain_id);
> +        else
> +            printk(XENLOG_ERR
> +                   "ERROR: IRQ %u is already in use by <current-vcpu>\n", irq);

AFAICT this function will be called one time per physical CPU. Could we
print something to differentiate error on CPU0 to CPU1?

>          return -EBUSY;
>      }
>  
> @@ -410,7 +417,7 @@ static int setup_guest_irq(struct irq_desc *desc, unsigned int virq,
>  {
>      const unsigned irq = desc->irq;
>      struct irqaction *action;
> -    int retval = 0;
> +    int retval;
>  
>      ASSERT(spin_is_locked(&desc->lock));
>  
> @@ -451,12 +458,21 @@ static int setup_guest_irq(struct irq_desc *desc, unsigned int virq,
>                         d->domain_id, irq, irq_get_guest_info(desc)->virq);
>                  retval = -EBUSY;
>              }
> +            else
> +                retval = 0;

I don't see why this change is necessary here. Shouldn't it belong to
patch #3?

>              goto out;
>          }
>  
>          if ( test_bit(_IRQ_GUEST, &desc->status) )
> -            printk(XENLOG_G_ERR "IRQ %u is already used by domain %u\n",
> -                   irq, ad->domain_id);
> +        {
> +            if ( !test_bit(_IRQ_PER_CPU, &desc->status) )
> +                printk(XENLOG_ERR

You switch the printk level from XENLOG_G_ERR to XENLOG_ERR. Is it wanted?

> +                       "ERROR: IRQ %u is already used by domain %u\n",
> +                       irq, ad->domain_id);
> +            else
> +                printk(XENLOG_ERR
> +                       "ERROR: IRQ %u is already used by <current-vcpu>\n", irq);

Same remark as setup_irq for the error message.

> +        }
>          else
>              printk(XENLOG_G_ERR "IRQ %u is already used by Xen\n", irq);
>          retval = -EBUSY;
> @@ -542,6 +558,54 @@ free_info:
>      return retval;
>  }
>  
> +/*
> + * Route a PPI such that it is always delivered to the current vcpu on
> + * the pcpu. The driver for the peripheral must use
> + * gic_{save_and_mask,restore}_hwppi as part of the context switch.
> + */
> +int route_hwppi_to_current_vcpu(unsigned int irq, const char *devname)
> +{
> +    struct irq_guest *info;
> +    struct irq_desc *desc;
> +    unsigned long flags;
> +    int retval = 0;
> +
> +    /* Can only route PPIs to current VCPU */
> +    if ( irq < 16 || irq >= 32 )
> +        return -EINVAL;
> +
> +    desc = irq_to_desc(irq);
> +
> +    info = xmalloc(struct irq_guest);
> +    if ( !info )
> +        return -ENOMEM;
> +
> +    info->d = NULL; /* Routed to current vcpu, so no specific domain */
> +    /* info->virq is set by gic_restore_hwppi. */
> +
> +    spin_lock_irqsave(&desc->lock, flags);
> +
> +    retval = setup_guest_irq(desc, irq, flags, info, devname);

Why do you set the parameter virq to irq?

> +    if ( retval )
> +    {
> +        xfree(info);
> +        return retval;
> +    }
> +
> +    retval = gic_route_irq_to_current_guest(desc, GIC_PRI_IRQ);
> +
> +    spin_unlock_irqrestore(&desc->lock, flags);
> +
> +    if ( retval )
> +    {
> +        release_irq(desc->irq, info);
> +        xfree(info);
> +        return retval;
> +    }
> +
> +    return 0;
> +}
> +

Regards,

-- 
Julien Grall

Re: [PATCH v2 7/8] xen: arm: context switch vtimer PPI state.

[Julien Grall]

Hi Ian,

On 10/11/15 16:21, Ian Campbell wrote:
> ... instead of artificially masking the timer interrupt in the timer
> state and relying on the guest to unmask (which it isn't required to
> do per the h/w spec, although Linux does).
> 
> By using the newly added hwppi save/restore functionality we make use
> of the GICD I[SC]ACTIVER registers to save and restore the active
> state of the interrupt, which prevents the nested invocations which
> the current masking works around.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Rebased, in particular over Julien's passthrough stuff which
>     reworked a bunch of IRQ related stuff.
>     Also largely rewritten since precursor patches now lay very
>     different groundwork.
> ---
>  xen/arch/arm/time.c          | 26 ++------------------------
>  xen/arch/arm/vtimer.c        | 41 +++++++++++++++++++++++++++++++++++++----
>  xen/include/asm-arm/domain.h |  1 +
>  3 files changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 5ded30c..2a1cdba 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -181,28 +181,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)
> -{
> -    /*
> -     * Edge-triggered interrupts can be used for the virtual timer. Even
> -     * if the timer output signal is masked in the context switch, the
> -     * GIC will keep track that of any interrupts raised while IRQS are
> -     * disabled. As soon as IRQs are re-enabled, the virtual interrupt
> -     * will be injected to Xen.
> -     *
> -     * If an IDLE vCPU was scheduled next then we should ignore the
> -     * interrupt.
> -     */
> -    if ( unlikely(is_idle_vcpu(current)) )
> -        return;
> -
> -    perfc_incr(virt_timer_irqs);

The performance counter virt_timer_irqs is not used anymore. Can you
drop it from include/asm-arm/perfc_defn.h ?

> -
> -    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);
> -}
> -
>  /*
>   * Arch timer interrupt really ought to be level triggered, since the
>   * design of the timer/comparator mechanism is based around that
> @@ -242,8 +220,8 @@ void __cpuinit init_timer_interrupt(void)
>  
>      request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
>                  "hyptimer", NULL);
> -    request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
> -                   "virtimer", NULL);
> +    route_hwppi_to_current_vcpu(timer_irq[TIMER_VIRT_PPI], "virtimer");
> +
>      request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
>                  "phytimer", NULL);

[..]

>  int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
> @@ -96,9 +106,12 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
>  
>  int vcpu_vtimer_init(struct vcpu *v)
>  {
> +    struct pending_irq *p;
>      struct vtimer *t = &v->arch.phys_timer;
>      bool_t d0 = is_hardware_domain(v->domain);
>  
> +    const unsigned host_vtimer_irq_ppi = timer_get_irq(TIMER_VIRT_PPI);
> +
>      /*
>       * Hardware domain uses the hardware interrupts, guests get the virtual
>       * platform.
> @@ -116,10 +129,16 @@ int vcpu_vtimer_init(struct vcpu *v)
>      init_timer(&t->timer, virt_timer_expired, t, v->processor);
>      t->ctl = 0;
>      t->irq = d0
> -        ? timer_get_irq(TIMER_VIRT_PPI)
> +        ? host_vtimer_irq_ppi
>          : GUEST_TIMER_VIRT_PPI;
>      t->v = v;
>  
> +    p = irq_to_pending(v, t->irq);
> +    p->irq = t->irq;
> +
> +    gic_hwppi_state_init(&v->arch.virt_timer.ppi_state,
> +                         host_vtimer_irq_ppi);
> +
>      v->arch.vtimer_initialized = 1;
>  
>      return 0;
> @@ -147,6 +166,15 @@ int virt_timer_save(struct vcpu *v)
>          set_timer(&v->arch.virt_timer.timer, ticks_to_ns(v->arch.virt_timer.cval +
>                    v->domain->arch.virt_timer_base.offset - boot_count));
>      }
> +
> +    /*
> +     * Since the vtimer irq is a PPI we don't need to worry about
> +     * racing against it becoming active while we are saving the
> +     * state, since that requires the guest to be reading the IAR.

It's true as long as the guest is not using I*ACTIVER register which we
don't yet implement.

> +     */
> +    gic_save_and_mask_hwppi(v, v->arch.virt_timer.irq,
> +                            &v->arch.virt_timer.ppi_state);
> +
>      return 0;
>  }
>  
> @@ -161,6 +189,11 @@ int virt_timer_restore(struct vcpu *v)
>      WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2);
>      WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0);
>      WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
> +
> +    gic_restore_hwppi(v,
> +                      v->arch.virt_timer.irq,
> +                      &v->arch.virt_timer.ppi_state);
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 550e28b..aff21dd 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -51,6 +51,7 @@ struct vtimer {
>      struct timer timer;
>      uint32_t ctl;
>      uint64_t cval;
> +    struct hwppi_state ppi_state;
>  };
>  
>  struct arch_domain
> 

Regards,

-- 
Julien Grall

Re: [PATCH v2 1/8] xen: arm: fix indendation of struct vtimer

[Stefano Stabellini]

On Tue, 10 Nov 2015, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/include/asm-arm/domain.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index e7e40da..c56f06e 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -35,11 +35,11 @@ extern int dom0_11_mapping;
>  #define is_domain_direct_mapped(d) ((d) == hardware_domain && dom0_11_mapping)
>  
>  struct vtimer {
> -        struct vcpu *v;
> -        int irq;
> -        struct timer timer;
> -        uint32_t ctl;
> -        uint64_t cval;
> +    struct vcpu *v;
> +    int irq;
> +    struct timer timer;
> +    uint32_t ctl;
> +    uint64_t cval;
>  };
>  
>  struct arch_domain
> -- 
> 2.1.4
> 

Re: [PATCH v2 2/8] xen: arm: fix typo in the description of struct pending_irq->desc

[Stefano Stabellini]

On Tue, 10 Nov 2015, Ian Campbell wrote:
> s/it/if/ makes more sense.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/include/asm-arm/vgic.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index cb51a9e..7d580cc 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -66,7 +66,7 @@ struct pending_irq
>  #define GIC_IRQ_GUEST_ENABLED  3
>  #define GIC_IRQ_GUEST_MIGRATING   4
>      unsigned long status;
> -    struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
> +    struct irq_desc *desc; /* only set if the irq corresponds to a physical irq */
>      unsigned int irq;
>  #define GIC_INVALID_LR         ~(uint8_t)0
>      uint8_t lr;
> -- 
> 2.1.4
> 

Re: [PATCH v2 4/8] xen: arm: remove vgic_vcpu_inject_spi()

[Stefano Stabellini]

On Tue, 10 Nov 2015, Ian Campbell wrote:
> Currently this has only a single caller, which I intend to teach about
> injecting PPIs shortly. This helper doesn't add much so remove it.
> 
> Drop a stray tab from the comment immediately preceding this change.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/irq.c         |  7 +++++--
>  xen/arch/arm/vgic.c        | 11 -----------
>  xen/include/asm-arm/vgic.h |  1 -
>  3 files changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 6918438..5b073d1 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -214,6 +214,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>      if ( test_bit(_IRQ_GUEST, &desc->status) )
>      {
>          struct irq_guest *info = irq_get_guest_info(desc);
> +        struct vcpu *v;
>  
>          perfc_incr(guest_irqs);
>          desc->handler->end(desc);
> @@ -223,8 +224,10 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>          /*
>           * The irq cannot be a PPI, we only support delivery of SPIs to
>           * guests.
> -	 */
> -        vgic_vcpu_inject_spi(info->d, info->virq);
> +         */
> +        v = vgic_get_target_vcpu(info->d->vcpu[0], info->virq);
> +        vgic_vcpu_inject_irq(v, info->virq);
> +
>          goto out_no_end;
>      }
>  
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7bb4570..7a76f00 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -477,17 +477,6 @@ out:
>      }
>  }
>  
> -void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq)
> -{
> -    struct vcpu *v;
> -
> -    /* the IRQ needs to be an SPI */
> -    ASSERT(virq >= 32 && virq <= vgic_num_irqs(d));
> -
> -    v = vgic_get_target_vcpu(d->vcpu[0], virq);
> -    vgic_vcpu_inject_irq(v, virq);
> -}
> -
>  void arch_evtchn_inject(struct vcpu *v)
>  {
>      vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 7d580cc..aa675cb 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -207,7 +207,6 @@ extern void domain_vgic_free(struct domain *d);
>  extern int vcpu_vgic_init(struct vcpu *v);
>  extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
>  extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
> -extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
>  extern void vgic_clear_pending_irqs(struct vcpu *v);
>  extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
>  extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq);
> -- 
> 2.1.4
> 

Re: [PATCH v2 7/8] xen: arm: context switch vtimer PPI state.

[Stefano Stabellini]

On Tue, 10 Nov 2015, Ian Campbell wrote:
> ... instead of artificially masking the timer interrupt in the timer
> state and relying on the guest to unmask (which it isn't required to
> do per the h/w spec, although Linux does).
> 
> By using the newly added hwppi save/restore functionality we make use
> of the GICD I[SC]ACTIVER registers to save and restore the active
> state of the interrupt, which prevents the nested invocations which
> the current masking works around.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Rebased, in particular over Julien's passthrough stuff which
>     reworked a bunch of IRQ related stuff.
>     Also largely rewritten since precursor patches now lay very
>     different groundwork.
> ---
>  xen/arch/arm/time.c          | 26 ++------------------------
>  xen/arch/arm/vtimer.c        | 41 +++++++++++++++++++++++++++++++++++++----
>  xen/include/asm-arm/domain.h |  1 +
>  3 files changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 5ded30c..2a1cdba 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -181,28 +181,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)
> -{
> -    /*
> -     * Edge-triggered interrupts can be used for the virtual timer. Even
> -     * if the timer output signal is masked in the context switch, the
> -     * GIC will keep track that of any interrupts raised while IRQS are
> -     * disabled. As soon as IRQs are re-enabled, the virtual interrupt
> -     * will be injected to Xen.
> -     *
> -     * If an IDLE vCPU was scheduled next then we should ignore the
> -     * interrupt.
> -     */
> -    if ( unlikely(is_idle_vcpu(current)) )
> -        return;
> -
> -    perfc_incr(virt_timer_irqs);
> -
> -    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);
> -}
> -
>  /*
>   * Arch timer interrupt really ought to be level triggered, since the
>   * design of the timer/comparator mechanism is based around that
> @@ -242,8 +220,8 @@ void __cpuinit init_timer_interrupt(void)
>  
>      request_irq(timer_irq[TIMER_HYP_PPI], 0, timer_interrupt,
>                  "hyptimer", NULL);
> -    request_irq(timer_irq[TIMER_VIRT_PPI], 0, vtimer_interrupt,
> -                   "virtimer", NULL);
> +    route_hwppi_to_current_vcpu(timer_irq[TIMER_VIRT_PPI], "virtimer");
> +
>      request_irq(timer_irq[TIMER_PHYS_NONSECURE_PPI], 0, timer_interrupt,
>                  "phytimer", NULL);
>  
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 1418092..82e2b88 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -55,9 +55,19 @@ static void phys_timer_expired(void *data)
>  static void virt_timer_expired(void *data)
>  {
>      struct vtimer *t = data;
> -    t->ctl |= CNTx_CTL_MASK;
> -    vgic_vcpu_inject_irq(t->v, t->irq);
> -    perfc_incr(vtimer_virt_inject);
> +    t->ctl |= CNTx_CTL_PENDING;
> +    if ( !(t->ctl & CNTx_CTL_MASK) )
> +    {
> +        /*
> +         * An edge triggered interrupt should now be pending. Since
> +         * this timer can never expire while the domain is scheduled
> +         * we know that the gic_restore_hwppi in virt_timer_restore
> +         * will cause the real hwppi to occur and be routed.
> +         */
> +        gic_hwppi_set_pending(&t->ppi_state);

I wonder if calling gic_hwppi_set_pending is actually redundant:
virt_timer_restore will write the expired time to cval, causing a new
interrupt to be immediately sent as soon as it's enabled, right?


> +        vcpu_unblock(t->v);
> +        perfc_incr(vtimer_virt_inject);
> +    }
>  }
>  
>  int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
> @@ -96,9 +106,12 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
>  
>  int vcpu_vtimer_init(struct vcpu *v)
>  {
> +    struct pending_irq *p;
>      struct vtimer *t = &v->arch.phys_timer;
>      bool_t d0 = is_hardware_domain(v->domain);
>  
> +    const unsigned host_vtimer_irq_ppi = timer_get_irq(TIMER_VIRT_PPI);
> +
>      /*
>       * Hardware domain uses the hardware interrupts, guests get the virtual
>       * platform.
> @@ -116,10 +129,16 @@ int vcpu_vtimer_init(struct vcpu *v)
>      init_timer(&t->timer, virt_timer_expired, t, v->processor);
>      t->ctl = 0;
>      t->irq = d0
> -        ? timer_get_irq(TIMER_VIRT_PPI)
> +        ? host_vtimer_irq_ppi
>          : GUEST_TIMER_VIRT_PPI;
>      t->v = v;
>  
> +    p = irq_to_pending(v, t->irq);
> +    p->irq = t->irq;
> +
> +    gic_hwppi_state_init(&v->arch.virt_timer.ppi_state,
> +                         host_vtimer_irq_ppi);
> +
>      v->arch.vtimer_initialized = 1;
>  
>      return 0;
> @@ -147,6 +166,15 @@ int virt_timer_save(struct vcpu *v)
>          set_timer(&v->arch.virt_timer.timer, ticks_to_ns(v->arch.virt_timer.cval +
>                    v->domain->arch.virt_timer_base.offset - boot_count));
>      }
> +
> +    /*
> +     * Since the vtimer irq is a PPI we don't need to worry about
> +     * racing against it becoming active while we are saving the
> +     * state, since that requires the guest to be reading the IAR.
> +     */
> +    gic_save_and_mask_hwppi(v, v->arch.virt_timer.irq,
> +                            &v->arch.virt_timer.ppi_state);
> +
>      return 0;
>  }
>  
> @@ -161,6 +189,11 @@ int virt_timer_restore(struct vcpu *v)
>      WRITE_SYSREG64(v->domain->arch.virt_timer_base.offset, CNTVOFF_EL2);
>      WRITE_SYSREG64(v->arch.virt_timer.cval, CNTV_CVAL_EL0);
>      WRITE_SYSREG32(v->arch.virt_timer.ctl, CNTV_CTL_EL0);
> +
> +    gic_restore_hwppi(v,
> +                      v->arch.virt_timer.irq,
> +                      &v->arch.virt_timer.ppi_state);
> +
>      return 0;
>  }
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 550e28b..aff21dd 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -51,6 +51,7 @@ struct vtimer {
>      struct timer timer;
>      uint32_t ctl;
>      uint64_t cval;
> +    struct hwppi_state ppi_state;
>  };
>  
>  struct arch_domain
> -- 
> 2.1.4
>