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

[Julien Grall <julien.grall@citrix.com>]

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