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

[Andre Przywara <andre.przywara@xxxxxxxxxx>]

Hi,

On 12/02/18 13:55, Julien Grall wrote:
> 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@xxxxxxxxxx>
>> ---
>>   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 that's why I had it just like this in my first version. However
this proved to be nasty because I now need to get this irq_desc pointer
first, as the caller doesn't have it already. Since all we have and need
is the actual hardware IRQ number, I found it more straight-forward to
just use that number directly instead of going via the pointer and back
(h/w intid => irq_desc => irq).

> and you could update the flags such as
> _IRQ_INPROGRESS which you don't do at the moment.

Mmh, interesting point. I guess I should also clear this bit in the new
VGIC. At least once I wrapped my head around what this flag is
*actually* for (in conjunction with _IRQ_GUEST).
Anyway I guess this bit would still be set in our case.

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

A certain hardware IRQ is assigned to one virtual IRQ on one VCPU at one
time only. Besides, GICD_ICACTIVERn has wired NAND semantics, so that's
naturally race free (as it was designed to be).
Unless I miss something here (happy to be pointed to an example where it
causes problems).

>> +{
>> +    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.

Sure.

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

I don't see why. I think this action has some posted semantics anyway,
so no need for any synchronisation. And also RWP does not track
I[SC]ACTIVER, only ICENABLER and some CTLR bits (ARM IHI 0069D, 8.9.4:
RWP[31]).

> 
>> +}
> 
> Why don't you use the function poke?

Ah, I didn't see this. But then this now does this quite costly RWP
dance now. We could add a check in there to only do this if we change
the affected registers or pass an explicit "bool wait_for_rwp" in there.

Thanks for staying awake on this ;-)

Cheers,
Andre.

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

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