Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

[Julien Grall <julien.grall@linaro.org>]

On 04/07/2014 04:03 PM, Ian Campbell wrote:
>>  /*
>>   * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
>> + * - desc.lock must be held
>>   * already called gic_cpu_init
> 
> I think you've injected that line in the middle of a sentence ;-)

Oops right.

>>   */
>> -static void gic_set_irq_properties(unsigned int irq, bool_t level,
>> +static void gic_set_irq_properties(struct irq_desc *desc,
>>                                     const cpumask_t *cpu_mask,
>>                                     unsigned int priority)
>>  {
>>      volatile unsigned char *bytereg;
>>      uint32_t cfg, edgebit;
>>      unsigned int mask;
>> +    unsigned int irq = desc->irq;
>> +    unsigned int type = desc->arch.type;
>> +
>> +    ASSERT(spin_is_locked(&desc->lock));
>>  
>>      spin_lock(&gic.lock);
>>  
>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>      /* Set edge / level */
>>      cfg = GICD[GICD_ICFGR + irq / 16];
>>      edgebit = 2u << (2 * (irq % 16));
>> -    if ( level )
>> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
> 
> Is getting DT_IRQ_TYPE_NONE here not an error?

No, there is some DT like the exynos one which is using 0 (i.e
DT_IRQ_TYPE_NONE) for the IRQ type.

I guess we have to skip setting level/edge property in this case.

> Oh, I see this is the innards of dt_irq_is_level_triggered. Could that
> be refactored e.g. into dt_irq_type_is_level_triggered(const something
> type)?

I was wondering something like that instead:

if ( (type & DT_IRQ_TYPE_LEVEL_MASK) )
...
else if ( (type & DT_IRQ_TYPE_EDGE_BOTH) )
...

So we skip the DT_IRQ_TYPE_NONE.

> 
>>          cfg &= ~edgebit;
>>      else
>>          cfg |= edgebit;
> 
>> @@ -82,6 +83,12 @@ static int __cpuinit init_local_irq_data(void)
>>          init_one_irq_desc(desc);
>>          desc->irq = irq;
>>          desc->action  = NULL;
>> +
>> +        /* PPIs are include in local_irqs, we have to copy the IRQ type from
>> +         * CPU0 otherwise we may miss the type if the IRQ type has been
>> +         * set early.
>> +         */
>> +        desc->arch.type = per_cpu(local_irq_desc, 0)[irq].arch.type;
> 
> I thought we had a boot_cpu(foo) accessor, but I guess not (or at least
> I can't find it right now).
> 
> That might be nicer to add than adding a hardcoded 0 (I suppose it isn't
> the only one though).

It's also used in mm.c when page tables is setup.

I will introduce boot_cpu macro.

>> @@ -379,6 +382,67 @@ void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
>>      BUG();
>>  }
>>  
>> +static inline int irq_set_type(struct irq_desc *desc, unsigned int type)
>> +{
>> +    unsigned int flags;
>> +    int ret = -EBUSY;
>> +
>> +    if ( type == DT_IRQ_TYPE_NONE )
>> +        return 0;
>> +
>> +    spin_lock_irqsave(&desc->lock, flags);
>> +
>> +    if ( desc->arch.type != DT_IRQ_TYPE_NONE && desc->arch.type != type )
>> +        goto err;
>> +
>> +    desc->arch.type = type;
> 
> There was an open coded assignment in the guest path which unfortunately
> I already trimmed. Shouldn't that have all these checks too?

No, because with patch #11 the desc->arch.type is only set once by IRQ.

>> +
>> +    ret = 0;
>> +
>> +err:
>> +    spin_unlock_irqrestore(&desc->lock, flags);
>> +    return ret;
>> +}
>> +
>> +unsigned int platform_get_irq(const struct dt_device_node *device,
>> +                              int index)
>> +{
>> +    struct dt_irq dt_irq;
>> +    struct irq_desc *desc;
>> +    unsigned int type, irq;
>> +    int res;
>> +
>> +    res = dt_device_get_irq(device, index, &dt_irq);
>> +    if ( res )
>> +        return 0;
> 
> Not an error? Do we take precautions against IRQ0 being actually used
> somewhere?

Yes in gic_interrupt. do_IRQ is by-passed because IRQ 0 is a SGI.

> We should have an explicit #define for an invalid IRQ number.

I don't think it's useful because the device tree can't provide an IRQ
smaller than 16.

>> +    irq = dt_irq.irq;
>> +    type = dt_irq.type;
>> +
>> +    /* Setup the IRQ type */
>> +
>> +    if ( irq < NR_LOCAL_IRQS )
>> +    {
>> +        unsigned int cpu;
>> +        /* For PPIs, we need to set IRQ type on every online CPUs */
>> +        for_each_cpu( cpu, &cpu_online_map )
>> +        {
>> +            desc = &per_cpu(local_irq_desc, cpu)[irq];
>> +            res = irq_set_type(desc, type);
>> +            if ( res )
>> +                return 0;
> 
> Error?
> 
> Also no need to undo any partial work?

desc->arch.type should be sync on every CPU. It would be crazy to have a
different IRQ type on every CPU.

> I haven't seen the caller yet, but for PPIs do we not get called for
> each CPU as it binds to the PPI anyway?

No, this function is only called once, when the DT is parsed at startup.

Basically, this function will replace dt_device_get_irq call.

>> +        }
>> +    }
>> +    else
>> +    {
>> +        res = irq_set_type(irq_to_desc(irq), type);
>> +        if ( res )
>> +            return 0;
>> +    }
>> +
>> +    return irq;
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
> 
>> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
>> index b52c26f..107c13a 100644
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -16,6 +16,7 @@ struct arch_pirq
>>  
>>  struct arch_irq_desc {
>>      int eoi_cpu;
>> +    unsigned int type;
> 
> I was wondering through the above if this ought to be a "bool_t level"
> or not. Thoughts?

We have 5 possibles types:
	- default : we don't have to set the edge bit
        - level high/low
        - edge rising/falling

Even if GICv2 is only handling 2 types, it can change for the next
version of GIC.

Regards,

-- 
Julien Grall