Re: [PATCH 22/57] ARM: GIC: Allow tweaking the active and pending state of an IRQ

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

Hi Andre,

On 05/03/18 16:03, Andre Przywara wrote:
> When playing around with hardware mapped, level triggered virtual IRQs,
> there is the need to explicitly set the active or pending state of an
> interrupt at some point.
> To prepare the GIC for that, we introduce a set_active_state() and a
> set_pending_state() function to let the VGIC manipulate the state of
> an associated hardware IRQ.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
> Changelog RFC ... v1:
> - use struct irq_desc* in the interface (instead of just the IRQ number)
> - add set_pending_state() (needed later)
> 
>   xen/arch/arm/gic-v2.c     | 32 ++++++++++++++++++++++++++++++++
>   xen/arch/arm/gic-v3.c     | 28 ++++++++++++++++++++++++++++
>   xen/arch/arm/gic.c        | 10 ++++++++++
>   xen/include/asm-arm/gic.h | 10 ++++++++++
>   4 files changed, 80 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index c5ec0d4d35..74169b5633 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -241,6 +241,36 @@ static void gicv2_poke_irq(struct irq_desc *irqd, uint32_t offset)
>       writel_gicd(1U << (irqd->irq % 32), offset + (irqd->irq / 32) * 4);
>   }
>   
> +static void gicv2_set_active_state(struct irq_desc *irqd, bool active)
> +{
> +    ASSERT(spin_is_locked(&irqd->lock));
> +
> +    if ( active )
> +    {
> +        set_bit(_IRQ_INPROGRESS, &irqd->status); > +        gicv2_poke_irq(irqd, GICD_ISACTIVER);
> +    }
> +    else
> +    {

Why don't you clear _IRQ_INPROGRESS here?

> +        gicv2_poke_irq(irqd, GICD_ICACTIVER);
> +    }
> +}
> +
> +static void gicv2_set_pending_state(struct irq_desc *irqd, bool pending)
> +{
> +    ASSERT(spin_is_locked(&irqd->lock));
> +
> +    if ( pending )
> +    {
> +        set_bit(_IRQ_INPROGRESS, &irqd->status);

Why do you set _IRQ_INPROGRESS here? If you set the hardware interrupt 
pending, it will fire and then set this bit for you.

> +        gicv2_poke_irq(irqd, GICD_ISPENDR);
> +    }
> +    else
> +    {
> +        gicv2_poke_irq(irqd, GICD_ICPENDR);
> +    }
> +}
> +
>   static void gicv2_set_irq_type(struct irq_desc *desc, unsigned int type)
>   {
>       uint32_t cfg, actual, edgebit;
> @@ -1251,6 +1281,8 @@ const static struct gic_hw_operations gicv2_ops = {
>       .eoi_irq             = gicv2_eoi_irq,
>       .deactivate_irq      = gicv2_dir_irq,
>       .read_irq            = gicv2_read_irq,
> +    .set_active_state    = gicv2_set_active_state,
> +    .set_pending_state   = gicv2_set_pending_state,
>       .set_irq_type        = gicv2_set_irq_type,
>       .set_irq_priority    = gicv2_set_irq_priority,
>       .send_SGI            = gicv2_send_SGI,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 44dfba2267..c96469f09d 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c

My remark are the same as GICv2.

> @@ -477,6 +477,32 @@ static unsigned int gicv3_read_irq(void)
>       return irq;
>   }
>   
> +static void gicv3_set_active_state(struct irq_desc *irqd, bool active)
> +{
> +    ASSERT(spin_is_locked(&irqd->lock));
> +
> +    if ( active )
> +    {
> +        set_bit(_IRQ_INPROGRESS, &irqd->status);
> +        gicv3_poke_irq(irqd, GICD_ISACTIVER, false);
> +    }
> +    else
> +        gicv3_poke_irq(irqd, GICD_ICACTIVER, false);
> +}
> +
> +static void gicv3_set_pending_state(struct irq_desc *irqd, bool pending)
> +{
> +    ASSERT(spin_is_locked(&irqd->lock));
> +
> +    if ( pending )
> +    {
> +        set_bit(_IRQ_INPROGRESS, &irqd->status);
> +        gicv3_poke_irq(irqd, GICD_ISPENDR, false);
> +    }
> +    else
> +        gicv3_poke_irq(irqd, GICD_ICPENDR, false);
> +}
> +
>   static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
>   {
>        uint64_t mpidr = cpu_logical_map(cpu);
> @@ -1723,6 +1749,8 @@ static const struct gic_hw_operations gicv3_ops = {
>       .eoi_irq             = gicv3_eoi_irq,
>       .deactivate_irq      = gicv3_dir_irq,
>       .read_irq            = gicv3_read_irq,
> +    .set_active_state    = gicv3_set_active_state,
> +    .set_pending_state   = gicv3_set_pending_state,
>       .set_irq_type        = gicv3_set_irq_type,
>       .set_irq_priority    = gicv3_set_irq_priority,
>       .send_SGI            = gicv3_send_sgi,
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 968e46fabb..f1329a630a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -87,6 +87,16 @@ void gic_restore_state(struct vcpu *v)
>       isb();
>   }
>   
> +void gic_set_active_state(struct irq_desc *irqd, bool state)
> +{
> +    gic_hw_ops->set_active_state(irqd, state);
> +}
> +
> +void gic_set_pending_state(struct irq_desc *irqd, bool state)
> +{
> +    gic_hw_ops->set_pending_state(irqd, state);
> +}

This possibly can be static inline in gic.h?

> +
>   /* desc->irq needs to be disabled before calling this function */
>   void gic_set_irq_type(struct irq_desc *desc, unsigned int type)
>   {
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 89a07ae6b4..46dcb0fe7c 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -239,6 +239,12 @@ DECLARE_PER_CPU(uint64_t, lr_mask);
>   
>   extern enum gic_version gic_hw_version(void);
>   
> +/* Force the active state of an IRQ. */
> +void gic_set_active_state(struct irq_desc *irqd, bool state);
> +
> +/* Force the pending state of an IRQ. */
> +void gic_set_pending_state(struct irq_desc *irqd, bool state);
> +
>   /* Program the IRQ type into the GIC */
>   void gic_set_irq_type(struct irq_desc *desc, unsigned int type);
>   
> @@ -348,6 +354,10 @@ struct gic_hw_operations {
>       void (*deactivate_irq)(struct irq_desc *irqd);
>       /* Read IRQ id and Ack */
>       unsigned int (*read_irq)(void);
> +    /* Force the active state of an IRQ by accessing the distributor */
> +    void (*set_active_state)(struct irq_desc *irqd, bool state);
> +    /* Force the pending state of an IRQ by accessing the distributor */
> +    void (*set_pending_state)(struct irq_desc *irqd, bool state);

Based on the discussion we had today, could expand the comment saying 
that anyone who wants to use those 2 helpers need to carefully think 
before calling them?

>       /* Set IRQ type */
>       void (*set_irq_type)(struct irq_desc *desc, unsigned int type);
>       /* Set IRQ priority */ >

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel