| Message ID | 1421159133-31526-11-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Ian, On 20/02/15 16:07, Ian Campbell wrote: > More importantly: We have (hopefully) guaranteed elsewhere that an PPI > or SGI can never make it here, I take it. If that's the case then either > the comment should say that, or more likely, the comment is redundently > restating the assert's condition. I will update the comment to /* Caller has already checked that the IRQ is an SPIs */ >> + ASSERT(virq >= 32 && virq < vgic_num_irqs(d)); > > NR_LOCAL_IRQS? Yes. > Also splitting the two conditions into two asserts will make it more > obvious which one failed if we hit it. Will do. >> + >> + vgic_lock_rank(v_target, rank, flags); >> + >> + if ( p->desc || >> + /* The VIRQ should not be already enabled by the guest */ >> + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) >> + goto out; >> >> desc->handler = gic_hw_ops->gic_guest_irq_type; >> set_bit(_IRQ_GUEST, &desc->status); >> >> - gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); >> + gic_set_irq_properties(desc, cpumask_of(v_target->processor), priority); > > This smells like a functional change, not a sanity check, what is it > for? Hmmm right. I will move it to a separate patch. > Is v_target->processor always configured, even for the first routing of > an IRQ to dom0? Yes, IRQ are routed to CPU0 by default. > > Care needs to be taken here that priority is not under unfettered guest > control -- since this configures the physical GIC we need to e.g. ensure > that Xen's own IPIs have higher priority than anything a guest can ever > set. (Realistically this probably means we want to constrain guests to > the bottom half of the priority range and expose different BPR etc in > the vgic, out of scope here though) The priority is controlled by route_irq_to_guest and set statically using GIC_PRI_IRQ. If we decide to hardcoded the priority here, we should drop the parameter on gic_route_irq_guest. But not keeping both. Regards,
Hi Ian, On 23/02/15 15:20, Ian Campbell wrote: > On Fri, 2015-02-20 at 17:28 +0000, Julien Grall wrote: >> The priority is controlled by route_irq_to_guest and set statically >> using GIC_PRI_IRQ. >> >> If we decide to hardcoded the priority here, we should drop the >> parameter on gic_route_irq_guest. But not keeping both. > > There is a middle ground, which is for guest-routed IRQs to be allowed a > subset of the real priorities, but until those associated checks are in > place I think hardcoding in gic_route_irq_to_guest leaves less scope for > mistakes. The interface for routing an IRQ to xen (gic_route_irq_to_xen) is taking the priority in parameter. I would prefer if we keep the same interface for guest and then hardcode the value in route_irq_to_guest. Regards,
On 23/02/15 15:52, Ian Campbell wrote: > On Mon, 2015-02-23 at 15:47 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 23/02/15 15:20, Ian Campbell wrote: >>> On Fri, 2015-02-20 at 17:28 +0000, Julien Grall wrote: >>>> The priority is controlled by route_irq_to_guest and set statically >>>> using GIC_PRI_IRQ. >>>> >>>> If we decide to hardcoded the priority here, we should drop the >>>> parameter on gic_route_irq_guest. But not keeping both. >>> >>> There is a middle ground, which is for guest-routed IRQs to be allowed a >>> subset of the real priorities, but until those associated checks are in >>> place I think hardcoding in gic_route_irq_to_guest leaves less scope for >>> mistakes. >> >> The interface for routing an IRQ to xen (gic_route_irq_to_xen) is taking >> the priority in parameter. > > It's useful and safe in the route to xen case. > >> I would prefer if we keep the same interface for guest and then hardcode >> the value in route_irq_to_guest. > > In which case I think route_irq_to_guest should complain/error-out if > the priority given is not the default one until such a time as it > understands which inputs are safe. I guess you mean gic_route_irq_to_guest? I could add a check and error-out for this case. Regards,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 15de283..240870f 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -126,22 +126,40 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, /* Program the GIC to route an interrupt to a guest * - desc.lock must be held */ -void gic_route_irq_to_guest(struct domain *d, unsigned int virq, - struct irq_desc *desc, - const cpumask_t *cpu_mask, unsigned int priority) +int gic_route_irq_to_guest(struct domain *d, unsigned int virq, + struct irq_desc *desc, unsigned int priority) { - struct pending_irq *p; + unsigned long flags; + /* Use vcpu0 to retrieve the pending_irq struct. Given that we only + * route SPIs to guests, it doesn't make any difference. */ + struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq); + struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq); + struct pending_irq *p = irq_to_pending(v_target, virq); + int res = -EBUSY; + ASSERT(spin_is_locked(&desc->lock)); + /* We only support SPIs */ + ASSERT(virq >= 32 && virq < vgic_num_irqs(d)); + + vgic_lock_rank(v_target, rank, flags); + + if ( p->desc || + /* The VIRQ should not be already enabled by the guest */ + test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) + goto out; desc->handler = gic_hw_ops->gic_guest_irq_type; set_bit(_IRQ_GUEST, &desc->status); - gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ); + gic_set_irq_properties(desc, cpumask_of(v_target->processor), priority); - /* Use vcpu0 to retrieve the pending_irq struct. Given that we only - * route SPIs to guests, it doesn't make any difference. */ - p = irq_to_pending(d->vcpu[0], virq); p->desc = desc; + res = 0; + +out: + vgic_unlock_rank(v_target, rank, flags); + + return res; } int gic_irq_xlate(const u32 *intspec, unsigned int intsize, diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index af408ac..0072347 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -483,14 +483,22 @@ int route_irq_to_guest(struct domain *d, unsigned int virq, if ( retval ) goto out; - gic_route_irq_to_guest(d, virq, desc, cpumask_of(smp_processor_id()), - GIC_PRI_IRQ); + retval = gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ); + spin_unlock_irqrestore(&desc->lock, flags); + + if ( retval ) + { + release_irq(desc->irq, info); + goto free_info; + } + return 0; out: spin_unlock_irqrestore(&desc->lock, flags); xfree(action); +free_info: xfree(info); return retval; diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index cf9f257..a8ac294 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -213,10 +213,9 @@ extern enum gic_version gic_hw_version(void); /* Program the GIC to route an interrupt */ extern void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask, unsigned int priority); -extern void gic_route_irq_to_guest(struct domain *, unsigned int virq, - struct irq_desc *desc, - const cpumask_t *cpu_mask, - unsigned int priority); +extern int gic_route_irq_to_guest(struct domain *, unsigned int virq, + struct irq_desc *desc, + unsigned int priority); extern void gic_inject(void); extern void gic_clear_pending_irqs(struct vcpu *v);
With the addition of interrupt assignment to guest, we need to make sure the guest don't blow up the interrupt management in Xen. Before associating the IRQ to a vIRQ we need to make sure: - the vIRQ is not already associated to another IRQ - the guest didn't enable the vIRQ Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v3: - Patch added --- xen/arch/arm/gic.c | 34 ++++++++++++++++++++++++++-------- xen/arch/arm/irq.c | 12 ++++++++++-- xen/include/asm-arm/gic.h | 7 +++---- 3 files changed, 39 insertions(+), 14 deletions(-)