Re: [PATCH v3 10/24] xen/arm: gic: Add sanity checks gic_route_irq_to_guest

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-01-13 at 14:25 +0000, Julien Grall wrote:
> With the addition of interrupt assignment to guest, we need to make sure
> the guest don't blow up the interrupt management in Xen.

s/don't/can't/

> 
> 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(-)
> 
> 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 */

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.

> +    ASSERT(virq >= 32 && virq < vgic_num_irqs(d));

NR_LOCAL_IRQS?

Also splitting the two conditions into two asserts will make it more
obvious which one failed if we hit it.

> +
> +    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?

Is v_target->processor always configured, even for the first routing of
an IRQ to dom0?

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)

Ian.