Re: [RFC PATCH 15/49] ARM: GIC: Allow tweaking the active state of an IRQ

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

Hi Andre,

On 09/02/18 14:39, Andre Przywara wrote:
> When playing around with hardware mapped, level triggered virtual IRQs,
> there is the need to explicitly set the active state of an interrupt at
> some point in time.
> To prepare the GIC for that, we introduce a set_active_state() function
> to let the VGIC manipulate the state of an associated hardware IRQ.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>   xen/arch/arm/gic-v2.c     |  9 +++++++++
>   xen/arch/arm/gic-v3.c     | 16 ++++++++++++++++
>   xen/arch/arm/gic.c        |  5 +++++
>   xen/include/asm-arm/gic.h |  5 +++++
>   4 files changed, 35 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 2e35892881..5339f69fbc 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -235,6 +235,14 @@ static unsigned int gicv2_read_irq(void)
>       return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);
>   }
>   
> +static void gicv2_set_active_state(int irq, bool active)

I would much prefer to have an irq_desc in parameter. This is matching 
the other interface and you could update the flags such as 
_IRQ_INPROGRESS which you don't do at the moment.

Also, who is preventing two CPUs to clear the active bit at the same time?

> +{
> +    if (active)
> +        writel_gicd(1U << (irq % 32), GICD_ISACTIVER + (irq / 32) * 4);
> +    else
> +        writel_gicd(1U << (irq % 32), GICD_ICACTIVER + (irq / 32) * 4);

You will have a few places in the code usually similar construct. It 
would make sense to introduce a helper poke as we have in the GICv3 code.

> +}
> +
>   static void gicv2_set_irq_type(struct irq_desc *desc, unsigned int type)
>   {
>       uint32_t cfg, actual, edgebit;
> @@ -1241,6 +1249,7 @@ 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_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 08d4703687..595eaef43a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -475,6 +475,21 @@ static unsigned int gicv3_read_irq(void)
>       return irq;
>   }
>   
> +static void gicv3_set_active_state(int irq, bool active)
> +{
> +    void __iomem *base;
> +
> +    if ( irq >= NR_GIC_LOCAL_IRQS)
> +        base = GICD + (irq / 32) * 4;
> +    else
> +        base = GICD_RDIST_SGI_BASE;
> +
> +    if ( active )
> +        writel(1U << (irq % 32), base + GICD_ISACTIVER);
> +    else
> +        writel(1U << (irq % 32), base + GICD_ICACTIVER);

Shouldn't you wait until RWP bits is cleared here?

> +}

Why don't you use the function poke?

> +
>   static inline uint64_t gicv3_mpidr_to_affinity(int cpu)
>   {
>        uint64_t mpidr = cpu_logical_map(cpu);
> @@ -1722,6 +1737,7 @@ 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_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 89873c1df4..dfc2108c4d 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -92,6 +92,11 @@ void gic_restore_state(struct vcpu *v)
>       isb();
>   }
>   
> +void gic_set_active_state(int irq, bool state)
> +{
> +    gic_hw_ops->set_active_state(irq, state);
> +}
> +
>   /* 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 c4c68c7770..d330860580 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -238,6 +238,9 @@ DECLARE_PER_CPU(uint64_t, lr_mask);
>   extern enum gic_version gic_hw_version(void);
>   extern int gic_get_nr_lrs(void);
>   
> +/* Force the state of an IRQ to active. */
> +void gic_set_active_state(int irq, bool state);
> +
>   /* Program the IRQ type into the GIC */
>   void gic_set_irq_type(struct irq_desc *desc, unsigned int type);
>   
> @@ -347,6 +350,8 @@ struct gic_hw_operations {
>       void (*deactivate_irq)(struct irq_desc *irqd);
>       /* Read IRQ id and Ack */
>       unsigned int (*read_irq)(void);
> +    /* Force the state of an IRQ to active */
> +    void (*set_active_state)(int irq, bool state);
>       /* 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