Mailing List Archive: [PATCH 2/2] xen/arm: initialize the GIC irq properties of interrupts routed to guests

[PATCH 2/2] xen/arm: initialize the GIC irq properties of interrupts routed to guests

Jan 9, 2013, 8:17 AM

Post #1 of 3 (597 views)

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

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

Re: [PATCH 2/2] xen/arm: initialize the GIC irq properties of interrupts routed to guests [ In reply to ]

Ian.Campbell at citrix

Jan 9, 2013, 8:29 AM

Post #2 of 3 (567 views)

Permalink

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;
> }

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

Re: [PATCH 2/2] xen/arm: initialize the GIC irq properties of interrupts routed to guests [ In reply to ]

stefano.stabellini at eu

Jan 9, 2013, 9:23 AM

Post #3 of 3 (567 views)

Permalink

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.

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