Stefano Stabellini
2013-Jan-09 16:17 UTC
[PATCH 2/2] xen/arm: initialize the GIC irq properties of interrupts routed to guests
We are currently initializing GIC irq properties (ITARGETSR, IPRIORITYR,
and GICD_ICFGR) only in gic_route_irq, that is not called for guest
interrupts at all.
Move the initialization into a separate function
(gic_set_irq_properties) and call it from gic_route_irq_to_guest.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
xen/arch/arm/gic.c | 49 +++++++++++++++++++++++++++++++------------------
1 files changed, 31 insertions(+), 18 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 8c00a1c..9f20c0f 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -174,12 +174,36 @@ static hw_irq_controller gic_guest_irq_type = {
.set_affinity = gic_irq_set_affinity,
};
+/* needs to be called with gic.lock held */
+static void gic_set_irq_properties(unsigned int irq, bool_t level,
+ unsigned int cpu_mask, unsigned int priority)
+{
+ volatile unsigned char *bytereg;
+ uint32_t cfg, edgebit;
+
+ /* Set edge / level */
+ cfg = GICD[GICD_ICFGR + irq / 16];
+ edgebit = 2u << (2 * (irq % 16));
+ if ( level )
+ cfg &= ~edgebit;
+ else
+ cfg |= edgebit;
+ GICD[GICD_ICFGR + irq / 16] = cfg;
+
+ /* Set target CPU mask (RAZ/WI on uniprocessor) */
+ bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
+ bytereg[irq] = cpu_mask;
+
+ /* Set priority */
+ bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR);
+ bytereg[irq] = priority;
+
+}
+
/* Program the GIC to route an interrupt */
static int gic_route_irq(unsigned int irq, bool_t level,
unsigned int cpu_mask, unsigned int priority)
{
- volatile unsigned char *bytereg;
- uint32_t cfg, edgebit;
struct irq_desc *desc = irq_to_desc(irq);
unsigned long flags;
@@ -202,22 +226,7 @@ static int gic_route_irq(unsigned int irq, bool_t level,
/* Disable interrupt */
desc->handler->shutdown(desc);
- /* Set edge / level */
- cfg = GICD[GICD_ICFGR + irq / 16];
- edgebit = 2u << (2 * (irq % 16));
- if ( level )
- cfg &= ~edgebit;
- else
- cfg |= edgebit;
- GICD[GICD_ICFGR + irq / 16] = cfg;
-
- /* Set target CPU mask (RAZ/WI on uniprocessor) */
- bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
- bytereg[irq] = cpu_mask;
-
- /* Set priority */
- bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR);
- bytereg[irq] = priority;
+ gic_set_irq_properties(irq, level, cpu_mask, priority);
spin_unlock(&gic.lock);
spin_unlock_irqrestore(&desc->lock, flags);
@@ -555,10 +564,13 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int
irq,
action->name = devname;
spin_lock_irqsave(&desc->lock, flags);
+ spin_lock(&gic.lock);
desc->handler = &gic_guest_irq_type;
desc->status |= IRQ_GUEST;
+ gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0);
+
retval = __setup_irq(desc, irq, action);
if (retval) {
xfree(action);
@@ -566,6 +578,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int
irq,
}
out:
+ spin_unlock(&gic.lock);
spin_unlock_irqrestore(&desc->lock, flags);
return retval;
}
--
1.7.2.5
Ian Campbell
2013-Jan-09 16:29 UTC
Re: [PATCH 2/2] xen/arm: initialize the GIC irq properties of interrupts routed to guests
On Wed, 2013-01-09 at 16:17 +0000, Stefano Stabellini wrote:> We are currently initializing GIC irq properties (ITARGETSR, IPRIORITYR, > and GICD_ICFGR) only in gic_route_irq, that is not called for guest > interrupts at all. > Move the initialization into a separate function > (gic_set_irq_properties) and call it from gic_route_irq_to_guest.Should this all be done in response to emulating the writes to the GICD which the guest itself makes? e.g. ITARGETSR needs to be emulated to map to VCPUs etc. Or maybe we want such interrupts to always come to any PCPU and we then take VCPUs based purely on an emulated ITARGETSR? Hrm, yes, I think I''ve convinced myself your approach is right, at least until we start doing direct interrupt injection to guests. (I''ll review the actual patch later)> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > xen/arch/arm/gic.c | 49 +++++++++++++++++++++++++++++++------------------ > 1 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 8c00a1c..9f20c0f 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -174,12 +174,36 @@ static hw_irq_controller gic_guest_irq_type = { > .set_affinity = gic_irq_set_affinity, > }; > > +/* needs to be called with gic.lock held */ > +static void gic_set_irq_properties(unsigned int irq, bool_t level, > + unsigned int cpu_mask, unsigned int priority) > +{ > + volatile unsigned char *bytereg; > + uint32_t cfg, edgebit; > + > + /* Set edge / level */ > + cfg = GICD[GICD_ICFGR + irq / 16]; > + edgebit = 2u << (2 * (irq % 16)); > + if ( level ) > + cfg &= ~edgebit; > + else > + cfg |= edgebit; > + GICD[GICD_ICFGR + irq / 16] = cfg; > + > + /* Set target CPU mask (RAZ/WI on uniprocessor) */ > + bytereg = (unsigned char *) (GICD + GICD_ITARGETSR); > + bytereg[irq] = cpu_mask; > + > + /* Set priority */ > + bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR); > + bytereg[irq] = priority; > + > +} > + > /* Program the GIC to route an interrupt */ > static int gic_route_irq(unsigned int irq, bool_t level, > unsigned int cpu_mask, unsigned int priority) > { > - volatile unsigned char *bytereg; > - uint32_t cfg, edgebit; > struct irq_desc *desc = irq_to_desc(irq); > unsigned long flags; > > @@ -202,22 +226,7 @@ static int gic_route_irq(unsigned int irq, bool_t level, > /* Disable interrupt */ > desc->handler->shutdown(desc); > > - /* Set edge / level */ > - cfg = GICD[GICD_ICFGR + irq / 16]; > - edgebit = 2u << (2 * (irq % 16)); > - if ( level ) > - cfg &= ~edgebit; > - else > - cfg |= edgebit; > - GICD[GICD_ICFGR + irq / 16] = cfg; > - > - /* Set target CPU mask (RAZ/WI on uniprocessor) */ > - bytereg = (unsigned char *) (GICD + GICD_ITARGETSR); > - bytereg[irq] = cpu_mask; > - > - /* Set priority */ > - bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR); > - bytereg[irq] = priority; > + gic_set_irq_properties(irq, level, cpu_mask, priority); > > spin_unlock(&gic.lock); > spin_unlock_irqrestore(&desc->lock, flags); > @@ -555,10 +564,13 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq, > action->name = devname; > > spin_lock_irqsave(&desc->lock, flags); > + spin_lock(&gic.lock); > > desc->handler = &gic_guest_irq_type; > desc->status |= IRQ_GUEST; > > + gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0); > + > retval = __setup_irq(desc, irq, action); > if (retval) { > xfree(action); > @@ -566,6 +578,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq, > } > > out: > + spin_unlock(&gic.lock); > spin_unlock_irqrestore(&desc->lock, flags); > return retval; > }
Stefano Stabellini
2013-Jan-09 17:23 UTC
Re: [PATCH 2/2] xen/arm: initialize the GIC irq properties of interrupts routed to guests
On Wed, 9 Jan 2013, Ian Campbell wrote:> On Wed, 2013-01-09 at 16:17 +0000, Stefano Stabellini wrote: > > We are currently initializing GIC irq properties (ITARGETSR, IPRIORITYR, > > and GICD_ICFGR) only in gic_route_irq, that is not called for guest > > interrupts at all. > > Move the initialization into a separate function > > (gic_set_irq_properties) and call it from gic_route_irq_to_guest. > > Should this all be done in response to emulating the writes to the GICD > which the guest itself makes?I don''t think so: no matter what the guest does we ought to provide a sensible default to make sure that the GIC behaves correctly. Of course we should also emulate those registers correctly and maybe set the hardware in response to a guest write too.> e.g. ITARGETSR needs to be emulated to map to VCPUs etc. Or maybe we > want such interrupts to always come to any PCPU and we then take VCPUs > based purely on an emulated ITARGETSR?Yeah, we might as well do that until we do direct interrupt injection.