Re: [PATCH v2 09/45] ARM: GIC: Allow tweaking the active and pending state of an IRQ

[Andre Przywara <andre.przywara@linaro.org>]

Hi,

On 15/03/18 20:30, 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.
> This takes care of properly setting the _IRQ_INPROGRESS bit.

after having discussed this with Julien off-line, we can simplify the
_IRQ_INPROGRESS setting. See below:

> For this it adds gicv2/3_peek_irq() helper functions, to read a bit in a
> bitmap spread over several MMIO registers.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
> Changelog v1 ... v2:
> - properly set _IRQ_INPROGRESS bit
> - add gicv[23]_peek_irq() (pulled in from later patch)
> - move wrappers functions into gic.h
> 
>  xen/arch/arm/gic-v2.c     | 48 +++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c     | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/gic.h | 24 ++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 7dfe6fc68d..c6fcbf59d0 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -243,6 +243,52 @@ static void gicv2_poke_irq(struct irq_desc *irqd, uint32_t offset)
>      writel_gicd(1U << (irqd->irq % 32), offset + (irqd->irq / 32) * 4);
>  }
>  
> +static bool gicv2_peek_irq(struct irq_desc *irqd, uint32_t offset)
> +{
> +    uint32_t reg;
> +
> +    reg = readl_gicd(offset + (irqd->irq / 32) * 4) & (1U << (irqd->irq % 32));
> +
> +    return reg;
> +}
> +
> +static void gicv2_set_active_state(struct irq_desc *irqd, bool active)
> +{
> +    ASSERT(spin_is_locked(&irqd->lock));
> +
> +    if ( active )
> +    {
> +        if ( test_bit(_IRQ_GUEST, &irqd->status) )
> +            set_bit(_IRQ_INPROGRESS, &irqd->status);
> +        gicv2_poke_irq(irqd, GICD_ISACTIVER);
> +    }
> +    else
> +    {
> +        gicv2_poke_irq(irqd, GICD_ICACTIVER);
> +        if ( !gicv2_peek_irq(irqd, GICD_ISPENDR) &&
> +             test_bit(_IRQ_GUEST, &irqd->status) )
> +            clear_bit(_IRQ_INPROGRESS, &irqd->status);

The check for the pending bit shouldn't be necessary:
- If the interrupt is (still) pending, clearing the active bit should
trigger it again. The _IRQ_INPROGRESS bit will be set in turn.
- If the interrupt is not pending, we need to clear the bit anyway.

So reading the pending state is not necessary, we can always
unconditionally clear the _IRQ_INPROGRESS bit, ideally before writing to
ICACTIVER.

> +    }
> +}
> +
> +static void gicv2_set_pending_state(struct irq_desc *irqd, bool pending)
> +{
> +    ASSERT(spin_is_locked(&irqd->lock));
> +
> +    if ( pending )
> +    {
> +        /* The INPROGRESS bit will be set when the interrupt fires. */
> +        gicv2_poke_irq(irqd, GICD_ISPENDR);
> +    }
> +    else
> +    {
> +        gicv2_poke_irq(irqd, GICD_ICPENDR);
> +        if ( !gicv2_peek_irq(irqd, GICD_ISACTIVER) &&
> +             test_bit(_IRQ_GUEST, &irqd->status) )
> +            clear_bit(_IRQ_INPROGRESS, &irqd->status);

We should not need to touch the _IRQ_INPROGRESS bit here. That bit
really shadows the *active* bit, so changing the pending state should
not matter here:
- If the h/w IRQ is active, the bit is set already and should remain so,
as Xen and the guest are still dealing with it. Clearing the h/w pending
state does not change that.
- If the h/w IRQ is not active, the _IRQ_INPROGRESS bit is not set, so
clearing it would be a NOP.
So we can remove  the _IRQ_INPROGRESS handling here completely.

I will amend the code accordingly, including the respective GICv3 parts.

Cheers,
Andre.

> +    }
> +}
> +
>  static void gicv2_set_irq_type(struct irq_desc *desc, unsigned int type)
>  {
>      uint32_t cfg, actual, edgebit;
> @@ -1277,6 +1323,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 392cf91b58..316f2c4142 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -444,6 +444,19 @@ static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset, bool wait_for_rwp)
>          gicv3_wait_for_rwp(irqd->irq);
>  }
>  
> +static bool gicv3_peek_irq(struct irq_desc *irqd, u32 offset)
> +{
> +    void __iomem *base;
> +    unsigned int irq = irqd->irq;
> +
> +    if ( irq >= NR_GIC_LOCAL_IRQS)
> +        base = GICD + (irq / 32) * 4;
> +    else
> +        base = GICD_RDIST_SGI_BASE;
> +
> +    return !!(readl(base + offset) & (1U << (irq % 32)));
> +}
> +
>  static void gicv3_unmask_irq(struct irq_desc *irqd)
>  {
>      gicv3_poke_irq(irqd, GICD_ISENABLER, false);
> @@ -477,6 +490,43 @@ 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 )
> +    {
> +        if ( test_bit(_IRQ_GUEST, &irqd->status) )
> +            set_bit(_IRQ_INPROGRESS, &irqd->status);
> +        gicv3_poke_irq(irqd, GICD_ISACTIVER, false);
> +    }
> +    else
> +    {
> +        gicv3_poke_irq(irqd, GICD_ICACTIVER, false);
> +        if ( !gicv3_peek_irq(irqd, GICD_ISPENDR) &&
> +             test_bit(_IRQ_GUEST, &irqd->status) )
> +            clear_bit(_IRQ_INPROGRESS, &irqd->status);
> +    }
> +}
> +
> +static void gicv3_set_pending_state(struct irq_desc *irqd, bool pending)
> +{
> +    ASSERT(spin_is_locked(&irqd->lock));
> +
> +    if ( pending )
> +    {
> +        /* The INPROGRESS bit will be set when the interrupt fires. */
> +        gicv3_poke_irq(irqd, GICD_ISPENDR, false);
> +    }
> +    else
> +    {
> +        gicv3_poke_irq(irqd, GICD_ICPENDR, false);
> +        if ( !gicv3_peek_irq(irqd, GICD_ISACTIVER) &&
> +             test_bit(_IRQ_GUEST, &irqd->status) )
> +            clear_bit(_IRQ_INPROGRESS, &irqd->status);
> +    }
> +}
> +
>  static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
>  {
>       uint64_t mpidr = cpu_logical_map(cpu);
> @@ -1766,6 +1816,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/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 565b0875ca..21cf35f106 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -344,6 +344,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);
>      /* Set IRQ type */
>      void (*set_irq_type)(struct irq_desc *desc, unsigned int type);
>      /* Set IRQ priority */
> @@ -392,6 +396,26 @@ static inline unsigned int gic_get_nr_lrs(void)
>      return gic_hw_ops->info->nr_lrs;
>  }
>  
> +/*
> + * Set the active state of an IRQ. This should be used with care, as this
> + * directly forces the active bit, without considering the GIC state machine.
> + * For private IRQs this only works for those of the current CPU.
> + */
> +static inline void gic_set_active_state(struct irq_desc *irqd, bool state)
> +{
> +    gic_hw_ops->set_active_state(irqd, state);
> +}
> +
> +/*
> + * Set the pending state of an IRQ. This should be used with care, as this
> + * directly forces the pending bit, without considering the GIC state machine.
> + * For private IRQs this only works for those of the current CPU.
> + */
> +static inline void gic_set_pending_state(struct irq_desc *irqd, bool state)
> +{
> +    gic_hw_ops->set_pending_state(irqd, state);
> +}
> +
>  void register_gic_ops(const struct gic_hw_operations *ops);
>  int gic_make_hwdom_dt_node(const struct domain *d,
>                             const struct dt_device_node *gic,
> 

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